1204269 - (CVE-2015-7197) WebSocket secure requirements can be bypassed in a worker

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[(no longer active)]

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().

Comment 1

[(no longer active)]

Note that this applies to all types of workers.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

EntryGlobal() is a bit security hazard indeed :(

Comment 3

[(no longer active)]

(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.

Comment 4

[(no longer active)]

Attachment: Use the worker private in order to determine the origin of the entry settings object for workers

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Why not use the principal of the websocket in case of workers?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Hmm, maybe that would be wrong?
/me tries to find what the entry settings object's scheme should be on workers.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

GetBaseURI() may return a data: url, right?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

In other words, as far as I see, we want to use principal here.

Comment 9

[(no longer active)]

The principal of what?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

of the worker?

Comment 11

[(no longer active)]

OK, that makes sense!  Comment 5 confused me.  :-)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[(no longer active)]

Attachment: Use the worker private in order to determine the origin of the entry settings object for workers

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 16

[(no longer active)]

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.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for checkin on October 6 (two weeks into the next cycle) since we're about to ship 41.

Comment 19

[(no longer active)]

(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?

Comment 20

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 21

[(no longer active)]

OK.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 23

[(no longer active)]

Thanks!  I have this on my calendar, but a reminder would be appreciated.  :-)

I can help with the uplifts as needed.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 26

[Carsten Book [:Tomcat]]

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

Comment 27

[(no longer active)]

(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?

Comment 28

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c56d2bde2883

Comment 29

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/512e9862f1a0
https://hg.mozilla.org/releases/mozilla-beta/rev/7827b7deded9

Comment 30

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/467ce7a8340d

Comment 31

[(no longer active)]

Should this land on some b2g branches too?

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Ehsan, I don't know. B2G isn't my call.

Comment 33

[(no longer active)]

Fabrice, can you help find the right person to figure out if we want to take this on b2g?

Comment 34

[Al Billings [:abillings - ex-MoCo]]

Adding some of the B2G security people to see if they can help.

Comment 35

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 36

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c56d2bde2883

Comment 38

[Daniel Veditz [:dveditz]]

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.

Comment 39

[(no longer active)]

Now that the fix for this has been shipped, can I land the test?

Comment 40

[Daniel Veditz [:dveditz]]

Yes, please land the tests.

Comment 41

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d691196ace

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0d691196ace