Last Comment Bug 724284 - (CVE-2012-0452) use after free in nsXBLDocumentInfo::ReadPrototypeBindings
(CVE-2012-0452)
: use after free in nsXBLDocumentInfo::ReadPrototypeBindings
Status: RESOLVED FIXED
[sg:critical][qa-]
: crash, regression, topcrash
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla10
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on:
Blocks: 94199 727692
  Show dependency treegraph
 
Reported: 2012-02-04 10:24 PST by Andrew McCreight [:mccr8]
Modified: 2012-04-20 12:23 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
fixed
10+
fixed
unaffected


Attachments
a patch (1.25 KB, patch)
2012-02-05 02:31 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
better, yet untested patch (2.87 KB, patch)
2012-02-05 03:12 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑release+
Details | Diff | Splinter Review
force a failure while reading the cache (1.11 KB, patch)
2012-02-06 16:56 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-02-04 10:24:11 PST
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] 2012-02-04 10:25:17 PST
Urgh, I meant to file this as a security bug...
Comment 2 Andrew McCreight [:mccr8] 2012-02-04 10:27:50 PST
If somebody could make it a real security bug I'd appreciate it...
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-04 10:55:03 PST
Done.
Comment 4 Andrew McCreight [:mccr8] 2012-02-04 10:56:18 PST
Thanks.  Bugzilla seems to be slow at loading pages but not at submitting new bugs...
Comment 5 Andrew McCreight [:mccr8] 2012-02-04 11:15:58 PST
I count 5 places in Read where it can return a failure without removing the binding from the table.
Comment 6 Andrew McCreight [:mccr8] 2012-02-04 15:03:46 PST
bug 722806 sounds like it could be a vaguely similar problem with XBL cache invalidation.
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-02-05 02:31:51 PST
Created attachment 594525 [details] [diff] [review]
a patch
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-02-05 02:51:02 PST
The patch is missing stuff after "  // Finally, read in the resources."
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-02-05 03:12:40 PST
Created attachment 594529 [details] [diff] [review]
better, yet untested patch
Comment 10 Andrew McCreight [:mccr8] 2012-02-05 05:53:31 PST
(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] 2012-02-05 09:07:31 PST
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] 2012-02-06 14:42:06 PST
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] 2012-02-06 16:56:50 PST
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.
Comment 14 Daniel Veditz [:dveditz] 2012-02-06 17:01:57 PST
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] 2012-02-06 17:04:13 PST
(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] 2012-02-06 17:09:20 PST
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] 2012-02-06 17:13:36 PST
(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] 2012-02-06 17:19:37 PST
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] 2012-02-06 17:22:37 PST
(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] 2012-02-06 17:47:18 PST
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 [:bz] 2012-02-06 18:46:58 PST
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 [:bz] 2012-02-06 18:47:39 PST
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] (vacation Aug 25-28) 2012-02-07 01:03:47 PST
https://hg.mozilla.org/mozilla-central/rev/3c1cdbbea964
I prefer doing cleaning up in the method.
Comment 24 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-02-07 07:47:02 PST
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] 2012-02-07 07:48:47 PST
(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] 2012-02-07 11:25:17 PST
There's a fair number of these crashes in 11, too.
Comment 27 Alex Keybl [:akeybl] 2012-02-07 15:27:48 PST
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] 2012-02-07 15:38:34 PST
Please also land on mozilla-esr10: https://hg.mozilla.org/releases/mozilla-esr10/
Comment 30 Andrew McCreight [:mccr8] 2012-02-07 16:25:43 PST
https://hg.mozilla.org/releases/mozilla-esr10/rev/c509bc9c3577
Comment 31 Alex Keybl [:akeybl] 2012-02-07 16:44:09 PST
Thanks Andrew!
Comment 32 Alex Keybl [:akeybl] 2012-02-07 18:04:59 PST
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] 2012-02-08 05:01:04 PST
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 - use ni] 2012-02-08 17:56:21 PST
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 - use ni] 2012-02-08 18:36:35 PST
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 - use ni] 2012-02-09 11:23:14 PST
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-09 11:25:04 PST
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-09 12:20:09 PST
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] 2012-02-13 05:00:17 PST
In bug 724129, Tomer Cohen reported that 10.0.1 fixed the issue for users he was communicating with.
Comment 40 Jason Smith [:jsmith] 2012-03-13 17:18:08 PDT
Is there a reproducible test case that could be used to verify this fix?
Comment 41 Al Billings [:abillings] 2012-03-16 13:06:11 PDT
Jason, I am assuming not at this point.
Comment 42 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-20 12:23:45 PDT
Marking qa- based on comment 41. Will verify once we have a reproducible case.

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