1804684 - Fragment navigation may change document URI scheme from https to http

Status: RESOLVED FIXED

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

Description

[Artur Iunusov]

Reproduction steps:

0. dom.security.https_first = true
1. page already loaded using https
2. during fragment navigation, so navigation within the page itself, changes its url to http (https://searchfox.org/mozilla-central/rev/a3c18883ef9875ba4bb0cc2e7d6ba5a198aaf9bd/dom/security/test/https-first/test_fragment.html#29)

this makes "secure" pages look like they aren't secure

Comment 1

[Frederik Braun [:freddy]]

This looks fun, but not like a security issue as per our client severity ratings.
Am I wrong or are you overly cautious?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Yeah, possibly not a security bug, but blocks a security bug.

Comment 3

[Artur Iunusov]

found this bug during working on Bug 1783504, where we stumbled upon that case:

RecvUpdateDocumentURI:
aURI='http://example.com/tests/dom/security/test/https-first/file_fragment.html#foo',
principal URI='https://example.com/tests/dom/security/test/https-first/file_fragment.html'

Comment 4

[Tomer Yavor]

I couldn't reproduce the issue in the "wild".
But reproduced it with the linked test file.
I think the problem occurs in the fragment test because of that line.

AFAIK fragmentation navigations don't lead to page (re-)loads. That is probably the bug here since http:// is the hardcoded scheme of the button location.

As the author of that test case I am sorry for bad practice (that is a copy paste of my first test I wrote for firefox :D ), unsure if that is not a very unlikely case for the "wild".

Comment 5

[Tomer Yavor]

I think it could be patched by implementing in the URI check for fragment navigations a URI modifier like: https://searchfox.org/mozilla-central/source/dom/security/nsHTTPSOnlyUtils.cpp#751,779-795.

Something that looks like:

              // If we entered here the current scheme is https (upgraded by
              // HTTPS-Only/-First mode) and the navigation is a fragment
              // navigation. For that reason we should navigate from https to
              // https.
              nsCOMPtr<nsIURI> newHTTPSFragNavURI;
              // Port should be set to the port value of the source (URI's port  that we are navigating from)
              rv = NS_MutateURI(oldHTTPFragNavURI)
                       .SetScheme("https"_ns)
                       .SetPort(port)
                       .Finalize(newHTTPSSchemeURI);
              NS_ENSURE_SUCCESS(rv, false);
              aLoadState->SetURI(newHTTPSFragNavURI);

I already tested it and it seems to work.

@smaug: I would like to add something similar to the code snippet from above as an helper function to nsHTTPSOnlyUtils.cpp and call it in nsDocShell.cpp. What are you thinking about that?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I was thinking something like that needs to happen. We have to change the scheme in the to-be-loaded url.

If it is nsDocShell::IsSameDocumentNavigation which ends up modifying the URL, better to add some good comment to the .h explaining the behavior.

Test will need to ensure document.documentURI and location.href are updated properly. And I guess it would be good to test that also the URI sent to the parent process is correct https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/dom/chrome-webidl/WindowGlobalActors.webidl#96

Comment 7

[Tomer Yavor]

Attachment: Bug 1804684 - Fragment navigation may change document URI scheme from https to http. r?smaug!,nika!

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Fragment navigation may change document URI scheme from https to http r=smaug
https://hg.mozilla.org/integration/autoland/rev/30d7636620f835c7aa3d3fc07508fbd8d72de8b0
https://hg.mozilla.org/mozilla-central/rev/30d7636620f8

Comment 9

[Narcis Beleuzu [:NarcisB]]

Backed out as requested by smaug
Backout link: https://hg.mozilla.org/integration/autoland/rev/4f2f6e92ee47a3e61122563a4e0eee07bb759d08

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/4f2f6e92ee47

Comment 11

[BugBot [:suhaib / :marco]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:lyavor, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Tomer, I guess we need to figure out how to avoid that sync IPC. Someone more familiar with http->https setup might have suggestions.

Comment 13

[Tomer Yavor]

Okay, thank you!
I will try to find someone or do you have someone in mind?

Comment 14

[Frederik Braun [:freddy]]

Do I correctly understand that calling NS_GetSecureUpgradedURI() from DocShell is causing synchronous IPC to the network process?

I wonder how we usually deal with utility functions that should be available cross process. This one in particular does not seem very process-specific to me..

Comment 15

[Tomer Yavor]

No, I think smaug meant this part in the patch:

mozilla::dom::ContentChild* cc = mozilla::dom::ContentChild::GetSingleton();
cc->SendIsSecureURI(currentURI, attrsForHSTS, &isStsHost);

Which is needed to check if the request has a HSTS header.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Fragment navigation may change document URI scheme from https to http r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/bb1f3463f21d75a3d72dfe29b791935f28e289a3
https://hg.mozilla.org/mozilla-central/rev/bb1f3463f21d

Comment 17

[BugBot [:suhaib / :marco]]

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

• If yes, please nominate the patch for beta approval.Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 18

[Tom Schuster]

Let's backout.

Comment 19

[Tom Schuster]

I am not sure what to do here. Seems like just modifying the URL is not the way to go.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing Bug 1823765 a=backout
https://hg.mozilla.org/mozilla-central/rev/b917c4511ec4

Comment 21

[Nika Layzell [:nika] (ni? for response)]

The error here is caused by the load validation which we do on nsDocShellLoadState to ensure that the content process does not tamper with the URI being loaded. You're re-writing the LoadState's URI within the content process in your case, so if the navigation ends up actually creating a real DocumentChannel (as it does in bug 1823765), it'll send the new URI to the parent process, which will be different than the original one.

Perhaps instead of changing the URI in the current place, where other decisions might make it not actually be a same-document navigation, record that the URI needs to have it's scheme changed in the SameDocumentNavigationState object. You can then do the change to the URI before it is used in HandleSameDocumentNavigation (https://searchfox.org/mozilla-central/rev/dd2fe65d792943365a03fa996cdf9766829575b6/docshell/base/nsDocShell.cpp#8865), which will ensure that it's not going to be used to create a channel (which should be done with the original URI anyway so that necko can upgrade it).

Adding a flag like mShouldSecureUpgradeURI to SameDocumentNavigationState, and changing all calls to aLoadState->URI() to instead happen on a local copy of the URI which is upgraded if mShouldSecureUpgradeURI is set.

Comment 22

[Tom Schuster]

Thank you Nika! I am investigating this new approach you suggested.

Comment 23

[Tom Schuster]

That seems to work fine, but do you have an idea what condition would reproduce the crashes? Something involving redirect GetPendingRedirectedChannel ?

Comment 24

[Tom Schuster]

Fragment navigation may change document URI scheme from https to http. r=ckerschb,nika,smaug
https://hg.mozilla.org/integration/autoland/rev/d841063b73d7d850d864f72123420931c602b9ee
https://hg.mozilla.org/mozilla-central/rev/d841063b73d7d850d864f72123420931c602b9ee

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Looks like the crashes have stayed away this time. Is this something we were thinking about backporting or can it ride the trains?

Comment 26

[Tom Schuster]

I think this can ride the trains. However as I understand it, this patch is required for bug 1783504, so we might uplift both of them if that get finished soon.