724284 - (CVE-2012-0452) use after free in nsXBLDocumentInfo::ReadPrototypeBindings

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Urgh, I meant to file this as a security bug...

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Johnny Stenback (:jst)]

Done.

Comment 4

[Andrew McCreight [:mccr8]]

Thanks.  Bugzilla seems to be slow at loading pages but not at submitting new bugs...

Comment 5

[Andrew McCreight [:mccr8]]

I count 5 places in Read where it can return a failure without removing the binding from the table.

Comment 6

[Andrew McCreight [:mccr8]]

bug 722806 sounds like it could be a vaguely similar problem with XBL cache invalidation.

Comment 7

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

Attachment: a patch

Comment 8

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

The patch is missing stuff after "  // Finally, read in the resources."

Comment 9

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

Attachment: better, yet untested patch

Comment 10

[Andrew McCreight [:mccr8]]

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

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Andrew McCreight [:mccr8]]

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

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Andrew McCreight [:mccr8]]

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

Comment 16

[Daniel Veditz [:dveditz]]

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?

Comment 17

[Andrew McCreight [:mccr8]]

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

Comment 18

[Andrew McCreight [:mccr8]]

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.

Comment 19

[Daniel Veditz [:dveditz]]

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

Comment 20

[Andrew McCreight [:mccr8]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/3c1cdbbea964
I prefer doing cleaning up in the method.

Comment 24

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

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

Comment 25

[Andrew McCreight [:mccr8]]

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

Comment 26

[Andrew McCreight [:mccr8]]

There's a fair number of these crashes in 11, too.

Comment 27

[Alex Keybl [:akeybl]]

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.

Comment 28

[Alex Keybl [:akeybl]]

Please also land on mozilla-esr10: https://hg.mozilla.org/releases/mozilla-esr10/

Comment 29

[Andrew McCreight [:mccr8]]

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

Comment 30

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/c509bc9c3577

Comment 31

[Alex Keybl [:akeybl]]

Thanks Andrew!

Comment 32

[Alex Keybl [:akeybl]]

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!

Comment 33

[Andrew McCreight [:mccr8]]

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 )

Comment 34

[Marcia Knous [:marcia]]

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.

Comment 35

[Marcia Knous [:marcia]]

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.

Comment 36

[Marcia Knous [:marcia]]

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.

Comment 37

[u279076]

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

Comment 38

[u279076]

Do we have enough data here to call this verified-fixed for Firefox 10.0.1 and 10.0.1ESR?

Comment 39

[Andrew McCreight [:mccr8]]

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

Comment 40

[Jason Smith [:jsmith]]

Is there a reproducible test case that could be used to verify this fix?

Comment 41

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

Jason, I am assuming not at this point.

Comment 42

[u279076]

Marking qa- based on comment 41. Will verify once we have a reproducible case.