1271198 - Convert Websockets to use AsyncOpen2()

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271198_asyncopen2_websockets.patch

Jason, it's time to convert websockets to use asyncOpen2() :-)

Instead of performing contentPolicy checks within ::init() we will now perform them within ::asyncOpen(), hence I suppose we should throw an exception if asyncOpen() fails. I agree that's not a full parity implementation because now we also throw an exception if really asyncOpen() fails and not only security checks, but I suppose it's the best we can do, right?

Since channel->AsyncOpen2() only takes one argument I updated ::OnDataAvailable(), ::OnStartRequest() and also ::OnStopRequest to use mHttpChannel instead of 'aContext' internally.

And finally suppose, since websockets are allowed cross origin, we should use: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL as the security flag, agreed?


One last question, do we need to worry about addons that potentially create channels without any loadInfo? If so, then we have to create an *if* check within BeginOpenInternal() and potentially fall back to channel->asyncOpen() instead of calling channel->asyncOpen2().


https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ffa892c9726

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Oh, one important question I forgot to ask: Don't websockets call DoCheckLoadURIWIthPrincipal() somewhere?

Comment 3

[Jason Duell]

Comment on attachment 8750221 [details] [diff] [review]
bug_1271198_asyncopen2_websockets.patch

Review of attachment 8750221 [details] [diff] [review]:
-----------------------------------------------------------------

> Instead of performing contentPolicy checks within ::init() we will now perform
> them within ::asyncOpen(), hence I suppose we should throw an exception if
> asyncOpen() fails. I agree that's not a full parity implementation because now
> we also throw an exception if really asyncOpen() fails and not only security
> checks, but I suppose it's the best we can do, right?

I think that's fine.

> do we need to worry about addons that potentially create channels without any
> loadInfo? If so, then we have to create an *if* check within
> BeginOpenInternal() and potentially fall back to channel->asyncOpen() instead
> of calling channel->asyncOpen2().

Sadly, I suspect we'll need to do that.  We provide the nsIWebSocketChannel.idl interface to addons (so they can do websocket protocol things without actually creating a DOM websocket).  Looking at the MXR addon repo, I see there are some addons that are using it:

  https://mxr.mozilla.org/addons/search?string=nsIWebSocketChannel

It's not a ton (only 22), and it also looks like most/many of them are mozilla projects (maybe leftover early attemps at implementing Push?)  But to be safe it's probably better to assume we might not have a loadInfo (especialy if we'd crash or do Bad Things).

> since websockets are allowed cross origin, we should use:
> SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL as the security flag, agreed?

I don't understand CORS nearly as well as I should.  I just poked around the net for a bit and honestly I'm confused that Websockets do seem to be considered cross-origin yet they also lock down a web page into only connecting to the same origin that served the page.  So the short answer is, you may need to figure this out yourself, or ask another reviewer (I'm OK with trusting your judgement if you're confident in the answer).  Take a look at these links:

  https://news.ycombinator.com/item?id=999255
  https://tools.ietf.org/html/rfc6455#section-1.6
  http://stackoverflow.com/questions/23674199/why-is-there-no-same-origin-policy-for-websockets-why-can-i-connect-to-ws-loc
  https://www.christian-schneider.net/CrossSiteWebSocketHijacking.html

(I haven't digged in to see if the issues mentioned in the last two were issues only with early drafts of the websockets RFC or whether they're still valid).

Giving this f+ since it sounds like we need a new patch for the missing loadInfo possibility.

Comment 4

[Jason Duell]

> Oh, one important question I forgot to ask: Don't websockets call 
> DoCheckLoadURIWIthPrincipal() somewhere?

Doesn't look like it--at least grep in the netwerk/protocol/websocket and dom/base directories isn't showing any call.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271198_asyncopen2_websockets.patch

Jason, thanks for the review. I agree we should have that *if* clause to make sure that the channel in fact has a loadInfo attached before calling AsyncOpen2() on it.

I would think that SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL is the right security flag to use but I'll consult Jonas later today to make sure he agrees before landing the patch.

Thanks!

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #4)
> > Oh, one important question I forgot to ask: Don't websockets call 
> > DoCheckLoadURIWIthPrincipal() somewhere?
> 
> Doesn't look like it--at least grep in the netwerk/protocol/websocket and
> dom/base directories isn't showing any call.

I couldn't find one either, which is slightly scary, but I suppose we have a check that the websocket is indeed of scheme ws: or wss:.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Jason, to sum it up, I am fairly confident we are passing the right security flags. In fact, we are doing exactly the same security checks we did before converting the callsites to use AsyncOpen2(), but we are calling CheckLoadURIWithPrincipal() in addition. Calling CheckLoadURIWithPrincipal() shouldn't make any difference in the end, because in the current setup we are already checking that we only allow ws: or wss: to load.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/854a8df494d3

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for many assertions at nsHttpChannel.cpp:5204:

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=854a8df494d359f601e1bbc26883ffc1c48de362

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8753305 [details] [diff] [review]
bug_1271198_asyncopen2_websockets.patch

Review of attachment 8753305 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1400,5 @@
>    }
>  #endif
>  
> +  nsCOMPtr<nsILoadInfo> loadInfo = localChannel->GetLoadInfo();
> +  if (loadInfo && loadInfo->GetEnforceSecurity()) {

Arrrh - obviously it needs to be:

if (loadInfo && loadInfo->GetSecurityMode()) {

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2292661153e3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Pretty sure https://treeherder.mozilla.org/logviewer.html#?job_id=28240342&repo=mozilla-inbound was yours as well. We'll find out on the re-land I guess.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Pretty sure
> https://treeherder.mozilla.org/logviewer.html#?job_id=28240342&repo=mozilla-
> inbound was yours as well. We'll find out on the re-land I guess.

Tested locally and can already confirm that this patch broke the webplatform tests as well - what a great day!

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/34e9970d625a

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271198_wpt_test_updates.patch

Jason, obviously since we moved the security checks from ::init() into ::asyncOpen2() we have to call |ws.send("");| to actually trigger the CSP error, hence we have to update that web platform test :-)

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271198_contentsecuritymanager_update.patch

Jonas, before converting websockets to use asyncOpen2() we used to perform 'nsIContentPolicy' checks on the 'ws://'-channel, but we are calling asyncOpen2() on the http://-channel, hence we have to query the proxyURI to allow CSP to block the load.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Jonas, Jason,

the two additional patches fix the problem in non e10s-land but unfortunately we are facing another problem. Whenever we call channel->AsyncOpen2() we perform security checks in the child, where we have a document available, from which we can query the CSP. However, we can't serialize the document between child and parent (for reasons we don't need to discuss here). However, we can serialize principals, but we don't actually serialize the CSP of the principal :-(

