Patrick, do you think you can find someone to investigate this further?
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.
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.
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.
The commit itself is public, here https://chromium.googlesource.com/chromium/src.git/+/81357b39c643fc746517fd6ce5cb2076b7ddc3f4
That commit pretty much says all we need...
Assignee: nobody → daniel
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 =)
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
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 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
daniel - can you confirm you double checked with chrome having shipped this? web compat concerns.
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
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?
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
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?
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
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+
Hi :bagder, Is this worth uplifting to 50 beta or 51 aurora?
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.
Hi :dveditz, Do you think this fix worth uplifting to 50 beta or 51 aurora?
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.
Mark 50 fixed as it's closed to RC. Hi :bagder, Can you help create uplift request for 51 aurora?
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]:
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+
Documentation needs to reflect this change because it may affect proxy configuration used by IT admins in enterprise.
Whiteboard: [necko-active] → [necko-active][adv-main51+]
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
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.