1252751 - <iframe sandbox srcdoc> can bypass mixed content restrictions

Status: RESOLVED FIXED

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

Description

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

Websockets initiated from pages secure origins (https) must also be secured by TLS (ie. they must have a scheme of wss:)

This restriction can be bypassed by embedding an sandboxed iframe and setting the srcdoc of this iframe to load the websocket on the parent page's behalf.

I'm not certain that this is actually a bug though. Is the origin of a sandboxed iframe considered different to the parent frame and therefore not be bound by the parent frame's restriction? Or should mixed content blocking take precedence and the insecure websocket connection should be blocked.

Comment 1

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

Attachment: index.html

When loaded on a TLS enabled site, this page can connect to a websocket server on the local network in Firefox 44.

FWIW its blocked with a mixed content error in Chrome 48.

Comment 2

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

Hmm bug 1204269 was rated as sec-moderate and it sounds like a similar thing. Not sure I agree with that rating, but perhaps I'm missing the risk here?

Comment 3

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

Attachment: testingXHRMixedContent.html

Same problem exists with XHR and sandbox iframe. XHR from https to http _should_ be blocked due to mixed content blocker, but iframe sandbox bypasses this restriction. Seems to be something with sandbox I tested with srcdoc, src=data:,foo and src=https://foo and all were able to make requests to http, even though the top level resource was https.

Comment 4

[Daniel Veditz [:dveditz]]

I think this is a real problem, and "sec-other" hides it. Clearing so we can make another triage pass at it.

Comment 5

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

See also bug 1073952

Comment 6

[Andrew Overholt [:overholt]]

Steve, wdyt?

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Tanvi? Chris? Any ideas about how to fix this one?

Comment 8

[Tanvi Vyas[:tanvi]]

I won't be able to look into this until the week after next.  Chris, if you have time, please take a look.  If it is a sec-low, it can wait.

Comment 9

[Anne (:annevk)]

I suspect someone from DOM needs to look at this, e.g., :baku or :smaug. It seems more likely that DOM doesn't pass the correct data to network than the other way around.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Anne (:annevk) from comment #9)
> I suspect someone from DOM needs to look at this, e.g., :baku or :smaug. It
> seems more likely that DOM doesn't pass the correct data to network than the
> other way around.

The MixedContentBlocker implementation accepts TYPE_WEBSOCKET [1] because the WebSocket API should already block such loads [2]. Now, whether we should update the mixed content implementation to block such loads is debatable. Anyway, the true reason why we are not blocking the load happens within Websocket.cpp [2]. The problem is that the |originURI| is "moz-nullprincipal:{e217e9cd-2c29-4677-af3a-452178f6c56f}" when running the testcase provided in Comment 0, hence originisHttps is false and we don't bail out with an error here [3].

Baku, I suppose we should rather use the originDoc's URI in that case, right?


