Bug 1204269 (CVE-2015-7197)

WebSocket secure requirements can be bypassed in a worker

RESOLVED FIXED in Firefox 42

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

({sec-moderate})

unspecified
mozilla44
sec-moderate
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41- wontfix, firefox42+ fixed, firefox43+ fixed, firefox44+ fixed, firefox-esr3842+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main42+][adv-esr38.4+])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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().
(Assignee)

Comment 1

2 years ago
Note that this applies to all types of workers.

Comment 2

2 years ago
EntryGlobal() is a bit security hazard indeed :(
(Assignee)

Comment 3

2 years ago
(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.
(Assignee)

Comment 4

2 years ago
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)

Comment 5

2 years ago
Why not use the principal of the websocket in case of workers?

Comment 6

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

Comment 7

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

Updated

2 years ago
Attachment #8660365 - Flags: review?(bugs) → review-

Comment 8

2 years ago
In other words, as far as I see, we want to use principal here.
(Assignee)

Updated

2 years ago
See Also: → bug 1198078
(Assignee)

Comment 9

2 years ago
The principal of what?
Flags: needinfo?(bugs)
of the worker?
Flags: needinfo?(bugs)
(Assignee)

Comment 11

2 years ago
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)
(Assignee)

Comment 14

2 years ago
Created attachment 8660380 [details] [diff] [review]
Use the worker private in order to determine the origin of the entry settings object for workers
Attachment #8660365 - Attachment is obsolete: true
Attachment #8660380 - Flags: review?(bugs)
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: review?(bugs)
Attachment #8660380 - Flags: review+
Attachment #8660380 - Flags: feedback?(amarchesini)
Attachment #8660380 - Flags: feedback?(amarchesini) → feedback+
(Assignee)

Comment 16

2 years ago
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.
Attachment #8660380 - Flags: sec-approval?
Attachment #8660380 - Flags: approval-mozilla-esr38?
Attachment #8660380 - Flags: approval-mozilla-beta?
Attachment #8660380 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
tracking-firefox-esr38: --- → ?
Keywords: sec-high
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-

Updated

2 years ago
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+
(Assignee)

Comment 19

2 years ago
(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?
Flags: needinfo?(abillings)
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.
Flags: needinfo?(abillings)
(Assignee)

Comment 21

2 years ago
OK.
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: --- → +
(Assignee)

Comment 23

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

I can help with the uplifts as needed.
Attachment #8660380 - Flags: approval-mozilla-esr38?
Attachment #8660380 - Flags: approval-mozilla-esr38+
Attachment #8660380 - Flags: approval-mozilla-aurora?
Attachment #8660380 - Flags: approval-mozilla-aurora+
Keywords: checkin-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+
Group: network-core-security
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
Flags: needinfo?(ehsan)
Keywords: checkin-needed
(Assignee)

Comment 27

2 years ago
(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?
Flags: needinfo?(ehsan)
(Assignee)

Comment 28

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56d2bde2883
Flags: in-testsuite?
Whiteboard: [checkin on 10/6]
(Assignee)

Comment 29

2 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/512e9862f1a0
https://hg.mozilla.org/releases/mozilla-beta/rev/7827b7deded9
status-firefox42: affected → fixed
status-firefox43: affected → fixed
status-firefox44: affected → fixed
(Assignee)

Comment 30

2 years ago
https://hg.mozilla.org/releases/mozilla-esr38/rev/467ce7a8340d
status-firefox-esr38: affected → fixed
(Assignee)

Comment 31

2 years ago
Should this land on some b2g branches too?
Flags: needinfo?(abillings)
Ehsan, I don't know. B2G isn't my call.
Flags: needinfo?(abillings)
(Assignee)

Comment 33

2 years ago
Fabrice, can you help find the right person to figure out if we want to take this on b2g?
Flags: needinfo?(fabrice)
Adding some of the B2G security people to see if they can help.
Flags: needinfo?(ptheriault)
Flags: needinfo?(cr)
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.
Flags: needinfo?(ptheriault)
Flags: needinfo?(fabrice)
Flags: needinfo?(cr)
https://hg.mozilla.org/mozilla-central/rev/c56d2bde2883
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]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+][adv-esr38.4+]
Alias: CVE-2015-7197
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
(Assignee)

Comment 39

2 years ago
Now that the fix for this has been shipped, can I land the test?
Flags: needinfo?(dveditz)
Yes, please land the tests.
Flags: needinfo?(dveditz)
(Assignee)

Comment 41

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d691196ace
Flags: in-testsuite? → in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.