1701386 - Disabling https only for a host doesn't disable "wss only" for it

Status: RESOLVED FIXED

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

Description

[u44081]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:87.0) Gecko/20100101 Firefox/87.0

Steps to reproduce:

I'm using https only mode for all sites, and have an exception for a local, http only site at http://homeassistant.local:8123, that works. I'm trying to access a websocket endpoint on that site.

Actual results:

I see from the console that Firefox is trying to access wss://homeassistant.local:8123 for the websocket endpoint because of https only mode, and it fails, because the endpoint is not available over (secure) wss, only (insecure) ws. ws would work. I am not presented an option to go insecure for that endpoint, nor can I find anything in preferences that would allow me to try setting an exception for it.

Expected results:

Firefox should have used my existing non-https exception and went with ws only instead of insisting on wss, or presented me an option to choose if I want that.

The only workaround I know of is to disable https only mode completely for the duration I want to access the websocket endpoint in question.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Networking: WebSockets' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1701386: Test if Principal is exempt when https-only is about to upgrade websockets

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Hey Ville, thanks for reporting the problem. We are already looking into it and I Just want to make sure I fully understand the problem you are reporting.

You are saying you visit http://homeassistant.local:8123 and then you add an exemption for http://homeassistant.local:8123, but the websocket within that webpage gets upgraded?

Put differently, if you add an exemption for top-level foo.com, then all subresources (when visiting top-level foo.com) are exempt. Now if you would visit top-level bar.com then subresources (image, script, style) of foo.com would not be exempt.

Back to your reported problem, are you loading the websocket on the exempted top-level page of: http://homeassistant.local:8123 or within a different top-level page?

FWIW, I added an automated testcase within D110104 which reflects what I just summarized above.

Thanks for your time and improving HTTPS-Only Mode.

Comment 4

[u44081]

I think you do get it correctly. All resources are served off homeassistant.local:8123, there is no other site/host/port at play here. Specifically the URL of the view that eventually causes the websocket attempt is
http://homeassistant.local:8123/api/hassio_ingress/[...]/novnc/vnc_lite.html?scale=yes
...which loads a bunch of other resources off the same host:port over plain http, successfully due to the exemption I've added. But from the console I see the browser eventually trying to load
wss://homeassistant.local:8123/api/hassio_ingress/[...]/novnc/websockify
...which fails as the resource is not available over wss, just ws.

The same app/view works fine and does ws://... when HTTPS-only is completely disabled.

Comment 5

[u44081]

Looking at the test case, I think you have it spot on there.

I was a bit confused when you mentioned "getting upgraded" -- I took it in the sense that http connections are upgraded to websocket ones via the Upgrade mechanism, but now I understand you likely meant HTTPS-only upgrading ws to wss.

Thanks for the quick replies and work on this!

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Hey Ville, thanks for getting back to me so quickly. What we currently observe (also in the automated test) is that the websocket does not get upgraded to wss in the case the top-level page is exempted.

In other words, is there any way you can share the code (attach a file to the bug, copy/paste the relevant code in a comment, or also email me that part) of how you load the websocket? Is it in some iframe, is it somehow generated on the fly?

Clearly there must be a bug somewhere, but not in the straight forward use case I wrote the automated testcase.

Thanks for your time!

Comment 7

[u44081]

I'm not sure how to capture the relevant part. There are some frames at play, but the docs are all from the same host.

Not sure if it's useful to you, but the Home Assistant main frontend code is available at https://github.com/home-assistant/frontend, and the issue reproduces with the ssh addon from https://github.com/hassio-addons/addon-ssh as well as the deconz addon from https://github.com/home-assistant/addons/tree/master/deconz

There is a public demo site at https://demo.home-assistant.io/ with various demo interfaces, but that's https, and I don't know if there are any websockets in any of them.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Arthur, as discussed in our meeting, maybe you can reproduce this bug (see also comment 7) - thank you!

Comment 9

[Daniel Holbert [:dholbert]]

Hi Christoph - for what it's worth, I can reproduce this bug locally, using my own HomeAssistant RaspberryPi, which I view at local insecure URL http://homeassistant.local:8123/core_ssh/dashboard (and I click through the https_only_mode warning page so that I can view it over insecure http, since I have the https_only_mode pref enabled).

