1422284 - CSP upgrade insecure requests follow through to new (insecured) domains

Status: RESOLVED FIXED

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

Description

[Martin]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171128222554

Steps to reproduce:

1) Generate a webste with a full CSP header. 
Including Content-Security-Policy: upgrade-insecure-requests;

2) Link from this website to another file on the same website that generates a transfer to a new domain. Such as 

3) https://www.site1.com/list_of_links.php ::
 
4) With a list of anchor links to: <a href=/linx.php?code=34>A Link</a>

5) The target website link address is in a database. The target website address is http:// The target website by default opens in a new tab/window.

ISSUE 1:

P1) Clicking on the link to the new website presents the main issue (set out below). 

ISSUE 2:

P2) Clicking on the link from the listing and selecting "open in new tab/window" avoids the issue. 




Actual results:

external website fails to load as loading is only carried out via HTTPS. If external website is not HTTPS this fails.

BUT:
When clicking the link from list.php (stage 1, above) and opening in a NEW TAB the issue does not appear. 

The issue has been found with absolute certainty to be due to:

header:: Content-Security-Policy: upgrade-insecure-requests; being applied when a transfer from a server side script to an external http:// website domain occurs.

This issue only occurs on Firefox not on other browsers. This issue appears to occur on 57 only. This issue only occurs on anchor click not opening in a new window AS DIRECTED BY THE USER. 

Process:
User clicks link from website ---> link opens a new target window and the redirection script on the server, loads the website page and exports with a redirect to the browser. 

Browser fails to load as the http:// has been silently upgraded 



Expected results:

New website should load via http:// channel as that is the direction and this channel should not be uploaded via the CSP of the OLD website the browser is coming from.

SOLUTION:
CSP should NOT upgrade header links to EXTERNAL website domains.

Comment 1

[Daniel Veditz [:dveditz]]

(In reply to Martin from comment #0)
> 2) Link from this website to another file on the same website that generates
> a transfer to a new domain. Such as 

What do you mean by "generates a transfer"? A 30x redirect? Do you have an example page somewhere?

> 4) With a list of anchor links to: <a href=/linx.php?code=34>A Link</a>
> 
> 5) The target website link address is in a database. The target website
> address is http:// The target website by default opens in a new tab/window.

How do they open a new window? You don't show a target=_blank in your 4) example, is there one? Or does the link.php? page actually have content that then does a window.open() of a new window?

Again, a testcase or site would really help here.

Comment 2

[Martin]

To clarify the new window is opened by a target="_blank" from the listing page. this opens the `linx.php` page in a new window. This page loads the external site in a 301/2 redirect. 

I did have a live site this was on but I found this critical and so have disabled CSP upgrade-insecure-requests on these pages; I will set up a replica page with the details as outlined in the bug report.

Cheers

Comment 3

[Martin]

An example of this issue can be found here: https://www.halesworth.net/links/list_two.php?code=3 

Linked sites this applies to are: 

New Cut Arts, Halesworth
Walpole Chapel

(and some others)

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

I think I understand the problem, let me summarize:
* click same-origin link to force top-level navigation
* the initial load is same origin (hence upgrade-insecure-requests applies)
* the requests hits a 30x redirect to navigate to cross origin page

Internally we check if uir applies only for the first load and store that information in the loadinfo. After the redirect we just clone the loadinfo of the initial channel instead of re-evaluating the same-origin property.

We should fix that - thanks for reporting.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Additional info: I think that never worked correctly, so it's not a regression for 57.

Comment 6

[Martin]

Thank you for confirming this, Christoph. 

I would note that the site has been running for a number of months (~12) with CSP in place and several months (~3) with Upgrade-insecure-requests in place. 

We had not noted this behaviour previously; so would have thought this behaviour re: upgrade-insecure-requests was not the same in FF56. 

Cheers

Martin

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Vino, can you take a look at that? We should get that fixed - thanks!

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Looking at the steps in comment 4 I think we should:
1) Move IsConsideredSameOriginForUIR() from docshell into the contentSecurityManager.cpp or nsCSPContext.cpp but docshell should still call into it
2) When redirecting, we should also call into that function, probably best within CloneLoadInfoForRedirect() [2]
3) Have SetUpgradeInsecureRequests() take a boolean argument which allows us to toggle that bit in the loadInfo. Please also add a big fat warning above that method that one should only toggle to false if that person knows exactly what the person is doing!

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10410
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#3153
[3] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.h#95

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Please also update test_upgrade_insecure_navigation.html to stress that behavior - thanks!

Comment 11

[Phabricator Automation]

Attachment: Bug 1422284 - Added upgrade insecure requests check after redirects.

Comment 12

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Phabricator Automation from comment #11)
> Created attachment 8951655 [details]
> Bug 1422284 - Added upgrade insecure requests check after redirects.

I made the code changes as suggested previously but still the issue exists,
may be this will be resolved if the CSP null issue from Bug 1437009 is resolved,
https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10753

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 60, this sounds like a good thing to fix for the next ESR.

Comment 14

[Vinothkumar Nagasayanan [:vinoth]]

Summary of the issue:

Visit the website: https://www.halesworth.net/links/list_two.php?code=3 

Click on any one link with the following heading,

New Cut Arts, Halesworth
Walpole Chapel

1. Normally clicking the link, opens a new tab and Security Error(your connection is not secure) is thrown because of "Content-Security-Policy: upgrade-insecure-requests"

2. Right-click and click "Open link in new tab", now the site loads and redirected to insecure page(http) even though "Content-Security-Policy: upgrade-insecure-requests" is present.

