nonce attribute applied to <img> when per spec it shouldn't be

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments, 1 obsolete attachment)

testcase: 

data:text/html,<meta http-equiv="content-security-policy" content="img-src 'nonce-abc'"><img src="https://www.mozilla.org/media/img/firefox/template/header-logo-inverse.510f97e92635.png" nonce=abc>

vs

data:text/html,<meta http-equiv="content-security-policy" content="img-src 'nonce-abc'"><img src="https://www.mozilla.org/media/img/firefox/template/header-logo-inverse.510f97e92635.png">

Mike West says the nonce attr should only apply to script/style things.

It looks to me like nsCSPContext::ShouldLoad just gets nonce for all HTML elements and then we enforce the nonce thing nsCSPPolicy::permits regardless of the aDir value, right?
Flags: needinfo?(ckerschb)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #0)
> It looks to me like nsCSPContext::ShouldLoad just gets nonce for all HTML
> elements and then we enforce the nonce thing nsCSPPolicy::permits regardless
> of the aDir value, right?

Yes, that is correct and we should fix that. For inline styles and scripts we operate correctly [1], but for loading external resources we currently also investigate the nonce for images which would allow an image to load if the nonce on the image matches.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#500
Assignee: nobody → ckerschb
Flags: needinfo?(ckerschb)
Priority: -- → P1
Whiteboard: [domsecurity-active]
Created attachment 8857533 [details] [diff] [review]
bug_1355801_nonce_image_tests.patch
Attachment #8857533 - Flags: review?(dveditz)
Created attachment 8857534 [details] [diff] [review]
bug_1355801_nonce_should_only_apply_to_script_and_style.patch

Dan, we should not extract the nonce for anything other than styles and scripts. Potentially the CSP parser shouldn't even parse "img-src 'nonce-abc'", but probably it's error prone to get that updated, so this sounds like the better solution. Besides, I we would run into the same problem with "default-src 'nonce-abc'".

What do you think? Acceptable?
Attachment #8857534 - Flags: review?(dveditz)
(Assignee)

Updated

2 years ago
Blocks: 1231788
Component: Security → DOM: Security
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 4

2 years ago
FWIW, I'm not philosophically opposed to supporting nonces for other resource types, it just doesn't seem very valuable (and it sounds like it's harder to do in Blink than it was in y'all's engine :) ). If y'all want to keep this behavior, file an issue against the spec (and HTML) so we can get everyone on the same page?
I can make a strong case for SRI for other types than script/style, but yeah, nonces not so much.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Potentially the CSP parser shouldn't even parse "img-src 'nonce-abc'",

It should maybe ignore the nonce in those cases, but it should parse it at least to the extent it can safely ignore it. At best spit out a warning about an unsupported keyword.

> Besides, I we would run into the same problem with "default-src 'nonce-abc'".

Yes, we have to support that, and then require nonces only for script and style.
Comment on attachment 8857534 [details] [diff] [review]
bug_1355801_nonce_should_only_apply_to_script_and_style.patch

Review of attachment 8857534 [details] [diff] [review]:
-----------------------------------------------------------------

The CSP internals seem _way_ too generic. Not knowing what kind of directive we're enforcing in the permits() loop is going to bite us every time there's some keyword or feature that only applies to some source directives. Ideally, for example, nsCSPNonceSrc::permits() would say "If I'm not checking script or style ignore it", and then if we add nonces elsewhere (<embed>? unlikely but go with it) we'd fix up that spot.

Even better would be to ignore it during parsing, but as you point out default-src complicates things.

Given all that, I guess this is a reasonable place to fix it. r=dveditz
Attachment #8857534 - Flags: review?(dveditz) → review+
Comment on attachment 8857533 [details] [diff] [review]
bug_1355801_nonce_image_tests.patch

Review of attachment 8857533 [details] [diff] [review]:
-----------------------------------------------------------------

Either the test is confused or I am. In the comments it says "make sure that all three images load" but then in the code it passes if the image is blocked. They are blocked because img-src effectively has the empty set, correct? Is there already a test (for script, I guess) that checks that a resource with an incorrect nonce but matching a host expression is still allowed?
Flags: needinfo?(ckerschb)
(Assignee)

Updated

2 years ago
Depends on: 1360130
(In reply to Daniel Veditz [:dveditz] from comment #6)
> The CSP internals seem _way_ too generic. Not knowing what kind of directive
> we're enforcing in the permits() loop is going to bite us every time there's
> some keyword or feature that only applies to some source directives.
> Ideally, for example, nsCSPNonceSrc::permits() would say "If I'm not
> checking script or style ignore it", and then if we add nonces elsewhere
> (<embed>? unlikely but go with it) we'd fix up that spot.
> 
> Even better would be to ignore it during parsing, but as you point out
> default-src complicates things.

I agree with all you just mentioned and I filed Bug 1360130 to address those issues. Ultimately we should fix that, but as we both agree. For now, let's go with the suboptimal fix in the backend.
No longer depends on: 1360130
Created attachment 8862321 [details] [diff] [review]
bug_1355801_nonce_image_tests.patch

(In reply to Daniel Veditz [:dveditz] from comment #7)
> Either the test is confused or I am. In the comments it says "make sure that
> all three images load" but then in the code it passes if the image is
> blocked.

You are not confused :-) I added the comment when I had a different test in mind. This test seems appropriate now but I forgot to update the comment before flagging you for review.

Here is a new version of the patch with the comment updated.
Attachment #8857533 - Attachment is obsolete: true
Attachment #8857533 - Flags: review?(dveditz)
Flags: needinfo?(ckerschb)
Attachment #8862321 - Flags: review?(dveditz)
Comment on attachment 8862321 [details] [diff] [review]
bug_1355801_nonce_image_tests.patch

Review of attachment 8862321 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz
Attachment #8862321 - Flags: review?(dveditz) → review+

Comment 11

2 years ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92676fadb9e2
Nonce should only apply to script and style. r=dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/b62ad39ce4a6
Nonce should not apply to images tests. r=dveditz

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92676fadb9e2
https://hg.mozilla.org/mozilla-central/rev/b62ad39ce4a6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.