crash in nsXBLService::GetBinding

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({csectype-uaf, sec-critical})

36 Branch
mozilla43
csectype-uaf, sec-critical
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41+ fixed, firefox42+ fixed, firefox43+ fixed, firefox-esr3841+ fixed)

Details

(Whiteboard: [adv-main41+][adv-esr38.3+][post-critsmash-triage])

Attachments

(1 attachment)

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1197321#c5
I think baseBindingURI (which is nIURI*) is deleted object in nsXBLService::GetBinding.
If we get nested GetBinding calls, LoadBindingDocumentInfo may spin event loop, as far as I see, and that may do, well, whatever.

See Bug 1197321
Kairo, does the crash happen often also on Nightlies?
Flags: needinfo?(kairo)
Created attachment 8658381 [details] [diff] [review]
pure guess fix

protoBinding = docInfo->GetPrototypeBinding(ref); is a bit annoying, but
protobindings aren't refcounted, so in theory the object might be dead.

I don't understand how this all might happen though. Starting to shutdown while we're still in process to start FF perhaps?
Attachment #8658381 - Flags: review?(bzbarsky)
Comment on attachment 8658381 [details] [diff] [review]
pure guess fix

> protoBinding = docInfo->GetPrototypeBinding(ref); is a bit annoying, but
> protobindings aren't refcounted, so in theory the object might be dead.

This is worth a comment...

r=me
Attachment #8658381 - Flags: review?(bzbarsky) → review+
Well, I explicitly didn't want to add a comment at this point - to not point so explicitly to the problem.
I'll ni? myself to add a comment some time later.
Flags: needinfo?(bugs)
Comment on attachment 8658381 [details] [diff] [review]
pure guess fix

Approval Request Comment
[Feature/regressing bug #]: This is _old_ stuff.
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: NA. This is speculative fix.
[Risks and why]: Should be safe, given that we ensure keeping certain objects alive
[String/UUID change made/needed]: NA

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I haven't figured out how to crash here. The fix is speculative based on the stack traces and code.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comment could be (indicating what the guess fix probably is about):
-u "Olli Pettay <Olli.Pettay@helsinki.fi>" -m "Bug 1202844, make nsXBLService::GetBinding deal with shutting down during binding loading, r=bz"
	
Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply easily to all branches. Just tested esr38 and applies there without any fuzz.

How likely is this patch to cause regressions; how much testing does it need?
Should be safe.
Attachment #8658381 - Flags: sec-approval?
Attachment #8658381 - Flags: approval-mozilla-esr38?
Attachment #8658381 - Flags: approval-mozilla-beta?
Attachment #8658381 - Flags: approval-mozilla-aurora?
How bad is the crash here? We need a security rating on this before we can check in.
This is about Bug 119732, which seems to be a topcrasher.
The crash is, based on crash-stat, about accessing deleted object. By default that would be sec-critical, but I have no idea how to trigger this crash.
Flags: needinfo?(bugs)
Blocks: 1197321
Keywords: csectype-uaf, sec-critical
[Tracking Requested - why for this release]:
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox41: --- → ?
tracking-firefox42: --- → +
tracking-firefox43: --- → +
tracking-firefox-esr38: --- → ?
Comment on attachment 8658381 [details] [diff] [review]
pure guess fix

sec-approval+ for trunk.
Attachment #8658381 - Flags: sec-approval? → sec-approval+
Comment on attachment 8658381 [details] [diff] [review]
pure guess fix

sec-critical fix, taking it, Beta41+, Aurora42+
Attachment #8658381 - Flags: approval-mozilla-beta?
Attachment #8658381 - Flags: approval-mozilla-beta+
Attachment #8658381 - Flags: approval-mozilla-aurora?
Attachment #8658381 - Flags: approval-mozilla-aurora+

Updated

3 years ago
tracking-firefox41: ? → +

Comment 12

3 years ago
(In reply to Olli Pettay [:smaug] from comment #1)
> Kairo, does the crash happen often also on Nightlies?

Not in a high frequency. I see 9 crashes on Firefox 43.0a1 in the last week when I look at the link in Crash Signatures of the other bug and expand the (huge) list under "Products" in the Signature Summary.
Flags: needinfo?(kairo)
https://hg.mozilla.org/mozilla-central/rev/b451aaf236d0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
https://hg.mozilla.org/releases/mozilla-aurora/rev/bb011b9221d6
status-firefox42: affected → fixed
Comment on attachment 8658381 [details] [diff] [review]
pure guess fix

sec-critical fix so let's take it for esr too. 
Though "pure guess fix" sounds a bit unnerving.
Attachment #8658381 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Group: core-security → core-security-release
I'm seeing another ::GetBinding crash on crash-stats/Nightly. Not about url object being bogus, but
baseBindingURI = baseProto->BindingURI();
So, apparently baseProto isn't valid, but is it protobinding or baseProto.
dmajor was very right https://bugzilla.mozilla.org/show_bug.cgi?id=1197321#c8

Investigating and I'll file a followup.
Flags: needinfo?(bugs)
Whiteboard: [adv-main41+][adv-esr38.3+]
Whiteboard: [adv-main41+][adv-esr38.3+] → [adv-main41+][adv-esr38.3+][post-critsmash-triage]
Based on comment 17, this was fixed in esr38.3.0. Updating the tracking flag to reflect that.
tracking-firefox-esr38: ? → 41+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.