User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/14.0.835.187 Safari/535.1 Steps to reproduce: having a html page on a web server listening on a non-80 port, and accessing such page specifiyng as host the IPv6 of such webserver (obviously plus its port...): If you open a cross-site XHR or WebSocket it will result in the browser sending an ambiguous Origin (or Sec-WebSocket-Origin) header if the said IPv6 contains at least 2 consecutive 16-bit fields of 0s. I think that at least at the moment this ambiguity does not pose security issues as I expect that most resources will not have a Origin permission list based on IPv6, but I'm flagging the hidden check anyway (better safe than sorry...) I've seen the same behaviour on Firefox 7.0.1 and Aurora 9.0a2 related specs/drafts: CORS http://www.w3.org/TR/cors WebSockets http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-17 IPv6 http://tools.ietf.org/html/rfc5952 Origin http://tools.ietf.org/html/draft-ietf-websec-origin-06 & http://tools.ietf.org/html/draft-abarth-origin-09 Actual results: Let's say that the web server is listening on port 8888 and can be reached with its IPv6 that is aa80:0000:0000:0000:19b7:231e:ff5d:34e3 (that compressed is aa80::19b7:231e:ff5d:34e3). The Origin header generated by Firefox will be like this: http://aa80::19b7:231e:ff5d:34e3:8888 Now let's say that the web server is listening on port 80 and can be reached with its IPv6 that is aa80:0000:0000:19b7:231e:ff5d:34e3:8888 (that compressed is aa80::19b7:231e:ff5d:34e3:8888). The Origin header generated by Firefox will be the same: http://aa80::19b7:231e:ff5d:34e3:8888 Expected results: The ambiguity is easily solved using square brakets so that the 2 Origins can be http://[aa80::19b7:231e:ff5d:34e3]:8888 and http://[aa80::19b7:231e:ff5d:34e3:8888] (at least Chrome behave like this)
Patrick, any thoughts on security severity here?
(In reply to Johnny Stenback (:jst, firstname.lastname@example.org) from comment #1) > Patrick, any thoughts on security severity here? there are a couple things about the report I want to verify, but at first pass its sg:moderate. can you change the component to core:networking:websockets ? (If I try and do that it tells me I can't set the sec group and threatens to drop the current sec classification)
ah, I just realized the report says all Origin headers - not just WS. That makes more sense to me (and is something I wanted to verify). so the component should be http not websockets. that comes from nsCString asciiOrigin; rv = nsContentUtils::GetASCIIOrigin(mPrincipal, asciiOrigin); NS_ENSURE_SUCCESS(rv, rv); maybe bz has some insight into whether  is the correct format..
Spec is at http://tools.ietf.org/html/draft-abarth-origin-09#section-6.1 and says: serialized-origin = scheme "://" host [ ":" port ] ; <scheme>, <host>, <port> productions from RFC3986 So yes, the  would be correct, I believe. Is that something that nsStandardURL::GetHost should handle, or do we need to somehow figure out in nsContentUtils that the host is an IPv6 literal? Seems like the former would be better....
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#419(In reply to Boris Zbarsky (:bz) from comment #4) > Is that something that nsStandardURL::GetHost should handle, or do we need hmm.. it looks like sethost was already trying to do it.. http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#419
Yes, but GetHost explicitly strips out the  right now. See nsStandardURL::Host....
BZ or Patrick - either of you want to own this?
(In reply to Josh Aas (Mozilla Corporation) from comment #8) > BZ or Patrick - either of you want to own this? I don't, particularly. Christian, would it make sense for GetHost to just always do that? If not, we need to make the content utils code do something like this (e.g. by moving the code you linked to to nsNetUtils).
I guess I see 4 paths, all kinda undesirable. 1] nsStandardURL:Host (used by getAsciiHost) has stripped  back to hg rev 1. There are obviously lots of callers; I'm not eager to change that behavior - regressions would be likely. 2] likewise, nsIURI has a bunch of downstream uses so adding a getAsciiHostPort to that which does the right thing doesn't seem real desirable for anyone that wants to implement nsIURI. 3] I guess I could add getAsciiHostPort() to nsIStandardURL (or better a whole new IDL that nsStandardURL also implements) and contentutils could QI that and use the existing code as a backup. Seems kinda expensive. 4] Finally, contentutils is already doing the concat so it really ought to absorb responsibility for the semantics (unless you argue that new methods/interfaces are needed because it shouldn't be in that business at all) much like nsHttpHandler::GenerateHostPort() does - so similar logic as seen in generatehostport can just be applied in contentutils when doing the port concat. I favor 4 as simple and efficient, though it muddles ownership of these things further. I guess failing that I favor a version of 3 with a new IDL and a QI. I'm happy to write the code for either path. Boris what do you think?
The problem with #4 is that then ContentUtils needs methods to detect when the hostname is actually an IP address, right? I guess you're suggesting that it just look for ':' in the hostname? For that matter, would it need to look for '%' that way as well? I have no a priori problem with contentutils doing something like that, as long as the code only lives in one shared place. I don't want multiple versions of code like that around.... So contentutils would either pass the host string or the nsIURI to some nsNetUtil method and out would pop the right thing. Whether 2 is better than 4... We should look at our other GetAsciiHost callers and see how many of them are actually implementing GetAsciiHostPort, I think. As far as 2 vs 3, 3 seems strictly worse since it'll give wrong behavior for some URIs, right?
Created attachment 583760 [details] [diff] [review] patch 0 The code to do host/port concatenation was duplicated in nsContentUtils and nsHttpHandler, with only the latter handling the  ipv6 literal case. This patch moves the httphandler algorithm into nsNetUtils and changes nsContentUtils and nsHttpHandler to both point at that.
Comment on attachment 583760 [details] [diff] [review] patch 0 This doesn't do the right thing when the port is set in the URI and set to the default port, does it? You need to keep those compares to the default port and reset the port to -1 if it's the default, I think.
my bad - I misread that as doing something else. easy to fixup - thanks.
Created attachment 583824 [details] [diff] [review] patch 1
Comment on attachment 583824 [details] [diff] [review] patch 1 r=me. Thanks!
https://hg.mozilla.org/mozilla-central/rev/c02f25c177e3 Patrick, I've set the affected/tracking flags, but I'll let you decide whether this wants beta/aurora approval requested if that's ok?
I'm not going to a? though someone from sec-team can if they disagree. to exploit you need to be a server with an origin ACL list using IPV6 literals of a particular pattern (and a particular matching pattern of the port you are running your server on - this is not in the attackers control). There is no evidence that has happened in the wild. so I'd rather the change get the chance to bake in aurora/beta in the usual way.
(In reply to Patrick McManus [:mcmanus] from comment #20) > I'm not going to a? though someone from sec-team can if they disagree. > > to exploit you need to be a server with an origin ACL list using IPV6 > literals of a particular pattern (and a particular matching pattern of the > port you are running your server on - this is not in the attackers control). > There is no evidence that has happened in the wild. > > so I'd rather the change get the chance to bake in aurora/beta in the usual > way. Given our intent to let this ride, minusing for tracking.
Mone, would you be able to verify this fixed in Firefox 12.0b3?
Hi All, Yes I confirm that the issue is solved on Firefox 12, at least since a2 (2012-03-13), thanks everybody :)
Marking verified based on comment 23.