Closed Bug 318006 Opened 18 years ago Closed 17 years ago

DNS checks not being consistently applied to URLs when proxy configured


(Core :: Networking, defect)

Not set





(Reporter: usenet, Assigned: darin.moz)


(Blocks 1 open bug, )


(Keywords: regression, verified1.8.0.7, verified1.8.1, Whiteboard: [sg:spoof])


(1 file)

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

Step 2: Visit (which contains an invalid "+" character in its domain name label: 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" 

or possibly a more accurate:

"Server not found:
The name 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.
Blocks: 316730
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.
Flags: blocking1.8.0.1?
Whiteboard: [sg:low] spoof
Assignee: nobody → darin
Component: General → Networking
Product: Firefox → Core
Whiteboard: [sg:low] spoof → [sg:investigate]
Version: 1.5 Branch → Trunk


(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תנ" 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.

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.
use view|character coding|utf-8 before entering non-ASCII characters
We do check proxys, but only under certain conditions. See this code from nsSocketTransport2.cpp:

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

        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

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?

Looking at the code,
would the fix be as simple as to change

 if (!mProxyTransparent || mProxyTransparentResolvesHost) 


 if (1)

and then remove this now-redundant if-statement?
(In reply to comment #7)
> Looking at the code, 
> 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.

(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'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.
(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.
Depends on: 320568
QA Contact: general → benc
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
blocking denied, not a regression, no trunk-baked patch.
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2-
Whiteboard: [sg:investigate] → [sg:spoof]
Flags: blocking1.8.1? → blocking1.8.1+
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?
Whiteboard: [sg:spoof] → [sg:spoof] qawanted
Keywords: qawanted
Whiteboard: [sg:spoof] qawanted → [sg:spoof]
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.
Flags: blocking1.8.0.6?
Keywords: qawantedregression
Target Milestone: --- → mozilla1.8.1beta2
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.
Attached patch v1 patchSplinter Review
OK, apply the hostname restriction earlier, and... allow hostnames with '+' chars.
Attachment #228500 - Flags: superreview?(dveditz)
Attachment #228500 - Flags: review?(cbiesinger)
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?
Attachment #228500 - Flags: review?(cbiesinger) → review+
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 on attachment 228500 [details] [diff] [review]
v1 patch

Attachment #228500 - Flags: superreview?(dveditz) → superreview+
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).
Attachment #228500 - Flags: approval1.8.1?
Attachment #228500 - Flags: approval1.8.0.6?
Closed: 17 years ago
Resolution: --- → FIXED
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.
Attachment #228500 - Flags: approval1.8.1? → approval1.8.1+
Keywords: fixed1.8.1
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Comment on attachment 228500 [details] [diff] [review]
v1 patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #228500 - Flags: approval1.8.0.7? → approval1.8.0.7+
Keywords: fixed1.8.0.7
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;נ

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


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

verified 1.8.1b2
Group: security
You need to log in before you can comment on or make changes to this bug.