1793597 - firefox crash on bad ssl https link in "HTTPS only" mode

Status: VERIFIED FIXED

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

Description

[matan.honig2]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:105.0) Gecko/20100101 Firefox/105.0
Firefox for Android

Steps to reproduce:

1. enable https-only globally. (or enable it only on private and open private tab)
2. click on this link http://kb.mozillazine.org/about:config while in https-only mode
   3.click Continue to HTTP Site

I also create github repository to test this bug - https://github.com/matan-h/firefox_bug

This also happened in the Android version

Actual results:

firefox crashed

Expected results:

firefox should go to the http (after click Continue ) site as it wold if you enter the link manually to the address bar

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Security' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Daniel Veditz [:dveditz]]

The crash is an IPC error (exception) and takes down the parent process which is VERY annoying.
bp-aca848f2-3564-4d5b-a8d5-830e70221011

Comment 3

[Frederik Braun [:freddy]]

Reminds me of bug 1746844.

Comment 4

[Frederik Braun [:freddy]]

Tomer, this looks fun. Can you take a look? I'll be happy to help you narrow down the root cause.

Comment 5

[Frederik Braun [:freddy]]

I believe this takes us to https://searchfox.org/mozilla-central/rev/e94c6cb9649bfe4e6a3888460f41bcd4fe30a6ca/dom/ipc/WindowGlobalParent.cpp#1276 and I guess adding some ifs/printfs will help narrow this down.

Comment 6

[Tomer Yavor]

Attachment: Network console if HOM enabled

Comment 7

[Tomer Yavor]

Attachment: Network console if HOM disabled and we navigate to HTTP site

Comment 8

[Tomer Yavor]

Attachment: Network console if HOM disabled and we navigate to HTTPS site (error displayed)

Comment 9

[Tomer Yavor]

My guess is that the website is redirecting not as expected. Instead of redirecting static to a URL it's using the scheme of the requested site.

For HTTP STR:

1. Disable https-only mode.
2. Visit http://kb.mozillazine.org/about:config
3. Website redirects us to http://kb.mozillazine.org/About:config.
   Result is as expected.

For HTTPS STR:

