Closed Bug 1791689 Opened 2 years ago Closed 2 years ago

Bug 1732792 retry polling requests without proxy | bypasses MOZ_PROXY_BYPASS_PROTECTION and network.proxy.allow_bypass

Categories

(Core :: Networking, defect, P3)

Firefox 102
defect

Tracking

()

RESOLVED FIXED
107 Branch
Tracking Status
firefox-esr102 107+ fixed
firefox105 --- wontfix
firefox106 --- wontfix
firefox107 --- fixed

People

(Reporter: dan, Assigned: dan)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [necko-triaged])

Attachments

(1 file)

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

Regressed by: 1732792

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.

Component: Untriaged → Networking
Product: Firefox → Core

: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.

Flags: needinfo?(rob)

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.

Flags: needinfo?(rob)

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: nobody → dan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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.

Flags: needinfo?(dan)

Set release status flags based on info from the regressing bug 1732792

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

Flags: needinfo?(dan)

(In reply to Dan Ballard from comment #8)

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

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:

When I review the config there, I see that some action is required:

See Also: → 1720221

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 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.

@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:

When I review the config there, I see that some action is required:

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)

Attachment #9295506 - Attachment description: Bug 1791689 - add check for MOZ_PROXY_BYPASS_PROTECTION in Utils.fetch r?robwu → Bug 1791689 - add check for MOZ_PROXY_BYPASS_PROTECTION in ServiceRequest.bypassProxyEnabled and definition of network.proxy.allow_bypass r?robwu

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!

(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?

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.

(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:

  1. 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.
  2. If yes, where should this lockPref call be placed?
Flags: needinfo?(mozilla)

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);

Flags: needinfo?(mozilla)
Severity: -- → S4
Priority: -- → P3
Whiteboard: [necko-triaged]

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.

Flags: needinfo?(dan)

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?

Flags: needinfo?(dan)

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!

Attachment #9295506 - Attachment description: Bug 1791689 - add check for MOZ_PROXY_BYPASS_PROTECTION in ServiceRequest.bypassProxyEnabled and definition of network.proxy.allow_bypass r?robwu → Bug 1791689 - StaticPrefList network.proxy.allow_bypass checks MOZ_PROXY_BYPASS_PROTECTION for default value r?robwu

Is there anything else that I have to do on my end now this is approved?

Nope, I went ahead and landed it. Sorry for the delay.

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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch

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 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(dan)

Doesn't seem like we need to get this into 106, but I could imagine Tor wanting this uplifted to ESR?

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
Attachment #9295506 - Flags: approval-mozilla-esr102?

(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.

Flags: needinfo?(dan)

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.

Attachment #9295506 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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

Flags: needinfo?(mozilla)

Nope, nothing QA needs to do. There would be no easy way to test this anyway.

Flags: needinfo?(mozilla)

Thanks Mike, removing the QA flags.

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: