1730920 - Https only mode shouldn't be triggered by the wifi portal code

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Julien Wajsberg [:julienw]]

The wifi portal code connects to http://detectportal.firefox.com/canonical.html, but https only (I believe) redirects this to https, which doesn't work to detect the wifi portal.

It may be that it works when checking in background but doesn't when the user clicks on the button to open the connection page.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dveditz, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Daniel Veditz [:dveditz]]

(In reply to Julien Wajsberg [:julienw] from comment #0)

It may be that it works when checking in background but doesn't when the user clicks on the button to open the connection page.

Clicks which button to open the page? Is the "connection page" one of our dev tools? Searchfox has not been helpful. It definitely works in the background checks (you can see it in the Browser Console). For foreground requests users can manually set an exception for that domain in about:preferences if this is something they need to do. Is it something that people will be doing a lot?

Comment 3

[Daniel Veditz [:dveditz]]

Ah, the infobar thingy, captivePortal.showLoginPage2:
https://searchfox.org/mozilla-central/rev/d488f68d845a87cc107612b667951152c34fb116/browser/locales/en-US/chrome/browser/browser.properties#933-940

Ah yes, we should definitely not upgrade that. This is probably due to creating an explicit null principal as the triggeringPrincipal here (the background requests have a triggeringPrincipal of the systemPrincipal). But this is being triggered by chrome code so I'm not sure what the worry is -- is there inheritance? If we can't fix the triggeringPrincipal we may have to add a property to skip https-only similar to the disableTRR that's used here.
https://searchfox.org/mozilla-central/rev/d488f68d845a87cc107612b667951152c34fb116/browser/base/content/browser-captivePortal.js#354-362

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Uh, we should fix that. I thought all captive portal detection goes through CaptivePortalDetect.jsm which we explicitly annotated with HTTPS_ONLY_EXEMPT - apparetly there is yet another case of captive portal detection.

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)

Uh, we should fix that. I thought all captive portal detection goes through CaptivePortalDetect.jsm which we explicitly annotated with HTTPS_ONLY_EXEMPT - apparetly there is yet another case of captive portal detection.

To be clear, the captive portal detection works fine (I properly get the banner that gives this information). When clicking on the button on this banner, we open again http://detectportal.firefox.com/canonical.html in a tab, and that's when there's a problem.

A possible fix would be to disable https_only for this URL.
Another possible fix is to save the redirection URL we got when detecting the captive portal in background, and use it directly. But this is a bigger change in the behavior and might bring regressions.

Comment 6

[Tomer Yavor]

Attachment: WIP: Bug 1730920 - Https only mode shouldn't be triggered by the wifi portal code. r=ckerschb

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: captive_portal.patch

Hey Tomer, I took a look at your patch from within D128387 and ran your modified testcase of browser_closeCapPortalTabCanonicalURL.js from within the same patch.

Ultimately the problem boils down to the fact that our implementation of HTTPS-Only-Mode tries to clear the flag HTTPS_ONLY_EXEMPT whenever possible to upgrade as many requests as possible. In more detail, the clearing happens twice:

• Within nsDocshell we call TestSitePermissionAndPotentiallyAddExemption. Until now we never had a usecase like the captive portal one within this bug and so TestSitePermissionAndPotentiallyAddExemption happily clears the HTTPS_ONLY_EXEMPT flag from the load.
• Further, after a redirect the DocumentLoadListener causes the setup of a ReplacementChannel which ultimately calls PotentiallyClearExemptFlag within nsHTTPChannel. Same as before, our implementation happily clears the flag HTTPS_ONLY_EXEMPT.

The problem in general is that regular web requests are not distinguishable from captive portal requests. I see two paths forward to fix this problem:

• We add a flag to the loadinfo named isCaptivePortal as implemented within this patch, or
• We query the pref names of captive portal associated requests and add a hardcoded exception for those within TestSitePermissionAndPotentiallyAddExemption and PotentiallyClearExemptFlag.

I am personally for the patch similar to what is implemented within the attached patch, though I can be convinced otherwise. When attaching the attached patch and running your modified test browser_closeCapPortalTabCanonicalURL.js I see:
["HTTPS-Only Mode: Not upgrading insecure request “http://localhost:8080/success” because it is exempt."]
["HTTPS-Only Mode: Not upgrading insecure request “http://localhost:8080/login” because it is exempt."]

Would be great if we can land an automated test for this problem as well, though that could happen as a follow up as well. Probably it's worth asking Gijs for input on the patch as well.

Comment 8

[Tomer Yavor]

Hey Christoph,
Thank you very much for taking a look into it.

I agree. To introduce a flag in the loadinfo sounds like the best solutions for the bug.

Thank you again, the patch looks great!
I will be in touch with Gijs.

Comment 9

[Tomer Yavor]

Attachment: Bug 1730920 - Https only mode shouldn't be triggered by the wifi portal code. r=ckerschb

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca4eac7e4444
Https only mode shouldn't be triggered by the wifi portal code. r=nhnt11,ckerschb

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ca4eac7e4444

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ca4eac7e4444

Comment 15

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

[Tracking Requested - why for this release]:
Users on ESR91 are still experiencing this issue (see bug 1747057).

@Tomer, should we uplift this to ESR?

Comment 16

[Tomer Yavor]

Comment on attachment 9250696 [details]
Bug 1730920 - Https only mode shouldn't be triggered by the wifi portal code. r=ckerschb

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Captive wifi portal detection works in https-first (which is enabled by default in private browsing mode) and https-only mode. Wifi access works in https-first and https-only mode as expected.
• User impact if declined: Users could not login to wifi portals in private browsing mode or if they use https-only mode or https-first mode in their browsers.
• Fix Landed on Version: 97
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): That patch only affects https-only and https-first modes behaviour and also only for captive wifi portals. So the damage that could occur is very isolated.
  The patch is also landed since 2 months and didn't lead to any error yet. That's why the risk is low to uplift the patch.

Comment 17

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9250696 [details]
Bug 1730920 - Https only mode shouldn't be triggered by the wifi portal code. r=ckerschb

Approved for 91.7esr.

Comment 18

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/0117fb7ddb1b