Bug 724284 (CVE-2012-0452)

use after free in nsXBLDocumentInfo::ReadPrototypeBindings

RESOLVED FIXED in Firefox 10

Status

()

Core
XBL
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: smaug)

Tracking

({crash, regression, topcrash})

Trunk
mozilla10
crash, regression, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10+ fixed, firefox11+ fixed, firefox12+ fixed, firefox13 fixed, firefox-esr1010+ fixed, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Olli found this.  Could be causing bug 724129.

nsXBLDocumentInfo::ReadPrototypeBindings:
653     nsXBLPrototypeBinding* binding = new nsXBLPrototypeBinding();
654     rv = binding->Read(stream, docInfo, doc, flags);
655     if (NS_FAILED(rv)) {
656       delete binding;
657       return rv;
658     }

nsXBLPrototypeBinding::Read:
1554   rv = aDocInfo->SetPrototypeBinding(id, this);
1555   NS_ENSURE_SUCCESS(rv, rv);
1556 
1557   nsCAutoString className;
1558   rv = aStream->ReadCString(className);
1559   NS_ENSURE_SUCCESS(rv, rv);
[... many other ways to fail without removing the binding ...]

ReadPrototypeBindings creates the binding, calls into Read, Read stores the binding in the hash table.  If the Read fails, then the binding is deleted.  But on any code path except one, the binding is kept in the table.

The cycle collector reads this table, grabs the bogus binding and attempts to do a virtual method call on it.  If bug 724129 is any indication, at least 90% of the time, this results in the browser attempting to execute at precisely the address 0x4246c83.  Maybe jemalloc ends up reusing the location for the binding immediately for something that points there.  That probably reduces how exploitable this is, but not all of these crashes look like that.

Read is only called in this one place.  I manually inspected the other calls to SetPrototypeBinding and they didn't seem to have this problem, but I'm not very familiar with this code.
(Reporter)

Updated

6 years ago
Group: mozilla-corporation-confidential
(Reporter)

Comment 1

6 years ago
Urgh, I meant to file this as a security bug...
(Reporter)

Comment 2

6 years ago
If somebody could make it a real security bug I'd appreciate it...

Updated

6 years ago
Group: mozilla-corporation-confidential → core-security
Done.
(Reporter)

Comment 4

6 years ago
Thanks.  Bugzilla seems to be slow at loading pages but not at submitting new bugs...
(Reporter)

Comment 5

6 years ago
I count 5 places in Read where it can return a failure without removing the binding from the table.
(Reporter)

Updated

6 years ago
status-firefox-esr10: --- → affected
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected

Updated

6 years ago
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox12: --- → +
(Reporter)

Comment 6

6 years ago
bug 722806 sounds like it could be a vaguely similar problem with XBL cache invalidation.
Created attachment 594525 [details] [diff] [review]
a patch
The patch is missing stuff after "  // Finally, read in the resources."
Created attachment 594529 [details] [diff] [review]
better, yet untested patch
Attachment #594525 - Attachment is obsolete: true
(Reporter)

Comment 10

6 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> Created attachment 594529 [details] [diff] [review]
> better, yet untested patch

Ah, nice.  That's much better than anything I'd come up with.
(Reporter)

Comment 11

6 years ago
Separately, I wonder if it makes sense to file a bug to invalidate the cache if there's an error while loading, or even invalidating it if the previous run ended in a crash.  It just seems like a shame for a perf enhancement to put the browser into a completely unusable state.
(Reporter)

Comment 12

6 years ago
Marcia was able to reproduce bug 724129 in a VM.  With the patch in this bug applied to release, we haven't been able to reproduce the crash, even with the same set of addons and toolbars applied.

Who would be good to review this patch?  If further testing doesn't show the problem, then Alex said it would be good if we could land this in m-c today, to prepare for landing in Aurora, Beta, Release tomorrow, if that's what they decide to do.
(Reporter)

Updated

6 years ago
Attachment #594529 - Flags: review?(enndeakin)
(Reporter)

Comment 13

6 years ago
Created attachment 594867 [details] [diff] [review]
force a failure while reading the cache

This patch adds a single line to cause a failure in nsXBLPrototypeBinding::Read:
   rv = aStream->ReadCString(className);
+  rv = NS_ERROR_FAILURE;
   NS_ENSURE_SUCCESS(rv, rv);

