Firefox crashes on XSL Transform

[andre]

Binary package hint: firefox-3.0

Firefox simply crashes if I open the XML File.

Description: Ubuntu 8.04.1
Release: 8.04
Package: :3.0.1+build1+nobinonly-0ubuntu0.8.04.3

To reproduce: Click the "test.xml" attachment from the second comment (http://launchpadlibrarian.net/16454122/test.xml). WARNING: On multiple systems causes Firefox to segfault!

ProblemType: Bug
Architecture: i386
Date: Thu Jul 31 15:59:57 2008
DistroRelease: Ubuntu 8.04
NonfreeKernelModules: nvidia
Package: firefox-3.0 3.0.1+build1+nobinonly-0ubuntu0.8.04.3
PackageArchitecture: i386
ProcEnviron:
 PATH=/opt/e17/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/bin/X11:/usr/games
 LANG=de_DE.UTF-8@euro
 SHELL=/bin/bash
SourcePackage: firefox-3.0
Uname: Linux 2.6.24-20-386 i686

[Michael Rooney (mrooney)]

Hi andre, thanks for using Ubuntu and thanks for your bug report! I can confirm this issue (Firefox segfaults), although the .xsd is irrelevant. Let me upload the files for easier testing.

[Michael Rooney (mrooney)]

The XML file, attempting to link right from Launchpad.

[Michael Rooney (mrooney)]

Still reproducible in Intrepid; sent upstream.

[Johnath (johnath)]

Found on security focus, not sure where the original came from. Exploit code at the link iframes a little xml file with an xslt transform that causes a crash reliably on 3.0 branch and trunk (and presumably 1.9.1, didn't test). Null, but it's being called, assuming the worst for the moment.

Mac 3.0 crash report: http://crash-stats.mozilla.com/report/index/73cd82a1-6465-4ca2-9467-ef4982090325?p=1

[Johnath (johnath)]

No reason to believe it's mac-specific.

[Olli-pettay (olli-pettay)]

Yeah, crashes on linux too.

[Johnath (johnath)]

Created attachment 369320
PoC

[Mrbkap (mrbkap)]

Created attachment 369321
Fix

Obvious bug once you see it -- we can't leave this particular eval context on the context stack for the execution state to clean up.

[Dveditz (dveditz)]

Original source appears to be http://milw0rm.com/exploits/8285 credited to Guido Landi http://milw0rm.com/author/1413

Not much point in keeping this bug hidden

[Mrbkap (mrbkap)]

http://hg.mozilla.org/mozilla-central/rev/bc263bec4a3e

I'll check this into the 1.9.5 branch once it's green again.

[Mrbkap (mrbkap)]

Comment on attachment 369321
Fix

This applies cleanly to the 1.9.0 branch.

[Mrbkap (mrbkap)]

Note for the 1.8 branch: this bug exists there, but the file moved from extensions/transformiix/source/xslt/functions/txKeyFunctionCall.cpp.

[Samuel-sidler+old (samuel-sidler+old)]

Comment on attachment 369321
Fix

Approved for 1.9.0.8. a=ss for release-drivers. Please land immediately on 1.9.0.

[Mrbkap (mrbkap)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/167d03699cdb

[Reed Loden (reed)]

Just for the record, CVS trunk:
mozilla/content/xslt/src/xslt/txKeyFunctionCall.cpp 1.42

[Mrbkap (mrbkap)]

Created attachment 369357
Fix for the 1.8.1 branch

This patch is for the 1.8.1 branch. I don't have a 1.8.0 tree to see if it applies there too.

[Samuel-sidler+old (samuel-sidler+old)]

Comment on attachment 369357
Fix for the 1.8.1 branch

Approved for 1.8.1.22. a=ss

[Mrbkap (mrbkap)]

Created attachment 369365
Crashtest

I'm checking this in everywhere now.

[Mrbkap (mrbkap)]

http://hg.mozilla.org/mozilla-central/rev/6586181d850b and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/291e1fee55b7 contain crashtests.

/cvsroot/mozilla/extensions/transformiix/source/xslt/functions/Attic/txKeyFunctionCall.cpp,v <-- txKeyFunctionCall.cpp
new revision: 1.33.28.1; previous revision: 1.33

is on the 1.8.1 branch.

[Caillon (caillon)]

Comment on attachment 369357
Fix for the 1.8.1 branch

This applies cleanly to the 1.8.0 branch

[Brendan-mozilla (brendan-mozilla)]

Macro-obscured return combined with manual storage management considered harmful. Wish for static analysis to find such bad patterns, but it seems hard.

/be

[Tglek (tglek)]

(In reply to comment #17)
> Macro-obscured return combined with manual storage management considered
> harmful. Wish for static analysis to find such bad patterns, but it seems hard.
>
> /be

This does seem to be superficially similar to push/pop analysis I did in spidermonkey. If we had some heuristic to decide when these patterns are function-local, might be able to do scan for them.

[Mrbkap (mrbkap)]

*** Bug 485382 has been marked as a duplicate of this bug. ***

[Dveditz (dveditz)]

*** Bug 460090 has been marked as a duplicate of this bug. ***

[Jbecerra-mozilla (jbecerra-mozilla)]

Verified fixed on:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729)
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.8) Gecko/2009032608 Firefox/3.0.8
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8) Gecko/2009032608 Firefox/3.0.8

Fx307 in all three platforms crashed with the test case here and the test case in bug 460090. It did not crash on Fx308.

[Martin Packman (gz)]

Bug 460090 comment 8 contains a fix, reasoning, and tests - that I posted back in November.

Clearly I don't deserve my name in the commit history for failing - over the course of four months - to navigate the review process to move one line of code up one space. My apologies.

[Jwalden+bmo (jwalden+bmo)]

(In reply to comment #22)
> Bug 460090 comment 8 contains a fix, reasoning, and tests - that I posted back
> in November.
>
> Clearly I don't deserve my name in the commit history for failing - over the
> course of four months - to navigate the review process to move one line of code
> up one space. My apologies.

Passive-aggressive insinuations of bad faith generally don't win friends and influence people.

That said, you're in luck, because I guess I'm just weird. I started a thread in mozilla.governance about this to figure out what we can do better on a variety of different levels; your feedback would be appreciated.

http://groups.google.com/group/mozilla.governance/browse_thread/thread/f01ea12ff3c36522

[Mrbkap (mrbkap)]

(In reply to comment #22)
> Clearly I don't deserve my name in the commit history for failing - over the
> course of four months - to navigate the review process to move one line of code
> up one space. My apologies.

To be clear, there was no offense or insult intended here. I wasn't aware of bug 460090 (and neither was anybody else involved in the patch here). The past few months have been extremely hectic at Mozilla as we've tried to push Firefox 3.5 out the door and there has been a conscious effort to focus on bugs that directly block the release (i.e. blocking1.9.1+). Unfortunately, nobody noticed the severity of your bug and in the heat of the moment when the 0-day vulnerability hit the waves, that same nobody (of which I'm a part, not ducking responsibility) looked for a duplicate bug that might already contain a patch. Please accept my apology for the oversight and also please contribute to Jeff's thread on how to avoid this in the future.

[Martijn-martijn (martijn-martijn)]

(In reply to comment #23)
> Passive-aggressive insinuations of bad faith generally don't win friends and
> influence people.

Jeff, he's frustrated with what happened here, so I think it's totally understandable he's being sarcastic.

[Mozilla-rowk (mozilla-rowk)]

(In reply to comment #24)
> Unfortunately, nobody noticed the severity of your bug and in the heat of
> the moment

Honestly I am not trying to ask this in a rude way, but what process are you using that allows you to miss a bug marked with Critical importance (bug 460090)? There is talk of processes and documentation but here we had a bug with a patch, marked Critical. Shouldn't this show up at the top of at least one bugzilla list someone prioritizes on?

[Michael Rooney (mrooney)]

Apparently the bug watch can't follow duplicates. I've changed it.

[Brendan-mozilla (brendan-mozilla)]

(In reply to comment #26)
> (In reply to comment #24)
> > Unfortunately, nobody noticed the severity of your bug and in the heat of
> > the moment
>
> Honestly I am not trying to ask this in a rude way, but what process are you
> using that allows you to miss a bug marked with Critical importance (bug
> 460090)?

That's a good question, I'm asking it too. I've been heads-down in TraceMonkey work, having long ago retired from release driving or bugzilla querying to find and triage important bugs. But I'm involved, on the hook even as the technical buck-stopper in mozilla.org.

> There is talk of processes and documentation but here we had a bug
> with a patch, marked Critical. Shouldn't this show up at the top of at least
> one bugzilla list someone prioritizes on?

That the bug was marked wanted1.9.1 instead of blocking1.9.1 when it involves a FreeMemoryRead was a mistake too.

And of course that the bug was forgotten even by sicking, who apologized in bug 460090 comment 19 and obviously regrets the error, was poor. We've had bugs "get lost" but you're right that severity critical should be monitored and reviewed. It must not be debased.

Processes and documentation won't solve every problem, of course, and they have their own overheads, as well as tending to be backward-looking. But they can absolutely help prevent a recurrence of what went wrong this time, in our world in a non-bureaucratic, personal-accountability fashion.

We have people other than sicking in the Mozilla community who are on the spot here, as noted above (i.e., including me). I hope to hear from others who may have seen this bug and misjudged it, in the mozilla.governance thread if not here, so that your entirely reasonable questions are answered satisfactorily.

Well, answered anyway. There may be little satisfaction, except in working to ensure that this doesn't happen again.

Not looking to lay blame, rather (ideally) to fix the bug diagnosis, severity judging, and bug-marking/querying problems that probably exist here.

/be

[Hskupin (hskupin)]

Verified fixed based on Tinderbox results on trunk and 1.9.1.

[Abillings (abillings)]

Verified fixed based on testcase in Tinderbox for 1.9.0.9.

[Abillings (abillings)]

The crash with the POC doesn't repro in Thunderbird 2.0.0.21 (or 22pre) or Seamonkey nightly based on 1.8.1.22.

[Micah Gersten (micahg)]

This was release in Firefox 3.0.9. Thank you and please report any other bugs you may find.