Now that nsIWebSocketChannel provides it's own asyncOpen() function, we already perform on roundtrip from child to parent where we loose the CSP on the principal. Once we actually call httpChannel->ASyncOpen2() we don't have a CSP available to actually block the load, because we haven't serialized it :-(

Any ideas how to best fix that problem? Serializing the CSP seems like it's not going to be an option. We could theoretically keep the CSP check within the ::init() function, but that's also not that great. Open for discussions - thanks!

Comment 18

[Jason Duell]

So for HTTP e10s we do the CSP checks on the child, and then when we gets to the parent to open the 'real' nsHttpChannel, I assume we skip doing the CSP tests again both because 1) we've already done them, and 2) just as for this bug, we don't have a CSP available, right?

If so I'd guess we want to do something similar here--let's do the CSP check on the child (should we add an AsyncOpen2 to WebSocketChannel{Child}? Or we could do it in WebSocket::init) and then skip it on the parent.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271198_asyncopen2_websockets_csp_test.patch

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #18)
> If so I'd guess we want to do something similar here--let's do the CSP check
> on the child (should we add an AsyncOpen2 to WebSocketChannel{Child}? Or we
> could do it in WebSocket::init) and then skip it on the parent.

Thanks Jason. In fact I totally agree; let's keep this simple and add an explicit CSP check within websocket::init(). I also added a comment explaining the serialization problem and since websockets can't follow redirects, we are good to go.

The update within this patch obviously renders the web platform test update to be obsolete. As for the other patch updating nsContentSecurityManager I will consult Jonas if we want to pass the ws:// or the http:// channel to contentPolicies. I suppose the ws:// channel. At least that's what we are doing currently on mozilla central.

Thanks for your help!

Comment 20

[Jason Duell]

Comment on attachment 8757829 [details] [diff] [review]
bug_1271198_asyncopen2_websockets_csp_test.patch

Review of attachment 8757829 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/WebSocket.cpp
@@ +1519,5 @@
>    }
>  
>    mOriginDocument = do_GetWeakReference(originDoc);
>  
> +  // The 'real' nsHttpChannel of the websocket gets openend in the parent.

typo: "opened" :)

@@ +1523,5 @@
> +  // The 'real' nsHttpChannel of the websocket gets openend in the parent.
> +  // Since we don't serialize the CSP within child and parent we have to
> +  // perform the CSP check here instead of AsyncOpen2().
> +  // Please note that websockets can't follow redirects, hence there is no
> +  // need to perform a CSP check after redirects.

We do redirects only for HSTS: does that require any extra work?

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8754781 [details] [diff] [review]
bug_1271198_contentsecuritymanager_update.patch

Jason, I chatted with Jonas earlier and he would feel more comfortable you reveiwing this patch.

To recap: Currently on mozilla-central we are passing the ws:// URI to contentPolicies [1]. Now that we are moving security checks into AsyncOpen2() the channel uri was already resolved to be a http:// URI. In order to keep parity between the old and the new implementation I would think we would still like to pass the ws:// URI to contentPolicies, hence I updated the securitymanager to query the proxy-URI from httpchannelinternal whenever we are loading TYPE_WEBSOCKET. I hope you agree - thanks!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/WebSocket.cpp#1558

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8754781 [details] [diff] [review]
bug_1271198_contentsecuritymanager_update.patch

Review of attachment 8754781 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsContentSecurityManager.cpp
@@ +306,5 @@
>      }
>  
>      case nsIContentPolicy::TYPE_WEBSOCKET: {
> +      // Websockets have to use the proxied URI:
> +      // ws:// instead of http:// for CSP checks

Obvisously I'll update that comment because CSP checks happen within :init()

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8754781 [details] [diff] [review]
bug_1271198_contentsecuritymanager_update.patch

Review of attachment 8754781 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsContentSecurityManager.cpp
@@ +178,4 @@
>  {
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
> +  NS_ENSURE_SUCCESS(rv, rv);  nsContentPolicyType contentPolicyType =

This needs a linebreak after ';'

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271198_asyncopen2_websockets.patch

qfolded all the patches into one, carrying over r+ from jduell.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad3bd17f3e9
Convert Websockets to use AsyncOpen2(). r=jduell

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ad3bd17f3e9