[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#462
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/WebSocket.cpp#1580
[3] http://mxr.mozilla.org/mozilla-central/source/dom/base/WebSocket.cpp#1608

Comment 11

[Anne (:annevk)]

For the specific WebSocket issue, yes, we should move the mixed content blocking away from the API into the network layer. See https://w3c.github.io/webappsec-mixed-content/#websockets-integration for details. I plan to reconcile WebSocket and Fetch in https://github.com/whatwg/fetch/issues/235 so they will more clearly share the same processing model.

Comment 12

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

It is still not clear to me what the spec says about this case.
"If secure is false but the origin specified by the entry settings object has a scheme component that is itself a secure protocol" sure, but what makes the sandboxed iframe to have "secure protocol"

Comment 13

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

...in case we're using srcdoc.

Comment 14

[Anne (:annevk)]

Olli, please read https://w3c.github.io/webappsec-mixed-content/#websockets-integration for the proper security check. The WebSocket API in HTML is a little unreliable at the moment. I'm fixing this with https://github.com/whatwg/fetch/pull/236 which rewrites part of the WebSocket protocol in terms of Fetch and then I'll create a separate PR against HTML which will make the WebSocket API hook into that. Then it should no longer have such naive Mixed Content checks.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Here a proposed fix: I get the principal from the parent window in case the current one is a null principal.
Smaug also asked this question: "what leads the iframe to have secure protocol per spec?" but I still don't have a valid answer.

Comment 16

[Anne (:annevk)]

Per https://w3c.github.io/webappsec-mixed-content/#categorize-settings-object mixed content uses "HTTPS state", not "secure protocol". As I said, what the HTML Standard says about the WebSocket API is wrong temporarily. I'm working on fixing it. (I think jwatt is implementing "HTTPS state" as part of secure contexts.)

Comment 17

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

Comment on attachment 8727809 [details] [diff] [review]
ws2.patch

What if an <iframe sandbox srcdoc> has another <iframe sandbox srcdoc> inside it?
You need to iterate higher up in the browsing context tree.


What if sandboxed iframe has 'allow-popups allow-scripts' and calls window.open("about:blank", ...) and then uses that window's WebSocket?
Don't you need to check the opener and if opener is has also null principal, iterate higher up?
(I didn't test this case)

Comment 18

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Test included

Comment 19

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

Comment on attachment 8727877 [details] [diff] [review]
ws2.patch

So nothing guarantees we don't end up to endless loop here. Remember
top level window's parent is the parent itself.

window.open case is still unclear to me. Which principal do we end up passing there when about:blank is opened?

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #19)
> Comment on attachment 8727877 [details] [diff] [review]
> ws2.patch
> 
> So nothing guarantees we don't end up to endless loop here. Remember
> top level window's parent is the parent itself.

Please note that all this docshell traversal logic is already happening within nsMixedContentBlocker. At the moment we bail out early for TYPE_WEBSOCKET within the mixed content implementation. Potentially the right fix is to update MixedContentBlocker instead of duplicating that logic within WebSocket.cpp.

Comment 21

[Anne (:annevk)]

Christoph, it sounds like that would be the right fix.

Note that going forward we don't want to throw for this case in the WebSocket API. It needs to be identical to a network failure.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Anne (:annevk) from comment #21)
> Christoph, it sounds like that would be the right fix.
> 
> Note that going forward we don't want to throw for this case in the
> WebSocket API. It needs to be identical to a network failure.

The only question I have is: The user is able to overwrite mixed content restrictions. I suppose that should also apply to the case described in this bug, right? In other words, if the user 'allows mixed content', then the insecure (ws://) websocket should load, right?

Comment 23

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

User should not be able to overwrite mixed content restriction, since the restriction is coming from the WebSocket API spec. (whether or not the limitation will be moved to some other spec doesn't really matter).  And throwing the security exception there has been in the API for quite some time.

Maybe. The spec situation is a bit too messy here when it comes to sandbox+srcdoc.

Comment 24

[Anne (:annevk)]

It's equivalent to allowing mixed content XMLHttpRequest. If they can do it for XMLHttpRequest, I guess it makes sense that we remain consistent. My personal view is that we should not offer this option to users to begin with. This is rare and users will only lose if they get tricked into doing it.

Comment 25

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

Perhaps I'm missing something here, but I don't see nsMixedContentBlocker to have code which could be reused here. It has code to check parent browsing contexts, but doesn't have anything to check opener.

Comment 26

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

FWIW, (3) in https://bugzilla.mozilla.org/show_bug.cgi?id=1220687#c19 would be really nice here, but we should probably fix this in branches too, so can't wait that bug to be fixed.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #25)
> Perhaps I'm missing something here, but I don't see nsMixedContentBlocker to
> have code which could be reused here. It has code to check parent browsing
> contexts, but doesn't have anything to check opener.

Potentially you are right and I confused those concepts. I ran the test from this bug and realized that within mixedContentBlocker we got the right 'context', but you are right, that's the parent browsing context, not the opener.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Indeed we should move this check outside the WebSocket API. Maybe nsContentUtils waiting for something better.

Comment 29

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

Comment on attachment 8728318 [details] [diff] [review]
ws2.patch

>+
>+      nsCOMPtr<nsPIDOMWindowInner> innerWindow;
>+
>+      while (true) {
>+        bool isNullPrincipal = true;
>+        if (principal) {
>+          aRv = principal->GetIsNullPrincipal(&isNullPrincipal);
>+          if (NS_WARN_IF(aRv.Failed())) {
>+            return;
>+          }
So we end up throwing some random error to web js here.


>+        }
>+
>+        if (!isNullPrincipal) {
>+          break;
>+        }
>+
>+        if (!innerWindow) {
>+          innerWindow = mWebSocket->GetOwner();
>+          if (NS_WARN_IF(!innerWindow)) {
So we shouldn't use owner here, but globalObject, defined above. The spec talks about entry settings object.


>+            aRv.Throw(NS_ERROR_FAILURE);
>+            return;
>+          }
and here NS_ERROR_FAILURE
same also elsewhere.

I think I'd prefer if we used NS_ERROR_DOM_SECURITY_ERR for these cases.

>+        // We are at the top. Let's see if we have a opener window.
s/a/an/


>+        nsCOMPtr<nsIDocument> document = innerWindow->GetDoc();
GetExtantDoc()


I think I'd like to see another patch using entry global and not the owner. Sorry that I didn't catch that before.

Comment 30

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Comment 31

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

Comment on attachment 8728607 [details] [diff] [review]
ws2.patch


>+} else if (location.search == '?timeout') {
>+  setTimeout(function() {
>+    try{
>+      var socket = new WebSocket('ws://mochi.test:8888/tests/dom/base/test/file_websocket_basic');
>+      socket.onerror = function(e) {
>+        opener.postMessage('WS onerror', '*');
>+      };
>+      socket.onopen = function(event) {
>+        opener.postMessage('WS onopen', '*');
>+      };
>+    } catch(e) {
>+      if (e.name == 'SecurityError') {
>+        opener.postMessage('WS Throws!', '*');
>+      } else {
>+        opener.postMessage('WS Throws something else!', '*');
>+      }
>+    }
>+  }, 0);
what is this timeout about? Please remove it. If it is needed, it hints we have a bug somewhere.
Don't you need to close() the window you opened? Usually mochitest warns about unclosed tabs/windows.
Fix those 2 cases and split the test to separate patch so that the actual patch can be landed before landing the tests.

Comment 32

[Andrea Marchesini [:baku]]

> what is this timeout about? Please remove it. If it is needed, it hints we
> have a bug somewhere.

The timeout is used when '?popup' is used. In this case we create webSocket from a window.open() from a function scheduled by a setTimeout(..., 0);

Mochitest and open windows are OK. browser tests complain about it. But I'll push to try for a double confirm.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: patch 2 - tests

Comment 34

[Andrea Marchesini [:baku]]

Attachment: patch 1 - fix

Comment 35

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

(In reply to Andrea Marchesini (:baku) from comment #32)
> > what is this timeout about? Please remove it. If it is needed, it hints we
> > have a bug somewhere.
> 
> The timeout is used when '?popup' is used. In this case we create webSocket
> from a window.open() from a function scheduled by a setTimeout(..., 0);
> 
> Mochitest and open windows are OK. browser tests complain about it. But I'll
> push to try for a double confirm.

What does the test complain? Creating websocket there should just work the same way as without timeout

Comment 36

[Andrea Marchesini [:baku]]

Attachment: patch 2 - tests

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Tanvi Vyas[:tanvi]]

Back from PTO, so chiming in late here.  The reason nsMixedContentBlocker bails out early for websockets is because the websocket code is supposed to ensure that mixed content websockets are not allowed (no ws on https) with no ability for user to override.

If we'd like nsMixedContentBlocker to handle this logic instead, we could make that change.  We would have to special case TYPE_WEBSOCKET to ensure that user overrides don't affect them.

Comment 39

[Kamil Jozwiak [:kjozwiak]]

Paul, would you be able to take a quick look at this bug make sure it's been fixed?

Comment 40

[(no longer active)]

Andrea, why did the tests here never land?

Comment 41

[(no longer active)]

(I tried, they don't pass on trunk, FWIW.)

Comment 42

[Andrea Marchesini [:baku]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #40)
> Andrea, why did the tests here never land?

For security reasons. We didn't land the test together with the patch.

Comment 43

[Andrea Marchesini [:baku]]

I'll file a bug to land them.

Comment 44

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

1404910 adds tests to make sure this is fixed.