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

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Christoph Kerschbaumer [: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

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1355801_nonce_image_tests.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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?

Comment 4

[Mike West]

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?

Comment 5

[Daniel Veditz [:dveditz]]

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 6

[Daniel Veditz [:dveditz]]

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

Comment 7

[Daniel Veditz [:dveditz]]

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?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8862321 [details] [diff] [review]
bug_1355801_nonce_image_tests.patch

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

r=dveditz

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92676fadb9e2
https://hg.mozilla.org/mozilla-central/rev/b62ad39ce4a6