Convert Websockets to use AsyncOpen2()

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Assignee: nobody → ckerschb
Blocks: 1182535
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
(Assignee)

Comment 1

3 years ago
Created attachment 8750221 [details] [diff] [review]
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
Attachment #8750221 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 2

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

Comment 3

3 years ago
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.
Attachment #8750221 - Flags: review?(jduell.mcbugs) → feedback+

Comment 4

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

Comment 5

3 years ago
Created attachment 8753305 [details] [diff] [review]
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!
Attachment #8750221 - Attachment is obsolete: true
Attachment #8753305 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 6

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

Comment 7

3 years ago
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.

Updated

3 years ago
Attachment #8753305 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 10

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

Updated

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

Comment 13

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

Comment 15

3 years ago
Created attachment 8754780 [details] [diff] [review]
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 :-)
Attachment #8754780 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 16

3 years ago
Created attachment 8754781 [details] [diff] [review]
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.
Attachment #8754781 - Flags: review?(jonas)
(Assignee)

Comment 17

3 years ago
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!
Flags: needinfo?(jonas)
Flags: needinfo?(jduell.mcbugs)

Comment 18

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

Updated

2 years ago
Attachment #8754780 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 19

2 years ago
Created attachment 8757829 [details] [diff] [review]
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!
Attachment #8754780 - Attachment is obsolete: true
Attachment #8757829 - Flags: review?(jduell.mcbugs)

Updated

2 years ago
Attachment #8757829 - Flags: review?(jduell.mcbugs) → review+

Comment 20

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

Updated

2 years ago
Flags: needinfo?(jonas)
(Assignee)

Comment 21

2 years ago
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
Attachment #8754781 - Flags: review?(jonas) → review?(jduell.mcbugs)
(Assignee)

Comment 22

2 years ago
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 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 ';'

Updated

2 years ago
Attachment #8754781 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 24

2 years ago
Created attachment 8763201 [details] [diff] [review]
bug_1271198_asyncopen2_websockets.patch

qfolded all the patches into one, carrying over r+ from jduell.
Attachment #8753305 - Attachment is obsolete: true
Attachment #8754781 - Attachment is obsolete: true
Attachment #8757829 - Attachment is obsolete: true
Attachment #8763201 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 25

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad3bd17f3e9
Convert Websockets to use AsyncOpen2(). r=jduell
Keywords: checkin-needed

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ad3bd17f3e9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

2 years ago
Depends on: 1299766
You need to log in before you can comment on or make changes to this bug.