Gregory Fleischer Reporter
	Description • 15 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.1pre) Gecko/20090713 Shiretoko/3.5.1pre

In Firefox, it is possible to make requests using IPv6 syntax (http://[example.com]/) via XMLHttpRequest objects.  If an HTTP proxy has been configured, the request will be handled by the proxy.

Depending on the proxy implementation a number of possible errors may occur.  For example, the proxy may not be prepared to handle the IPv6 syntax and will incorrectly parse the request leading to an error.

Error messages from HTTP proxies often include sensitive network diagnostic information such as client IP addresses, internal hostnames, email addresses and possibly a copy of the HTTP request.  

Because http://example.com/ and http://[example.com]/ are same origin, the XMLHttpRequest object can be used to read this information from the response.

A remote site may be able to construct such requests in order to reduce a user's privacy.  Additionally, if a copy of the HTTP request is included in the error response, it may be possible to read cookies marked as HttpOnly in XSS situations.


Reproducible: Always

Steps to Reproduce:
1. Configure HTTP proxy
2. Visit listed URL
3. Run test



Tested with squid-cache proxy (squid/3.0.STABLE16) with default configuration.

	Daniel Veditz [:dveditz]
	Comment 1 • 15 years ago

Why are we treating these two forms as same-origin? We don't treat a site and its IP address as same-origin, and we don't treat http://accounting/ and http://accounting.mycompany.com/ as same-origin. They might be, they probably are, but we don't _know_ so we don't.   "example.com" and "[example.com]" should be handled the same way.

It's not just XHR, two windows/frames are treated as same-origin when they may not be if IPv6 is routed somewhere differently than the IPv4 version (due to a proxy or whatever reason).

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 15 years ago

Hmm.  That happens because we separately compare host and port.  And for http://[example.com]/ necko reports "example.com" as the host, but "[example.com]" as the hostPort.

It should probably report [example.com] as the host, no?

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 3 • 15 years ago

http://[example.com] is not valid syntax for anything...

	Daniel Veditz [:dveditz]
	Comment 4 • 13 years ago

So does this bug morph into "enforce valid rfc3986 syntax"? That would solve the initial proxy abuse problem and the same-origin issue by simply disallowing such URIs. While we're at it we should fix IPv4 addresses to follow the spec an disallow malformed numerics (e.g. http://1249763378/ is invalid and should not go to Google as an alias for http://74.125.224.50/)

	Michal Novotny [:michal] Assignee
	Comment 5 • 13 years ago

Tryserver seems to be green as much as possible.


(In reply to comment #4)
> So does this bug morph into "enforce valid rfc3986 syntax"? That would solve
> the initial proxy abuse problem and the same-origin issue by simply
> disallowing such URIs. While we're at it we should fix IPv4 addresses to
> follow the spec an disallow malformed numerics (e.g. http://1249763378/ is
> invalid and should not go to Google as an alias for http://74.125.224.50/)

This isn't easy to do. I've also implemented net_IsValidIPv4Addr() and tried to filter out any invalid IPv4/IPv6 address in nsHostResolver::ResolveHost() (http://hg.mozilla.org/mozilla-central/annotate/8fff6db8b016/netwerk/dns/nsHostResolver.cpp#l538). But the same "invalid" conversion is done by system's getaddrinfo(). So if we really don't want to interpret hostname 1249763378 as IP address 74.125.224.50, we can't call PR_GetAddrInfoByName(). But IMO (I can be wrong) we can't block URIs like http://123/ because 123 can be valid NetBIOS name. Also if there is some search domain in the system, it can be resolved as 123.domain.

	Jeff Walden [:Waldo]
	Comment 6 • 13 years ago

Comment on attachment 537123 [details] [diff] [review]
fix

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

Just some driveby comments I have from looking at this out of curiosity...

::: netwerk/base/src/nsURLHelper.cpp
@@ +991,5 @@
>      return PR_StringToNetAddr(strhost.get(), &addr) == PR_SUCCESS;
>  }
> +
> +PRBool
> +net_IsValidIPv6Addr(const char *addr, PRInt32 addrLen)

I seem to recall this being the Necko way, handing around raw pointers and stuff like this.  But must it be?  I bet many people would be happier if the data were passed along in some way that provided debug-build bounds-checking.  A dependent string or similar would give this to you.

@@ +1012,5 @@
> +                colons++;
> +            } else if (colons == 1) {
> +                colons++;
> +                if (zeroes)
> +                    return PR_FALSE; // only one occurance is allowed

occurrence

::: netwerk/base/src/nsURLHelper.h
@@ +250,5 @@
>  
> +/**
> + * Checks if the IPv6 address is valid according to RFC 3986 section 3.2.2.
> + */
> +NS_HIDDEN_(PRBool) net_IsValidIPv6Addr(const char *addr, PRInt32 addrLen = -1);

Explicitly specified length, except when it's not?  Ugh.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 13 years ago

Comment on attachment 537123 [details] [diff] [review]
fix

>Bug 504014 - Requests using IPv6 hostname syntax through HTTP proxies may generate errors

How about: "Bug 504014.  Enforce RFC 3986 syntax for IPv6 literals."

That has two benefits:

1) It describes what the patch actually does.
2) It does not leak information about the security issue.

>+    const char *end = addr + (addrLen == -1 ? strlen(addr) : addrLen);

Given that the only caller so far has the length, I think we should make the length required and not compute it here.

>+    const char *p;

Now that we can, please make this a mozilla::RangePtr.

>+    PRBool zeroes = PR_FALSE; // true if double colon is present in the address

Maybe haveZeros?  Otherwise it ends up looking like a counter akin to digits/colons/blocks, not a boolean.

>+    for (p=addr ; p < end ; p++) {

spaces around '=' please.

>+        if (*p == ':') {

I think you can move the |colons++| out of the two cases its in and to the bottom of this block.

Alternately, you could replace the if cascade with a switch on colons++.  Either way.

>+                    return PR_FALSE; // only one occurance is allowed

"occurrence"

>+                // too much colons in a row

"too many colons in a row"

>+            if (digits == 4) // too much digits

"too many digits"

>+            // invalid character

I don't believe this matches the RFC 3986 BNF.  For example, this will disallow the following valid IPv6 address:

  ::192.168.1.1

(see the definition of "1s32" in section 3.2.2).

>+    if ((zeroes && blocks < 8) ||
>+        (!zeroes && blocks == 8))
>+        return PR_TRUE;
>+
>+    return PR_FALSE;

Probably simpler as:

  return (zeroes && blocks < 8) || (!zeroes && blocks == 8);

>+ * Checks if the IPv6 address is valid according to RFC 3986 section 3.2.2.

s/if/whether/

>+    if (*hostnameLen > 1 && *(serverinfo + *hostnamePos) == '[' &&
>+        *(serverinfo + *hostnamePos + *hostnameLen - 1) == ']' &&
>+        !net_IsValidIPv6Addr(serverinfo + *hostnamePos + 1, *hostnameLen - 2))

I really wish there were a way to make this more readable, but it's eluding me at the moment.  :(

r- because the syntax this is enforcing is incomplete...

	Jeff Walden [:Waldo]
	Comment 8 • 13 years ago

mozilla::RangedPtr is bug 662001, only landed in TM at the moment.  I'll land it in m-c singly, since it sounds like a TM->m-c merge is not immediately forthcoming (having just happened within a day or so).

	Jeff Walden [:Waldo]
	Comment 9 • 13 years ago

...and it's landed in m-c now, so you should be good to go.

	Michal Novotny [:michal] Assignee
	Comment 10 • 13 years ago

> >Bug 504014 - Requests using IPv6 hostname syntax through HTTP proxies may generate errors
> 
> How about: "Bug 504014.  Enforce RFC 3986 syntax for IPv6 literals."

I've changed the description in the patch. Not sure if you meant to rename the bug too...

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 13 years ago

> Not sure if you meant to rename the bug too

I did not.  There is absolutely no reason the patch description (which is a description of what changes are being made) should match a bug summary (which is a description of a problem).  In fact, any time they match one or the other is probably wrong.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 13 years ago

Comment on attachment 539195 [details] [diff] [review]
patch v2

>+++ b/netwerk/base/src/nsURLHelper.cpp

>+    PRInt32 cnt = 0;    // number of dec-octets in the address

It's really number of '.' seen...  In fact, I'd rename it to dotCount or something like that (and fix the comment).

>+    for (; addrLen-- ; p++) {

I'd prefer:

  for (; addrLen; ++p, --addrLen)

to make it explicit what's test and what's loop increment.  Since addrLen is
not used in the loop, this should be fine.

>+            if (octet<0 || octet>255) {

I believe this will fail in cases when enough overflow happens to wrap all the way around.  It would be better to do the >255 check after the "multiply by 10 and add the digit" step.  You would still need the <0 check here, but add spaces around the '<'.

>+net_IsValidIPv6Addr(const char *addr, PRInt32 addrLen)
>+    for (; addrLen-- ; p++) {

As above.

>+            if (!net_IsValidIPv4Addr(p.get() - digits, addrLen + digits + 1))

The +1 would need to go away when addrLen is not modified until the increment step of the for loop.

r=me with those nits fixed.

	Michal Novotny [:michal] Assignee
	Comment 14 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/8b5646a07963

	LegNeato
	Comment 15 • 13 years ago

Does this affect 1.9.2? If so we'll need to take it for 1.9.2.21

	Daniel Veditz [:dveditz]
	Comment 16 • 13 years ago

It does affect 1.9.2, but the fix depends on a trunk-only feature (RangedPtr) and there might be some site breakage if people were relying on broken behavior.

	Michal Novotny [:michal] Assignee
	Comment 18 • 13 years ago

Comment on attachment 561085 [details] [diff] [review]
1.9.2 back-port w/out RangedPtr

> +PRBool
> +net_IsValidIPv4Addr(const char *addr, PRInt32 addrLen)
> +{
> +    const char *p;
> +
> +    PRInt32 octet = -1;   // means no digit yet
> +    PRInt32 dotCount = 0; // number of dots in the address
> +
> +    for (; addrLen; ++p, --addrLen) {

You need to assign addr into p somewhere, e.g.

  for (p=addr; addrLen; ++p, --addrLen)

And the same in net_IsValidIPv6Addr().


Why didn't you include test_bug504014.js in this backport patch?

	Daniel Veditz [:dveditz]
	Comment 19 • 13 years ago

Still need that 1.9.2 version of a patch. Are you still working on it Michal?

	Daniel Veditz [:dveditz]
	Comment 21 • 13 years ago

Comment on attachment 578826 [details] [diff] [review]
patch for 1.9.2

Approved for 1.9.2.26, a=dveditz

	Michal Novotny [:michal] Assignee
	Comment 22 • 13 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a46281bc813e

	u279076
	Comment 23 • 13 years ago

@Greg, I'm not set up with any proxy server and I'm not technically inclined to do so. Would it be possible for you to verify the fix? If so you can use the following build:
ftp://ftp.mozilla.org/pub/firefox/nightly/3.6.26-candidates/build1/

Thanks in advance.

	kbtomo30
	Comment 24 • 12 years ago

(In reply to Gregory Fleischer from comment #0)
> User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US;
> rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11
> Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.1pre)
> Gecko/20090713 Shiretoko/3.5.1pre
> 
> In Firefox, it is possible to make requests using IPv6 syntax
> (http://[example.com]/) via XMLHttpRequest objects.  If an HTTP proxy has
> been configured, the request will be handled by the proxy.
> 
> Depending on the proxy implementation a number of possible errors may occur.
> For example, the proxy may not be prepared to handle the IPv6 syntax and
> will incorrectly parse the request leading to an error.
> 
> Error messages from HTTP proxies often include sensitive network diagnostic
> information such as client IP addresses, internal hostnames, email addresses
> and possibly a copy of the HTTP request.  
> 
> Because http://example.com/ and http://[example.com]/ are same origin, the
> XMLHttpRequest object can be used to read this information from the response.
> 
> A remote site may be able to construct such requests in order to reduce a
> user's privacy.  Additionally, if a copy of the HTTP request is included in
> the error response, it may be possible to read cookies marked as HttpOnly in
> XSS situations.
> 
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. Configure HTTP proxy
> 2. Visit listed URL
> 3. Run test
> 
> 
> 
> Tested with squid-cache proxy (squid/3.0.STABLE16) with default
> configuration.

AWESOME AND AMAZING
http://twitter.com/kbtomo30