Closed Bug 1151609 Opened 5 years ago Closed 5 years ago
Wrong use of Web
Socket Impl::Close Connection
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] 001_closeConnection.patch 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+
[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.
Comment on attachment 8589653 [details] [diff] [review] 001_closeConnection.patch 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+
Please get this nominated for Aurora/Beta ASAP so we can hopefully get it landed in time for 38b3.
Comment on attachment 8589653 [details] [diff] [review] 001_closeConnection.patch 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
Comment on attachment 8589653 [details] [diff] [review] 001_closeConnection.patch Should be in 38 beta 3.
You need to log in before you can comment on or make changes to this bug.