CSP: Evaluate upgrade-insecure-requests before block-all-mixed-content

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox48+ fixed, firefox49+ fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → ckerschb
Blocks: 1122236
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
(Assignee)

Comment 1

2 years ago
Firefox is not spec compliant [1] because block-all-mixed-content gets precedence over upgrade-insecure-requests at the moment, but it should be the other way around.

[1] https://www.w3.org/TR/upgrade-insecure-requests/#mix
(Assignee)

Comment 2

2 years ago
Created attachment 8753266 [details] [diff] [review]
bug_1273418_upgrade_insecure_before_block_all_mixed_content.patch

Please note that websockets are blocked by default if mixed-content. If upgrade-insecure-requests and block-all-mixed-content are set within one policy than websockets are upgraded correctly. In other words, the only change we have to perform is within nsMixedContentBlocker to make Firefox's implementation spec compliant.
Attachment #8753266 - Flags: review?(tanvi)
(Assignee)

Comment 3

2 years ago
Created attachment 8753267 [details] [diff] [review]
bug_1273418_upgrade_insecure_before_block_all_mixed_content_tests.patch
Attachment #8753267 - Flags: review?(tanvi)
(Assignee)

Comment 4

2 years ago
Once reviewed we should set the appropriate uplift request flags so we don't ship the feature without being spec compliant.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Created attachment 8753266 [details] [diff] [review]
> bug_1273418_upgrade_insecure_before_block_all_mixed_content.patch
> 
> Please note that websockets are blocked by default if mixed-content. If
> upgrade-insecure-requests and block-all-mixed-content are set within one
> policy than websockets are upgraded correctly.
What do you mean by this?  In nsMixedContentBlocker, we return accept for websockets and proceed with the load.  Does the websocket code look for the upgrade-insecure-request directive?  I guess there is no need for it to look for block-all, since it blocks all by default.

> In other words, the only
> change we have to perform is within nsMixedContentBlocker to make Firefox's
> implementation spec compliant.
Comment on attachment 8753266 [details] [diff] [review]
bug_1273418_upgrade_insecure_before_block_all_mixed_content.patch

>   // Disallow mixed content loads for workers, shared workers and service
>   // workers.
>   if (isWorkerType) {
>     // For workers, we can assume that we're mixed content at this point, since
>     // the parent is https, and the protocol associated with innerContentLocation
>     // doesn't map to the secure URI flags checked above.  Assert this for
>     // sanity's sake

Is there a reason the worker code was in between the block-all and the upgrade-insecure portions of the code?  I assume not, but just checking.
Attachment #8753266 - Flags: review?(tanvi) → review+
[Tracking Requested - why for this release]:
We need to fix this in 49 and uplift to 48 in order to be spec compliant.  Otherwise we may break some websites that rely on upgrade-insecure to be processed before block-all.
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
Comment on attachment 8753267 [details] [diff] [review]
bug_1273418_upgrade_insecure_before_block_all_mixed_content_tests.patch

We should have at least one test that just has upgrade-insecure-requests and not block-all.  You can add a new test, or just remove your second addition of block-all from this patch.
Attachment #8753267 - Flags: review?(tanvi) → review-
(Assignee)

Comment 9

2 years ago
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #6)
> Is there a reason the worker code was in between the block-all and the
> upgrade-insecure portions of the code?  I assume not, but just checking.

I think that's just coincidence. At least there is no reason I would know why this code was between block all and upgrade-insecure.
(Assignee)

Comment 10

2 years ago
Created attachment 8754785 [details] [diff] [review]
bug_1273418_upgrade_insecure_before_block_all_mixed_content_tests.patch

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #8)
> We should have at least one test that just has upgrade-insecure-requests and
> not block-all.  You can add a new test, or just remove your second addition
> of block-all from this patch.

Actually, there are a lot of other upgrade-insecure-request mochitests, that's why I added it for both tests within that file. Either way is fine for me though; we should just have one test to make sure that upgrade-insecure has precedence over block-all to make sure we are not regressing it - thanks!
Attachment #8753267 - Attachment is obsolete: true
Attachment #8754785 - Flags: review?(tanvi)
Tracking this bug for 48/49 based on Comment 7 and the potential to break websites.
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox48: ? → +
tracking-firefox49: ? → +
Attachment #8754785 - Flags: review?(tanvi) → review+

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd62602ddbf6
https://hg.mozilla.org/mozilla-central/rev/25805491cfe8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Christoph, are you going to request the uplift to 48? Thanks
Flags: needinfo?(ckerschb)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8753266 [details] [diff] [review]
bug_1273418_upgrade_insecure_before_block_all_mixed_content.patch

Approval Request Comment
We need to fix this in 49 and uplift to 48 in order to be spec compliant.  Otherwise we may break some websites that rely on upgrade-insecure to be processed before block-all.

[Feature/regressing bug #]:
Bug 1122236 - Implement block-all-mixed-content CSP directive

[User impact if declined]:
Websites using the CSP directive upgrade-insecure-requests and block-all-mixed-content within the same policy would experience blocked requests instead of upgraded requests.

[Describe test coverage new/current, TreeHerder]:
We have mixed content as well as CSP tests within dom/security/test on Treeherder, in addition we modified one of those tests (other patch within this bug) to account for this scenario.

[Risks and why]:
Low - Since it only affects pages that use the CSP directive upgrade-insecure-requests as well as block-all-mixed-content within the same policy.

[String/UUID change made/needed]:
no
Flags: needinfo?(ckerschb)
Attachment #8753266 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

2 years ago
Comment on attachment 8754785 [details] [diff] [review]
bug_1273418_upgrade_insecure_before_block_all_mixed_content_tests.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

See comment 15.
Attachment #8754785 - Flags: approval-mozilla-aurora?
Comment on attachment 8753266 [details] [diff] [review]
bug_1273418_upgrade_insecure_before_block_all_mixed_content.patch

Taking it to avoid breaking websites
Attachment #8753266 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8754785 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.