Last Comment Bug 485217 - (CVE-2009-1169) Exploitable crash in [@txMozillaXSLTProcessor::TransformToDoc ]
(CVE-2009-1169)
: Exploitable crash in [@txMozillaXSLTProcessor::TransformToDoc ]
Status: VERIFIED FIXED
[sg:critical]
: fixed-seamonkey1.1.16, fixed1.8.1.22, verified1.9.0.8, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9.2a1
Assigned To: Blake Kaplan (:mrbkap)
:
: Andrew Overholt [:overholt]
Mentors:
http://www.securityfocus.com/bid/3423...
: 460090 485382 (view as bug list)
Depends on: 504947
Blocks: 485286
  Show dependency treegraph
 
Reported: 2009-03-25 11:31 PDT by Johnathan Nightingale [:johnath]
Modified: 2010-09-19 23:21 PDT (History)
29 users (show)
mbeltzner: blocking1.9.1+
dveditz: blocking1.9.0.8+
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
samuel.sidler+old: blocking1.8.1.next+
samuel.sidler+old: wanted1.8.1.x+
caillon: blocking1.8.0.next+
mrbkap: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
PoC (10.00 KB, application/x-tar)
2009-03-25 11:48 PDT, Johnathan Nightingale [:johnath]
no flags Details
Fix (1.23 KB, patch)
2009-03-25 11:51 PDT, Blake Kaplan (:mrbkap)
peterv: review+
peterv: superreview+
samuel.sidler+old: approval1.9.0.9+
Details | Diff | Splinter Review
Fix for the 1.8.1 branch (1.39 KB, patch)
2009-03-25 14:03 PDT, Blake Kaplan (:mrbkap)
mrbkap: review+
mrbkap: superreview+
samuel.sidler+old: approval1.8.1.next+
caillon: approval1.8.0.next+
Details | Diff | Splinter Review
Crashtest (1.48 KB, patch)
2009-03-25 14:21 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review

Description Johnathan Nightingale [:johnath] 2009-03-25 11:31:16 PDT
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
Comment 1 Johnathan Nightingale [:johnath] 2009-03-25 11:32:26 PDT
No reason to believe it's mac-specific.
Comment 2 Olli Pettay [:smaug] 2009-03-25 11:39:04 PDT
Yeah, crashes on linux too.
Comment 3 Johnathan Nightingale [:johnath] 2009-03-25 11:48:50 PDT
Created attachment 369320 [details]
PoC
Comment 4 Blake Kaplan (:mrbkap) 2009-03-25 11:51:09 PDT
Created attachment 369321 [details] [diff] [review]
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.
Comment 5 Daniel Veditz [:dveditz] 2009-03-25 13:19:12 PDT
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
Comment 6 Blake Kaplan (:mrbkap) 2009-03-25 13:20:49 PDT
http://hg.mozilla.org/mozilla-central/rev/bc263bec4a3e

I'll check this into the 1.9.5 branch once it's green again.
Comment 7 Blake Kaplan (:mrbkap) 2009-03-25 13:24:25 PDT
Comment on attachment 369321 [details] [diff] [review]
Fix

This applies cleanly to the 1.9.0 branch.
Comment 8 Blake Kaplan (:mrbkap) 2009-03-25 13:25:20 PDT
Note for the 1.8 branch: this bug exists there, but the file moved from extensions/transformiix/source/xslt/functions/txKeyFunctionCall.cpp.
Comment 9 Samuel Sidler (old account; do not CC) 2009-03-25 13:29:00 PDT
Comment on attachment 369321 [details] [diff] [review]
Fix

Approved for 1.9.0.8. a=ss for release-drivers. Please land immediately on 1.9.0.
Comment 10 Blake Kaplan (:mrbkap) 2009-03-25 13:42:17 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/167d03699cdb
Comment 11 Reed Loden [:reed] (use needinfo?) 2009-03-25 13:46:06 PDT
Just for the record, CVS trunk:
mozilla/content/xslt/src/xslt/txKeyFunctionCall.cpp 	1.42
Comment 12 Blake Kaplan (:mrbkap) 2009-03-25 14:03:40 PDT
Created attachment 369357 [details] [diff] [review]
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.
Comment 13 Samuel Sidler (old account; do not CC) 2009-03-25 14:18:30 PDT
Comment on attachment 369357 [details] [diff] [review]
Fix for the 1.8.1 branch

Approved for 1.8.1.22. a=ss
Comment 14 Blake Kaplan (:mrbkap) 2009-03-25 14:21:40 PDT
Created attachment 369365 [details] [diff] [review]
Crashtest

I'm checking this in everywhere now.
Comment 15 Blake Kaplan (:mrbkap) 2009-03-25 14:44:51 PDT
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.
Comment 16 Christopher Aillon (sabbatical, not receiving bugmail) 2009-03-25 16:41:32 PDT
Comment on attachment 369357 [details] [diff] [review]
Fix for the 1.8.1 branch

This applies cleanly to the 1.8.0 branch
Comment 17 Brendan Eich [:brendan] 2009-03-25 21:45:10 PDT
Macro-obscured return combined with manual storage management considered harmful. Wish for static analysis to find such bad patterns, but it seems hard.

/be
Comment 18 (dormant account) 2009-03-26 10:31:47 PDT
(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.
Comment 19 Blake Kaplan (:mrbkap) 2009-03-26 13:50:24 PDT
*** Bug 485382 has been marked as a duplicate of this bug. ***
Comment 20 Daniel Veditz [:dveditz] 2009-03-26 15:55:15 PDT
*** Bug 460090 has been marked as a duplicate of this bug. ***
Comment 21 juan becerra [:juanb] 2009-03-26 19:06:24 PDT
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.
Comment 22 Martin 2009-03-26 21:04:44 PDT
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.
Comment 23 Jeff Walden [:Waldo] (remove +bmo to email) 2009-03-26 22:47:37 PDT
(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
Comment 24 Blake Kaplan (:mrbkap) 2009-03-26 23:51:16 PDT
(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.
Comment 25 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-03-27 04:56:16 PDT
(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.
Comment 26 Mike Rooney 2009-03-27 10:11:43 PDT
(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?
Comment 27 Brendan Eich [:brendan] 2009-03-27 15:46:55 PDT
(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
Comment 28 Henrik Skupin (:whimboo) 2009-03-30 03:09:57 PDT
Verified fixed based on Tinderbox results on trunk and 1.9.1.
Comment 29 Al Billings [:abillings] 2009-04-02 14:57:39 PDT
Verified fixed based on testcase in Tinderbox for 1.9.0.9.
Comment 30 Al Billings [:abillings] 2009-06-04 16:14:31 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.