Closed Bug 280280 Opened 15 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: BenB, Assigned: manishearth)

References

Details

Attachments

(1 file, 2 obsolete files)

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...
> 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.
Attached patch Fix, v1 (obsolete) — Splinter Review
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.
Assignee: darin → ben.bucksch
Status: NEW → ASSIGNED
Attachment #172772 - Flags: review?(darin)
*** Bug 277288 has been marked as a duplicate of this bug. ***
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?
Attachment #172772 - Flags: review?(darin) → review-
here's the host comparison code:

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

why not just fix that?
Because it's easier to add a dot to the string than to special-case the
comparison (4 cases).
*** Bug 286746 has been marked as a duplicate of this bug. ***
Ben: What's next?
Taking this, I have a patch
Assignee: ben.bucksch → manishearth
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #172772 - Attachment is obsolete: true
Attachment #8675248 - Flags: review?(nfroyd)
Attachment #8675248 - Flags: feedback?(darin.moz)
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.
Attachment #8675248 - Flags: review?(nfroyd) → review?(mcmanus)
Attachment #8675248 - Flags: feedback?(darin.moz)
Comment on attachment 8675248 [details] [diff] [review]
Patch v2

I'm going to ask daniel to do this
Attachment #8675248 - Flags: review?(mcmanus) → review?(daniel)
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.
Attachment #8675248 - Flags: review?(daniel) → 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...


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.
Attached patch Patch v3Splinter Review
Fixed nits.
Attachment #8675248 - Attachment is obsolete: true
Attachment #8676742 - Flags: review?(daniel)
Attachment #8676742 - Flags: review?(daniel) → review+
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a8e7b6b3cf (previous patch, but the patch hasn't changed much since then)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f9323837e0d2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.