Open Bug 1422284 Opened 2 years ago Updated 3 months ago
CSP upgrade insecure requests follow through to new (insecured) domains
45 bytes, text/x-phabricator-request
|Details | Review|
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.
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.
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
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
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
Vino, can you take a look at that? We should get that fixed - thanks!
Assignee: nobody → cegvinoth
Status: NEW → ASSIGNED
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()  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!  https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10410  https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#3153  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
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 . 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  Please let me know if this is related this bug, https://bugzilla.mozilla.org/show_bug.cgi?id=1374741  https://dxr.mozilla.org/mozilla-central/rev/21f7f94a2c18dc8010ac2f300a36cc4ddd16081c/docshell/base/nsDocShell.cpp#10812  https://dxr.mozilla.org/mozilla-central/rev/21f7f94a2c18dc8010ac2f300a36cc4ddd16081c/browser/base/content/nsContextMenu.js#799
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.
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?
Sure, if you don't think there is any plan to fix this for 60 then there's no need to track. Thanks!
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?
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.)
(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
Assignee: jkt → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Whiteboard: [domsecurity-active] → [domsecurity-backlog1]
You need to log in before you can comment on or make changes to this bug.