Closed Bug 1112307 Opened 5 years ago Closed 5 years ago

WebSockets + e10s + workers use a non thread-safe ChannelEventQueue

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
e10s - ---
firefox34 --- unaffected
firefox35 --- disabled
firefox36 + fixed
firefox37 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

(Keywords: sec-high)

Attachments

(1 file)

Attached patch ws3.patchSplinter Review
We keep a reference of ChannelEventQueue to avoid that, during some callback, the webSocketChannelChild gets freed. But this queue is not thread-safe and we have a crash in e10s when this is used from workers.
Attachment #8537426 - Flags: review?(jduell.mcbugs)
Is this e10s only issue?
(In reply to Olli Pettay [:smaug] from comment #1)
> Is this e10s only issue?

Yes it is.
I'm not sure if e10s is enabled for Aurora, but it certainly is not enabled for Beta.
Comment on attachment 8537426 [details] [diff] [review]
ws3.patch

Review of attachment 8537426 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Andrea!
Attachment #8537426 - Flags: review?(jduell.mcbugs) → review+
Keywords: checkin-needed
This needs sec-approval before landing.
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Comment on attachment 8537426 [details] [diff] [review]
ws3.patch

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

e10s is not enabled by default in beta, but this issue is easy to reproduce.

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

Yes. We just have a thread-safe refcounter.

Which older supported branches are affected by this flaw?

n/a

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

n/a

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

n/a

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

No regressions.
Flags: needinfo?(amarchesini)
Attachment #8537426 - Flags: sec-approval?
Comment on attachment 8537426 [details] [diff] [review]
ws3.patch

sec-approval+ for trunk. Please make an Aurora patch as well and nominate it so we can get it there. We have a lot of folks using e10s in Aurora on the Web Developer edition.
Attachment #8537426 - Flags: sec-approval? → sec-approval+
> sec-approval+ for trunk. Please make an Aurora patch as well and nominate it
> so we can get it there. We have a lot of folks using e10s in Aurora on the
> Web Developer edition.

I guess the same patch works for aurora.
Comment on attachment 8537426 [details] [diff] [review]
ws3.patch

Approval Request Comment
[Feature/regressing bug #]: 504553
[User impact if declined]: a crash for e10s
[Describe test coverage new/current, TBPL]: none
[Risks and why]: this patch is extremely simple.
[String/UUID change made/needed]: none
Attachment #8537426 - Flags: approval-mozilla-aurora?
Attachment #8537426 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/8ef1de3364d7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.