1202844 - crash in nsXBLService::GetBinding

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

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

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

Comment 1

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

Kairo, does the crash happen often also on Nightlies?

Comment 2

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

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

Comment 3

[Boris Zbarsky [: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

Comment 4

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

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.

Comment 5

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

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 <[e-mail]>" -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.

Comment 6

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

How bad is the crash here? We need a security rating on this before we can check in.

Comment 7

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

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.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:

Comment 9

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

Comment on attachment 8658381 [details] [diff] [review]
pure guess fix

sec-approval+ for trunk.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8658381 [details] [diff] [review]
pure guess fix

sec-critical fix, taking it, Beta41+, Aurora42+

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b451aaf236d0

Comment 12

[Robert Kaiser]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b69bd5eaad2a

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b451aaf236d0

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb011b9221d6

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/9665a2406b3c

Comment 18

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

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.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Based on comment 17, this was fixed in esr38.3.0. Updating the tracking flag to reflect that.