Bug 1732792 retry polling requests without proxy | bypasses MOZ_PROXY_BYPASS_PROTECTION and network.proxy.allow_bypass
Categories
(Core :: Networking, defect, P3)
Tracking
()
People
(Reporter: dan, Assigned: dan)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [necko-triaged])
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr102+
|
Details | Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:104.0) Gecko/20100101 Firefox/104.0
Steps to reproduce:
as per https://bugzilla.mozilla.org/show_bug.cgi?id=1732792
attempts to by pass the proxy should consult MOZ_PROXY_BYPASS_PROTECTION
and network.proxy.allow_bypass
Actual results:
Utils.fetch will bypass the proxy regardless of settings.
Expected results:
MOZ_PROXY_BYPASS_PROTECTION
and network.proxy.allow_bypass
should be respected and Utils.fetch should fail rather than bypass proxy if they specify so
Updated•2 years ago
|
Comment 1•2 years ago
|
||
The Bugbug bot thinks this bug should belong to the 'Core::Networking' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.
Comment 2•2 years ago
|
||
:robwu, since you are the author of the regressor, bug 1732792, could you take a look? Also, could you set the severity field?
For more information, please visit auto_nag documentation.
Comment 3•2 years ago
|
||
What's your use case and objective?
network.proxy.allow_bypass
is already consulted, here: https://searchfox.org/mozilla-central/rev/b1e5f2c7c96be36974262551978d54f457db2cae/toolkit/modules/ServiceRequest.jsm#163
Where does your expectation on the behavior of MOZ_PROXY_BYPASS_PROTECTION
come from?
If you're creating a build of Firefox yourself, you can use the --disable-proxy-direct-failover
build flag (which sets the MOZ_PROXY_DIRECT_FAILOVER
constant) to turn off the failover behavior.
Assignee | ||
Comment 4•2 years ago
|
||
Sorry, working on an MR to accompany this, just signing up to phabricator and getting the code checked out.
I missed that bypassProxyEnabled()
so that helps. I did the following:
https://gitlab.torproject.org/tpo/applications/tor-browser/-/merge_requests/357/diffs
I'll remove the redundant check to network.proxy.allow_bypass and try and get you a MR tomorrow morning if you think it's worth having MOZ_PROXY_BYPASS_PROTECTION
also be consulted to stop it
Assignee | ||
Comment 5•2 years ago
|
||
Updated•2 years ago
|
Comment 6•2 years ago
|
||
Could you please explain what you are trying to achieve here? You're adding an extra check in some place, but it isn't clear what you want to achieve. Without that context, it's difficult to tell whether the proposed patch is going to resolve whatever you're thinking of.
From the MR link, it looks like you're trying to cover Tor. We explicitly accounted for that use case by supporting specific flags/prefs, which I have referred to in my previous comment. If you believe that anything is missing, please provide a clear description and test case. Then I can evaluate whether there is a bug with how you're using it, or with the application itself.
Comment 7•2 years ago
|
||
Set release status flags based on info from the regressing bug 1732792
Updated•2 years ago
|
Assignee | ||
Comment 8•2 years ago
|
||
Moz configure supports a --enable-proxy-bypass-protection
flag which defines MOZ_PROXY_BYPASS_PROTECTION
This MR captures that in AppConstants.jsm exposing it to JS and then Utils.fetch will also consult that to determine if it should short circuit and not attempt a proxy bypass. It's another layer of proxy bypass protection from built time that supersedes the runtime config option of network.proxy.allow_bypass
Comment 9•2 years ago
|
||
(In reply to Dan Ballard from comment #8)
Moz configure supports a
--enable-proxy-bypass-protection
flag which definesMOZ_PROXY_BYPASS_PROTECTION
This MR captures that in AppConstants.jsm exposing it to JS and then Utils.fetch will also consult that to determine if it should short circuit and not attempt a proxy bypass. It's another layer of proxy bypass protection from built time that supersedes the runtime config option of
network.proxy.allow_bypass
So to phrase things differently - is this an attempt to make it impossible to bypass the proxy?
Or is it to have more strict-privacy-friendly defaults?
If you want to have defaults that match the intend behind the enable-proxy-bypass-protection
flag, make this pref value conditional on the MOZ_PROXY_BYPASS_PROTECTION
flag: https://searchfox.org/mozilla-central/rev/52da19becaa3805e7f64088e91e9dade7dec43c8/modules/libpref/init/StaticPrefList.yaml#10902 (i.e. value: false
). That would be the recommended way to ensure that the pref and all dependent features are disabled.
Important note (same area):
I am not very familiar with how the Tor build is configured, but it looks like the flags are set at:
- https://gitlab.torproject.org/tpo/applications/tor-browser/-/blob/5bee9df56ac4e1fe3097338854003f748ba47190/browser/config/mozconfigs/tor-browser
- https://gitlab.torproject.org/tpo/applications/tor-browser/-/blob/5bee9df56ac4e1fe3097338854003f748ba47190/browser/config/mozconfigs/base-browser
When I review the config there, I see that some action is required:
ac_add_options --disable-proxy-direct-failover
from bug 1720221 should be added, to avoid automatic failover to direct requests in some cases.- An alternative was to set
network.proxy.failover_direct
tofalse
, but I don't see that at https://gitlab.torproject.org/tpo/applications/tor-browser/-/blob/5bee9df56ac4e1fe3097338854003f748ba47190/browser/app/profile/001-base-profile.js
- An alternative was to set
Comment 10•2 years ago
•
|
||
HI Rob,
(In reply to Rob Wu [:robwu] from comment #9)
So to phrase things differently - is this an attempt to make it impossible to bypass the proxy?
Or is it to have more strict-privacy-friendly defaults?
Both, we need to ensure the browser does not access the clearnet to avoid fingerprinting/linkability issues; all network traffic coming from the browser must go through tor.
If you want to have defaults that match the intend behind the
enable-proxy-bypass-protection
flag, make this pref value conditional on theMOZ_PROXY_BYPASS_PROTECTION
flag: https://searchfox.org/mozilla-central/rev/52da19becaa3805e7f64088e91e9dade7dec43c8/modules/libpref/init/StaticPrefList.yaml#10902 (i.e.value: false
). That would be the recommended way to ensure that the pref and all dependent features are disabled.
@dan seems reasonable to me
Important note (same area):
I am not very familiar with how the Tor build is configured, but it looks like the flags are set at:
- https://gitlab.torproject.org/tpo/applications/tor-browser/-/blob/5bee9df56ac4e1fe3097338854003f748ba47190/browser/config/mozconfigs/tor-browser
- https://gitlab.torproject.org/tpo/applications/tor-browser/-/blob/5bee9df56ac4e1fe3097338854003f748ba47190/browser/config/mozconfigs/base-browser
When I review the config there, I see that some action is required:
ac_add_options --disable-proxy-direct-failover
from bug 1720221 should be added, to avoid automatic failover to direct requests in some cases.
- An alternative was to set
network.proxy.failover_direct
tofalse
, but I don't see that at https://gitlab.torproject.org/tpo/applications/tor-browser/-/blob/5bee9df56ac4e1fe3097338854003f748ba47190/browser/app/profile/001-base-profile.js
This pref is being set in browser/app/profile/000-tor-browser.js: https://gitlab.torproject.org/tpo/applications/tor-browser/-/blob/5bee9df56ac4e1fe3097338854003f748ba47190/browser/app/profile/000-tor-browser.js#L25
@dan we probably want to migrate this pref to 001-base-browser and add --disable-proxy-direct-failover /browser/config/mozconfigs/base-browser given that we enable --enable-proxy-bypass-protection there as well
EDIT: @rob we want to both disable this feature and ensure users cannot re-enable it (hence the preference for a compile time check)
Updated•2 years ago
|
Assignee | ||
Comment 11•2 years ago
|
||
Thanks, updated the MR based on feed back
- network.proxy.allow_bypass is now set to false if MOZ_PROXY_BYPASS_PROTECTION is defined
- removed my change to fetch()
- added a check to AppConstants.MOZ_PROXY_BYPASS_PROTECTION in bypassProxyEnabled
the last one is something we're comfortable with, we don't want to hand the user an option to shoot themselves in the foot if we've compiled the browser this was but may not be something you care for, so let me know and I can pull it out of this MR and just apply it tor-browser side :)
Thanks for your help!
Comment 12•2 years ago
|
||
(In reply to Dan Ballard (Tor Browser Dev) from comment #11)
- added a check to AppConstants.MOZ_PROXY_BYPASS_PROTECTION in bypassProxyEnabled
the last one is something we're comfortable with, we don't want to hand the user an option to shoot themselves in the foot if we've compiled the browser
What is the scenario that you'd like to defend against? The user visiting about:config
and flipping random preferences?
That scenario can be countered by using lockPref
, without any code changes in m-c (except for changing the default pref value).
How does that work with other proxy-related preferences? The user can also shoot themselves in their feet, can't they?
But if you want to completely disable the functionality at compile time, some more changes are needed as explained at https://phabricator.services.mozilla.com/D157774?id=628772#inline-869052 . While ServiceRequest.jsm is currently the only direct non-test consumer of the API, there is nothing that stops the addition of new uses in the future.
Speaking of tests, this change will break some unit tests, such as https://searchfox.org/mozilla-central/rev/cefb4bc274c71aa4badbec09da9c056fd8ca4472/toolkit/modules/tests/xpcshell/test_servicerequest_xhr.js#50-62. How do you ensure that there is sufficient test coverage?
Assignee | ||
Comment 13•2 years ago
|
||
Yes I'm looking to prevent the user flipping that pref and making themselves unsafe.
So thanks! I'll look into finding a place to use lockPref on enable-proxy-bypass-protection
and follow up with the other places you mentioned to look.
It will break that test if compiled with the flag yes. My thought would be to extend the test to detect if the flag was used, and expect different results accordingly, but I'm not sure what is mozilla best practices.
Comment 14•2 years ago
|
||
(In reply to Dan Ballard (Tor Browser Dev) from comment #13)
Yes I'm looking to prevent the user flipping that pref and making themselves unsafe.
So thanks! I'll look into finding a place to use lockPref on
enable-proxy-bypass-protection
and follow up with the other places you mentioned to look.
With the lockPref
approach in Tor, you can reduce the patch in m-c to just changing the defaults in StaticPrefList.yml.
The use of lockPref
would allow us to only consider the (potentially locked) preference value as the sole source of truth for this feature, instead of the current patch, which introduces a new flag to AppConstants. The rarity of the lockPref
feature in m-c plus makes me wonder whether and how this feature can best be used.
Mike, questions:
- Would you recommend the use of
lockPref
here, and if so where? The intention is to lock a preference value to its defaults and make it obviously difficult for users to change the pref. - If yes, where should this
lockPref
call be placed?
Comment 15•2 years ago
|
||
Is there a place where tor sets preferences like an all.js?
There a mechanism for locking prefs in that file:
pref("network.proxy.failover_directI", false, locked);
Updated•2 years ago
|
Comment 16•2 years ago
|
||
Did you see the recent comments here? With that, it seems that there is a straight path forward, even without requiring changes to m-c.
Here is an example of an in-tree way to use the locked
feature that :mkaply has mentioned: https://searchfox.org/mozilla-central/rev/f0ffbdcf82000a8ecab13954f6618797e37c6173/modules/libpref/init/all.js#4352-4358
I think that something like that, in the Tor-specific build would work exactly the way you want.
Assignee | ||
Comment 17•2 years ago
|
||
awesome, thanks!
Will try that out next week and get back to you. It's looking then like all that'd be left of this MR is having the compile time flag set the pref in StaticPrefList.yaml if mozilla is interested in that at all?
Assignee | ||
Comment 18•2 years ago
|
||
Ok updated the patch to just the StaticPref setting default to false if MOZ_PROXY_BYPASS_PROTECTION is set, will do the rest in tor-browser! thanks again for the pointers!
Updated•2 years ago
|
Assignee | ||
Comment 19•2 years ago
|
||
Is there anything else that I have to do on my end now this is approved?
Comment 20•2 years ago
|
||
Nope, I went ahead and landed it. Sorry for the delay.
Comment 21•2 years ago
|
||
Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/2581ff75a3de StaticPrefList network.proxy.allow_bypass checks MOZ_PROXY_BYPASS_PROTECTION for default value r=robwu
Comment 22•2 years ago
|
||
bugherder |
Comment 23•2 years ago
|
||
The patch landed in nightly and beta is affected.
:dan, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox106
towontfix
.
For more information, please visit auto_nag documentation.
Comment 24•2 years ago
|
||
Doesn't seem like we need to get this into 106, but I could imagine Tor wanting this uplifted to ESR?
Comment 25•2 years ago
|
||
Comment on attachment 9295506 [details]
Bug 1791689 - StaticPrefList network.proxy.allow_bypass checks MOZ_PROXY_BYPASS_PROTECTION for default value r?robwu
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: TOR specific fix
- User impact if declined: None
- Fix Landed on Version: 107
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Just adds an #ifdef around a pref
Comment 26•2 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
Doesn't seem like we need to get this into 106, but I could imagine Tor wanting this uplifted to ESR?
Tor should be using the mechanism explained in comment 15 to fully achieve its goal of the functionality being non-toggleable. The patch here will ensure that the appropriate default value for the feature flag is chosen based on compile-time constants, independent of that other work.
Comment 27•2 years ago
|
||
Comment on attachment 9295506 [details]
Bug 1791689 - StaticPrefList network.proxy.allow_bypass checks MOZ_PROXY_BYPASS_PROTECTION for default value r?robwu
Approved for 102.5esr.
Updated•2 years ago
|
Comment 28•2 years ago
|
||
bugherder uplift |
Updated•2 years ago
|
Updated•2 years ago
|
Comment 29•2 years ago
|
||
Hi Mike, Is there anything for the QA to verify here in Firefox ? are there any steps ? or is this issue related to the TOR browser ?
Thank you
Comment 30•2 years ago
|
||
Nope, nothing QA needs to do. There would be no easy way to test this anyway.
Comment 31•2 years ago
|
||
Thanks Mike, removing the QA flags.
Description
•