The HomeAssistant "web terminal" extension is broken for me, apparently due to this bug.

When I load it with dom.security.https_only_mode enabled, network devtools shows a GET request failing, with Scheme: wss.

When I load it without that pref enabled, my network devtools show that the corresponding GET request is Schemed: ws (not wss).

Notably, when the request succeeds (in a build with the pref turned off), it does show that the request has a Status of 101 Switching Protocols (rather than e.g. status 200 like most of the other requests in network devtools). I'm not sure if that's relevant/interesting or not.

Comment 10

[Daniel Holbert [:dholbert]]

I have this caught in rr and have submitted it to pernosco, and I'm hoping maybe :ckerschb can take a look there and reconstruct what's going on. (I can't provide a more direct response to comment 6's request-for-a-testcase, since (a) I don't know websockets particularly well, and (b) the homeassistant JavaScript is minified/obfuscated and has a lot of async hopping around.)

From skimming our C++, I suspect the relevant code here is in WebSocketImpl::Init which has a section:

  // If the HTTPS-Only mode is enabled, we need to upgrade the websocket
  // connection from ws:// to wss:// and mark it as secure.

I added a bit of logging (included in my rr/pernosco trace) which I confirmed does get triggered -- it's a good signal for when we're converting from ws to wss for the relevant URIs here.

That upgrade seems to be behind a call to nsHTTPSOnlyUtils::ShouldUpgradeWebSocket, which is returning true in this case, apparently because this if-check is failing -- so apparently this load isn't properly tagged as https-only-exempt:

  // 3. Check if NoUpgrade-flag is set in LoadInfo
  uint32_t httpsOnlyStatus = aLoadInfo->GetHttpsOnlyStatus();
  if (httpsOnlyStatus & nsILoadInfo::HTTPS_ONLY_EXEMPT) {

Comment 11

[Daniel Holbert [:dholbert]]

Note, my rr trace does have one earlier trip through WebSocketImpl::Init and nsHTTPSOnlyUtils::ShouldUpgradeWebSocket where we behave like you would expect -- we decide not to update, because in that case, httpsOnlyStatus does include HTTPS_ONLY_EXEMPT -- it's 0x19 i.e. binary 11001.

In the later-in-the-trace scenario, though, where WebSocketImpl::Init does decide to upgrade, we have a httpsOnlyStatus of 0x11 i.e. binary 10001, lacking the HTTPS_ONLY_EXEMPT bit.

Comment 12

[Tom S]

I think I debugged the same issue in bug 1728981

Comment 13

[Daniel Holbert [:dholbert]]

Aha, thank you for noticing before I dug much further. :)

I'll add that as a see-also; up to ckerschb or whoever ends up fixing this to dupe one way or the other (assuming the bugs are in fact dupes).

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Hey Tom, since you fixed Bug 1728981, I think this is a duplicate of it - however, do you think we could use the testcase here? If so, do you wanna give it a try?

Thank you!

Comment 15

[Tom S]

This test case has the same problem, we decide not to upgrade based on httpsOnlyStatus & nsILoadInfo::HTTPS_ONLY_EXEMPT not TestIfPrincipalIsExempt.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Hey Daniel, I really want to fix the underlying problem here - since you mentioned you can reproduce, I was wondering if you can help me debug the problem on more time. In particular within Comment 7 within Bug 1728981 you were talking about a same-origin iframe. Hence I was trying to come up with all sorts of scenarios which would trigger this problem. Within D110104 I have the latest testcase which uses a same-origin iframe. Additionally, I included some old school debug printf statements.

In all the cases I tested we don't even get to the check of TestIfPrincipalIsExempt which we added within Bug 1728981, the loadinfo always has the flag HTTPS_ONLY_EXEMPT set.

The information most useful for me would be:
a) The top-level URL (the one from the address bar) which we added the exception for.
b) The URL of the aLoadInfo->TriggeringPrincipal() within ShouldUpgradeWebSocket (you can see how to query that in the attached phabricator patch).
c) If available, the URL of the iframe which loads the websocket (most likely that is identical to aLoadInfo->TriggeringPrincipal().

If you can help me query a), b) (and potentially, but not strictly necessary c)) - I think we could generate a testcase.

