1832249 - upgrade-insecure-requests is enforced in Report-Only mode

Status: RESOLVED FIXED

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

Description

[mk.bugzilla]

Steps to reproduce:

Visit a HTTP website (in my case, a local test website) with the following Content Security Policy in Report-Only mode:

content-security-policy-report-only: default-src 'self'; img-src data: 'self'; script-src 'self' 'unsafe-eval' 'unsafe-inline' https://maps.google.com https://*.googleapis.com; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; upgrade-insecure-requests; frame-src 'self' https://www.google.com/maps/ .google.com; connect-src 'self' https://.googleapis.com .google.com https://.gstatic.com data: blob:; font-src 'self' data: https://fonts.gstatic.com;

Actual results:

All assets of the page, despite being referenced via HTTP, are loaded via HTTPS since upgrade-insecure-requests is enforced.

Expected results:

All assets of the page should be loaded via HTTP and upgrade-insecure-requests should be ignored in Report-Only mode.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Security' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Frederik Braun [:freddy]]

Uh, that's unfortunate. Thanks for the report!

Looks like in nsCSPContext::GetUpgradeInsecureRequests(bool* outUpgradeRequest) , we're going through all policies that have the directive in order to check that the current context is subject to upgrading, but we forget to check whether it's a report-only CSP e.g. using mPolicies[i]->getReportOnlyFlag().

June, do you want to take a look when you get back?
It looks like a simple missing if, but not sure if we have tests for this that need changing (or need to be added).

Comment 3

[Daniel Veditz [:dveditz]]

I agree that bit of code looks wrong and doesn't take report-only into account, but the reporter should be aware that browsers are moving in the future to do more aggressive upgrading as standard behavior, apart from the opt-in nature of CSP. "Mixed Content level 2" (which chrome is shipping now) will upgrade image and media URLs, for example. And the three browser engines webkit (Apple), blink (chromium), and gecko (mozilla) have various opt-in or opt-out experiments in upgrading content. In Firefox it's the opt-in "HTTPS-only" mode, for example.

Comment 4

[mk.bugzilla]

I am aware of this. The report-only is currently enabled to fix issues before using it. But since this error occurred on my local machine (since the production servers already use HTTPS of course), it’s unlikely for me and for many others to issue local TLS certificates just because one browser ignores a flag.

Comment 5

[Daniel Veditz [:dveditz]]

Will that stop the reports, also? or just the enforcement?

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to mk.bugzilla from comment #4)

I am aware of this. The report-only is currently enabled to fix issues before using it.

Report-only is ignored for upgrade-insecure-request. See https://w3c.github.io/webappsec-upgrade-insecure-requests/#delivery

Monitoring the upgrade-insecure-requests directive has no effect: the directive is ignored when sent via a Content-Security-Policy-Report-Only header. Authors can determine whether or not upgraded resources' original URLs were insecure via Content-Security-Policy-Report-Only. For example, Content-Security-Policy-Report-Only: default-src https:; report-uri /endpoint. See § 3.4 Reporting Upgrades for additional detail.

Comment 7

[mk.bugzilla]

It’s not about reports. I’m aware of that there is no monitoring for this directive. But as you mentioned: “the directive is ignored”. And that’s not the case for Firefox.

Comment 8

[June Wilde (she/her) [:jewilde]]

Attachment: Bug 1832249 - Consider report-only flag when upgrading insecure requests; r=dveditz

Comment 9

[June Wilde (she/her) [:jewilde]]

I was able to reproduce that we're performing the upgrade due to upgrade-insecure-requests in the csp report-only header when we shouldn't be and that the proposed fix does solve the problem

I need to add some finishing touches to the test case so we don't regress on this and then I'll assign someone for review

thanks all!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/711260366cf7
Consider report-only flag when upgrading insecure requests; r=dveditz

Comment 11

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/711260366cf7