When applied to a release debug build on Mac it crashes with exactly the same stack as in bug 724129.
Where do the various failures in Read() come from? Is it something web content could cause by loading some of our built-in XBL (e.g. <marquee>)? Or does it only happen if the XBL itself is somehow malformed? Did we change something here or is this an old latent bug that somehow got triggered recently?

Given bug 724129 it sounds like the problem only arises with certain addons. Are only the users of those add-ons vulnerable? Start-up crashes probably aren't usually exploitable, but could the crash be triggered at other times when an attacker potentially has prepared an attack?

The questions above are aimed at determining if the current "sg:critical" rating appropriate ("could be used to perform a drive-by malware install" basically) or if it's more of an "sg:moderate" type bug.
(Reporter)

Comment 15

6 years ago
(In reply to Andrew McCreight [:mccr8] from comment #13)
> When applied to a release debug build on Mac it crashes with exactly the
> same stack as in bug 724129.

When the modification in comment 13 is applied on top of Olli's patch, there is no crash, and the RemovePrototypeBinding code is hit.
bug 720292 added (more?) cycle collection to XBL, is that the triggering change? Not necessarily the underlying bug fixed in this patch, but something that exposed the problem more often?
(Reporter)

Comment 17

6 years ago
(In reply to Daniel Veditz from comment #16)
> bug 720292 added (more?) cycle collection to XBL, is that the triggering
> change? Not necessarily the underlying bug fixed in this patch, but
> something that exposed the problem more often?

The XBL cache thing is newly added in Firefox 10 (bug 94199).  Errors are improperly handled in a portion of the cache loading code, causing a dangling pointer.  The cycle collector just happens to be the first thing that attempts to invoke a method on this invalid pointer.
(Reporter)

Comment 18

6 years ago
Bug 720292 should have only affected the behavior of the cycle collector when a special debug mode for the CC is set, and even then it should only cause additional strings to be saved, so is unlikely to be related to this bug.
(In reply to Andrew McCreight [:mccr8] from comment #6)
> bug 722806 sounds like it could be a vaguely similar problem with XBL cache
> invalidation.

The guess in comment 16 was based on the regression range from bug 722806, but that would be a Firefox 12 regression and bug 724129 is hitting Firefox 10 users. Bug 720292 can't be the cause of bug 724129, but maybe it made the conditions more likely to happen and thus be noticed by :graememcc in bug 722806.
status1.9.2: --- → unaffected
Blocks: 94199
Keywords: regression
(Reporter)

Comment 20

6 years ago
I'm not really very familiar with this code outside of these two functions I've read over, so I'm not sure of the broader implications about the cache and how things are read from or written to it.  Neil or Boris may know better.

Comment 21

6 years ago
Sorry for the lag here; it was mostly the weekend...

> Where do the various failures in Read() come from? 

Good question.  I don't quite know the new cache setup.  I don't _think_ web content can trigger them, though.  Neil, do you know for sure?

Comment 22

6 years ago
Comment on attachment 594529 [details] [diff] [review]
better, yet untested patch

This looks fine, but we could also handle this in the caller where we actually call delete:  GetBindingURI(), if non-null get the ref, and that's the id to remove...  Either way, I guess.
Attachment #594529 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/3c1cdbbea964
I prefer doing cleaning up in the method.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Assignee: nobody → bugs
(Reporter)

Updated

6 years ago
status-firefox13: affected → fixed
Keywords: crash
Target Milestone: --- → mozilla13
Comment on attachment 594529 [details] [diff] [review]
better, yet untested patch

[Approval Request Comment]
Regression caused by (bug #): bug 94199
User impact if declined: Startup crashes
Testing completed (on m-c, etc.): Marcia and Andrew tested the patch on beta
Risk to taking this patch (and alternatives if risky):
should be very low, since it is adding just some error checking
String changes made by this patch: N/A
Attachment #594529 - Flags: approval-mozilla-release?
Attachment #594529 - Flags: approval-mozilla-beta?
Attachment #594529 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 25

6 years ago
(In reply to Olli Pettay [:smaug] from comment #24)
> Testing completed (on m-c, etc.): Marcia and Andrew tested the patch on beta

Minor correction: we tested on release, not beta.
(Reporter)

Updated

6 years ago
Keywords: topcrash
(Reporter)

Comment 26

6 years ago
There's a fair number of these crashes in 11, too.
Whiteboard: [sg:critical] → [sg:critical][qa+]
Comment on attachment 594529 [details] [diff] [review]
better, yet untested patch

[Triage Comment]
Please land on all branches. We have decided to chemspill for this security/crash issue.
Attachment #594529 - Flags: approval-mozilla-release?
Attachment #594529 - Flags: approval-mozilla-release+
Attachment #594529 - Flags: approval-mozilla-beta?
Attachment #594529 - Flags: approval-mozilla-beta+
Attachment #594529 - Flags: approval-mozilla-aurora?
Attachment #594529 - Flags: approval-mozilla-aurora+
Please also land on mozilla-esr10: https://hg.mozilla.org/releases/mozilla-esr10/
tracking-firefox-esr10: --- → .1+
(Reporter)

Comment 29

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/623643384911
https://hg.mozilla.org/releases/mozilla-beta/rev/ab1685df6e2f
https://hg.mozilla.org/releases/mozilla-release/rev/f40eead88b1c
status-firefox10: affected → fixed
status-firefox11: affected → fixed
status-firefox12: affected → fixed
Target Milestone: mozilla13 → mozilla10
(Reporter)

Comment 30

6 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/c509bc9c3577
status-firefox-esr10: affected → .1-fixed
Thanks Andrew!
status-firefox-esr10: .1-fixed → affected
status-firefox10: fixed → affected
status-firefox11: fixed → affected
status-firefox12: fixed → affected
Target Milestone: mozilla10 → mozilla13
Is XUL Fennec affected by this bug? We're trying to figure out whether or not we should QA and ship the 10.0.1 update. Thanks!
status-firefox-esr10: affected → .1-fixed
status-firefox10: affected → fixed
status-firefox11: affected → fixed
status-firefox12: affected → fixed
Target Milestone: mozilla13 → mozilla10
(Reporter)

Comment 33

6 years ago
One thing to be aware of there is that Android tests aren't running on Mozilla-release due to bug 725193, so all patches that landed for the chem spill are not tested against 10 for Android. (see https://tbpl.mozilla.org/?tree=Mozilla-Release )
Verified fixed using  Mozilla/5.0 (Windows NT 5.1; rv:10.0.1) Gecko/20100101 Firefox/10.0.1 and  Mozilla/5.0 (Windows NT 5.1; rv:10.0.1) Gecko/20100101 Firefox/10.0.1 ESR on Windows XP.

I verified by going back to my crashing VM and launching both builds. After more than 20-25 repeated startups I could not generate a crash.

I can double check this in the QA lab as well where I have another crashing instance.
This evening I couldn't generate a crash on the QA lab Win XP machine with 10.0 where I previously could with my toolbar laden profile, so for now I was only able to verify the fix on my crashing Win XP VM instance.
Anthony asked me to verify on 11.0b2, but unfortunately one of the extensions in my crashing profile is not compatible (Ant toolbar). So I can test repeated stops/starts, but it would be testing without the same set of extensions.
(In reply to Marcia Knous [:marcia] from comment #36)
> Anthony asked me to verify on 11.0b2, but unfortunately one of the
> extensions in my crashing profile is not compatible (Ant toolbar). So I can
> test repeated stops/starts, but it would be testing without the same set of
> extensions.

Would it be enough to assume this is fixed for Firefox 11+ by putting faith in our 10.0.1 testing and watching crashstats for 11.0b2?
Do we have enough data here to call this verified-fixed for Firefox 10.0.1 and 10.0.1ESR?
Alias: CVE-2012-0452
(Reporter)

Comment 39

6 years ago
In bug 724129, Tomer Cohen reported that 10.0.1 fixed the issue for users he was communicating with.

Updated

6 years ago
status-firefox-esr10: .1-fixed → fixed
tracking-firefox-esr10: .1+ → 10+

Updated

6 years ago
Blocks: 727692
Is there a reproducible test case that could be used to verify this fix?
Jason, I am assuming not at this point.
Group: core-security
Marking qa- based on comment 41. Will verify once we have a reproducible case.
Whiteboard: [sg:critical][qa+] → [sg:critical][qa-]
You need to log in before you can comment on or make changes to this bug.