Closed Bug 1151609 Opened 5 years ago Closed 5 years ago

Wrong use of WebSocketImpl::CloseConnection


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

Not set



Tracking Status
firefox37 --- unaffected
firefox38 + fixed
firefox39 + fixed
firefox40 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- fixed


(Reporter: baku, Assigned: baku)


(Keywords: sec-critical)


(1 file, 1 obsolete file)

Attached patch 001_closeConnection.patch (obsolete) — Splinter Review
Checking if we can reduce the number of sync runnable in WebSocketImpl, I found this issue. We call CloseConnection also from the non-target thread. For instance in the ::Observe() and in the ::Initialization().
Attachment #8588750 - Flags: review?(bugs)
Comment on attachment 8588750 [details] [diff] [review]

Could you move the calls to be outside the method.
Makes the code easier to read, IMO.

We need this on branches too.
Attachment #8588750 - Flags: review?(bugs) → review+
Keywords: sec-critical
Group: core-security
[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I tried but I've never succeeded. But still I guess it's possible.

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

Yes. CloseConnection must be executed in the target thread.

Which older supported branches are affected by this flaw?

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

from beta to m-i. and b2g v3.

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

Easy to backport in case it's needed.

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

It's green on try. I don't see why it should introduce regressions.
Attachment #8588750 - Attachment is obsolete: true
Attachment #8589653 - Flags: sec-approval?
Comment on attachment 8589653 [details] [diff] [review]

Sec-approval+ for trunk. We should get Aurora and Beta nominations as well so we can avoid shipping this issue.
Attachment #8589653 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Please get this nominated for Aurora/Beta ASAP so we can hopefully get it landed in time for 38b3.
Flags: needinfo?(amarchesini)
Comment on attachment 8589653 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: bug 504553
[User impact if declined]: possible a crash.
[Describe test coverage new/current, TreeHerder]: Hard to reproduce. I tried but I'm not able to produce a valid mochitest.
[Risks and why]: none
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8589653 - Flags: approval-mozilla-beta?
Attachment #8589653 - Flags: approval-mozilla-aurora?
Comment on attachment 8589653 [details] [diff] [review]

Should be in 38 beta 3.
Attachment #8589653 - Flags: approval-mozilla-beta?
Attachment #8589653 - Flags: approval-mozilla-beta+
Attachment #8589653 - Flags: approval-mozilla-aurora?
Attachment #8589653 - Flags: approval-mozilla-aurora+
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.