694576 - (CVE-2012-0475) Ambiguous IPv6 in Origin (and Sec-WebSocket-Origin) request header

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Mone]

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)

Comment 1

[Johnny Stenback (:jst)]

Patrick, any thoughts on security severity here?

Comment 2

[Patrick McManus [:mcmanus]]

(In reply to Johnny Stenback (:jst, [e-mail]) 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)

Comment 3

[Patrick McManus [:mcmanus]]

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..

Comment 4

[Boris Zbarsky [:bzbarsky]]

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....

Comment 5

[Johnny Stenback (:jst)]

Thanks Patrick!

Comment 6

[Patrick McManus [:mcmanus]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, but GetHost explicitly strips out the [] right now.  See nsStandardURL::Host....

Comment 8

[Josh Aas]

BZ or Patrick - either of you want to own this?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Compare code at http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#548

Comment 10

[Boris Zbarsky [:bzbarsky]]

(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).

Comment 11

[Patrick McManus [:mcmanus]]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: 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 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Patrick McManus [:mcmanus]]

my bad - I misread that as doing something else.

easy to fixup - thanks.

Comment 16

[Patrick McManus [:mcmanus]]

Attachment: patch 1

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 583824 [details] [diff] [review]
patch 1

r=me.  Thanks!

Comment 18

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c02f25c177e3

Comment 19

[Ed Morley [:emorley]]

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?

Comment 20

[Patrick McManus [:mcmanus]]

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.

Comment 21

[Alex Keybl [:akeybl]]

(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.

Comment 22

[u279076]

Mone, would you be able to verify this fixed in Firefox 12.0b3?

Comment 23

[Mone]

Hi All,

Yes I confirm that the issue is solved on Firefox 12, at least since a2 (2012-03-13), thanks everybody :)

Comment 24

[u279076]

Marking verified based on comment 23.