1271173 - Missing spec on Upgrade Insecure Requests(Navigational Upgrades)

Status: RESOLVED FIXED

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

Description

[Takashi]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160508030214

Steps to reproduce:

1. Go to page which is enabled  "Content-Security-Policy: upgrade-insecure-requests"

2. When a user clicks a link, Firefox will not treat a http link as "https" link.


Actual results:

When a user clicks a link, Firefox will not treat a http link as "https" link.


Expected results:

When I submit a form witch "action" = http url,  Firefox changed http to https. I think this is the solution.

W3C specification has an example....

--------------------------------------

Navigational Upgrades

Megacorp, Inc. isnft quite ready to deliver Strict Transport Security headers [RFC6797], but does want to keep users on secure pages when possible. Happily, this comes for free with upgrade-insecure-requests. That is, theyfre already delivering pages with the following header:

Content-Security-Policy: upgrade-insecure-requests


This allows user agents to treat the following HTML code:

<a href="http://example.com/">Home</a>

as though it had been delivered as:

<a href="https://example.com/">Home</a>


Source: http://www.w3.org/TR/upgrade-insecure-requests/#example-navigation


3. Authors should be able to ensure that all internal links correctly send users to the sitefs secure address, and not to its pre-migration insecure address.

Source:http://www.w3.org/TR/upgrade-insecure-requests/#goals


----------------------------------------------

Comment 1

[Takashi]

I uploaded my poc video to google drive. Please watch it.

https://drive.google.com/open?id=0B1vJ77JB-BeoT3M0VnV4NHpBOG8

Comment 2

[Takashi]

There are typos in explaining Navigational Upgrades.

theyfre already delivering ---> they're already....

all internal links correctly send users to the sitefs ---> site's

