Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Description • 11 years ago

ejpbruel discovered that we're not actually tracing the worker global's JSObject.  Turns out that PreserveWrapper() is required, not just HoldJSObjects.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 11 years ago

Why HoldJSObjects isn't enough?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 2 • 11 years ago

Because we don't trace the wrapper unless it's preserved.  http://mxr.mozilla.org/mozilla-central/source/dom/base/nsWrapperCache.h#172

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 3 • 11 years ago

Fwiw, I haven't actually been able to get this to happen in practice.  Any event listener or timeout will entrain the global cyclically.  So while this is sec-critical in theory I'm not sure how one would go about exploiting anything.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 4 • 11 years ago

Comment on attachment 8379176 [details] [diff] [review]
Patch

I'm not the right reviewer for this.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 11 years ago

Please update the comment in http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerScope.cpp#45

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 11 years ago

Comment on attachment 8379176 [details] [diff] [review]
Patch

Actually, still trying to understand this (since I'm not familiar with worker global stuff).

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 7 • 11 years ago

(In reply to Olli Pettay [:smaug] from comment #5)
> Please update the comment in
> http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerScope.cpp#45

Actually I think that could go away entirely.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 11 years ago

Comment on attachment 8379176 [details] [diff] [review]
Patch

Could we assert in CreateGlobal that UnwrapDOMObjectToISupports(global)
is non-null. (Just because TryPreserveWrapper has odd behavior.)

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 9 • 11 years ago

Do I need to request sec-approval here or can we go straight to branch approvals?

	Andrew McCreight [:mccr8]
	Comment 10 • 11 years ago

This affects more than trunk, and is sec-high or sec-critical, so it needs sec-approval.  Al can probably give you branch approvals at the same time.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 11 • 11 years ago

Comment on attachment 8379176 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Unclear.  My attempts to produce a crash were unsuccessful.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes.  I think it's reasonably obvious what is going on.

Which older supported branches are affected by this flaw?

Beta is as far back as it goes.

If not all supported branches, which bug introduced the flaw?

Bug 936327.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backports are trivial.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely to cause regressions.

	Al Billings [:abillings - ex-MoCo]
	Comment 12 • 11 years ago

Comment on attachment 8379176 [details] [diff] [review]
Patch

sec-approval+ for trunk. Please backport to Beta and Aurora and nominate those patches so we can get them in following mozilla-central.

	Andrew McCreight [:mccr8]
	Comment 13 • 11 years ago

Is this ready to land, Kyle?  Thanks.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 14 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/36b02f8ee773

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/36b02f8ee773

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

Please nominate this for Aurora/Beta uplift when ready :)

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 17 • 11 years ago

Oh, is that a separate step?  Lovely.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 18 • 11 years ago

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 936327
User impact if declined: Potentially an sg:crit, although we don't know how to exploit it.
Testing completed (on m-c, etc.): Baked on m-c for a few days, simple enough to verify via code inspection.
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch:None

	Al Billings [:abillings - ex-MoCo]
	Comment 19 • 11 years ago

Comment on attachment 8383934 [details] [diff] [review]
As landed

Yes, each branch is its own thing, especially for security bugs. Approved for beta and aurora.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d0bf1e91ffd
https://hg.mozilla.org/releases/mozilla-beta/rev/1d3379354e09

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/1d3379354e09