Reason for the issue:

We check for the upgrade-insecure-requests CSP from the triggering principal [1]. 
Correct Triggering is passed for the normal click event.
But triggering principal doesn't contain the CSP for the right click event which is handled in openLinkInTab function in nsContextMenu.js [2]

Please let me know if this is related this bug, https://bugzilla.mozilla.org/show_bug.cgi?id=1374741


[1] https://dxr.mozilla.org/mozilla-central/rev/21f7f94a2c18dc8010ac2f300a36cc4ddd16081c/docshell/base/nsDocShell.cpp#10812
[2] https://dxr.mozilla.org/mozilla-central/rev/21f7f94a2c18dc8010ac2f300a36cc4ddd16081c/browser/base/content/nsContextMenu.js#799

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

As discussed, let's wait for Bug 1374741- cnce we have the right triggeringPrincipal then hopefully those errors will just disappear and we just have to incorporate the steps from comment 9.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Hey Liz,
that part of upgrade-insecure-requests never worked correctly since it's introduction within Bug 1391011. I think there is no need for tracking this for 60. I think we need to wait for Bug 1374741 to be resolved anyway, before we can move this bug forward. Would you be fine with clearing the tracking flag?

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Sure, if you don't think there is any plan to fix this for 60 then there's no need to track. Thanks!

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Anne, I had a quick discussion with :jkt regarding what should be expected in that case. Let me summarize the scenario.

Imagine a.com ships with upgrade-insecure-requests. A link on that page causes a new top-level navigation to the same-origin. In that case upgrade-insecure-requests applies and the load gets upgraded. Now the top-level loads hits a 302 to perform a cross origin load. Would you expect that load to be upgraded or not. Asked, differently should upgrade-insecure-requests apply to the redirected cross-origin top-level load?

Comment 19

[Anne (:annevk)]

For navigations you only upgrade those origins in https://w3c.github.io/webappsec-upgrade-insecure-requests/#upgrade-insecure-navigations-set and this new origin would not be in that set, so no.

Also, this seems like a duplicate of bug 1417901? (I had some "déjà vu", so found my old specification issue which led me there.)

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Anne (:annevk) from comment #19)
> For navigations you only upgrade those origins in
> https://w3c.github.io/webappsec-upgrade-insecure-requests/#upgrade-insecure-
> navigations-set and this new origin would not be in that set, so no.

Thanks for confirming.

> Also, this seems like a duplicate of bug 1417901? (I had some "déjà vu", so
> found my old specification issue which led me there.)

Indeed, that is a duplicate.

Comment 23

[Dave Brondsema]

Any updates on fixing this? I am still seeing the problematic behavior in 65.0.1

Comment 24

[jscher2000]

Apparently this affects links to artist's sites on Deviant Art because the site uses a redirector script. For example, the link to smbhax.com in the description paragraph below the image on:

https://www.deviantart.com/smbhax/art/Visual-792896874

Firefox users get stuck; other browsers proceed. Related SuMo thread from today:

https://support.mozilla.org/questions/1255969

Comment 25

[antoine.gersant]

Hi :ckerschb, can you clarify why the priority of this defect was lowered? This issue affects real world websites and is making Firefox look like a janky browser to users.

Because of this issue, I had to recommend Chrome to several users of Polaris (https://github.com/agersant/polaris) so they could use all the features of its web client (specifically, integration with Last.fm). DeviantArt, a very high-traffic website, is also negatively impacted, as detailed above.

Looking at the P1 and P2 items for this component, I see several tickets which seem to have much lower impact such as:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1451938
• https://bugzilla.mozilla.org/show_bug.cgi?id=1537685
• https://bugzilla.mozilla.org/show_bug.cgi?id=1457503

I do understand there is a large backlog of items to work on and I don't expect a quick resolution, but I would like to know why this isn't deemed more important.

Thank you.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

(In reply to antoine.gersant from comment #25)

Hi :ckerschb, can you clarify why the priority of this defect was lowered? This issue affects real world websites and is making Firefox look like a janky browser to users.

The priority in the bug (whether it's P2 or P3) does not necessarily reflect the actual priority. In this particular case it's that :jkt is going to work on some other (higher priority) project and we can not have P2 bugs that are not assigned to someone.

The good news is that fixing upgrade-insecure-requests bugs is on our Q2 roadmap. In other words, this bug as well as some other UIR bugs will get fixed within this quarter!

Comment 27

[antoine.gersant]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)

That is great news, thank you very much for the update!

Comment 29

[antoine.gersant]

Assuming this got de-prioritized from the 2019 roadmap, is there a new estimate for when this may get fixed?

Comment 30

[ben.kay01]

Hey there,

At the BBC we have a mixed estate that we are trying to fix to get HSTS switched on, but in the mean time having upgrade-insecure-requests in our CSP's would be a massive help in getting this done, however due to this bug also being the same in same origin links we can't switch it.

Any word on where this is up to?

Ben

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

(In reply to ben.kay01 from comment #30)

Any word on where this is up to?

Clearing priority flags so this bug shows up in our next Triage Meeting (Tuesday).

Comment 32

[ben.kay01]

Thanks very much

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Beats me that we never fixed that problem (we should have) - I'll assign to myself and get that fixed!

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1422284 - Upgrade insecure requests should only apply to top-level same-origin redirects. r=baku,valentin

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f4e018c0af1
Upgrade insecure requests should only apply to top-level same-origin redirects. r=baku,valentin,necko-reviewers

Comment 36

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1f4e018c0af1