Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Origin handling in websockets is bogus

RESOLVED FIXED in Firefox 6

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bz, Assigned: mcmanus)

Tracking

Trunk
mozilla8
x86
All
Points:
---

Firefox Tracking Flags

(firefox6+ fixed, firefox7+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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?
(Reporter)

Comment 1

6 years ago
This seems like a major bug in a security aspect of websockets, which are new in Fx6.  Requesting tracking pending security evaluation.
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox6: --- → ?
tracking-firefox7: --- → ?
(Reporter)

Comment 2

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

Updated

6 years ago
tracking-firefox6: ? → +
tracking-firefox7: ? → +
OS: Mac OS X → All
tracking-firefox6: + → ?
tracking-firefox7: + → ?
Keywords: sec-review-needed
OS: All → Mac OS X
tracking-firefox6: ? → +
tracking-firefox7: ? → +
OS: Mac OS X → All
Over to Pat.  Pat, this is tracking Firefox 6 and Firefox 7.
Assignee: nobody → mcmanus
(Assignee)

Comment 4

6 years ago
Created attachment 546609 [details] [diff] [review]
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)
Attachment #546609 - Flags: review?(bzbarsky)
(Reporter)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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.
Attachment #546609 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
(Assignee)

Comment 7

6 years ago
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.
Attachment #546609 - Flags: approval-mozilla-beta?
Attachment #546609 - Flags: approval-mozilla-aurora?

Comment 8

6 years ago
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?
(Reporter)

Comment 9

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

Comment 10

6 years ago
(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?).
(Reporter)

Comment 11

6 years ago
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.
http://hg.mozilla.org/mozilla-central/rev/4e34e92f1605
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8

Comment 13

6 years ago
Comment on attachment 546609 [details] [diff] [review]
patch v1

Approved for both releases/mozilla-beta and releases/mozilla-aurora
Attachment #546609 - Flags: approval-mozilla-beta?
Attachment #546609 - Flags: approval-mozilla-beta+
Attachment #546609 - Flags: approval-mozilla-aurora?
Attachment #546609 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/b8bb40d78826
http://hg.mozilla.org/releases/mozilla-beta/rev/78f4d3045e26
status-firefox6: affected → fixed
status-firefox7: affected → fixed
Whiteboard: [inbound]
(Reporter)

Comment 15

6 years ago
Patrick, since you left mUTF16Origin in place, can you please file a bug to remove it and the unnecessary string conversions it involves?
(Assignee)

Comment 16

6 years ago
(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.
(Reporter)

Comment 17

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

Comment 18

6 years ago
Created attachment 547763 [details] [diff] [review]
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.
Attachment #547763 - Flags: review?(bzbarsky)
(Reporter)

Comment 19

6 years ago
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.
Attachment #547763 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

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

jonas, can you eyeball this too per bz's request?
Attachment #547763 - Flags: review?(jonas)
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.
(Assignee)

Comment 22

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

Updated

6 years ago
Blocks: 676277
(Assignee)

Comment 25

6 years ago
track the event.origin bits over in 676277 - this bug can remain about the Sec-WebSocket-Origin header.
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.
Attachment #547763 - Flags: review?(jonas) → review+
removing sec-review flag as this appears good
Keywords: sec-review-needed
qa- as no QA fix verification necessary
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.