1391011 - upgrade-insecure-requests not applied to navigational requests

Status: RESOLVED FIXED

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

Description

[Eric Lawrence (@ericlaw)]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170810180547

Steps to reproduce:

Click the "Insecure Link" at https://s3.amazonaws.com/share.sbndev.net/csp/csp-upgrade-insecure-requests-navigation/index.html


Actual results:

User navigates to HTTP page.


Expected results:

Expect upgrade to HTTPS per the upgrade-insecure-requests directive as this is a navigation to the same host.

Comment 1

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Kate, any thoughts on this?

Comment 2

[Daniel Veditz [:dveditz]]

Christoph's original patches for bug 1139297 included navigation support, and even had to go back and disable upgrades for cross-origin navigations because our first version was overzealous. Are there no tests? When did this break?

We should double-check that UIR is still applied to form posts (even cross-origin) while we're doing this.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> Christoph's original patches for bug 1139297 included navigation support,
> and even had to go back and disable upgrades for cross-origin navigations
> because our first version was overzealous. Are there no tests? When did this
> break?
> 
> We should double-check that UIR is still applied to form posts (even
> cross-origin) while we're doing this.

Beats me that we don't have a testcase for toplevel same-origin and cross-origin navigation. I will add that with this patch. I am not sure what refressed that, but I found out that the upgrade-insecure-requests flag is false here [1]. I will need to investigate a little closer to find out why.

Thanks for reporting Eric!

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#2595

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1391011_uir_toplevel_nav.patch

First, this is not a regression, this never worked properly.
Apparently we have a testcase (test_upgrade_insecure_navigation.html) which tests toplevel same and cross origin navigations, but starting out with an http page. Within docshell we perform an 'Equals' operation on the triggeringPrincipal and the resultPrincipal which works, because the http toplevel load still has an http scheme.

In the example from comment 0, we start out with an https page hence the equals operations fails on the http page load. In more details:
[works] http://example.com/foo.html -> http://example.com/bar.html (will be upgraded)
[fails] https://example.com/foo.html -> http://example.com/bar.html (because schemes are different).


Olli I guess there is no 'equals' or 'subsumes' function defined on the principal that would allow us to ignore the scheme (or basically treat http(s)://example.com) to be same origin). Within this patch (even though it's a strawman patch) I am writing our own SOP function for upgrade-insecure-requests. If we refine that function, would that be acceptable or is there potentially a better API available I am not aware of?

Please note that we can clean up NsNetutil.cpp because there is no need to perform the cross-origin check twice. If we do that within docshell then the backend code for upgrading does not need to perform the same cross-origin check again. In other words, if the loadinfo says we should upgrade, then we upgrade, period.

I verified that test_upgrade_insecure_navigation.html keeps working and I also added a new tests which tests using an https page to begin with (see next patch).

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1391011_uir_toplevel_nav_tests.patch

Comment 6

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

Comment on attachment 8898732 [details] [diff] [review]
bug_1391011_uir_toplevel_nav.patch

I'm not aware principal check not caring about http vs https, so the approach should be fine.

Perhaps IsConsideredSameOriginForUIR could be called something a bit different since null principal certainly isn't ever in any way same origin as some http principal for example.

What if the check was done using principals, but you'd just create a temporary principal if schemes are different?

Comment 7

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

Comment on attachment 8898733 [details] [diff] [review]
bug_1391011_uir_toplevel_nav_tests.patch

>+function checkResults(aResult) {
>+  ok(aResult == "https://example.com/tests/dom/security/test/csp/file_uir_top_nav_dummy.html" ||
>+  	 aResult == "http://test1.example.com/tests/dom/security/test/csp/file_uir_top_nav_dummy.html",
>+  	 "same origin should be upgraded to https, cross origin should remain http");
You have some tabs here, which is why indentation looks odd.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1391011_uir_toplevel_nav.patch

Creating a tmp principal sounds like the right path forward and makes me also feel more comfortable doing this check. Thanks!

Comment 9

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

Comment on attachment 8899120 [details] [diff] [review]
bug_1391011_uir_toplevel_nav.patch

The nsNetUtils.cpp check is also to report to console.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/525e9e4dac88
CSP: Fix upgrade-insecure-requests for toplevel navigations when base it https. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/729120b96378
CSP: Test upgrade-insecure-requests for toplevel navigations when base it https. r=smaug

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Dan, how would you feel about uplifting that change? Even though it's not a regression, this part never worked correctly. I think we should consider uplifting that change if possible.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/525e9e4dac88
https://hg.mozilla.org/mozilla-central/rev/729120b96378

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> Dan, how would you feel about uplifting that change? Even though it's not a
> regression, this part never worked correctly. I think we should consider
> uplifting that change if possible.

This doesn't seem like the kind of serious bug (security or otherwise) that would jump the trains to a mid-to-late beta.