1. Disable https-only mode.
2. Visit https://kb.mozillazine.org/about:config
3. Website tries to redirect us to https://kb.mozillazine.org:80/About:config
4. Which leads to an error page (since the scheme is https:// but the port is 80).

For illustration I attached screenshots of the network console for https-only mode enabled, HOM disabled requesting http page and HOM disabled requesting https page

Comment 10

[Tomer Yavor]

So clicking on "continue to http" is not working since HOM never upgraded the navigation.
Tricky to solve. But seems to me like an edge case where it might be worth to change the websites behavior instead of https-only mode.
We could also thinking about changing the behavior of displaying error pages in HOM...

Comment 11

[Frederik Braun [:freddy]]

That's not what I had in mind. The description at the very top of this bug is a bit different and leads to a crash in the parent process that we need to address. I agree that the web page is not redirecting properly, but we still shouldn't take the whole browser down.

Comment 12

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

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

For more information, please visit auto_nag documentation.

Comment 14

[Daniel Veditz [:dveditz]]

from bug 1798276 comment 3

This is a similar issue to bug 1793521, but not the same.
Bug 1793521 crashed because the URL would changed when parsed again, leading to different principals. I assume we have a similar situation here, where the principal URL changes between http and https and doesn't match, leading to error being returned here and deserialization to fail.

Comment 15

[Frederik Braun [:freddy]]

I can easily reproduce this and catch the parent crash in gdb and rr. However, I have a hard time finding (and breaking) at the right kind of call from the child process which is where it will likely need to be fixed. Mhh.

Comment 16

[Paul Adenot (:padenot)]

pernosco link of this in a debug build: https://pernos.co/debug/39xkyeILZpWbVV7npfF9Jg/index.html

Comment 17

[Valentin Gosu [:valentin] (he/him)]

When working on bug 1793521 which had a similar crash, I applied the following patch and moved the crash to the parent process.
We might consider making it permanent, since delaying this crash doesn't help that much.

diff --git a/ipc/glue/BackgroundUtils.cpp b/ipc/glue/BackgroundUtils.cpp
--- a/ipc/glue/BackgroundUtils.cpp
+++ b/ipc/glue/BackgroundUtils.cpp
@@ -309,6 +309,16 @@ nsresult PrincipalToPrincipalInfo(nsIPri
     return rv;
   }
 
+  nsCOMPtr<nsIURI> uri;
+  rv = NS_NewURI(getter_AddRefs(uri), spec);
+  nsAutoCString checkOriginNoSuffix;
+  rv =
+      ContentPrincipal::GenerateOriginNoSuffixFromURI(uri, checkOriginNoSuffix);
+  // Better to fail early.
+  if (originNoSuffix != checkOriginNoSuffix) {
+    return NS_ERROR_UNEXPECTED;
+  }
+
   nsCOMPtr<nsIURI> domainUri;
   rv = aPrincipal->GetDomain(getter_AddRefs(domainUri));
   if (NS_WARN_IF(NS_FAILED(rv))) {

Comment 19

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

Copying crash signatures from duplicate bugs.

Comment 20

[Frederik Braun [:freddy]]

TIL: The fact that the parent is crashing on erroneous serialization is going to be solved within our IPC code.
https://bugzilla.mozilla.org/show_bug.cgi?id=1614764
https://bugzilla.mozilla.org/show_bug.cgi?id=1794828

This bug will only fix the client accidentally sending the wrong message.

Comment 21

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

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 content process crashes on release

:lyavor, could you consider increasing the severity of this top-crash bug?

For more information, please visit auto_nag documentation.

Comment 22

[Tom Schuster]

The page seems to have changed. The following link should reproduce it: (CRASH!) http://httpbun.org/redirect-to?url=https://httpbun.org:80 (CRASH!)

Comment 23

[Tom Schuster]

In the method WindowGlobalParent::RecvReloadWithHttpsOnlyException we change the https URI to http. However we just keep whatever port was specified for the https URL. In this specific case it happens to be default http port :80. Adding SetPort(-1) fixes the crash.

However I think there might still be some kind of roundtrip issue, where sometimes we strip the default port :80 for URIs when creating the origin and sometimes we don't?

Comment 24

[Tom Schuster]

So the actual crash is caused by the code in PrincipalInfoToPrincipal. were we create a new principal by parsing the URI spec. That operation doesn't roundtrip for the port 80, so parsing "http://example.org:80" will result in the origin "http://example.org". We also don't use the originNoSuffix for creating content principal with BasePrincipal::CreateContentPrincipal, but instead just use it for checking that we computed an equal origin.

I am not sure if we want to do a more general fix for this or just add the SetPort(-1) call. I think we might have similar issues in other https-only mode code. So changing the way the URI mutator works might have some credence. For reference URL::SetProtocol reparses the whole URL to prevent similar issues.

Comment 25

[Valentin Gosu [:valentin] (he/him)]

Ah, that's frustrating.

Given that we only have this issue for http&https, I wrote a quick fix for it nsStandardURL::SetScheme
https://phabricator.services.mozilla.com/D177048

Assuming this doesn't break anything else, it should be be good enough for now.
I agree with you that nsIMutator.setScheme should probably reparse the URL according to the new scheme's rules. But considering that the scheme change is pretty much only done for HTTPS only mode, and that reparsing for every scheme change would have a small perf impact, let's go with this targetted fix for now, and we can reconsider that in the future.

Comment 26

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1793597 - Also update the default port when changing the scheme of a URL r=#necko,tschuster

Comment 27

[Tom Schuster]

Thank you Valentin for providing this patch.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1227992bbec
Also update the default port when changing the scheme of a URL r=tschuster,necko-reviewers,kershaw

Comment 29

[Iulian Moraru]

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

Comment 30

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

The patch landed in nightly and beta is affected.
:valentin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox114 to wontfix.

For more information, please visit BugBot documentation.

Comment 31

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9331393 [details]
Bug 1793597 - Also update the default port when changing the scheme of a URL r=#necko,tschuster

Beta/Release Uplift Approval Request

• User impact if declined: Crash with HTTPS only mode caused by mismatched principals because URL has wrong default port.
  This can happen when upgrading URLs such as http://example.com:443 (note http and 443)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fixes rather obvious obvious issue with changing the scheme of the URL.
  The fix is limited to nsStandardURL changing the scheme to http/https/ws/wss when the current port equals the default port for that scheme.
• String changes made/needed:
• Is Android affected?: Yes

Comment 32

[Pascal Chevrel:pascalc]

Comment on attachment 9331393 [details]
Bug 1793597 - Also update the default port when changing the scheme of a URL r=#necko,tschuster

Approved for 114 beta 3, thanks.

Comment 33

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/59a2a5281bf9

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for Release approval for next week's dot release, Valentin?

Comment 35

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9331393 [details]
Bug 1793597 - Also update the default port when changing the scheme of a URL r=#necko,tschuster

(In reply to Valentin Gosu [:valentin] (he/him) from comment #31)

Comment on attachment 9331393 [details]
Bug 1793597 - Also update the default port when changing the scheme of a URL r=#necko,tschuster

Beta/Release Uplift Approval Request

• User impact if declined: Crash with HTTPS only mode caused by mismatched principals because URL has wrong default port.
  This can happen when upgrading URLs such as http://example.com:443 (note http and 443)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fixes rather obvious obvious issue with changing the scheme of the URL.
  The fix is limited to nsStandardURL changing the scheme to http/https/ws/wss when the current port equals the default port for that scheme.
• String changes made/needed:
• Is Android affected?: Yes

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9331393 [details]
Bug 1793597 - Also update the default port when changing the scheme of a URL r=#necko,tschuster

Approved for 113.0.2.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/7164d6f3416c

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Valentin, can you confirm that the remaining crashes showing up under this signature are unrelated?

Comment 39

[Valentin Gosu [:valentin] (he/him)]

rank	moz crash reason	count	%
1	IPDL error: "SharedMemory::Map failed". abort()ing as a result.	11	61.11 %
2	MOZ_CRASH(IPC FatalError in the parent process!)	5	27.78 %
3	IPDL error: "Error bulk reading fields from int64_t". abort()ing as a result.	1	5.56 %
4	IPDL error: "Error deserializing 'overrideReferrerInfo' (nsIReferrerInfo) member of 'HttpChannelOnStartRequestArgs'". abort()ing as a result.	1	5.56 %

Yes, as far as I can tell, the remaining crashes have different failure reasons.

Comment 40

[Rares Doghi, Desktop QA]

Hi @Valentin, is there a different test page we could use ? it seems that http://kb.mozillazine.org/about:config no longer redirects the user to the :80 page, I cannot reproduce this issue in older builds with the Test page from the description.

Comment 41

[Valentin Gosu [:valentin] (he/him)]

Hi,

I don't know if there is such a page on the internet.
Here's a test server I made. I can run the server on linux. Hope it works for you.

# add `127.0.0.1 bla.test` to /etc/hosts
git clone https://github.com/valenting/bug1793597-repro.git
cd bug1793597-repro
sudo node index.js
load  https://bla.test:443, accept certificate
# click on lock doorhanger and choose HTTPS-only mode ON
load https://bla.test:443/testytest # with old builds you should get a crash

Comment 42

[Rares Doghi, Desktop QA]

I tried to reproduce this issue using the steps from comment 41, I started the server and accepted the certificate, but after I enable HTTPS only mode and reach https://bla.test:443/testytest I'm just getting the "aaaa" from the json file. No crashes occur, I get the same results in older builds as well as our latest.

Maybe @Valentin or the reporter can test this on their end and let us know if the crash still occurs in our latest builds ?

Comment 43

[Frederik Braun [:freddy]]

I think Tom reproduced this with http://httpbun.org/redirect-to?url=https://httpbun.org:80 (before it was fixed). Maybe that's easier than modifying the hosts file?

Comment 44

[Frederik Braun [:freddy]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #38)

Valentin, can you confirm that the remaining crashes showing up under this signature are unrelated?

These might be in https://bugzilla.mozilla.org/show_bug.cgi?id=1614764 and https://bugzilla.mozilla.org/show_bug.cgi?id=1794828 (which is really the better fix than this one). The parent process really shouldn't die when receiving bad messages from the child.

Comment 45

[Rares Doghi, Desktop QA]

Thanks a lot for this link : http://httpbun.org/redirect-to?url=https://httpbun.org:80 @Frederik. I was able to reproduce the issue in older builds and Verify the Fix in our latest Release 113.0.2 , Beta 114.0b7 and our latest Nightly build 115.0a1 (2023-05-23).