670687 - Origin handling in websockets is bogus

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We get mUTF16Origin by calling GetOrigin on an nsIPrincipal.  The only place we seem to use it (modulo bug 662692) is to send the Sec-WebSocket-Origin header.  But per http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-10 that header is supposed to be the ASCII serialization of a webapps origin which is distinctly NOT what GetOrigin on an nsIPrincipal returns.  GetOrigin returns something that is neither a serialization of a webapps origin nor guaranteed to be ASCII.

Is there a reason we're using GetOrigin here instead of doing what the spec calls for?

Comment 1

[Boris Zbarsky [:bzbarsky]]

This seems like a major bug in a security aspect of websockets, which are new in Fx6.  Requesting tracking pending security evaluation.

Comment 2

[Boris Zbarsky [:bzbarsky]]

In particular, seems like we should just use nsContentUtils::GetASCIIOrigin here.

Comment 3

[Christopher Blizzard (:blizzard)]

Over to Pat.  Pat, this is tracking Firefox 6 and Firefox 7.

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: patch v1

(In reply to comment #0)
> http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-10 that
> header is supposed to be the ASCII serialization of a webapps origin which
> is distinctly NOT what GetOrigin on an nsIPrincipal returns.  GetOrigin
> returns something that is neither a serialization of a webapps origin nor
> guaranteed to be ASCII.
> 
> Is there a reason we're using GetOrigin here instead of doing what the spec
> calls for?

you've gotten radio silence from me on this because it's content code inherited from the original -76 implementation. I don't know what drove the choice.

That being said, I'm not sure I understand the difference beyond "it might not be ascii" - I get that part. Can you elucidate?

(patch attached)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well, for example for a file:// URI the webapps origin is the string "null", which is decidedly not what nsIPrincipal::GetOrigin will return.  It'll return "file://" instead.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

r=me, though it seems like you can nix mUTF16Origin in favor of an nsCString mOrigin.

Comment 7

[Patrick McManus [:mcmanus]]

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

This doesn't necessarily seem important enough to move to aurora/beta but it was nom'd for tracking so I'll ask the cmte to decide.

Comment 8

[Asa Dotzler [:asa]]

We were tracking this because it was described as "a major bug in a security aspect of websockets".  Do we have a better assessment of the security impact?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Someone familiar with the protocol would have to assess that.  The main web-facing impact of the bug is that non-ASCII hostnames generate incorrect Origin headers, at the very least.  What does the protocol use this information for?

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to comment #9)
> Someone familiar with the protocol would have to assess that.  The main
> web-facing impact of the bug is that non-ASCII hostnames generate incorrect
> Origin headers, at the very least.  What does the protocol use this
> information for?

The client puts it into a http request header. The server uses the value to decide whether or not to reject the handshake according to whatever (undefined) cross-origin policy it may care to implement.

I suppose it could screw up request header parsing on the server, but I don't believe you could inject an extra \r or \n through a multibyte utf-8 can you? That would be the biggest issue. (not my area of expertise but I recall all those multibyte sequences have their high bits set?).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ok, so the most likely failure modes would be servers either accepting handshakes they didn't mean to or (more likely) refusing handshakes that they actually want to accept.  The latter would likely manifest itself as websocket randomly not working for non-ASCII hostnames.

> but I don't believe you could inject an extra \r or \n through a multibyte
> utf-8 can you?

You cannot, no.  The non-ascii parts of UTF-8 do all have the high bit set.

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/4e34e92f1605

Comment 13

[LegNeato]

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

Approved for both releases/mozilla-beta and releases/mozilla-aurora

Comment 14

[Patrick McManus [:mcmanus]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/b8bb40d78826
http://hg.mozilla.org/releases/mozilla-beta/rev/78f4d3045e26

Comment 15

[Boris Zbarsky [:bzbarsky]]

Patrick, since you left mUTF16Origin in place, can you please file a bug to remove it and the unnecessary string conversions it involves?

Comment 16

[Patrick McManus [:mcmanus]]

(In reply to comment #15)
> Patrick, since you left mUTF16Origin in place, can you please file a bug to
> remove it and the unnecessary string conversions it involves?

can you clarify why you think utf16origin is a bad idea? mUTF16Origin is used in createanddispatchmessageevent(), which is called everytime a websockets message is received and passed to JS.

Most of the other utf8 uses of origin are used one time.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ah, I'd missed that use.  But shouldn't that be the Unicode origin, not the ASCII origin?  As in, didn't this patch break that usage?

Comment 18

[Patrick McManus [:mcmanus]]

Attachment: message-event-should-be-utf.1

keep mUTF16Origin (and make it UTF), and just deal with ASCII Origin at the callsite without a member variable.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 547763 [details] [diff] [review]
message-event-should-be-utf.1

r=me, but please get someone who understands message events (sicking? smaug?) to double-check this as well.

Comment 20

[Patrick McManus [:mcmanus]]

Comment on attachment 547763 [details] [diff] [review]
message-event-should-be-utf.1

jonas, can you eyeball this too per bz's request?

Comment 21

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

Comment on attachment 547763 [details] [diff] [review]
message-event-should-be-utf.1

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

::: content/base/src/nsWebSocket.cpp
@@ +308,2 @@
>    rv = mWebSocketChannel->AsyncOpen(mOwner->mURI,
> +                                     asciiOrigin, this, nsnull);

Assuming 'asciiOrigin' is the value we send in a http header when connecting, then that seems correct. Except that the indentation is off by one.

@@ +1034,5 @@
>      return NS_ERROR_DOM_SYNTAX_ERR;
>    }
>  
> +  rv = nsContentUtils::GetUTFOrigin(mPrincipal, mUTF16Origin);
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SYNTAX_ERR);

What is this used for? For the .origin property when dispatching 'message' events?

If so, are we grabbing the correct origin? Isn't mPrincipal the principal of the page opening the connection? It seems like we should use the origin of the server that we're connected to.

Comment 22

[Patrick McManus [:mcmanus]]

> What is this used for? For the .origin property when dispatching 'message'
> events?
> 

yes - the .origin property.

> If so, are we grabbing the correct origin? Isn't mPrincipal the principal of
> the page opening the connection? It seems like we should use the origin of
> the server that we're connected to.

Honestly, that's inherited -76 code (well, I broke it be ascii, but it always derived from mPrincipal) so I'm not sure how the decision was made - your expertise is all I've got here.

Message Event defines:
"The origin attribute represents, in server-sent events and cross-document messaging, the origin of the document that sent the message (typically the scheme, hostname, and port of the document, but not its path or fragment identifier)."

So you're saying the the "document that sent the message" should be ws://foobar.com/whatever instead of the script at http://bazoff.com/hello.js ?

Comment 23

[Wellington Fernando de Macedo]

> Message Event defines:
> "The origin attribute represents, in server-sent events and cross-document 
> messaging, the origin of the document that sent the message (typically the 
> scheme, hostname, and port of the document, but not its path or fragment 
> identifier)."
>
> So you're saying the the "document that sent the message" should be 
> ws://foobar.com/whatever instead of the script at http://bazoff.com/hello.js ?

Also in the MessageEvent spec there is:

"Except where otherwise specified, when the user agent creates and dispatches a message event (..) the origin attribute must be the empty string."

In the WebSockets spec the origin is used only when establishing the connection. In Server Sent Events, however, its spec defines to use the target url origin. So, as it is, the origin in the MessageEvents of WebSockets should be empty.

Comment 24

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

That sounds like a bug in the spec. We should make the origin be the origin of the server we're connected to.

I filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=13525 on the spec.

Comment 25

[Patrick McManus [:mcmanus]]

track the event.origin bits over in 676277 - this bug can remain about the Sec-WebSocket-Origin header.

Comment 26

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

Comment on attachment 547763 [details] [diff] [review]
message-event-should-be-utf.1

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

r=me on the nsWebSocketEstablishedConnection::Init changs. r- on the rest as it seems wrong.

Comment 27

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

removing sec-review flag as this appears good

Comment 28

[u279076]

qa- as no QA fix verification necessary