Open Bug 1422284 Opened 2 years ago Updated 3 months ago

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

Categories

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

57 Branch
defect

Tracking

()

Tracking Status
firefox60 - affected

People

(Reporter: martin, Unassigned)

References

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 file)

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.
Severity: normal → major
Component: Untriaged → New Tab Page
Summary: CSP upgrade insecure requests follows through to new (insecured) domains → CSP upgrade insecure requests follow through to new (insecured) domains
Component: New Tab Page → DOM: Security
Product: Firefox → Core
(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.
Flags: needinfo?(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
Flags: needinfo?(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)
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.
Assignee: nobody → ckerschb
Priority: -- → P2
Whiteboard: [domsecurity-active]
Severity: major → normal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Additional info: I think that never worked correctly, so it's not a regression for 57.
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
Assignee: ckerschb → cfu
Assignee: cfu → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 1433687
Vino, can you take a look at that? We should get that fixed - thanks!
Assignee: nobody → cegvinoth
Status: NEW → ASSIGNED
Flags: needinfo?(cegvinoth)
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
Please also update test_upgrade_insecure_navigation.html to stress that behavior - thanks!
(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
Flags: needinfo?(cegvinoth)
Tracking for 60, this sounds like a good thing to fix for the next ESR.
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
Flags: needinfo?(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.
Flags: needinfo?(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?
Flags: needinfo?(lhenry)
Sure, if you don't think there is any plan to fix this for 60 then there's no need to track. Thanks!
Flags: needinfo?(lhenry)
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?
Flags: needinfo?(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.)
Flags: needinfo?(annevk)
Duplicate of this bug: 1417901
(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.
Assignee: cegvinoth → jkt
Duplicate of this bug: 1497003

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

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

Assignee: jkt → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Whiteboard: [domsecurity-active] → [domsecurity-backlog1]

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:

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.

(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!

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

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

Duplicate of this bug: 1557399

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

You need to log in before you can comment on or make changes to this bug.