Last Comment Bug 504014 - (CVE-2011-3670) Requests using IPv6 hostname syntax through HTTP proxies may generate errors
(CVE-2011-3670)
: Requests using IPv6 hostname syntax through HTTP proxies may generate errors
Status: RESOLVED FIXED
[sg:low]
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla7
Assigned To: Michal Novotny (:michal)
:
Mentors:
http://pseudo-flaw.net/p/proxy-testin...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-13 20:59 PDT by Gregory Fleischer
Modified: 2012-12-26 09:18 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
.26+
.26-fixed


Attachments
fix (6.64 KB, patch)
2011-06-03 05:06 PDT, Michal Novotny (:michal)
bzbarsky: review-
Details | Diff | Splinter Review
patch v2 (9.91 KB, patch)
2011-06-14 07:14 PDT, Michal Novotny (:michal)
bzbarsky: review+
Details | Diff | Splinter Review
patch v3 (10.01 KB, patch)
2011-06-20 07:59 PDT, Michal Novotny (:michal)
jaas: superreview+
Details | Diff | Splinter Review
1.9.2 back-port w/out RangedPtr (5.00 KB, patch)
2011-09-19 17:21 PDT, Daniel Veditz [:dveditz]
michal.novotny: review-
dveditz: approval1.9.2.23-
Details | Diff | Splinter Review
patch for 1.9.2 (8.23 KB, patch)
2011-12-03 05:01 PST, Michal Novotny (:michal)
dveditz: approval1.9.2.26+
Details | Diff | Splinter Review

Description Gregory Fleischer 2009-07-13 20:59:35 PDT
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.
Comment 1 Daniel Veditz [:dveditz] 2009-11-06 19:47:03 PST
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).
Comment 2 Boris Zbarsky [:bz] 2009-11-06 20:54:55 PST
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?
Comment 3 Christian :Biesinger (don't email me, ping me on IRC) 2009-11-07 02:49:17 PST
http://[example.com] is not valid syntax for anything...
Comment 4 Daniel Veditz [:dveditz] 2011-04-18 10:26:35 PDT
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/)
Comment 5 Michal Novotny (:michal) 2011-06-03 05:06:06 PDT
Created attachment 537123 [details] [diff] [review]
fix

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.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-03 14:42:34 PDT
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.
Comment 7 Boris Zbarsky [:bz] 2011-06-07 13:38:11 PDT
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...
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-07 13:53:36 PDT
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).
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-07 14:35:44 PDT
...and it's landed in m-c now, so you should be good to go.
Comment 10 Michal Novotny (:michal) 2011-06-14 07:14:29 PDT
Created attachment 539195 [details] [diff] [review]
patch v2

> >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...
Comment 11 Boris Zbarsky [:bz] 2011-06-15 11:07:12 PDT
> 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.
Comment 12 Boris Zbarsky [:bz] 2011-06-15 12:29:40 PDT
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.
Comment 13 Michal Novotny (:michal) 2011-06-20 07:59:38 PDT
Created attachment 540466 [details] [diff] [review]
patch v3
Comment 14 Michal Novotny (:michal) 2011-06-23 08:07:47 PDT
http://hg.mozilla.org/mozilla-central/rev/8b5646a07963
Comment 15 christian 2011-08-29 10:15:38 PDT
Does this affect 1.9.2? If so we'll need to take it for 1.9.2.21
Comment 16 Daniel Veditz [:dveditz] 2011-09-17 09:31:30 PDT
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.
Comment 17 Daniel Veditz [:dveditz] 2011-09-19 17:21:16 PDT
Created attachment 561085 [details] [diff] [review]
1.9.2 back-port w/out RangedPtr
Comment 18 Michal Novotny (:michal) 2011-09-23 15:51:08 PDT
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?
Comment 19 Daniel Veditz [:dveditz] 2011-12-02 14:15:33 PST
Still need that 1.9.2 version of a patch. Are you still working on it Michal?
Comment 20 Michal Novotny (:michal) 2011-12-03 05:01:36 PST
Created attachment 578826 [details] [diff] [review]
patch for 1.9.2
Comment 21 Daniel Veditz [:dveditz] 2012-01-02 22:16:54 PST
Comment on attachment 578826 [details] [diff] [review]
patch for 1.9.2

Approved for 1.9.2.26, a=dveditz
Comment 22 Michal Novotny (:michal) 2012-01-10 14:14:13 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a46281bc813e
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-24 14:19:58 PST
@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.
Comment 24 kbtomo30 2012-12-26 09:18:09 PST
(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

Note You need to log in before you can comment on or make changes to this bug.