280280 - "No proxy for" does string suffix comparison, not domain comparison

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. Add "you.example.net" to "no proxy for:" in prefs
2. Create host hello-you.example.net,
   which does not resolve inside the local network,
   but is reachable for the proxy
3. Try to go to <http://hello-you.example.net>

Actual result:
"Cannot find hello-you.example.net"
because Mozilla does not go through the proxy for that host,
but tries to resolve and access it directly,
as if it were in "no proxy for" list

Expected result:
Access host through proxy.
It is *not* in the "no proxy for" list.
hello-you.example.net is *not* a subnet of you.example.net, but a sibling.
hello.you.example.net is a subnet of you.example.net.

Code:
See 277288 comment 0.
<http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsProtocolProxyService.cpp#488>

uhm...

Comment 1

[Ben Bucksch (:BenB)]

> See 277288 comment 0.

Make that linking correctly: bug 277288 comment 0.
It has a simple suggestion, to add a dot before the comparison, if there isn't
one already. But need to do that for both compared hostnames.

Comment 2

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

I have a fix for this, implemented the above (good idea, Benjamin Chuang!).

noproxy example.net, *example.net, .example.net and *.example.net are now
equivalent and match anything ending with .example.net plus the host
example.net, but not my-example.net.
This should work as expected, with one exception:
*example.net used to match my-example.net, but no longer does.

Comment 3

[Ben Bucksch (:BenB)]

*** Bug 277288 has been marked as a duplicate of this bug. ***

Comment 4

[Darin Fisher]

Comment on attachment 172772 [details] [diff] [review]
Fix, v1

>Index: netwerk/base/src/nsProtocolProxyService.cpp

>+    else if (host[0] != '.') // hello-you.example.net not match you.example.net
>+      host = NS_LITERAL_CSTRING(".") + host;

nit: indents are 4 whitespace characters in this file.

nit: use |host.Insert('.', 0)| instead.  that way you can reuse
available storage space in |host|.


>-            hinfo->name.host = ToNewCString(Substring(str, startIndex, endIndex));
>+            nsCAutoString dotted(Substring(str, startIndex, endIndex));
>+            if (dotted[0] != '.') // hello-you.example.net not you.example.net
>+              dotted = NS_LITERAL_CSTRING(".") + dotted;
>+            hinfo->name.host = ToNewCString(dotted);

hmm... can you avoid constructing |dotted| when not necessary?
maybe that isn't a big deal since this code is only run when
reading the pref, but it's usually nice to avoid doing extra
work when you don't have to.


also, i'm curious:  why did you not try to fix this in the code
that actually performs the host comparisons?  it would seem that
you could fix that code instead to make it check that the lead
character is a dot.  that would seem like a smaller/simpler patch
to fix this bug, no?

Comment 5

[Darin Fisher]

here's the host comparison code:

http://lxr.mozilla.org/mozilla/source/netwerk/base/src/nsProtocolProxyService.cpp#488

why not just fix that?

Comment 6

[Ben Bucksch (:BenB)]

Because it's easier to add a dot to the string than to special-case the
comparison (4 cases).

Comment 7

[OstGote!]

*** Bug 286746 has been marked as a duplicate of this bug. ***

Comment 8

[benc]

Ben: What's next?

Comment 9

[Manish Goregaokar [:manishearth]]

Taking this, I have a patch

Comment 10

[Manish Goregaokar [:manishearth]]

Attachment: Patch v2

This fix seems to work.

accounts.google.com now bypasses the proxy when the ignore list has one of:

 - *.google.com
 - google.com
 - .google.com
 - accounts.google.com

but not s.google.com

Note that if you plan on testing this with non-resolving proxies, you'll hit bug 1215784, which will crash. Reversing the patch from bug 1121800 is the temporary fix. Treating `nc -l 8000` on localhost as a proxy works for testing purposes.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8675248 [details] [diff] [review]
Patch v2

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

I'm not a Necko peer, sorry; the patch looks reasonable enough, but I didn't examine the logic with a fine-toothed comb.

Comment 12

[Manish Goregaokar [:manishearth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a8e7b6b3cf

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 8675248 [details] [diff] [review]
Patch v2

I'm going to ask daniel to do this

Comment 14

[Daniel Stenberg [:bagder]]

Comment on attachment 8675248 [details] [diff] [review]
Patch v2

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

It would be *really* good to have a test case or two to accompany this patch since clearly this slipped through before we know no existing test cases will catch a possible logical flaw here...

::: netwerk/base/nsProtocolProxyService.cpp
@@ +772,5 @@
>                  const char *host_tail = host.get() + host_len - filter_host_len;
> +                if (!PL_strncasecmp(host_tail, hinfo->name.host, filter_host_len)) {
> +                    // If the tail of the host string matches the filter
> +
> +                    if (filter_host_len > 0 && hinfo->name.host[0] == '.') {

Can 'filter_host_len' truly be zero at this point? A zero length host name sounds wrong and it would simplify the logic if we could skip that check. If it really can be zero length, then wouldn't it be suitable to also skip the previous string comparison asince matching with a zero length string seems unnecessary?

@@ +784,5 @@
> +                    // We check that the filter doesn't start with a `.`. If it does,
> +                    // then the strncasecmp above should suffice. If it doesn't,
> +                    // then we should only consider it a match if the strncasecmp happened
> +                    // at a subdomain boundary
> +                    if (host_len > filter_host_len && *(host_tail - 1) == '.') {

how about host_tail[-1] == '.' ? It matches how you use indexes elsewhere.

Comment 15

[Manish Goregaokar [:manishearth]]

> It would be *really* good to have a test case or two to accompany this patch
> since clearly this slipped through before we know no existing test cases
> will catch a possible logical flaw here...


I'm not sure how easy it is to test proxies. I could file a followup.

> ::: netwerk/base/nsProtocolProxyService.cpp
> @@ +772,5 @@
> >                  const char *host_tail = host.get() + host_len - filter_host_len;
> > +                if (!PL_strncasecmp(host_tail, hinfo->name.host, filter_host_len)) {
> > +                    // If the tail of the host string matches the filter
> > +
> > +                    if (filter_host_len > 0 && hinfo->name.host[0] == '.') {
> 
> Can 'filter_host_len' truly be zero at this point? A zero length host name
> sounds wrong and it would simplify the logic if we could skip that check. If
> it really can be zero length, then wouldn't it be suitable to also skip the
> previous string comparison asince matching with a zero length string seems
> unnecessary?

I'd rather not segfault if the length ever becomes zeroable. I can move the check up though.


> how about host_tail[-1] == '.' ? It matches how you use indexes elsewhere.

I'll fix that.

Comment 16

[Manish Goregaokar [:manishearth]]

Attachment: Patch v3

Fixed nits.

Comment 17

[Manish Goregaokar [:manishearth]]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a8e7b6b3cf (previous patch, but the patch hasn't changed much since then)

Comment 18

[Pulsebot]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f9323837e0d2