485217 - (CVE-2009-1169) Exploitable crash in [@txMozillaXSLTProcessor::TransformToDoc ]

Status: VERIFIED FIXED

(Core :: XSLT, defect, P1)

Description

[Johnathan Nightingale [: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

Comment 1

[Johnathan Nightingale [:johnath]]

No reason to believe it's mac-specific.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Yeah, crashes on linux too.

Comment 3

[Johnathan Nightingale [:johnath]]

Attachment: PoC

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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]]

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) (inactive)]

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) (inactive)]

Comment on attachment 369321 [details] [diff] [review]
Fix

This applies cleanly to the 1.9.0 branch.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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)]

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) (inactive)]

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

Comment 11

[Reed Loden [:reed]]

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

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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)]

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) (inactive)]

Attachment: Crashtest

I'm checking this in everywhere now.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

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)]

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]]

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)]

(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 21

[juan becerra [:juanb]]

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]

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]]

(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) (inactive)]

(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 (dead)]

(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]

(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]]

(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][⌚️UTC+2]]

Verified fixed based on Tinderbox results on trunk and 1.9.1.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Verified fixed based on testcase in Tinderbox for 1.9.0.9.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

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.