Thank you for your time and help!

Comment 17

[Daniel Holbert [:dholbert]]

I actually added HTTPS to my homeassistant server in the time since comment 9, so I can't immediately reproduce at the moment - but I believe I can revert that change and get back to you with some answers.

Comment 18

[Daniel Holbert [:dholbert]]

OK, my homeassistant server is back up in HTTP-only mode, and I can't reproduce this bug in current Nighlty (but I can in Nightly 2021-11-01), I assume because bug 1728981 fixed this.

I assume you'd like me to capture the information in an up-to-date build (one which includes the fix), and I should capture the info from (b) at the point where TestIfPrincipalIsExempt gets called -- correct?

I can answer (a) and (c) right now with my Nightly; I'll get at (b) once I've got this running in a debug build with a debugger attached.

(a) The address bar is: http://homeassistant.local:8123/core_ssh/dashboard
(b) [TBD]
(c) It looks like we actually have a doubly-nested iframe. The outer one does not have an explicit URL -- devtools inspector just shows it as <iframe>. I assume its contents are populated dynamically from the outer page. Within that iframe, we have another iframe, which devtools inspector shows as <iframe src="/api/hassio_ingress/4uYoh4WlFuhqNx96DME6OCnEqfYiTR1enejoUFGvJNo/"></iframe>. i.e. its full URL is http://homeassistant.local:8123/api/hassio_ingress/4uYoh4WlFuhqNx96DME6OCnEqfYiTR1enejoUFGvJNo/

I wonder if the doubly-nested-iframe is the relevant special condition here.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: screenshot showing nested iframes in devtools, when dholbert is reproducing the bug (in Nightly 2021-11-01)

Comment 20

[Daniel Holbert [:dholbert]]

(Note that there are also several #shadow-root containers, in the outer doc as well as the URL-less iframe in the middle. Not sure if those are relevant/necessary for triggering this as well.)

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: logging patch that dholbert used

Here's the patch that I used (applied to today's mozilla-central) to answer part (b).

I get this logging, after visiting http://homeassistant.local:8123/core_ssh/dashboard (and clicking through the HTTP-only splash page):

  XXX ShouldUpgradeWebSocket with principal: http://homeassistant.local:8123/api/hassio_ingress/4uYoh4WlFuhqNx96DME6OCnEqfYiTR1enejoUFGvJNo/
 XXX Passed TestIfPrincipalIsExempt ws://homeassistant.local:8123/api/hassio_ingress/4uYoh4WlFuhqNx96DME6OCnEqfYiTR1enejoUFGvJNo/ws

The former line there (the http one) is from the very beginning of ShouldUpgradeWebSocket and is printing out aLoadInfo->TriggeringPrincipal()->GetURI(), which answers your (b) question I think.

The latter line is printing out aURI at the point where our new TestIfPrincipalIsExempt check saves us and lets us return false.

Comment 22

[Daniel Holbert [:dholbert]]

Note that there's also one other websocket URL that we already were correctly declining-to-upgrade earlier in the pageload (different from the websocket in question in this bug here). For this properly-handled one, we handle it via the 3. Check if NoUpgrade-flag is set in LoadInfo case.

For that one, the aURI is ws://homeassistant.local:8123/api/websocket, and aLoadInfo->TriggeringPrincipal()->GetURI() is http://homeassistant.local:8123/core_ssh/dashboard.

Comment 23

[Daniel Holbert [:dholbert]]

FWIW:

• In that earlier ShouldUpgradeWebSocket call (for the outer doc, where we were already correctly declining-to-upgrade), aLoadInfo->GetHttpsOnlyStatus() is 25.
• In the later ShouldUpgradeWebSocket call (where we're relying on the new TestIfPrincipalIsExempt() check), aLoadInfo->GetHttpsOnlyStatus() is 17.

The difference (8) is precisely the HTTPS_ONLY_EXEMPT bit, defined as 1<<3 here:
https://searchfox.org/mozilla-central/rev/21f0bb70ce5747d18625ad1c67f03ed2af7f9c56/netwerk/base/nsILoadInfo.idl#457

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b4c39cb1426
Test if Principal is exempt when https-only is about to upgrade websockets r=freddyb

Comment 25

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/6b4c39cb1426