664284 - Add HSTS support for websockets

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[David Chan [:dchan]]

If a server opts-in to HSTS, websockets should also obey this settings (ws rewritten to wss) . 

I'm not sure if HSTS settings should be allowed during the initial websockets handshake. Most secure sites will perform a redirect if accessed over http, however the current websockets spec doesn't seem to allow redirects. HSTS could only be set over a wss handshake in this case.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Also, we need to make sure that when the server is HSTS, that WSS:// isn't blocked. I think I remember that some of the HSTS code hard-codes "HTTPS".

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

> that WSS:// isn't blocked

... or, worse, rewritten to HTTPS.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

We need tests to determine what the current behavior is, and verify what it should be to guide what needs to be tweaked (if anything) to resolve this bug.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

When a HTTPS response includes an HSTS header, should we also rewrite ws:// links to wss:// for that domain? (When a HTTPS response includes an HSTS header, we transparently rewrite all http:// links to https:// for that domain. When we open a wss:// websocket that returns the HSTS header, it seems clear that we should transparently rewrite all ws:// links to wss:// for that domain.)

I think we need these tests:
* Create a wss:// connection to a server that has an expired certificate. The connection should fail.

* Create a wss:// connection to a server with an untrusted self-signed certificate. The connection should fail.

* Create a wss:// connection to a server with a trusted self-signed certificate. The connection should succeed.

* Create a wss:// websocket to a domain that returns a HSTS header in the WebSocket handshake. Then, create a ws:// websocket to the same domain.

* Create a wss:// websocket to a domain that returns a HSTS header in the handshake. Then, create another wss:// websocket to the same domain, but have the server use an untrusted self-signed certificate. The connection must fail.

* Make a HTTPS request to a domain that returns an HSTS header in a response. Then create a ws:// websocket to the same domain. Expected result: ???

* Create a wss:// websocket that has a HSTS header in the response. Then, make a HTTP request to the same domain. Expected result for the HTTP request: ???

I think HSTS should not distinguish between the protocols (wss or https, ws or http) when enforcing HSTS policy. That is, HSTS headers returned in HTTPS responses would be effective for ws:// and wss:// connections to the same domain, and HSTS headers returned in wss:// handshakes would be effective for http:// and https:// connections to the same domain.

Thoughts?

Comment 5

[Dirkjan Ochtman (:djc)]

Sounds sane, but we might want to ask the WebSockets editors/group on their thoughts about the interaction with HSTS.

Comment 6

[Chris Evans]

FWIW, I implemented this suggestion in Chrome. I think it made it into Chrome 12:
http://src.chromium.org/viewvc/chrome?view=rev&revision=82069

I also implemented the recommendation that HSTS on blah.com also upgrade to https for non-port-80 requests.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Chris, I don't understand your comment "I also implemented the recommendation that HSTS on blah.com also upgrade to https for non-port-80 requests." Which recommendation are you referring to? My understanding is that HSTS is port-specific on purpose.

Sid, does the above seem reasonable to you?

Comment 8

[Chris Evans]

See
http://code.google.com/p/chromium/issues/detail?id=77467
And in particular,
http://securityretentive.blogspot.com/2010/11/quick-clarification-on-hsts-http-strict.html

Comment 9

[Patrick McManus [:mcmanus]]

I am hoping we won't need any code changes here.

Websockets bootstraps itself using a handshake performed with the main HTTP stack, making an HTTP(S) request. It expects a 101 response to start talking websockets, but before that everything is absolute HTTP using the HTTP stack. So we can leverage the HSTS work already done.

That transaction will honor the Strict-Transport-Security response header in the usual way - seeding the HSTS permission DB if apropos.

The URI for that transaction is http or https, because the websockets code creates an http or https transaction for the bootstrap to take place. If it opens an http channel the existing hsts/http code will redirect that to an https channel if that is the right policy. The http code isn't aware that websockets is driving this channel instead of docshell (or something else).

That's all good - there is no problem in this context for hardcoded "http" or "https" checks in the hsts code because the handshake is http. Indeed making the websocket handshake follow all HTTP semantics (instead of just kind of looking like HTTP) was a conscious decision to be able to leverage all of these sorts of technologies.

The only potential trip up I see is that I forget exactly how the http->https redirect is done and whether or not it involves an nsIChannelEventSink and a verifier.. Some small amount of code would be needed to support that in the websocket channel (which owns the http channel during bootstrap). Not a big deal.

I'll look into developing tests.

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to comment #4)
>
> * Make a HTTPS request to a domain that returns an HSTS header in a
> response. Then create a ws:// websocket to the same domain. Expected result:
> ???
> 

expect the websocket object to be carried out over ssl.

> * Create a wss:// websocket that has a HSTS header in the response. Then,
> make a HTTP request to the same domain. Expected result for the HTTP
> request: ???

The HSTS header was learned through the HTTPS bootstrap transaction of the wss:// handshake, so its valid HSTS data. I would expect the latter http request to be redirected as https://

Every ws(s) session includes 1 http(s) transaction by definition. The requirements and side effects of that transaction aren't any different than any other HTTP transaction, imo.

