318006 - DNS checks not being consistently applied to URLs when proxy configured

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Neil Harris]

Step 1: Install tinyproxy, set browser to use tinyproxy via port 8888 (tinyproxy v 1.6.3 in this case)

Step 2: Visit http://bad+transformers.istheshit.net/ (which contains an invalid "+" character in its domain name label: istheshit.net appears to do wildcard DNS, then generate amusing output based on the FQDN given)

What happens: 

tinyproxy returns a page containing the following:

Cache Error!
The following error has occured: tinyproxy was unable to connect to the remote web server.

As far as I can tell this means that, the URL is simply being handed off to the proxy without checking for DNS name validity, and the proxy is then rejecting the URL as invalid itself. The browser then blindly displays the content returned by the proxy. 

This risks the exposure of various DNS and/or Unicode spoofing problems which the paranoid DNS name checking code is designed to prevent, were the proxy to be permissive about what it passes to the DNS.

What should happen:

Mozilla should validate URLs for valid syntax before handing them off to proxies, and, in this case, display the standard warning message page 

"Server not found:
Firefox can't find the server at bad+transformers.istheshit.net." 

or possibly a more accurate:

"Server not found:
The name bad+transformers.istheshit.net. is not a valid server name" 

I have not yet investigated any of the other possible problems this might be leaking (Unicode display problems, %-escape issues, etc.). 

However, I think that this is a significant cause for concern, given that this alternative logic path circumvents at least one of the several checks that have been added to the DNS logic with great effort with the intent of preventing DNS/URL spoofing: it is unlikely that all DNS proxies will be as cautious as our DNS code.

Comment 1

[Jesse Ruderman]

According to Reed in bug 317946, some proxies (at least Google Web Accelerator) do not block "+" in hostnames, which Firefox blocks when not using a proxy.

Comment 2

[Neil Harris]

Visiting

http://xxxתנyyyy.istheshit.net/

(where the two characters between xxx and yyyy are Hebrew characters) also causes different behaviour with and without the proxy.

Without a proxy, you get the "Server not found: Firefox can't find the server at xxxתנyyyy.istheshit.net." error page.

With the proxy, you get through to the proxy, which then returns its own error message, suggesting that a more permissive proxy might cause peculiar things to happen.

Comment 3

[Neil Harris]

Note that the URL above has been scrambled by Bugzilla character encoding issues: you will have to insert your own Hebrew characters to replicate the example.

Comment 4

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

use view|character coding|utf-8 before entering non-ASCII characters

Comment 5

[Andreas Otte]

We do check proxys, but only under certain conditions. See this code from nsSocketTransport2.cpp:

nsresult
nsSocketTransport::ResolveHost()
{
    LOG(("nsSocketTransport::ResolveHost [this=%x]\n", this));

    nsresult rv;

    if (!mProxyHost.IsEmpty()) {
        if (!mProxyTransparent || mProxyTransparentResolvesHost) {
            // When not resolving mHost locally, we still want to ensure that
            // it only contains valid characters. See bug 304904 for details.
            if (!net_IsValidHostName(mHost))
                return NS_ERROR_UNKNOWN_HOST;

...

Maybe we should check regardless ...

Comment 6

[Neil Harris]

(In reply to comment #5)
> 
> Maybe we should check regardless ...
> 

Yes, I think so, unless there's a good reason not to: AFAIK, valid URLs require that the hostname part, if present, to be a valid domain name. See RFC 1738:

    host
        The fully qualified domain name of a network host, or its IP
        address as a set of four decimal digit groups separated by
        ".". Fully qualified domain names take the form as described
        in Section 3.5 of RFC 1034 [13] and Section 2.1 of RFC 1123
        [5]: a sequence of domain labels separated by ".", each domain
        label starting and ending with an alphanumerical character and
        possibly also containing "-" characters. The rightmost domain
        label will never start with a digit, though, which
        syntactically distinguishes all domain names from the IP
        addresses.

Obviously, we need to be _slightly_ looser than this for interoperability reasons, and things like IPv6 (yes, there's a new RFC for this, but I can't remember the number: the code already allows for this) but this is already implemented in the current pragmatic and only-slightly-more-permissive DNS name-checking code. 

Do you know what the reasoning was behind the program logic of the quoted code?

Comment 7

[Neil Harris]

Looking at the code, 
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsSocketTransport2.cpp#895
would the fix be as simple as to change

 if (!mProxyTransparent || mProxyTransparentResolvesHost) 

to 

 if (1)

and then remove this now-redundant if-statement?

Comment 8

[Andreas Otte]

(In reply to comment #7)
> Looking at the code, 
> http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsSocketTransport2.cpp#895
> would the fix be as simple as to change
> 
>  if (!mProxyTransparent || mProxyTransparentResolvesHost) 
> 
> to 
> 
>  if (1)
> 
> and then remove this now-redundant if-statement?
> 

Yes, I think so, unless there are other places in the code that access proxys that I'm not aware off.

Comment 9

[Andreas Otte]

(In reply to comment #6)
> 
> Do you know what the reasoning was behind the program logic of the quoted code?

No, but if you search for discussions on what a hostname has to look like or how long it can be you will find tons of it. There is no definite answer. 

Comment 10

[Andreas Otte]

I've just attached a patch on bug 309671 for the 1.8 branch which includes the change to look for a valid host in a case when using a proxy.

Comment 11

[Neil Harris]

(In reply to comment #9)
> (In reply to comment #6)
> > 
> > Do you know what the reasoning was behind the program logic of the quoted code?
> 
> No, but if you search for discussions on what a hostname has to look like or
> how long it can be you will find tons of it. There is no definite answer. 
> 

I agree: there is no definite answer. given the multiple conflicting documents and the vagaries of actual practice -- but for sanity's sake, the software should have only a single piece of code that defines what it considers to be valid, and it looks like net_IsValidHostName() should be it.

Comment 12

[Daniel Veditz [:dveditz]]

blocking 1.8.0.2 denied, not a regression, no trunk-baked patch.

Comment 13

[Darin Fisher]

Is anyone able to reproduce this bug using a nightly build of Firefox 2.0 alpha?  As far as I can tell, we are properly blocking invalid hostnames before communicating to the proxy server.

What version of the browser was tested?

Comment 14

[Darin Fisher]

IE allows '+' in hostname, so it's possible that this impacts more than just this site.  I'm going to go ahead and allow the hostname because I don't see it as a significant mechanism to spoof users.  It does look like a little bit like a 't', but given that second-level domain nodes won't have '+' chars in them (if we trust the registrars), I don't worry that much.

Comment 15

[Darin Fisher]

OK, I figured out what is going on here.  The problem is that the connection to the proxy server may be a reused "keep-alive" connection, which when originally constructed may have been loading a valid URL.  As a result, we would never apply the net_IsValidHostName check to subsequently loaded URLs that get fetched using that "keep-alive" connection.  The real solution, I think, is to add host checking code to somewhere in the HTTP implementation.

Comment 16

[Darin Fisher]

Attachment: v1 patch

OK, apply the hostname restriction earlier, and... allow hostnames with '+' chars.

Comment 17

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

Comment on attachment 228500 [details] [diff] [review]
v1 patch

r=biesi, but since this is a malformed hostname, I wonder if maybe NS_ERROR_MALFORMED_URI might be a better error?

Comment 18

[Darin Fisher]

There's another bug on file to introduce a better error code for invalid hostnames.  Since this checking is now done in three places, I'd like to change all of the error codes uniformly.  Unfortunately, we can't introduce that better error UI in FF2 since it would require l10n changes.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 228500 [details] [diff] [review]
v1 patch

sr=dveditz

Comment 20

[Darin Fisher]

Comment on attachment 228500 [details] [diff] [review]
v1 patch

Low-risk patch.  Seems like a good thing from a consistency point of view.    Proxy users are impacted by the change to nsHttpChannel::Connect, and all users are impacted by the other change (which allows "+" chars in hostnames).

Comment 21

[Darin Fisher]

fixed-on-trunk

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 228500 [details] [diff] [review]
v1 patch

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have done so.

Comment 23

[Darin Fisher]

fixed1.8.1

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 228500 [details] [diff] [review]
v1 patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 25

[Darin Fisher]

fixed1.8.0.7

Comment 26

[alice nodelman [:alice] [:anode]]

Firefox server not found error message not displayed for incorrectly-formed urls using both direct connections to the internet or a proxy.  Also, allow urls with '+'.

tested with url: http://xxx&/#1514;נyyyy.istheshit.net/

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060831 Firefox/1.5.0.7

verified 1.8.0.7

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060901 BonEcho/2.0b2

verified 1.8.1b2