5.99 KB, patch
|Details | Diff | Splinter Review|
See https://alice.csrf.jp/sw/websocket/. What is happening is that GetEntryGlobal() in <https://dxr.mozilla.org/mozilla-central/rev/9771bd5e56edd07e21cf008dcadc20d7ed970ce3/dom/base/WebSocket.cpp#1551> is returning null, and our stack looks like this: * frame #0: 0x00000001030fcab1 XUL`mozilla::dom::ScriptSettingsStack::EntryGlobal() + 17 at ScriptSettings.cpp:72 frame #1: 0x00000001030cdeb9 XUL`mozilla::dom::GetEntryGlobal() + 9 at ScriptSettings.cpp:188 frame #2: 0x00000001030eae27 XUL`mozilla::dom::WebSocketImpl::Init(this=0x00000001300c3280, aCx=0x0000000000000000, aPrincipal=0x000000011e50a340, aURL=0x0000000130804720, aProtocolArray=0x0000000130804188, aScriptFile=0x000000012ad70de0, aScriptLine=5, aScriptColumn=13, aRv=0x00000001308046e8, aConnectionFailed=0x0000000130804137) + 2839 at WebSocket.cpp:1551 frame #3: 0x00000001030fa4aa XUL`mozilla::dom::(anonymous namespace)::InitRunnable::InitWindowless(this=0x000000012ad70da0, aTopLevelWorkerPrivate=0x000000011e997600) + 330 at WebSocket.cpp:1093 frame #4: 0x00000001030fa08d XUL`mozilla::dom::(anonymous namespace)::WebSocketMainThreadRunnable::MainThreadRun(this=0x000000012ad70da0) + 157 at WebSocket.cpp:1029 frame #5: 0x0000000104eb87d3 XUL`mozilla::dom::workers::WorkerMainThreadRunnable::Run(this=0x000000012ad70da0) + 35 at WorkerRunnable.cpp:584 frame #6: 0x00000001016f00a3 XUL`nsThread::ProcessNextEvent(this=0x000000010063d800, aMayWait=false, aResult=0x00007fff5fbfcee3) + 1795 at nsThread.cpp:874 frame #7: 0x000000010176c331 XUL`NS_ProcessPendingEvents(aThread=0x000000010063d800, aTimeout=10) + 145 at nsThreadUtils.cpp:219 frame #8: 0x00000001051f2f9e XUL`nsBaseAppShell::NativeEventCallback(this=0x00000001007b74c0) + 190 at nsBaseAppShell.cpp:97 frame #9: 0x000000010528189a XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x00000001007b74c0) + 442 at nsAppShell.mm:387 frame #10: 0x00007fff8a719a01 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 frame #11: 0x00007fff8a70bb8d CoreFoundation`__CFRunLoopDoSources0 + 269 frame #12: 0x00007fff8a70b1bf CoreFoundation`__CFRunLoopRun + 927 frame #13: 0x00007fff8a70abd8 CoreFoundation`CFRunLoopRunSpecific + 296 frame #14: 0x00007fff8d0bd56f HIToolbox`RunCurrentEventLoopInMode + 235 frame #15: 0x00007fff8d0bd2ea HIToolbox`ReceiveNextEventCommon + 431 frame #16: 0x00007fff8d0bd12b HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71 frame #17: 0x00007fff941608ab AppKit`_DPSNextEvent + 978 frame #18: 0x00007fff9415fe58 AppKit`-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 346 frame #19: 0x0000000105280404 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x00000001007b7560, _cmd=0x00007fff94a7c7f8, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x00007fff79fe1040, flag='\x01') + 116 at nsAppShell.mm:121 frame #20: 0x00007fff94155af3 AppKit`-[NSApplication run] + 594 frame #21: 0x000000010528226c XUL`nsAppShell::Run(this=0x00000001007b74c0) + 172 at nsAppShell.mm:661 frame #22: 0x000000010626976b XUL`nsAppStartup::Run(this=0x0000000119732be0) + 155 at nsAppStartup.cpp:281 frame #23: 0x000000010633c991 XUL`XREMain::XRE_mainRun(this=0x00007fff5fbfee80) + 5841 at nsAppRunner.cpp:4292 frame #24: 0x000000010633d247 XUL`XREMain::XRE_main(this=0x00007fff5fbfee80, argc=5, argv=0x00007fff5fbff788, aAppData=0x00007fff5fbff108) + 935 at nsAppRunner.cpp:4389 frame #25: 0x000000010633d6c2 XUL`XRE_main(argc=5, argv=0x00007fff5fbff788, aAppData=0x00007fff5fbff108, aFlags=0) + 98 at nsAppRunner.cpp:4478 frame #26: 0x0000000100001c48 firefox`do_main(argc=5, argv=0x00007fff5fbff788, xreDirectory=0x0000000100632ac0) + 1768 at nsBrowserApp.cpp:212 frame #27: 0x000000010000108d firefox`main(argc=5, argv=0x00007fff5fbff788) + 285 at nsBrowserApp.cpp:399 frame #28: 0x0000000100000b54 firefox`start + 52 I think we need to compute originIsHttps in the constructor of WebSocket and pass it down to WebSocketImpl::Init().
Note that this applies to all types of workers.
EntryGlobal() is a bit security hazard indeed :(
(In reply to Ehsan Akhgari (don't ask for review please) from comment #0) > I think we need to compute originIsHttps in the constructor of WebSocket and > pass it down to WebSocketImpl::Init(). Except that some of these objects are main-thread only... What we can do instead is to look at the base url of the worker.
Created attachment 8660365 [details] [diff] [review] Use the worker private in order to determine the origin of the entry settings object for workers
Attachment #8660365 - Flags: review?(bugs)
Why not use the principal of the websocket in case of workers?
Hmm, maybe that would be wrong? /me tries to find what the entry settings object's scheme should be on workers.
GetBaseURI() may return a data: url, right?
In other words, as far as I see, we want to use principal here.
The principal of what?
of the worker?
OK, that makes sense! Comment 5 confused me. :-)
Yeah, sorry about that. I wasn't sure at that point what the spec says about loading the main script. (it is still actually a bit unclear to me what should happen in case of a redirect)
Ah, that wouldn't affect to the environment settings object https://html.spec.whatwg.org/multipage/workers.html#set-up-a-worker-environment-settings-object
Created attachment 8660380 [details] [diff] [review] Use the worker private in order to determine the origin of the entry settings object for workers
Comment on attachment 8660380 [details] [diff] [review] Use the worker private in order to determine the origin of the entry settings object for workers (I'd prefer if baku would take a look at too, so asking feedback.) We need on branches too, I guess.
Attachment #8660380 - Flags: feedback?(amarchesini) → feedback+
Comment on attachment 8660380 [details] [diff] [review] Use the worker private in order to determine the origin of the entry settings object for workers [Security approval request comment] How easily could an exploit be constructed based on the patch? Assuming that we land without tests, it's not too hard to determine what the patch is fixing. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The code changes should be self explanatory. The only thing that is on our side for hiding this is that the semantics of GetEntryGlobal() and how it interacts with delivering the async message to the main thread may not be obvious. Which older supported branches are affected by this flaw? 38 and above. If not all supported branches, which bug introduced the flaw? Bug 504553. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The backports should be easy. How likely is this patch to cause regressions; how much testing does it need? See below, I don't think it's too much prone to regressions. Approval Request Comment [Feature/regressing bug #]: As far as I can tell, this was exposed in bug 504553 when we exposed WebSocket to workers, so it affects 38 and above. [User impact if declined]: Bypassing mixed content WebSocket policy. [Describe test coverage new/current, TreeHerder]: Has a test. [Risks and why]: The patch itself is pretty small and well understood, so I'm not worried about regressions much. [String/UUID change made/needed]: None.
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
tracking-firefox-esr38: --- → ?
Comment on attachment 8660380 [details] [diff] [review] Use the worker private in order to determine the origin of the entry settings object for workers Taking any patches at this point in RC is very risky. The criteria is limited to critical sec issues that are easily exploitable, issues impacting a significant set of end-users, major regressions during beta cycle, crash/hang up spikes, etc. This bug does not meet that criteria. It is too late to take a fix in 41 for this. DVeditz also agrees.
Attachment #8660380 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox41: affected → wontfix
tracking-firefox41: ? → -
Sec-approval+ for checkin on October 6 (two weeks into the next cycle) since we're about to ship 41.
tracking-firefox42: ? → +
tracking-firefox43: ? → +
tracking-firefox-esr38: ? → 42+
Whiteboard: [checkin on 10/6]
Attachment #8660380 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #18) > Sec-approval+ for checkin on October 6 (two weeks into the next cycle) since > we're about to ship 41. Is someone going to poke this bug when it's time? Or am I expected to remember to check it in on that date?
No one is going to poke it. If I notice it hasn't gone in, I'll ask but I figure since it is assigned to you, you'll hopefully set a reminder in calendar or something. We don't have a good system for that. This came in a couple of weeks too late to make the current release though.
Setting 44 tracking flag. Ehsan, in theory one of the release managers will give this a poke since it's tracked across all the current branches. We will need to reset the approval request flags or straighten out which patch goes with which branch at that point.
status-firefox44: --- → affected
tracking-firefox44: --- → +
Thanks! I have this on my calendar, but a reminder would be appreciated. :-) I can help with the uplifts as needed.
Comment on attachment 8660380 [details] [diff] [review] Use the worker private in order to determine the origin of the entry settings object for workers Resetting the beta flag. I denied this uplift during Beta41 timeframe. This time the question is whether to land it in Beta42.
Attachment #8660380 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
Comment on attachment 8660380 [details] [diff] [review] Use the worker private in order to determine the origin of the entry settings object for workers Approvals given.
Attachment #8660380 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi, this does not apply cleanly: adding Bug-1204269---Use-the-worker-private-in-order-to-d.patch to series file applying Bug-1204269---Use-the-worker-private-in-order-to-d.patch patching file dom/workers/test/mochitest.ini Hunk #1 FAILED at 107 1 out of 2 hunks FAILED -- saving rejects to file dom/workers/test/mochitest.ini.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh Bug-1204269---Use-the-worker-private-in-order-to-d.patch
(In reply to Carsten Book [:Tomcat] from comment #26) > Hi, this does not apply cleanly: > > adding Bug-1204269---Use-the-worker-private-in-order-to-d.patch to series > file > applying Bug-1204269---Use-the-worker-private-in-order-to-d.patch > patching file dom/workers/test/mochitest.ini > Hunk #1 FAILED at 107 > 1 out of 2 hunks FAILED -- saving rejects to file > dom/workers/test/mochitest.ini.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working directory > errors during apply, please fix and refresh > Bug-1204269---Use-the-worker-private-in-order-to-d.patch On what branch?
Whiteboard: [checkin on 10/6]
status-firefox42: affected → fixed
status-firefox43: affected → fixed
status-firefox44: affected → fixed
status-firefox-esr38: affected → fixed
Should this land on some b2g branches too?
Ehsan, I don't know. B2G isn't my call.
Fabrice, can you help find the right person to figure out if we want to take this on b2g?
Adding some of the B2G security people to see if they can help.
I dont think its needed. B2G-2.2 is Gecko37 (which seems unaffected) and B2G2.5 is not yet branched. As far as I can tell, websocket in workers landed disabled in 35, and was enabled in 38. I wonder if it was enabled on b2g at the same time - I assume so? If 2.2 IS affected, we should backport this, since we have devices in the pipeline for next year which will be based on 2.2 and would take this fix. But if we can just confirm that websockets in workers was not available in b2g-37, we dont need to backport.
Looks like they were disabled in B2G 2.2 so no backport is required: http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/b48f0cd1bc45/modules/libpref/init/all.js#l137
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: dom-core-security, network-core-security → core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+][adv-esr38.4+]
sec-high overstates the risk. This can't be used to compromise Firefox, or the user directly. The spec rule is in place to keep web app developers from designing insecure apps. Despite our flaw they might follow the spec anyway (they want it to work in other browsers), and even if they don't the risk depends on what information they transmit in the clear that should be private. "moderate" is a better rating.
Keywords: sec-high → sec-moderate
Now that the fix for this has been shipped, can I land the test?
Yes, please land the tests.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.