There is so little websockets particular code here that I suspect the full test matrix is overkill - WS just doesn't interact with facets like expired vs valid certs, those are all handled by http. Indeed WS does not even make the "isStsURI()" style checks, those are all a layer deeper, and WS never adds exceptions for self-signed objects. The only WS particular code is to accept the particular redirect HSTS does. So something more basic is imo sufficient.

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: patch v1

This patch is really simple - it lets the websockets handler accept a redirect of its bootstrapping http handshake from http://FOO to https://FOO. That's the only change necessary (redirects are normally not allowed). 

The patch also includes tests showing the default absence of hsts, a wss transaction with the strict-transport-security response header, and finally a ws transaction rewritten to wss in the presence of the hsts policy.

Comment 12

[Jason Duell]

Sid, any chance we can get this reviewed soon?  If there are necko bits you don't understand well, you can farm that part of the review out to me.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 549436 [details] [diff] [review]
patch v1

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

Hey Jason, sorry for the delay.  I am not a peer (don't have L2 commit access), but can provide feedback.  Flagging you for the official sign-off.

Test looks sufficient and most of the code is indeed straightforward.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1735,5 @@
> +    PRBool uriEqual = PR_FALSE;
> +    rv = clonedNewURI->Equals(currentURI, &uriEqual);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (!(!currentIsHttps && newuriIsHttps && uriEqual)) {

This logic is a bit confusing to me... It is saying "the only acceptable situation is where the starting point of the redirect is not secure, the ending point of the redirect is secure and everything except the scheme is equal".  While this makes sense, it took me twenty minutes of looking at the code to figure this out (maybe I'm slow).

Would it be clearer to write the condition as a disjunction?
  
if (currentIsHttps || !newuriIsHttps || !uriEqual) {
  // if redirecting from HTTPS, to non-HTTPS URL or 
  // to a different URL, block the redirect.

I know it's equivalent, but is easier for me to read than "!(situation-that-is-okay)".  Either way is probably fine, but if you stick with the existing line, please include a comment along the lines of:
  // disallow all redirects that do *not* start with an
  // insecure URL, redirect to secure URL, and are identical 
  // except for the scheme.

Comment 14

[Jason Duell]

Attachment: v2: unbitrotted, and pywebsockets issue fixed.

Now applies on top of binary WS (bug 676439), though the code is unrelated, and the pywebsockets patch (bug 689006), which is related.  I've fixed the pywebsocket issue (which was due to a file rename).

This all looks great.  One quick question before I +r:  in WebSocketChannel::AsyncOnChannelRedirect you sometimes simply return with an error code (NS_ENSURE_SUCCESS, etc.), and sometimes call OnRedirectVerifyCallback(error) then return NS_OK.  Glancing at nsIChannelEventSink.idl, it would seem that both of these wind up canceling the redirect.  If they're equivalent, it would seem simpler/cleaner if we always simply return an error code, rather than use the "OnRedirectVerifyCallback(error) then return NS_OK" idiom.  Thoughts?

Comment 15

[Jason Duell]

Also, I see this is marked [parity-chrome], but I don't see any info about chrome using HSTS for websockets from a quick google search, and there's no support for testing it in pywebsockets (which is why we have to add it).  Are we sure this is a chrome-parity issue, and if not, have we let Chromium (and the WS spec lists) know that we're taking this approach?  Seems like an important security issue...

Comment 16

[Jason Duell]

D'oh--so now I see comment 6, so my parity-chrome issue is answered.  But have we alerted the hybi or W3C lists to the issue?

I'm also going to post a new patch that has Sid's suggested comment added.

Comment 17

[Jason Duell]

Attachment: v2.1: now with comment

Comment 18

[Jason Duell]

Attachment: v.2.2: and with add'l README step for upgrading to new versions of pywebsocket

Sigh--yet another minor tweak, hopefully the last one.

Still need feedback on AsyncRedirectHandler error handling.

Comment 19

[Patrick McManus [:mcmanus]]

Comment on attachment 577904 [details] [diff] [review]
v.2.2: and with add'l README step for upgrading to new versions of pywebsocket

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

Sorry about the delay. The bugzilla interdiff tools would not give me useful output so I had to pick through the 2 patches line by line.

you can either r+ me or leave me as the author with your r. Either way is fine by me.

::: content/base/src/nsWebSocket.cpp
@@ +300,5 @@
>    if (!mRequestedProtocolList.IsEmpty()) {
>      mChannel->GetProtocol(mEstablishedProtocol);
>    }
>  
> +  UpdateURI();

The order of this changed in your patch. In mine it was after GetExtensions() and it was moved to be before it. I don't think it matters but I'm trying to figure out why

Comment 20

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95b76549de82

> The order of this changed in your patch

I don't think it mattered either, but I changed it back to your original order just in case.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/95b76549de82

Comment 22

[Patrick McManus [:mcmanus]]

awesome

Comment 23

[Boris Zbarsky [:bzbarsky]]

> But have we alerted the hybi or W3C lists to the issue?

Apperently not.  Nor did we do it after we changed our behavior.  We really need to get things like this upstreamed into specs, please.