nsXBLService::GetBinding is still crashing

RESOLVED FIXED in Firefox 42



4 years ago
3 years ago


(Reporter: smaug, Assigned: smaug)


({csectype-uaf, sec-critical})

36 Branch
Dependency tree / graph

Firefox Tracking Flags

(firefox41- wontfix, firefox42+ fixed, firefox43+ fixed, firefox44+ fixed, firefox-esr3842+ fixed)


(Whiteboard: [post-critsmash-triage][adv-main42+][adv-esr38.4+])


(1 attachment)

Group: core-security → dom-core-security


4 years ago
Blocks: 1197321

Comment 1

4 years ago
Posted patch v1Splinter Review
I think we can have deleted baseProto if it is from some different XBL document 
and we've cleared protocache or something.
We should keep all the relevant XBLDocumentInfos alive (strong mBaseXBLDocInfo in nsXBLPrototypeBinding?), but that is _a_lot_ more riskier fix than this one
which is basically just trying to make null checks always valid by using WeakPtr and not foo*

The change to WeakPtr is to make it possible to construct/assign it with nullptr.
The current setup there is totally broken and I'm surprised we haven't seen crashes in code using WeakPtr.

-    protoBinding = docInfo->GetPrototypeBinding(ref);
is about backing out https://hg.mozilla.org/releases/mozilla-esr38/rev/9665a2406b3c#l1.82
(Bug 1202844) since by using WeakPtr it shouldn't be needed anymore.
Attachment #8663636 - Flags: review?(bzbarsky)
Comment on attachment 8663636 [details] [diff] [review]

r=me, but you may want to get review from whoever owns WeakPtr.
Attachment #8663636 - Flags: review?(bzbarsky) → review+

Comment 3

4 years ago
Comment on attachment 8663636 [details] [diff] [review]

Waldo, review request for the WeakPtr change
Attachment #8663636 - Flags: review?(jwalden+bmo)
Comment on attachment 8663636 [details] [diff] [review]

Review of attachment 8663636 [details] [diff] [review]:

::: mfbt/WeakPtr.h
@@ +173,5 @@
>    WeakPtr& operator=(T* aOther)
>    {
> +    if (aOther) {
> +      *this = aOther->SelfReferencingWeakPtr();
> +    } else if (!mRef || mRef->get()) {

Unless I'm misreading, I don't believe it's ever the case that !mRef.  WeakPtr is constructed with this field allocated (and we're presuming it's infallible, bleh).  It's only ever assigned a |new| expression in another place, another WeakPtr::mRef that's non-null inductively, and |explicit WeakPtr(const RefPtr<WeakReference>& aOther) : mRef(aOther) {}| looks completely unused to me.

So remove the !mRef here, and please remove that extra, unused constructor while you're at it.
Attachment #8663636 - Flags: review?(jwalden+bmo) → review+

Comment 6

4 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Unless I'm misreading, I don't believe it's ever the case that !mRef. 
There is, when the method is called from ctor(T*)

Comment 7

4 years ago
(and would like to keep the changes minimum since this needs to land on branches, so no random code removals)
(In reply to Olli Pettay [:smaug] from comment #6)
> There is, when the method is called from ctor(T*)

Oh, bah.  Okay.  On trunk at some point, I think I'd rather see the constructors initializing members and not having not-initialized cases to deal with in non-constructor methods.  But for branch this is fine.  (As is minimizing changes, of course.)

Comment 9

4 years ago
Comment on attachment 8663636 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: This is _old_ stuff.
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: NA. This is _still_ speculative fix. No one has managed to reproduce the crash. But after bug 1202844 it became more clear what the issue could be.
[Risks and why]: Should be safe, given that we make raw pointers to weak pointers. So, no touching to deleted objects anymore.
[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 one part of the fix):
-u "Olli Pettay <Olli.Pettay@helsinki.fi>" -m "Bug 1204669 optimize out hashtable lookups caused by extra GetPrototypeBinding call, r=bz,waldo"
Which older supported branches are affected by this flaw?

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 #8663636 - Flags: sec-approval?
Attachment #8663636 - Flags: approval-mozilla-esr38?
Attachment #8663636 - Flags: approval-mozilla-beta?
Attachment #8663636 - Flags: approval-mozilla-aurora?
Comment on attachment 8663636 [details] [diff] [review]

Approvals given.
Attachment #8663636 - Flags: sec-approval?
Attachment #8663636 - Flags: sec-approval+
Attachment #8663636 - Flags: approval-mozilla-esr38?
Attachment #8663636 - Flags: approval-mozilla-esr38+
Attachment #8663636 - Flags: approval-mozilla-beta?
Attachment #8663636 - Flags: approval-mozilla-beta+
Attachment #8663636 - Flags: approval-mozilla-aurora?
Attachment #8663636 - Flags: approval-mozilla-aurora+
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: dom-core-security → core-security-release
Given that this is a speculative fix and not an easy exploit, I don't feel the need to take this fix in 41, is now a wontfix.

Comment 16

4 years ago
So far I haven't found any 42.0b2 crashes, but there were crashes still in b1. The patch should be in b2. So hopefully this ancient bug (see also Bug 720991) is now fixed. But I'll keep looking at the crash stats.
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+][adv-esr38.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.