['] became f.... I am really sorry for making mistakes.

------------------------------------------------------------------

Last note:

I forgot to mention that I tested this on local server. 

firefox = 49.0a1

Comment 3

[:Gijs (he/him)]

Freddy / Christoph, can you take a look at this?

(not sure if this needs to be sec-sensitive; I kind of suspect it doesn't?)

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

At this point I am not sure whether this is a regression or if upgrading (same origin) navigational requests never worked correctly. Fact is, we have no test for navigational requests and this is indeed a bug we should fix.

Note: Support for upgrade-insecure-requests landed within Bug 1139297.

Takashi, thanks for filing!

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271173_upgrade_insecure_navigation_test.patch

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

This is not a regression, upgrading navigational requests apparently never worked correctly.

Let me explain why it's not working based on the testcase I just wrote (see attachment). The testcase loads a testpage into an iframe which includes a link that navigates the iframe to a different location. When loading the iframe we apply 'upgrade-insecure-requests', so theoretically the navigation should be upgraded from http to https.

The documentURI of the iframe is: http://mochi.test:8888/tests/dom/security/test/csp/file_upgrade_insecure_navigation.sjs?csp=upgrade-insecure-requests%3B&action=peform_navigation
which initializes mUpgradeInsecureRequests correctly to 'true' here [1].

When we click the link to navigate the iframe we create a new channel and hence a new loadInfo. The newly created loadInfo has the following arguments:

* LoadingPrincipal (queried from aLoadingContext):
http://mochi.test:8888/tests/dom/security/test/csp/test_upgrade_insecure_navigation.html

* TriggeringPrincipal:
http://mochi.test:8888/tests/dom/security/test/csp/file_upgrade_insecure_navigation.sjs?csp=upgrade-insecure-requests%3B&action=peform_navigation

In other words, we use the top level document as the loadingContext (LoadingPrincipal) which does not have the upgrade-insecure-requests flag set on the document, see [2].

In fact, the triggeringPrincipal (the iframe's document) would have the CSP attached which would include the right information.

Tanvi, you recently worked on setting the right loadingPrincipal and loadingNode within docshell within Bug 1105556, any ideas what we could do here to fix this issue?

Please note that the actual problem described in comment 0 and 1 might use a slighlty different codepath because my testcase loads into an iframe whereas the test described in comment 1 uses a top level navigation.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2606
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#134

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Stacktrace for creating the loadinfo from within docshell:

#01: mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsIPrincipal*, nsINode*, unsigned int, unsigned int) (/home/ckerschb/moz/mc/netwerk/base/LoadInfo.cpp:168)
#02: nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, nsIURI*, bool, unsigned int, nsISupports*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsAString_internal const&, nsIURI*, unsigned int) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10712 (discriminator 2))
#03: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsISupports*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10466)
#04: nsDocShell::OnLinkClickSync(nsIContent*, nsIURI*, char16_t const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, nsIDocShell**, nsIRequest**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:13788)
#05: OnLinkClickEvent::Run() (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:13557)
#06: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:991 (discriminator 1))
#07: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:290)
#08: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:98)
#09: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:234)
#10: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:227)
#11: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:206)
#12: nsBaseAppShell::Run() (/home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:158)
#13: nsAppStartup::Run() (/home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:284)
#14: XREMain::XRE_mainRun() (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4368)
#15: XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4472)
#16: XRE_main (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4580)
#17: do_main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:220)
#18: main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:360)
#19: __libc_start_main (/build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:321)
#20: _start (/home/ckerschb/moz/mc-obj-ff-dbg/dist/bin/firefox)
#21: ??? (???:???)

Comment 8

[Daniel Veditz [:dveditz]]

This is not a "vulnerability" through which you can attack a user, it does not need to be hidden or rated. If the site wants all its links to be secure it should write secure links. Features like upgrade-insecure-request are a backstop to help a web site, but even when it fails it's not a vulnerability in Firefox. It's not implemented by all browsers so web sites cannot rely on it yet.

It's not even an official standard yet, it's a work in progress and not yet final. This is purely a spec-compliance bug.

Comment 9

[Takashi]

> If the site wants all its links to be secure it should write secure links.

So why did Mozilla implement this function on Firefox? 

W3C stated...

-------------------------------

4. Authors should be able to achieve all these goals without editing a site’s content. This is particularly important for archived content and legacy systems for which maintenance is difficult enough, never mind upgrades.

Sourece: http://www.w3.org/TR/upgrade-insecure-requests/#goals

Comment 10

[Takashi]

is it possible to rewrite over 1000 http links to https? Or check all links wether these links were http or not on the big site?

Comment 11

[Daniel Veditz [:dveditz]]

We obviously think this feature will help site authors, especially for legacy sites as you point out. That's why we're participating in the creation of the standard and working on an implementation for it. This is a spec-compliance bug and we should fix it.

Comment 12

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> This is not a regression, upgrading navigational requests apparently never
> worked correctly.
> 
> Let me explain why it's not working based on the testcase I just wrote (see
> attachment). The testcase loads a testpage into an iframe which includes a
> link that navigates the iframe to a different location. When loading the
> iframe we apply 'upgrade-insecure-requests', so theoretically the navigation
> should be upgraded from http to https.
> 
> The documentURI of the iframe is:
> http://mochi.test:8888/tests/dom/security/test/csp/
> file_upgrade_insecure_navigation.sjs?csp=upgrade-insecure-
> requests%3B&action=peform_navigation
> which initializes mUpgradeInsecureRequests correctly to 'true' here [1].
> 
> When we click the link to navigate the iframe we create a new channel and
> hence a new loadInfo. The newly created loadInfo has the following arguments:
> 
> * LoadingPrincipal (queried from aLoadingContext):
> http://mochi.test:8888/tests/dom/security/test/csp/
> test_upgrade_insecure_navigation.html
> 
> * TriggeringPrincipal:
> http://mochi.test:8888/tests/dom/security/test/csp/
> file_upgrade_insecure_navigation.sjs?csp=upgrade-insecure-
> requests%3B&action=peform_navigation
> 
> In other words, we use the top level document as the loadingContext
> (LoadingPrincipal) which does not have the upgrade-insecure-requests flag
> set on the document, see [2].
> 
> In fact, the triggeringPrincipal (the iframe's document) would have the CSP
> attached which would include the right information.
> 
> Tanvi, you recently worked on setting the right loadingPrincipal and
> loadingNode within docshell within Bug 1105556, any ideas what we could do
> here to fix this issue?
> 
> Please note that the actual problem described in comment 0 and 1 might use a
> slighlty different codepath because my testcase loads into an iframe whereas
> the test described in comment 1 uses a top level navigation.
> 
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2606
> [2]
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#134

You would have to use the triggineringPrincipal's CSP to determine if a same-origin referrer had the upgrade-insecure-request directive.  But per https://bugzilla.mozilla.org/show_bug.cgi?id=1181370, we know that the triggeringPrincipal is not reliable yet.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271173_upgrade_insecure_navigation.patch

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271173_upgrade_insecure_navigation_test.patch

Comment 15

[Frederik Braun [:freddy]]

Comment on attachment 8824030 [details] [diff] [review]
bug_1271173_upgrade_insecure_navigation_test.patch

Review of attachment 8824030 [details] [diff] [review]:
-----------------------------------------------------------------

Test look good in terms of coverage and how they were laid out in general.

I *do* have a question about the origins of your test files though, see below.

::: dom/security/test/csp/test_upgrade_insecure_navigation.html
@@ +22,5 @@
> +var tests = [
> +  {
> +    csp: "upgrade-insecure-requests;",
> +    result: "https",
> +    origin: "http://example.com",

I think you have your test cases mixed up. 
According to https://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt#57 the default origin is http://mochi.test:8888, so this case here shouldn't be called "same origin" but "cross origin".
If this is the case, then your tests should not succeed, of which I'd be surprised if this is the case (still compiling here, next review is on its way)
Please tell me where I'm wrong :-)

@@ +23,5 @@
> +  {
> +    csp: "upgrade-insecure-requests;",
> +    result: "https",
> +    origin: "http://example.com",
> +    desc: "upgrade-insecure-requests same origin should upgrade" 

Nit: trailing whitespace

@@ +35,5 @@
> +  {
> +    csp: "upgrade-insecure-requests;",
> +    result: "http",
> +    origin: "http://mochi.test:8888",
> +    desc: "upgrade-insecure-requests cross origin should not upgrade" 

Nit: trailing whitespace

@@ +57,5 @@
> +window.addEventListener("message", receiveMessage, false);
> +function receiveMessage(event) {
> +  var result = event.data.result;
> +  // query the scheme from the URL before comparing the result
> +  var scheme = result.substring(0, result.indexOf(":"));

Should we prefer the built-in URL Parser?
e.g., var scheme = new URL(result).protocol.slice(0, -1);

Comment 16

[Frederik Braun [:freddy]]

Comment on attachment 8824029 [details] [diff] [review]
bug_1271173_upgrade_insecure_navigation.patch

Review of attachment 8824029 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +10985,5 @@
> +      rv = secMan->GetChannelResultPrincipal(channel,
> +                                             getter_AddRefs(resultPrincipal));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (resultPrincipal->Equals(aTriggeringPrincipal)) {
> +        static_cast<mozilla::LoadInfo*>(loadInfo.get())->SetUpgradeInsecureRequests();

Why the static_cast here? The loadInfo is created and used many times in the same function before this line.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Frederik Braun [:freddyb] from comment #15)
> I think you have your test cases mixed up. 
> According to
> https://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.
> txt#57 the default origin is http://mochi.test:8888, so this case here
> shouldn't be called "same origin" but "cross origin".
> If this is the case, then your tests should not succeed, of which I'd be
> surprised if this is the case (still compiling here, next review is on its
> way)
> Please tell me where I'm wrong :-)

So, the default origin for the test is http://mochi.test. Please note that http://mochi.test is not available to be https. Hence I have to use http://example.com for the test which we can upgrade to https. In particular, we only care about the frame navigation. So here is how the test works. The origin of the iframe we are navigating to is alwasy http://example.com which needs to be updated only if upgrade-insecure-request is set and if the original origin of the iframe is http://example.com. Hence, description should be correct. Makes sense?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Frederik Braun [:freddyb] from comment #16)
> > +        static_cast<mozilla::LoadInfo*>(loadInfo.get())->SetUpgradeInsecureRequests();
> 
> Why the static_cast here? The loadInfo is created and used many times in the
> same function before this line.

We are casting to a mozilla::Loadinfo*, which is different from an nsILoadInfo xpcom pointer.

Comment 19

[Frederik Braun [:freddy]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> In particular, we only care about the frame navigation. 

Of course. I was missing this. Thanks!

Comment 20

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

Comment on attachment 8824030 [details] [diff] [review]
bug_1271173_upgrade_insecure_navigation_test.patch

Need tests for top level navigation.
This is clearly error prone stuff so need to get as good tests as possible.

Comment 21

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

Comment on attachment 8824029 [details] [diff] [review]
bug_1271173_upgrade_insecure_navigation.patch

>+  // Navigational requests that are same origin need to be upgraded in case
>+  // upgrade-insecure-requests is present. Please note that in that case
>+  // the triggeringPrincipal is holding the CSP that potentially
>+  // holds upgrade-insecure-requests.
>+  nsCOMPtr<nsIContentSecurityPolicy> csp;
>+  aTriggeringPrincipal->GetCsp(getter_AddRefs(csp));
>+  if (csp) {
>+    bool upgradeInsecureRequests = false;
>+    csp->GetUpgradeInsecureRequests(&upgradeInsecureRequests);
>+    if (upgradeInsecureRequests) {
>+      // only upgrade if the navigation is same origin
>+      nsCOMPtr<nsIScriptSecurityManager> secMan =
>+        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
Use nsContentUtils::GetSecurityManager() 

>+
>+      nsCOMPtr<nsIPrincipal> resultPrincipal;
>+      rv = secMan->GetChannelResultPrincipal(channel,
>+                                             getter_AddRefs(resultPrincipal));
This doesn't work with sandboxed iframes, I think, since we end up getting different null principal, right?
Or am I missing something here (if so, ask review again). Please add a test for sandboxed frames too.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271173_upgrade_insecure_navigation.patch

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271173_upgrade_insecure_navigation_test.patch

Hey Smaug, I extended the tests to include top level navigation and further sandboxed iframes. Please note that it only works for sandboxed iframes if the value 'allow-same-origin' is set. I left a comment in the test as well.

Comment 24

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

Why only allow-same-origin ? Why would the feature be limited to that?

Comment 25

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

Comment on attachment 8826191 [details] [diff] [review]
bug_1271173_upgrade_insecure_navigation_test.patch

So this is fine, but we need support for non- allow-same-origin case too in sandboxing case.

Comment 26

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

oh, I see, since non- allow-same-origin  isn't same origin, no need to upgrade.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] (review queue closed until backlog cleared) from comment #24)
> Why only allow-same-origin ? Why would the feature be limited to that?

As discussed over IRC, upgrade-insecure-requests is limited to same-origin-requests. Further, it would be nice to have some web-platform tests to verify top level navigation upgrades work correctly. Currently there are uir-tests within:
testing/web-platform/tests/upgrade-insecure-requests
but those do not test top-level navigation. Let's see if we can find someone to extend that test suite to cover same origin top level navigations:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d53901509f9ccb81af09b8d5e4de1625205089fc

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d8bd115d6b3
Upgrade-insecure-requests for navigational requests. r=smaug,freddyb
https://hg.mozilla.org/integration/mozilla-inbound/rev/03f5f083bdc0
Test upgrade-insecure-requests for navigational requests. r=smaug,freddyb

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/1d8bd115d6b3
https://hg.mozilla.org/mozilla-central/rev/03f5f083bdc0