855326 - CSP 1.1 : nonce-source (experimental)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Ian Melven :imelven]

The CSP 1.1 spec includes an experimental script-nonce directive : https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#script-nonce

The purpose of script-nonce (script-hash has also been discussed) is to allow a policy to selectively whitelist inline script that otherwise would be blocked by CSP (assuming the policy does not specify script-src: unsafe-inline)

This bug can be used to track experiments in this direction including patches.

Comment 1

[Devdatta Akhawe [:devd]]

Not only does script-nonce allow inline scripts, it also allows you to bypass script-src restrictions. For example, even if you you don't have any allowed script sources (except for the nonce- source), a script element with arbitrary src can be injected if it has the right nonce value (the nonce overrides any src restrictions).

Looking at the way the code is written, the check for script sources is done by an nsIContentPolicy listener in the CSPService, but the check for inline scripts is done in content/base/src/nsScriptLoader.cpp. First step will likely be changing the latter to allow inline scripts if it has the right nonce. Next step would be to check the nsIContentPolicy implementation (shouldLoad) to check whether the element in question has the right nonce value (and thus can ignore the script-src restrictions).

Comment 2

[Garrett Robinson [:grobinson]]

Updated link to relevant part of spec: https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#script-src

Comment 3

[Garrett Robinson [:grobinson]]

Attachment: Test 1

This is a first draft of a test. It only tests script-nonce on inline scripts, with the following test cases:

1. inline script has the correct nonce (allowed)
2. inline script has an incorrect nonce (blocked)
3. inline script has no nonce (blocked)

Comment 4

[Garrett Robinson [:grobinson]]

Attachment: Platform 1

This is a first pass (WIP) of implementing script-nonce. It only checks inline scripts for the nonce, which is the second check (in content/base/src/nsScriptLoader.cpp) mentioned by devd in Comment 1.

This is experimental and may be subject to drastic change. Test 1 passes with this patch applied. Note that you need to apply the most recent patch from Bug 764937 to fix misbehavior in the 1.0 parser when default-src is not specified.

Comment 5

[Garrett Robinson [:grobinson]]

Attachment: Test 2

New test also tests for external script loads with the same 3 tests types (correct nonce, incorrect nonce, and no nonce). Test currently fails, as I have not yet added support to load external scripts based on nonce-source.

This test continues to use the "counter" method employed by test_CSP_inlinescript.html. I originally rewrote it using the more precise method used in test_CSP.html, but it is unfortunately impossible AFAICT to distinguish between different inline script test failures, so we would have to resort to a hybrid specific-tests + counter method. I decided to use the counter method throughout to keep things simple for now.

Comment 6

[Garrett Robinson [:grobinson]]

Attachment: Test 3

Improve comments and add example external .js file to Makefile.in so it can be found by the test server.

Comment 7

[Garrett Robinson [:grobinson]]

Attachment: Platform 2

Load external scripts based on nonce-source.

This code ran into the same issues with script preloading as seen in Bug 612921. For now, I am inferring whether we are calling the CSP ShouldLoad on a preload from the given context, which may be sufficient for scripts (and later, styles).

The current technique of storing and checking nonces (getAllowsNonce) is debatable. Since they are stored as CSPSources, it would be nice if they were checked along with everything else in the chained calls to permits(). However, these calls all only take an nsIURI, so we would have to add a parameter for the nonce to *every* call to permits.

Comment 8

[Garrett Robinson [:grobinson]]

Attachment: Platform 3

Turns out it's not so hard to get the nonce in CSPUtils.jsm. This rewrite has much better design and is more true to the new definition of nonce as source.

Comment 9

[Garrett Robinson [:grobinson]]

Attachment: Platform 4

Complete implementation of nonce-source for inline and external scripts and stylesheets.

Comment 10

[Garrett Robinson [:grobinson]]

Attachment: Test 4

Comment 11

[Devdatta Akhawe [:devd]]

I am not a fan of sending sending a REJECT_SERVER for preload requests. I think that is a bad idea---you are basically disabling script and source preloads for a document implementing CSP.

Instead, maybe move this to the bottom of the shouldLoad: if it would have been accepted by the CSP policies already in effect, it will get accepted. If it would have been rejected by them (but accepted by nonce-source), then we reject the load during preload but allow the load the second time shouldLoad is called with the scriptelement context. If it would have been rejected by both HOSTSRCs and nonce-srcs, then it will get rejected both times.

Comment 12

[Devdatta Akhawe [:devd]]

s/source/style

Comment 13

[Garrett Robinson [:grobinson]]

Attachment: Patch 5

Hide script-nonce behind a pref (security.csp.experimentalEnabled) that is off by default.

I consolidated the patch into a single file since there was is no reason to separate the code into platform/test now that development has converged.

Comment 14

[Garrett Robinson [:grobinson]]

(In reply to Devdatta Akhawe [:devd] from comment #11)
> I am not a fan of sending sending a REJECT_SERVER for preload requests. I
> think that is a bad idea---you are basically disabling script and source
> preloads for a document implementing CSP.
> 

I agree - thanks for reminding me to revisit this issue.

> Instead, maybe move this to the bottom of the shouldLoad: if it would have
> been accepted by the CSP policies already in effect, it will get accepted.
> If it would have been rejected by them (but accepted by nonce-source), then
> we reject the load during preload but allow the load the second time
> shouldLoad is called with the scriptelement context. If it would have been
> rejected by both HOSTSRCs and nonce-srcs, then it will get rejected both
> times.

It is slightly tricker than this because

1) shouldLoad caches the result of policy decisions, so the (potentially incorrect, since it does not have the nonce) decision for the preload is automatically applied and no re-check is performed for the "real load".
2) If the current code rejects the preload, it will report a violation, which will cause the unit tests to erroneously fail, since they listen for the csp-on-violate-policy observer topic to determine what CSP blocks. Further, this would be confusing to anybody who is reading the CSP report - the same script is both blocked and allowed? - a confusing behavior which has already been reported in Bug 612921.

I am attaching a patch which essentially implements your suggestion, plus workarounds for the issues I describe above. It returns the policy decision for CSP (without the nonce) for the preload, but does not cache the result and does not report a violation. When the "real" load happens, shouldLoad behaves as usual.

Comment 15

[Garrett Robinson [:grobinson]]

Attachment: Patch 6

Comment 16

[Devdatta Akhawe [:devd]]

Comment on attachment 767335 [details] [diff] [review]
Patch 6

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

mostly lgtm; remove the random whitespace changes for in CSPUtils.jsm (cspwarn). Does the spec really say "*" for the nonce-<nonce-value>? If so, we should change the spec or we should follow the spec.

Comment 17

[Garrett Robinson [:grobinson]]

Attachment: Patch 7

Fixes nits from devd's feedback.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Sorry for the delay on the review.  I'll try to get it done next week.  Please pester me on IRC if I don't!

Comment 19

[Ian Melven :imelven]

Comment on attachment 769500 [details] [diff] [review]
Patch 7

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

I went through with with Garrett in person.

Check that the violation reporting for when the nonce is wrong (ie the nonce in the element doesn't match the policy)
works as we think. Consider adding a test for this case.

It seems fine to me to have one pref enable/disable *all* of the experimental CSP stuff

Overall, this looks pretty good - the main thing seems to be to decide if we want to use nonces to allow
external loads as well, or if they should just be for inline scripts/styles. My personal opinion is to do the latter for now.

::: content/base/src/CSPUtils.jsm
@@ +67,5 @@
>  const R_KEYWORDSRC = new RegExp ("^('self'|'unsafe-inline'|'unsafe-eval')$", 'i');
>  
> +// nonce-source      = "'nonce-" nonce-value "'"
> +// nonce-value       = *( ALPHA / DIGIT )
> +const R_NONCESRC = new RegExp ("^'nonce-([a-zA-Z0-9]*)'$", 'i');

i know you discussed this with Adam et al and you both agreed this should probably be a + and not a * (no empty nonce). I would go ahead and implement it that way now and then make sure the spec change happens.

@@ +200,5 @@
>    // Default to false if not specified.
>    this._specCompliant = (aSpecCompliant !== undefined) ? aSpecCompliant : false;
>  
> +  // Does this CSP contain directives from the experimental CSP 1.1 spec?
> +  this._isExperimental = false;

this is no longer needed in this iteration

@@ +767,3 @@
>     *        one of the SRC_DIRECTIVES defined above
> +   * @param aContext
> +   *        the element in question

we only need this if we want to allow nonces for elements other than inline styles and scripts - discussion has so far focused on only allowing nonces for inline styles and scripts so we probably could (should?) get rid of this

::: content/base/src/contentSecurityPolicy.js
@@ +160,5 @@
> +    if (!CSPPrefObserver.experimentalEnabled)
> +      return false;
> +
> +    // TODO: shouldReportViolation?
> +    var contentTypeSources

normally the = would be on this line i think

@@ +559,5 @@
>      }
>  
>      // otherwise, honor the translation
>      // var source = aContentLocation.scheme + "://" + aContentLocation.hostPort;
> +    var res = this._policy.permits(aContentLocation, cspContext, aContext)

we talked about removing the aContext argument

cspContext is the type of CSP directive we want, aContext
is the original DOM element

@@ +565,5 @@
>                : Ci.nsIContentPolicy.REJECT_SERVER;
>  
> +    // Infer if this is a preload for elements that use nonce-source. Since
> +    // aContext is the document and not the element associated with the
> +    // resource for preloads, we cannot determine the nonce. See Bug 612921

i would reword the comment : Since for preloads aContext is the document ... we cannot determine the nonce in this case.

::: content/base/src/nsCSPService.cpp
@@ +23,5 @@
>  #include "mozilla/Preferences.h"
>  #include "nsIScriptError.h"
>  #include "nsContentUtils.h"
> +#include "nsIScriptElement.h"
> +#include "nsIDocument.h"

these look like they might be leftovers that can be removed

::: content/base/src/nsScriptLoader.cpp
@@ +628,5 @@
> +      scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
> +      if (!nonce.IsEmpty()) {
> +        rv = csp->GetAllowsNonce(nonce, nsIContentPolicy::TYPE_SCRIPT, &nonceOK);
> +        NS_ENSURE_SUCCESS(rv, false);
> +      }

we discussed making this a function possibly

@@ +633,5 @@
> +    }
> +
> +    bool scriptElementOK = inlineOK || nonceOK;
> +
> +    if (!scriptElementOK && reportViolations) {

it seems fine to use the reportViolations value from checking allows inline here, since it will be the same for the policy overall.

::: content/base/test/test_CSP_nonce.html
@@ +72,5 @@
> +  ok('rgb(0, 0, 0)' === getElementColorById('inline-style-no-nonce'), "Inline style with no nonce blocked");
> +
> +  ok('rgb(0, 128, 0)' === getElementColorById('external-style-correct-nonce'), "External style with correct nonce allowed");
> +  ok('rgb(0, 0, 0)' === getElementColorById('external-style-incorrect-nonce'), "External style with incorrect nonce blocked");
> +  ok('rgb(0, 0, 0)' === getElementColorById('external-style-no-nonce'), "External style with no nonce blocked");

note that we want to get rid of the external style tests *if* we decide to only allow nonce sources for inline scripts/styles

@@ +101,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +SpecialPowers.pushPrefEnv(
> +  {'set':[["security.csp.speccompliant", true],
> +          ["security.csp.debug", true],

remove enabling debug pref before committing

::: content/smil/nsSMILCSSValueType.cpp
@@ +392,5 @@
>      return;
>    }
>  
>    nsIDocument* doc = aTargetElement->GetCurrentDoc();
> +  if (doc && !nsStyleUtil::CSPAllowsInlineStyle(nullptr,

a note that we don't have an nsIContent here..

it looks to us that nsIDocument does not inherit from nsIContent

::: layout/style/nsStyleUtil.cpp
@@ +417,5 @@
>  }
>  
>  /* static */ bool
> +nsStyleUtil::CSPAllowsInlineStyle(nsIContent* aContent,
> +                                  nsIPrincipal* aPrincipal,

We can get the principal from the content but in the SMIL case, we don't have an nsIContent...

@@ +449,5 @@
>        return false;
>      }
>  
> +    bool nonceOK = false;
> +    // If unsafe-insline is set, skip the nonce check

typo : unsafe-inline

@@ +455,5 @@
> +      nsAutoString nonce;
> +      aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
> +      if (!nonce.IsEmpty()) {
> +        rv = csp->GetAllowsNonce(nonce, nsIContentPolicy::TYPE_STYLESHEET, &nonceOK);
> +        NS_ENSURE_SUCCESS(rv, false);

you want to set aRv here if one is passed in

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 769500 [details] [diff] [review]
Patch 7

Clearing the review flag since Ian gave you lots of good advice.  Flag me again when you've got a new patch.

Comment 21

[Garrett Robinson [:grobinson]]

Attachment: Patch 8

This patch does two things with imelven's review:

1. Removes the script nonce handling for everything but inline scripts and styles. While Chrome's current implementation also allows it to be used on external scripts and styles, and the current CSP 1.1 spec draft suggests it could be used on all of the directives, discussions on webappsec are trending towards only allowing nonce-source to be used for inline scripts and styles.

If, in the future, we want to change this and allow nonce-source to be used for whitelisting external resources as well, Patch 7 has the necessary code.

2. Otherwise, implements all of the review suggestions that are applicable after removing the external resources code.

This patch adds report violation for nonce errors. I am open to other ways to do or improve this - for example, it might be nice to specifically say which nonce was invalid in the report, but that would an additional parameter to logViolationDetails. I also wonder if I shouldn't add something to the unit tests specifically for violation reporting.

There is also some cleanup in contentSecurityPolicy.js.

Try: https://tbpl.mozilla.org/?tree=Try&rev=196e8634021b

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 778749 [details] [diff] [review]
Patch 8

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

Looks good.  I don't think you need to test if violation reports work with nonces, but in case you'd like to try it, add a test here:

http://mxr.mozilla.org/mozilla-central/source/content/base/test/unit/test_cspreports.js#162

One thing to note: in the try run, there's a B2G test failure:
  19:35:45 INFO - 45 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_nonce.html | Inline style with correct nonce allowed
  20:12:52 INFO - I/GeckoDump( 685): 45 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_nonce.html | Inline style with correct nonce allowed

You might wanna dig into that and see why it failed.

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +105,5 @@
>  
>    const unsigned short VIOLATION_TYPE_INLINE_SCRIPT = 1;
>    const unsigned short VIOLATION_TYPE_EVAL = 2;
>    const unsigned short VIOLATION_TYPE_INLINE_STYLE = 3;
> +  const unsigned short VIOLATION_TYPE_NONCE = 4;

Do we want violation reports to be more specific: e.g., do we want to say "hey, that nonce is invalid for scripts" or "that nonce is invalid for styles"?  If so, we should turn these constants into flags and do a bitmask.  If not, then doesn't matter.

::: content/base/src/CSPUtils.jsm
@@ +1624,5 @@
>        s = s + this._host;
>      if (this.port)
>        s = s + ":" + this.port;
> +    if (this._nonce)
> +      s = s + " nonce-" + this._nonce;

shouldn't this be simply:
s = "'nonce-"+this._nonce+"'"?

Are there any cases where there's a _nonce and a value for one of (scheme, _host, port)? If so, this might get kind of wonky with the "s = s +" part.

::: content/base/src/contentSecurityPolicy.js
@@ +162,5 @@
> +
> +    // allow it to execute?
> +    let nonceAllowed = false;
> +    var contentTypeSources =
> +      this._policy._directives[ContentSecurityPolicy._MAPPINGS[aContentType]]._sources;

what if _directives doesn't have that key?  For example, if there's no script-src directive?  This will throw.

You probably want to make sure _directives.hasOwnProperty(that directive) and return false if it doesn't, right?  Or is nonce- allowed in default-src?

::: content/base/test/file_CSP_nonce.html
@@ +27,5 @@
> +    <style nonce=correctnonce>
> +      li#inline-style-correct-nonce { color: green; }
> +    </style>
> +    <style nonce=incorrectnonce>
> +      li#inline-style-incorrect-nonce { color: green; }

If this style is intended to be blocked, please make it red (easier to visually identify a test fail)

::: modules/libpref/src/init/all.js
@@ +4317,5 @@
>  #else
>  pref("dom.forms.inputmode", true);
>  #endif
> +
> +pref("security.csp.experimentalEnabled", false);

Please stick this near the other CSP prefs up by line 1507.

Comment 23

[Garrett Robinson [:grobinson]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #22)
> Comment on attachment 778749 [details] [diff] [review]
> Patch 8
> 
> Review of attachment 778749 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  I don't think you need to test if violation reports work with
> nonces, but in case you'd like to try it, add a test here:
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/unit/
> test_cspreports.js#162
> 

Ok, I won't then :)

> One thing to note: in the try run, there's a B2G test failure:
>   19:35:45 INFO - 45 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/base/test/test_CSP_nonce.html | Inline style with correct
> nonce allowed
>   20:12:52 INFO - I/GeckoDump( 685): 45 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/base/test/test_CSP_nonce.html | Inline style with correct
> nonce allowed
> 
> You might wanna dig into that and see why it failed.
> 

Dug into it - it was actually quite fun. What was happening was the check for the experimentalEnabled flag on the CSPPrefObserver in getAllowsNonce was causing a Javascript Error (could not find object) on B2G *only*. This was due to changes from Bug 798491 that merges JSM's code into a single scope (wrapping them in functions to avoid naming conflicts) in order to cut down own the overhead of maintaining a separate global object for each JSM. By making the CSPPrefObserver an attribute of "this" in the JSM's global scope (a convention that is used everywhere else in the file), the export works properly on both desktop and b2g.

> ::: content/base/public/nsIContentSecurityPolicy.idl
> @@ +105,5 @@
> >  
> >    const unsigned short VIOLATION_TYPE_INLINE_SCRIPT = 1;
> >    const unsigned short VIOLATION_TYPE_EVAL = 2;
> >    const unsigned short VIOLATION_TYPE_INLINE_STYLE = 3;
> > +  const unsigned short VIOLATION_TYPE_NONCE = 4;
> 
> Do we want violation reports to be more specific: e.g., do we want to say
> "hey, that nonce is invalid for scripts" or "that nonce is invalid for
> styles"?  If so, we should turn these constants into flags and do a bitmask.
> If not, then doesn't matter.
> 

I split that flag into two (one for nonce on inline scripts, and one for nonce on inline styles). I could go either way on whether this is the right place to break this down.

> ::: content/base/src/CSPUtils.jsm
> @@ +1624,5 @@
> >        s = s + this._host;
> >      if (this.port)
> >        s = s + ":" + this.port;
> > +    if (this._nonce)
> > +      s = s + " nonce-" + this._nonce;
> 
> shouldn't this be simply:
> s = "'nonce-"+this._nonce+"'"?
> 
> Are there any cases where there's a _nonce and a value for one of (scheme,
> _host, port)? If so, this might get kind of wonky with the "s = s +" part.
> 

Fixed the quotes. The case you describe can never happen because a nonce is a "source" (a space-separated field after a directive). If it matches the NONCE_SRC regex, then we parse it, fill in the CSPSource._nonce field, and return. See CSPSource.fromString for more.

> ::: content/base/src/contentSecurityPolicy.js
> @@ +162,5 @@
> > +
> > +    // allow it to execute?
> > +    let nonceAllowed = false;
> > +    var contentTypeSources =
> > +      this._policy._directives[ContentSecurityPolicy._MAPPINGS[aContentType]]._sources;
> 
> what if _directives doesn't have that key?  For example, if there's no
> script-src directive?  This will throw.
> 
> You probably want to make sure _directives.hasOwnProperty(that directive)
> and return false if it doesn't, right?  Or is nonce- allowed in default-src?
> 

Good catch! Fixed.

> ::: content/base/test/file_CSP_nonce.html
> @@ +27,5 @@
> > +    <style nonce=correctnonce>
> > +      li#inline-style-correct-nonce { color: green; }
> > +    </style>
> > +    <style nonce=incorrectnonce>
> > +      li#inline-style-incorrect-nonce { color: green; }
> 
> If this style is intended to be blocked, please make it red (easier to
> visually identify a test fail)
> 

Done.

> ::: modules/libpref/src/init/all.js
> @@ +4317,5 @@
> >  #else
> >  pref("dom.forms.inputmode", true);
> >  #endif
> > +
> > +pref("security.csp.experimentalEnabled", false);
> 
> Please stick this near the other CSP prefs up by line 1507.

Done.

Comment 24

[Garrett Robinson [:grobinson]]

Attachment: Patch 9

Implements Sid's review suggestions and fixes test failure on B2G. Carrying over r+ from sstamm.

Try: https://tbpl.mozilla.org/?tree=Try&rev=9b8baea852c4

Comment 25

[Garrett Robinson [:grobinson]]

Comment on attachment 780603 [details] [diff] [review]
Patch 9

Want to get some reviewers on the non-CSP code in this patch.

mrbkap, please review the changes to nsScriptLoader.cpp.

dholbert, please review the changes to:

* nsStyleLinkElement.cpp
* nsStyledElement.cpp
* nsSMILCSSValueType.cpp
* nsStyleUtil.cpp
* nsStyleUtil.h

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 780603 [details] [diff] [review]
Patch 9

Not done reviewing yet, but here are some drive-by nits on various parts of the patch:

>Bug 855326 - CSP : script-nonce directive (experimental)

Looks like the commit message is a bit stale -- this is called the "script-src directive" and "style-src directive" now, right? (not "script-nonce" anymore) (I'm going off of the spec linked from this bug's URL field, which never mentions "script-nonce" but does use those other terms.)

>+++ b/content/base/public/nsIContentSecurityPolicy.idl
>   /**
>+   * Whether this policy recognizes a given nonce
>+   * @return Whether or not the nonce is allowed (block the call if false)
>+   */
>+   boolean getAllowsNonce(in AString aNonce,
>+                          in unsigned long aContentType,
>+                          out boolean shouldReportViolation);
>+

Could you document the parameters here, too, like the contextual code does? (with @param)

>diff --git a/layout/style/nsStyleUtil.h b/layout/style/nsStyleUtil.h
>index 99bad2d..753c765 100644
>--- a/layout/style/nsStyleUtil.h
>+++ b/layout/style/nsStyleUtil.h
>@@ -97,17 +97,18 @@ public:
>    *  inline styles ? Returns false if application of the style should
>    *  be blocked.
>    *
>    *  Note that the principal passed in here needs to be the principal
>    *  of the document, not of the style sheet. The document's principal
>    *  is where any Content Security Policy that should be used to
>    *  block or allow inline styles will be located.
>    */
>-  static bool CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
>+  static bool CSPAllowsInlineStyle(nsIContent* aContent,
>+                                   nsIPrincipal* aPrincipal,

Could you mention what this new arg 'aContent' is for, in the header-comment here?  (Ideally it'd be great if we documented the parameters declaratively with @param or similar; if you feel like doing that for the existing params, that would be awesome, but I won't mandate it since there are already a bunch of parameters and they're just documented as prose instead of with @param.)

>+++ b/modules/libpref/src/init/all.js
[...]
>@@ -4315,8 +4316,9 @@ pref("captivedetect.pollingTime", 3000);
> pref("captivedetect.maxRetryCount", 5);
> #endif
> 
> #ifdef RELEASE_BUILD
> pref("dom.forms.inputmode", false);
> #else
> pref("dom.forms.inputmode", true);
> #endif
>+

Looks like you accidentally added a blank line at the end of all.js. Probably want to revert that.

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 780603 [details] [diff] [review]
Patch 9

So, one spec-conformance question, for <link>:

>diff --git a/content/base/src/nsStyleLinkElement.cpp b/content/base/src/nsStyleLinkElement.cpp
>index 99289b2..dc60cb6 100644
>--- a/content/base/src/nsStyleLinkElement.cpp
[...]
>-    if (!nsStyleUtil::CSPAllowsInlineStyle(thisContent->NodePrincipal(),
>+    if (!nsStyleUtil::CSPAllowsInlineStyle(thisContent,
>+                                           thisContent->NodePrincipal(),
>                                            doc->GetDocumentURI(),
>                                            mLineNumber, text, &rv))

If I'm reading this correctly, this will make us check the <link> element for a valid nonce.

The spec doesn't seem to call for that, though.

It *does* mention valid nonces for <script src=...>, in section 4.10:
  # Whenever the user agent fetches a URI... the user agent
  # must act as if it had received an empty HTTP 400 response
  # and report a violation:
  #  * Requesting a script while processing the src attribute
  #    of a script element _that lacks a valid nonce_
[emphasis added]
https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#script-src

But it does _not_ mention nonces at all in the analogous section about external style sheets from <link> & @import, in section 4.11:
  # Whenever the user agent fetches a URI... the user agent
  # must act as if it had received an empty HTTP 400 response
  # and report a violation:
  #  * Requesting external style sheets, such as when
  #    processing the href attribute of a link element
  #    with a rel attribute containing the token stylesheet
  #    or when processing the @import directive in a
  #    stylesheet.
https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#style-src

Since there's no "that lacks a valid nonce" clause there, it sounds like this nonce stuff doesn't apply to <link>. (Maybe I'm misreading, but as far as I can tell, the spec-language is largely identical between <script> and <link>, but with nonce notably absent in the <link> area.)

Comment 28

[Daniel Holbert [:dholbert]]

I have similar questions about the change to nsStyledElement.cpp -- should we really be checking for nonces on any random element that happens to have a style attribute?  (The spec mentions nonces for the style *element*, but it doesn't say anything about them for the style *attribute*.)

And following up slightly on comment 27, RE nsStyleLinkElement -- it looks like I was only partly right -- that's not just <link>, after all -- it's the superclass for HTMLStyleElement, SVGStyleElement, and HTMLLinkElement. If I'm understanding the spec correctly, I think we just want this for <style>, but *not* for <link>, right?

(We could perhaps achieve that by adding a protected method DoesSupportsNonces() on nsStyleLinkElement, which returns false -- and then HTMLStyleElement and SVGStyleElement would get an override that returns true. Or something like that.)

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 780603 [details] [diff] [review]
Patch 9

Test nits:

>+++ b/content/base/test/file_CSP_nonce.html^headers^
>@@ -0,0 +1,2 @@
>+Content-Security-Policy: script-src 'nonce-correctnonce'; style-src 'nonce-correctnonce';

So right now, your test uses the same nonce for script-src and style-src.

It's probably worth making those nonces *different*, and testing that we don't mistakenly accept the script nonce as a style nonce & vice versa.

>diff --git a/content/base/test/test_CSP_nonce.html b/content/base/test/test_CSP_nonce.html
[...]
>+<p id="display"></p>
>+<div id="content" style="display: none">
>+</div>
>+<iframe style="width:100%;" id='cspframe'></iframe>

Technically, I think the <div id="content"> is there for you to put your content in. So, your iframe should be in there, not after it.  (If you have to, you can remove the "display: none")

>+function checkStyles () {
>+  var cspframe = document.getElementById('cspframe');
>+  var getElementColorById = function (id) {
>+    return window.getComputedStyle(cspframe.contentDocument.getElementById(id), null).color;
>+  };
>+  // Inline style tries to change an element's color to green. If blocked, the
>+  // element's color will be the default black.
>+  var green = "rgb(0, 128, 0)";
>+  var black = "rgb(0, 0, 0)";
>+
>+  ok(getElementColorById('inline-style-correct-nonce') === green, "Inline style with correct nonce allowed");
>+  ok(getElementColorById('inline-style-incorrect-nonce') === black, "Inline style with incorrect nonce blocked");
>+  ok(getElementColorById('inline-style-no-nonce') === black, "Inline style with no nonce blocked");

In a mochitest, when you're comparing two values for equality, don't use "ok(foo === expected, msg)" -- instead, use "is(foo, expected, msg)". That way, if the comparison fails, the mochitest harness will report what the actual (non-matching) value was, rather than just telling you it wasn't equal.

Comment 30

[Daniel Holbert [:dholbert]]

Also: your test needs to include a <link> element and an inline style elements (<div style="color: teal">), verifying that those respond correctly to nonces. (whatever the correct behavior is, per questions in comment 27 & 28)

Comment 31

[Daniel Holbert [:dholbert]]

(sorry, s/an inline style elements/an element with a style attribute/)

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 780603 [details] [diff] [review]
Patch 9

>diff --git a/layout/style/nsStyleUtil.cpp b/layout/style/nsStyleUtil.cpp
>@@ -434,42 +436,63 @@ nsStyleUtil::CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
>+    if (reportInlineViolation || reportNonceViolation) {

From my reading, if we get inlineOK=false and reportInlineViolation=true (i.e. inline scripts are blocked by default), but then we end up with a correct nonce, we'll *still report* an inline violation (even though we have a valid nonce and allow the style), since reportInlineViolation is true.

Is that what's supposed to happen?  (or, maybe I'm misunderstanding the logic flow?)

>-    if (!inlineOK) {
>+    if (!(inlineOK || nonceOK)) {
>         // The inline style should be blocked.
>         return false;
>     }

Maybe it's just me, but I find the extra-parens confusing here. I marginally prefer:
   if (!inlineOK && !nonceOK)) {
(which I read as "if all of our checks said this is not OK, [then bail out]" basically)

Also: after that early-return-clause, we can probably assert something useful about reportNonceViolation and reportInlineViolation, right? (both as a sanity-check and for documentation purposes)

Intuitively, I'd expect those bools both to be false at that point (i.e. if we reported a violation, we definitely shouldn't end up honoring the script...), but my intuition may or may not be right depending on your response about "Is that what's supposed to happen" above.

(All of these comments ^^ apply equally well to the analogous code in nsScriptLoader, too.)

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 780603 [details] [diff] [review]
Patch 9

r=me on the scriptloader changes.

Comment 34

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> Comment on attachment 780603 [details] [diff] [review]
> Patch 9
> 
> Not done reviewing yet, but here are some drive-by nits on various parts of
> the patch:
> 
> >Bug 855326 - CSP : script-nonce directive (experimental)
> 
> Looks like the commit message is a bit stale -- this is called the
> "script-src directive" and "style-src directive" now, right? (not
> "script-nonce" anymore) (I'm going off of the spec linked from this bug's
> URL field, which never mentions "script-nonce" but does use those other
> terms.)
> 

Good catch, that has changed over the lifetime of the patch. I will change the name to "Bug 855326 - CSP 1.1 nonce-source directive for inline scripts and styles".

> >+++ b/content/base/public/nsIContentSecurityPolicy.idl
> >   /**
> >+   * Whether this policy recognizes a given nonce
> >+   * @return Whether or not the nonce is allowed (block the call if false)
> >+   */
> >+   boolean getAllowsNonce(in AString aNonce,
> >+                          in unsigned long aContentType,
> >+                          out boolean shouldReportViolation);
> >+
> 
> Could you document the parameters here, too, like the contextual code does?
> (with @param)
> 

Done.

> >diff --git a/layout/style/nsStyleUtil.h b/layout/style/nsStyleUtil.h
> >index 99bad2d..753c765 100644
> >--- a/layout/style/nsStyleUtil.h
> >+++ b/layout/style/nsStyleUtil.h
> >@@ -97,17 +97,18 @@ public:
> >    *  inline styles ? Returns false if application of the style should
> >    *  be blocked.
> >    *
> >    *  Note that the principal passed in here needs to be the principal
> >    *  of the document, not of the style sheet. The document's principal
> >    *  is where any Content Security Policy that should be used to
> >    *  block or allow inline styles will be located.
> >    */
> >-  static bool CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
> >+  static bool CSPAllowsInlineStyle(nsIContent* aContent,
> >+                                   nsIPrincipal* aPrincipal,
> 
> Could you mention what this new arg 'aContent' is for, in the header-comment
> here?  (Ideally it'd be great if we documented the parameters declaratively
> with @param or similar; if you feel like doing that for the existing params,
> that would be awesome, but I won't mandate it since there are already a
> bunch of parameters and they're just documented as prose instead of with
> @param.)
> 

I updated it to use the @param style.

> >+++ b/modules/libpref/src/init/all.js
> [...]
> >@@ -4315,8 +4316,9 @@ pref("captivedetect.pollingTime", 3000);
> > pref("captivedetect.maxRetryCount", 5);
> > #endif
> > 
> > #ifdef RELEASE_BUILD
> > pref("dom.forms.inputmode", false);
> > #else
> > pref("dom.forms.inputmode", true);
> > #endif
> >+
> 
> Looks like you accidentally added a blank line at the end of all.js.
> Probably want to revert that.

Done.

Comment 35

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #28)
> I have similar questions about the change to nsStyledElement.cpp -- should
> we really be checking for nonces on any random element that happens to have
> a style attribute?  (The spec mentions nonces for the style *element*, but
> it doesn't say anything about them for the style *attribute*.)
> 

No, we should not. Fixed.

> And following up slightly on comment 27, RE nsStyleLinkElement -- it looks
> like I was only partly right -- that's not just <link>, after all -- it's
> the superclass for HTMLStyleElement, SVGStyleElement, and HTMLLinkElement.
> If I'm understanding the spec correctly, I think we just want this for
> <style>, but *not* for <link>, right?
> 

That is correct. If you read nsStyleLinkElement::DoUpdateStyleSheet, you will see that we only call CSPAllowsInlineStyle if we are called on an HTMLStyleElement or SVGStyleElement. This happens implicitly, due to the early return on the condition (!uri && !isInline) currently on line 344 and also by the subsequent check for isInline currently on line 366. isInline is set by GetStyleSheetURL, which returns a different value for HTMLStyleElement and SVGStyleElement as opposed to StyleLinkElement (implicitly performing the overloaded check you suggest below). 

> (We could perhaps achieve that by adding a protected method
> DoesSupportsNonces() on nsStyleLinkElement, which returns false -- and then
> HTMLStyleElement and SVGStyleElement would get an override that returns
> true. Or something like that.)

I prefer explicit over implicit, but in this case I do not think it is necessary for the reasons given above. Please reply if you disagree.

Comment 36

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #29)
> Comment on attachment 780603 [details] [diff] [review]
> Patch 9
> 
> Test nits:
> 
> >+++ b/content/base/test/file_CSP_nonce.html^headers^
> >@@ -0,0 +1,2 @@
> >+Content-Security-Policy: script-src 'nonce-correctnonce'; style-src 'nonce-correctnonce';
> 
> So right now, your test uses the same nonce for script-src and style-src.
> 
> It's probably worth making those nonces *different*, and testing that we
> don't mistakenly accept the script nonce as a style nonce & vice versa.
> 

Done.

> >diff --git a/content/base/test/test_CSP_nonce.html b/content/base/test/test_CSP_nonce.html
> [...]
> >+<p id="display"></p>
> >+<div id="content" style="display: none">
> >+</div>
> >+<iframe style="width:100%;" id='cspframe'></iframe>
> 
> Technically, I think the <div id="content"> is there for you to put your
> content in. So, your iframe should be in there, not after it.  (If you have
> to, you can remove the "display: none")
> 

Ok. I removed the display:none because I like *seeing* the style tests fail, which is also a widely used convention in the CSP tests.

> >+function checkStyles () {
> >+  var cspframe = document.getElementById('cspframe');
> >+  var getElementColorById = function (id) {
> >+    return window.getComputedStyle(cspframe.contentDocument.getElementById(id), null).color;
> >+  };
> >+  // Inline style tries to change an element's color to green. If blocked, the
> >+  // element's color will be the default black.
> >+  var green = "rgb(0, 128, 0)";
> >+  var black = "rgb(0, 0, 0)";
> >+
> >+  ok(getElementColorById('inline-style-correct-nonce') === green, "Inline style with correct nonce allowed");
> >+  ok(getElementColorById('inline-style-incorrect-nonce') === black, "Inline style with incorrect nonce blocked");
> >+  ok(getElementColorById('inline-style-no-nonce') === black, "Inline style with no nonce blocked");
> 
> In a mochitest, when you're comparing two values for equality, don't use
> "ok(foo === expected, msg)" -- instead, use "is(foo, expected, msg)". That
> way, if the comparison fails, the mochitest harness will report what the
> actual (non-matching) value was, rather than just telling you it wasn't
> equal.

Done.

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to Garrett Robinson [:grobinson] from comment #36)
> Ok. I removed the display:none because I like *seeing* the style tests fail,
> which is also a widely used convention in the CSP tests.

Gotcha. Are you intending to add the "display:none" back before landing, though?  (I'm not clear on that from your comment.)

(The "It's easier to debug stuff when I can see the failing content" argument is valid *when you're debugging stuff*, and it applies to most mochitests.  But on our automated test boxes, it's better to run them in the display:none div, if possible, to save resources. You can always make a temporary local tweak, if you need to, but the checked-in test should probably have the display:none, as long as it doesn't break what you're testing.)

Comment 38

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #37)
> (In reply to Garrett Robinson [:grobinson] from comment #36)
> > Ok. I removed the display:none because I like *seeing* the style tests fail,
> > which is also a widely used convention in the CSP tests.
> 
> Gotcha. Are you intending to add the "display:none" back before landing,
> though?  (I'm not clear on that from your comment.)
> 
> (The "It's easier to debug stuff when I can see the failing content"
> argument is valid *when you're debugging stuff*, and it applies to most
> mochitests.  But on our automated test boxes, it's better to run them in the
> display:none div, if possible, to save resources. You can always make a
> temporary local tweak, if you need to, but the checked-in test should
> probably have the display:none, as long as it doesn't break what you're
> testing.)

display:none breaks what I'm testing because I need to see if inline styles were applied via window.getComputedStyle, and the no style is computed if it is inside a display:none element. I can get the test to pass by setting the div to visibility:hidden, would that be preferable? How much of a difference in resource consumption does this cause?

Generally, since we're testing whether CSS rules were applied or not, it is helpful to see those elements.

Comment 39

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #32)
> Comment on attachment 780603 [details] [diff] [review]
> Patch 9
> 
> >diff --git a/layout/style/nsStyleUtil.cpp b/layout/style/nsStyleUtil.cpp
> >@@ -434,42 +436,63 @@ nsStyleUtil::CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
> >+    if (reportInlineViolation || reportNonceViolation) {
> 
> From my reading, if we get inlineOK=false and reportInlineViolation=true
> (i.e. inline scripts are blocked by default), but then we end up with a
> correct nonce, we'll *still report* an inline violation (even though we have
> a valid nonce and allow the style), since reportInlineViolation is true.
> 
> Is that what's supposed to happen?  (or, maybe I'm misunderstanding the
> logic flow?)
> 

That is correct. I've cleaned up the logic to make it correct and also easier to understand.

A nonce violation is reported when an element has a nonce= attribute that specifies a nonce not provided in the policy. An inline violation is reported when an inline element exists in a policy that doesn't whitelist it.

* If !reportInlineViolation && !reportNonceViolation, we don't report any violations.
* If reportInlineViolation && !reportNonceViolation, we report the inline violation.
* If !reportInlineViolation && reportNonceViolation, we report the nonce violation.
* If reportInlineViolation && reportNonceViolation, we only report the nonce violation and not the inline violation. This case occurs if inline scripts are blocked (default) and an incorrect nonce is given, so in this case it is likely that the mistake was made with the nonce and we want to focus developers on that.

Note that at the moment, even if 'unsafe-inline' is given, the inline resource will be blocked if it provides an incorrect nonce. i.e, providing a nonce-source overrides unsafe-inline if they are both specified. This behavior mirrors the consensus in recent discussions on webappsec (http://lists.w3.org/Archives/Public/public-webappsec/2013Jul/0019.html) and the draft proposal for hash-source (http://lists.w3.org/Archives/Public/public-webappsec/2013Jul/0027.html).

> >-    if (!inlineOK) {
> >+    if (!(inlineOK || nonceOK)) {
> >         // The inline style should be blocked.
> >         return false;
> >     }
> 
> Maybe it's just me, but I find the extra-parens confusing here. I marginally
> prefer:
>    if (!inlineOK && !nonceOK)) {
> (which I read as "if all of our checks said this is not OK, [then bail out]"
> basically)
> 

Yes, that's a little easier to read. Fixed.

> Also: after that early-return-clause, we can probably assert something
> useful about reportNonceViolation and reportInlineViolation, right? (both as
> a sanity-check and for documentation purposes)
> 
> Intuitively, I'd expect those bools both to be false at that point (i.e. if
> we reported a violation, we definitely shouldn't end up honoring the
> script...), but my intuition may or may not be right depending on your
> response about "Is that what's supposed to happen" above.
> 

Sure. I added an assertion to make sure we report some violation if we're blocking the resource.

> (All of these comments ^^ apply equally well to the analogous code in
> nsScriptLoader, too.)

Changes carried to nsScriptLoader code.

Comment 40

[Garrett Robinson [:grobinson]]

After reviewing, correct violation reporting w/ nonce is a little different than above (actually, simpler). We allow nonces to override 'unsafe-inline' as mentioned in the previous comment. If an inline element has a nonce, we allow it if the nonce is good and block it if it is not (even if unsafe-inline is allowed). In either case we report a nonce violation. Since we allow nonces to completely override the default inline element handling, we can simplify the code by passing the same flags (inlineOK and reportViolation) to both getAllows functions. The only trick is we need to be able to distinguish which set of behavior caused a violation in order to report a more specific violation for inline or nonces.

I have also added some code to the tests to check for errors in violation reporting. It is a bit crude (counts the number of violations of each type we expect to see), but works well enough.

Comment 41

[Garrett Robinson [:grobinson]]

Attachment: patch10

This update implements all of dholbert's suggestions from the past reviews.

It has a few other updates as well:
1. It implements the "nonce-source overrides unsafe-inline" behavior that webappsec is converging to (see links in Comment 39)
2. It extends the allowable characters in a nonce source to encompass Base64 (http://lists.w3.org/Archives/Public/public-webappsec/2013Aug/0019.html)

Do note that this implementation is *not* 1.1 draft spec-compliant in the sense that it only allows the use of nonces to whitelist *inline* scripts and styles, and not external scripts and styles. While the draft spec currently allows nonces to whitelist external scripts (see 4.10.1, Usage of script-src), discussion on webappsec is also trending towards limiting this feature to inline scripts only. If we also implement path matching from CSP 1.1 (Section 3.2.2.2.1), then there will be no use case at all for nonce-source on external scripts. I am repeating this here because there was some confusion in recent comments regarding the spec and this patch.

For future reference, Comment 21 has more information and also refers to an earlier patch which has the code to allow nonces to whitelist external resources, if we later decide to reimplement that behavior.

Carrying over r+ on CSP code from sstamm and r+ from mrbkap on changes to nsScriptLoader.

Comment 42

[Daniel Holbert [:dholbert]]

(In reply to Garrett Robinson [:grobinson] from comment #38)
> display:none breaks what I'm testing because I need to see if inline styles
> were applied via window.getComputedStyle, and the no style is computed if it
> is inside a display:none element

OK, that's fine then; I wasn't sure if it'd break things or not.

> I can get the test to pass by setting the
> div to visibility:hidden, would that be preferable?

No, no need; sorry about that. display:none would save us from building the frame tree, but visibility:hidden doesn't save us much, IIUC.

> How much of a difference
> in resource consumption does this cause?

Not much. (I was just suggesting best-practices and keeping with the default mochitest style, but if the test breaks without display:none, then it's fine to remove it of course.)

Comment 43

[Daniel Holbert [:dholbert]]

(In reply to Garrett Robinson [:grobinson] from comment #35)
> > If I'm understanding the spec correctly, I think we just want this for
> > <style>, but *not* for <link>, right?
> 
> That is correct. If you read nsStyleLinkElement::DoUpdateStyleSheet, you
> will see that we only call CSPAllowsInlineStyle if we are called on an
> HTMLStyleElement or SVGStyleElement. This happens implicitly, due to the
> [...] subsequent check for isInline currently on line 366.

Ah, that makes sense -- thanks for explaining that.

IMHO, it'd be worth adding an explicit assertion, somewhere inside that "isInline" clause (maybe just before the CSPAllowsInlineStyle check?), like this:
  MOZ_ASSERT(Tag() != nsGkAtoms::link,
             "<link> is not 'inline', and needs different CSP checks");
This would be good both for documentation and for sanity-checking purposes.

Comment 44

[Daniel Holbert [:dholbert]]

Comment on attachment 786684 [details] [diff] [review]
patch10

>@@ -614,29 +629,29 @@ CSPRep.fromStringSpecCompliant = function(aStr, self, docRequest, csp) {
>           uri.host;
> 
>           // Verify that each report URI is in the same etld + 1 and that the
>           // scheme and port match "self" if "self" is defined, and just that
>           // it's valid otherwise.
>           if (self) {
>             if (gETLDService.getBaseDomain(uri) !==
>                 gETLDService.getBaseDomain(selfUri)) {
>-              cspWarn(aCSPR, 
>+              cspWarn(aCSPR,
>                       CSPLocalizer.getFormatStr("notETLDPlus1",
>                                             [gETLDService.getBaseDomain(uri)]));
[plus several more of these in a row]

It took me a minute to see what you were changing here.  It looks like this patch-chunk is several consecutive whitespace fixes (removing end-of-line whitespace).

That's great for code-cleanup, but for future reference: unless the whitespace-cleanup is directly next to some functional code-changes, it's best to do whitespace cleanup in a separate patch (and maybe not even as part of a bug), so that the cleanup doesn't add to the size & apparent-complexity of the main patch.

(This might also have been your editor being "helpful" and doing auto-cleanup on a file that you touched -- if that's the case, you might want to disable that feature, so your editor doesn't bulk up all of your future patches with unrelated whitespace tweaks.)

Anyway - these are fine to leave in at this point - just pointing them out, for future reference.

>+++ b/content/base/src/nsScriptLoader.cpp
>@@ -609,44 +609,64 @@ nsScriptLoader::ProcessScriptElement(nsIScriptElement *aElement)


>+    bool foundNonce = false;
>+    nsAutoString nonce;
>+    scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
>+    if (!nonce.IsEmpty()) {
[...]
>+      foundNonce = true;

Technically, you should be checking whether GetAttr succeeds, to see whether there was a nonce attribute -- you shouldn't be checking whether its outparam is non-empty.  Most of the time it won't make a difference, but it matters for a case like:
  <script nonce="">
For that case, your current patch would leave foundNonce at false -- but I believe you should treat that as "there was a nonce attribute, and it was wrong", and take the VIOLATION_TYPE_NONCE_SCRIPT code-path.

So: I'd suggest just replacing the code quoted above with the following:
  nsAutoString nonce;
  bool foundNonce = scriptContent->GetAttr(kNameSpaceID_None,
                                           nsGkAtoms::nonce, nonce);
  if (foundNonce) {
   [...]

That should take care of it.

>+    if (!nonce.IsEmpty()) {
>+      // We need this flag to distinguish between inline script violatoins and

typo: s/violatoins/violations/

(though you might want to move/rework this comment slightly, due to my above suggestion for where to set 'foundNonce')

>+++ b/content/base/test/test_CSP_nonce.html
>@@ -0,0 +1,120 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <title>Test for Content Security Policy Nonce Source in Script Directive</title>

nit: This test-title just mentions scripts, but it should mention styles as well. Also, I'm not sure what "Script Directive" means here. (and while we're at it, super-nit: the spec technically calls this "nonce-source" (hyphenated) -- it never says "Nonce Source".)

Anyway -- maybe replace with something like the following, cribbed straight from your commit message:
  <title>Test for nonce-source directive for inline scripts and styles</title>

Also, speaking of tests -- given that this is a security feature that web authors will be depending on, I think it's particularly important to make sure this has good test coverage.  It looks like we still need test-coverage for the following things:
 a) nonce on <script src="..."> tags
    (I think (?) you're just testing inline <script> tags, right now)
    (The spec says if the nonce doesn't match on <script src>, "the user agent MUST act as if it had received an empty HTTP 400 response and report a violation")

 b) nonce being ignored on other style sources:
     i.   <animate attributeName="fill" nonce="...">
     ii.  <div style="foo" nonce="...">
     iii. <link href="..." nonce="...">
     iv.  <style> in an SVG document

We also don't have any test-coverage for nonce validation, beyond testing that "correctscriptnonce" and "correctstylenonce" are both valid.  It'd be worth also testing the following:
 e) empty-string shouldn't be a valid nonce (i.e. "nonce-" in the headers, nonce="" on a style element)
 f) special characters (beyond "+" and "/" should invalidate the nonce (e.g. "nonce-12%^"), or a random UTF-8 character, etc
 g) a nonce that exercises much (all?) of the range of valid characters *should* be allowed (both lower and uppercase characters, digits, +, and /)
 h) a nonce that starts with "NONCE-" (uppercase) is rejected

Since this is preffed off by default, I'd be fine with this test-writing being tracked in followup bugs, as long as the followups (particularly for things being blocked) are resolved before this gets preffed on. Please file followups for those (marked as depending on this bug) before closing this out, though, so that we don't forget about them.

>+// set up and go
>+window.examiner = new examiner();
>+SimpleTest.waitForExplicitFinish();
>+
>+SpecialPowers.pushPrefEnv(
>+  {'set':[["security.csp.speccompliant", true],
>+          ["security.csp.experimentalEnabled", true]]},

If you haven't already -- it'd be worth doing a local sanity-check to be sure the test fails in the way you'd expect, if you delete that second line up to the "]},". (leaving experimentalEnabled at its default value, which should be false)

>+    // We need this flag to distinguish between inline style violations and
>+    // nonce violations. Otherwise, passing the same variables to
>+    // GetAllowsNonce simplifies the logic.
>+    bool foundNonce = false;
>+    nsAutoString nonce;
>+    // We can only find a nonce if aContent is provided
>+    if (aContent) {
>+      aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
>+      if (!nonce.IsEmpty()) {
>+        rv = csp->GetAllowsNonce(nonce, nsIContentPolicy::TYPE_STYLESHEET,
>+                                 &reportViolation, &inlineOK);

Similarly to above, do something like:
 bool foundNonce = !!aContent &&
   aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
 if (foundNonce) {
   rv = csp->[stuff]

(Also: it looks like the current patch *never* sets foundNonce to true in this code.  That looks like a bug, though I think it only breaks the type of violation we log, fortunately. Any chance we can test for that?)

>+++ b/layout/style/nsStyleUtil.h
>+   *  @param aContent
>+   *      The inline style HTML DOM element. Included to check a nonce
>+   *      attribute if one is provided.

The term "inline style HTML DOM element" is a bit wordy and confusing. Also, the "HTML" adjective probably shouldn't be there, right? Presumably this applies to XHTML and SVG as well?

Maybe replace that text with something like:
  "The <style> element that the caller wants to know whether to honor."

Also: it'd be worth explicitly mentioning that this can be null -- e.g.:
  "Allowed to be null, if this is for something other than a <style> element (in which case nonces won't be checked)." or something like that.

>+   *  @param aStyleText
>+   *      Contents of the inline style element (for reporting violations)
>+   *  @param aRv
>+   *      Return error code in case of failure
>    */
>-  static bool CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
>+  static bool CSPAllowsInlineStyle(nsIContent* aContent,

Thanks for adding all of this @param documentation! Please also add a line for "@return" here, too, for completeness/clarity.

Comment 45

[Daniel Holbert [:dholbert]]

(In reply to Garrett Robinson [:grobinson] from comment #39)
> > From my reading, if we get inlineOK=false and reportInlineViolation=true
> > (i.e. inline scripts are blocked by default), but then we end up with a
> > correct nonce, we'll *still report* an inline violation
> 
> That is correct. I've cleaned up the logic to make it correct and also
> easier to understand.

Thanks -- I think it makes sense now, with one caveat: perhaps "inlineOK" should now be renamed to "allowScript"?  (and "allowStyle" in the style case)

The word "inlineOK" sounds very similar to "allowsInlineScript", which (before this patch) it used to directly correspond to.  But it now represents something slightly more complex than that, and adding that complexity while keeping the old name is a bit confusing.

Comment 46

[Daniel Holbert [:dholbert]]

(or even just "shouldAllow", if you like. The point is, it's a more general "are we allowing this" bool now, and it's not inline-specific anymore.)

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 786684 [details] [diff] [review]
patch10

r=me with the notes in comment 43, comment 44, & comment 45 addressed or responded to.

Comment 48

[Garrett Robinson [:grobinson]]

Attachment: 11.patch

This is a very-rewritten script-nonce patch, which is why I am re-flagging all of you for review. This patch now supports and tests nonces on inline *and* external scripts and styles, per discussion in the WG.

dholbert, I addressed all of your comments *except* the numerous additional tests you requested in comment 44. I may add those to a final revision of the patch for this bug, or track them in a follow-up.

mrbkap, please review the changes to:

* nsScriptLoader.cpp
* caps/src/nsScriptSecurityManager.cpp
* dom/src/jsurl/nsJSProtocolHandler.cpp
* content/events/src/nsEventListenerManager.cpp

(only nsScriptLoader.cpp has substantial changes; the others just add default "empty" values to a CSP function call)

dholbert, please review the changes to:

* nsStyleLinkElement.cpp
* nsStyledElement.cpp
* nsSMILCSSValueType.cpp
* nsStyleUtil.cpp
* nsStyleUtil.h

Try: https://tbpl.mozilla.org/?tree=Try&rev=a56e0e99a83a

Comment 49

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 823662 [details] [diff] [review]
11.patch

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

What happens if a resource requested is allowed by nonce but not source and vice-versa?  Is the idea that if either a nonce or scheme/host/port-expr allows it, the load happens?

More coming, but I'd like to hear your thoughts on my questions.

::: content/base/src/CSPUtils.jsm
@@ +816,3 @@
>     *        one of the SRC_DIRECTIVES defined above
> +   * @param aContext
> +   *        Context of the resource being requested

Please elaborate: what types or objects are expected here?  Is it required?  What uses this?

@@ +820,5 @@
>     *        true if the policy permits the URI in given context.
>     */
>    permits:
> +  function csp_permits(aURI, aDirective, aContext) {
> +    //if (!aURI) return false;

Shouldn't be commented out.

@@ +1101,5 @@
>     * @returns
>     *        true if the URI matches a source in this source list.
>     */
>    permits:
> +  function cspsd_permits(aURI, aContext) {

Please document aContext in the doc comment for permits()

@@ +1372,5 @@
> +  if (R_NONCESRC.test(aStr)) {
> +    var nonceSrcMatch = R_NONCESRC.exec(aStr);
> +    sObj._nonce = nonceSrcMatch[1];
> +    return sObj;
> +  }

Would it make sense to only parse this if the experimental bits are enabled?  Policies are parsed once and enforced many times, so it would be more efficient to act as if nonce parsing was not enabled when experimental is off.  (The enforcement will never have _nonce set, so it'll be faster than checking for both that and the pref.)

::: content/base/src/contentSecurityPolicy.js
@@ +202,5 @@
> +    }
> +    let ct = ContentSecurityPolicy._MAPPINGS[aContentType];
> +
> +    // allow it to execute?
> +    let policyAllowsNonce = [ policy.permits(null, ct, aNonce) for (policy of this._policies) ];

nice, but sending in null for the resource concerns me a bit.  Is this less risky than making a permitsNonce() function for CSPRep?

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 823662 [details] [diff] [review]
11.patch

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

I have a bunch of nits and one real question that I'd like answered before I'll stamp.

::: caps/src/nsScriptSecurityManager.cpp
@@ +489,5 @@
>          csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_EVAL,
>                                   fileName,
>                                   scriptSample,
> +                                 lineNum,
> +                                 NS_LITERAL_STRING(""));

Please use EmptyString().

::: content/base/src/nsScriptLoader.cpp
@@ +626,5 @@
>    if (csp) {
> +    PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("New ScriptLoader ****with CSP****"));
> +
> +    bool allowInlineScript = true;
> +    bool reportViolation = false;

Uber-nit: please declare these in the order of use (reportViolation and then allowInlineScript).

@@ +634,5 @@
> +    nsAutoString nonce;
> +    bool foundNonce = scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
> +    if (foundNonce) {
> +      rv = csp->GetAllowsNonce(nonce, nsIContentPolicy::TYPE_SCRIPT,
> +          &reportViolation, &allowInlineScript);

You don't seem to ever look at reportViolation or allowInlineScript if there's a nonce on the element. Is that intended? Either way, comments describing the control flow here would be really helpful.

@@ +649,5 @@
>  
>        // cap the length of the script sample at 40 chars
>        if (scriptText.Length() > 40) {
>          scriptText.Truncate(40);
>          scriptText.Append(NS_LITERAL_STRING("..."));

While you're here: scriptText.AppendLiteral("...");

@@ +653,5 @@
>          scriptText.Append(NS_LITERAL_STRING("..."));
>        }
>  
> +      if (foundNonce) {
> +        csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_NONCE_SCRIPT,

These two LogViolationDetails calls are similar enough that I wonder if it might be better to combine them and simply pass the correct arguments.

@@ +659,5 @@
> +            aElement->GetScriptLineNumber(), nonce);
> +      } else {
> +        csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_SCRIPT,
> +            NS_ConvertUTF8toUTF16(asciiSpec), scriptText,
> +            aElement->GetScriptLineNumber(), NS_LITERAL_STRING(""));

EmptyString().

::: content/events/src/nsEventListenerManager.cpp
@@ +723,5 @@
>          csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_SCRIPT,
>                                   NS_ConvertUTF8toUTF16(asciiSpec),
>                                   scriptSample,
> +                                 0,
> +                                 NS_LITERAL_STRING(""));

EmptyString().

::: dom/src/jsurl/nsJSProtocolHandler.cpp
@@ +183,5 @@
>              csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_SCRIPT,
>                                       NS_ConvertUTF8toUTF16(asciiSpec),
>                                       NS_ConvertUTF8toUTF16(mURL),
> +                                     0,
> +                                     NS_LITERAL_STRING(""));

EmptyString().

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 823662 [details] [diff] [review]
11.patch

>+++ b/content/base/public/nsIContentSecurityPolicy.idl
>+   * @param aNonce
>+   *     (optional) If this is a nonce violation, include the nonce should we
>+   *     can recheck to determine which policies were violated and send the
>+   *     appropriate reports.

drive-by nit on this part: s/should we/so we/ (I think?)

>+++ b/content/base/src/nsStyleLinkElement.cpp
>@@ -355,17 +355,20 @@ nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument *aOldDocument,
>   if (isInline) {
>     nsAutoString text;
>     nsContentUtils::GetNodeTextContent(thisContent, false, text);
> 
>-    if (!nsStyleUtil::CSPAllowsInlineStyle(thisContent->NodePrincipal(),
>+    MOZ_ASSERT(Tag() != nsGkAtoms::link,
>+             "<link> is not 'inline', and needs different CSP checks");

nit: that last line needs 2 more spaces of indentation.

>diff --git a/layout/style/nsStyleUtil.cpp b/layout/style/nsStyleUtil.cpp
>@@ -458,43 +460,62 @@ nsStyleUtil::CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
>+    bool allowInlineStyle = true;
>     bool reportViolation;
>-    rv = csp->GetAllowsInlineStyle(&reportViolation, &inlineOK);
>+    rv = csp->GetAllowsInlineStyle(&reportViolation, &allowInlineStyle);
>     if (NS_FAILED(rv)) {
>       if (aRv)
>         *aRv = rv;
>       return false;
>     }
> 
>+    nsAutoString nonce;
>+    // We can only find a nonce if aContent is provided
>+    bool foundNonce = !!aContent &&
>+      aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
>+    if (foundNonce) {
>+      rv = csp->GetAllowsNonce(nonce, nsIContentPolicy::TYPE_STYLESHEET,
>+                               &reportViolation, &allowInlineStyle);

So in the "foundNonce" case, it looks like we stomp on the outparams of GetAllowsInlineStyle() (namely, allowInlineStyle and reportViolation) without ever checking them.

That's probably undesirable. (and might allow for mischief?)  Perhaps we should only be checking for a nonce *after* we've confirmed that inline styles are allowed?

Alternately, if the current code is correct (i.e. if GetAllowsNonce will only return true in cases where GetAllowsInlineStyle would also return true), then maybe we should restructure this slightly to avoid calling GetAllowsInlineStyle entirely when we find a nonce?  (This would avoid the stomping-on-outparams problem and make things a bit clearer.)

>     if (reportViolation) {
>       // Inline styles are not allowed by CSP, so report the violation

This comment needs to be made more specific, due to your code changes here.
e.g.
 /Inline styles are/This inline style is/

Also: Probably worth asserting that allowInlineStyle is false here, right? (to be sure the comment isn't lying)

>+      if (foundNonce) {
>+        csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_NONCE_STYLE,
>+            NS_ConvertUTF8toUTF16(asciiSpec), aStyleText, aLineNumber, nonce);
>+      } else {
>+        csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_STYLE,
>+            NS_ConvertUTF8toUTF16(asciiSpec), aStyleText, aLineNumber, NS_LITERAL_STRING(""));
>+      }

I agree with Blake's comment about the the equivalent script code -- it'd be be great to collapse this into a single function-call.

This is made easier by the fact that you don't actually need to bother with the  NS_LITERAL_STRING("") here (aka EmptyString()).  You can just unconditionally pass |nonce|, because if foundNonce is false, then |nonce| will *be* the empty string.

Comment 52

[Garrett Robinson [:grobinson]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #49)
> Comment on attachment 823662 [details] [diff] [review]
> 11.patch
> 
> Review of attachment 823662 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What happens if a resource requested is allowed by nonce but not source and
> vice-versa?  Is the idea that if either a nonce or scheme/host/port-expr
> allows it, the load happens?

Yes. nonce is a source expression like any other, and only one match is needed to whitelist a resource.

> More coming, but I'd like to hear your thoughts on my questions.
> 
> ::: content/base/src/CSPUtils.jsm
> @@ +816,3 @@
> >     *        one of the SRC_DIRECTIVES defined above
> > +   * @param aContext
> > +   *        Context of the resource being requested
> 
> Please elaborate: what types or objects are expected here?  Is it required? 
> What uses this?

To answer your question, either an nsIDOMHTMLElement (for external elements, the element in the HTML document that is requesting a resource load) or a string (for inline elements, we extract the nonce in Gecko and pass it as a string to getAllowsNonce). I will update this comment.
 
> @@ +820,5 @@
> >     *        true if the policy permits the URI in given context.
> >     */
> >    permits:
> > +  function csp_permits(aURI, aDirective, aContext) {
> > +    //if (!aURI) return false;
> 
> Shouldn't be commented out.

Good catch. This needs to handle the case for inline resources in getAllowsNonce where aURI is *intentionally* null. I will add a sanity check for this case.

> @@ +1101,5 @@
> >     * @returns
> >     *        true if the URI matches a source in this source list.
> >     */
> >    permits:
> > +  function cspsd_permits(aURI, aContext) {
> 
> Please document aContext in the doc comment for permits()

Done.

> @@ +1372,5 @@
> > +  if (R_NONCESRC.test(aStr)) {
> > +    var nonceSrcMatch = R_NONCESRC.exec(aStr);
> > +    sObj._nonce = nonceSrcMatch[1];
> > +    return sObj;
> > +  }
> 
> Would it make sense to only parse this if the experimental bits are enabled?
> Policies are parsed once and enforced many times, so it would be more
> efficient to act as if nonce parsing was not enabled when experimental is
> off.  (The enforcement will never have _nonce set, so it'll be faster than
> checking for both that and the pref.)

That makes sense. Would need to make sure that if not experimentalEnabled, nonce sources are correctly ignored by the parser and not added to the source list (they will still be considered matches in CSPSourceList.fromString, because they are included in R_SOURCEEXP).

> ::: content/base/src/contentSecurityPolicy.js
> @@ +202,5 @@
> > +    }
> > +    let ct = ContentSecurityPolicy._MAPPINGS[aContentType];
> > +
> > +    // allow it to execute?
> > +    let policyAllowsNonce = [ policy.permits(null, ct, aNonce) for (policy of this._policies) ];
> 
> nice, but sending in null for the resource concerns me a bit.  Is this less
> risky than making a permitsNonce() function for CSPRep?

This is the permitsNonce function (getAllowsNonce), which is used to check inline resources. I decided to extend the permits functions so I can use the same logic to check nonces on inline *and* external resources. The other way to do it would be to write a checkNonce function which we can additionally call in shouldLoad, and or the result of that into the result of shouldLoad. I considered this approach to be cleaner and more DRY. I think passing null for aURI is ok as long as we do careful parameter checking in the series of permits functions.

Comment 53

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] from comment #51)
> Comment on attachment 823662 [details] [diff] [review]
> 11.patch
> 
> >+++ b/content/base/public/nsIContentSecurityPolicy.idl
> >+   * @param aNonce
> >+   *     (optional) If this is a nonce violation, include the nonce should we
> >+   *     can recheck to determine which policies were violated and send the
> >+   *     appropriate reports.
> 
> drive-by nit on this part: s/should we/so we/ (I think?)

Fixed.

> >+++ b/content/base/src/nsStyleLinkElement.cpp
> >@@ -355,17 +355,20 @@ nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument *aOldDocument,
> >   if (isInline) {
> >     nsAutoString text;
> >     nsContentUtils::GetNodeTextContent(thisContent, false, text);
> > 
> >-    if (!nsStyleUtil::CSPAllowsInlineStyle(thisContent->NodePrincipal(),
> >+    MOZ_ASSERT(Tag() != nsGkAtoms::link,
> >+             "<link> is not 'inline', and needs different CSP checks");
> 
> nit: that last line needs 2 more spaces of indentation.

Fixed.

> >diff --git a/layout/style/nsStyleUtil.cpp b/layout/style/nsStyleUtil.cpp
> >@@ -458,43 +460,62 @@ nsStyleUtil::CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
> >+    bool allowInlineStyle = true;
> >     bool reportViolation;
> >-    rv = csp->GetAllowsInlineStyle(&reportViolation, &inlineOK);
> >+    rv = csp->GetAllowsInlineStyle(&reportViolation, &allowInlineStyle);
> >     if (NS_FAILED(rv)) {
> >       if (aRv)
> >         *aRv = rv;
> >       return false;
> >     }
> > 
> >+    nsAutoString nonce;
> >+    // We can only find a nonce if aContent is provided
> >+    bool foundNonce = !!aContent &&
> >+      aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
> >+    if (foundNonce) {
> >+      rv = csp->GetAllowsNonce(nonce, nsIContentPolicy::TYPE_STYLESHEET,
> >+                               &reportViolation, &allowInlineStyle);
> 
> So in the "foundNonce" case, it looks like we stomp on the outparams of
> GetAllowsInlineStyle() (namely, allowInlineStyle and reportViolation)
> without ever checking them.

Correct. I originally thought this was desirable per some discussion on the list, but on closer inspection this doesn't do what we want. Updated in latest patch.

> That's probably undesirable. (and might allow for mischief?)  Perhaps we
> should only be checking for a nonce *after* we've confirmed that inline
> styles are allowed?
> 
> Alternately, if the current code is correct (i.e. if GetAllowsNonce will
> only return true in cases where GetAllowsInlineStyle would also return
> true), then maybe we should restructure this slightly to avoid calling
> GetAllowsInlineStyle entirely when we find a nonce?  (This would avoid the
> stomping-on-outparams problem and make things a bit clearer.)
> 
> >     if (reportViolation) {
> >       // Inline styles are not allowed by CSP, so report the violation
> 
> This comment needs to be made more specific, due to your code changes here.
> e.g.
>  /Inline styles are/This inline style is/
> 
> Also: Probably worth asserting that allowInlineStyle is false here, right?
> (to be sure the comment isn't lying)
> 
> >+      if (foundNonce) {
> >+        csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_NONCE_STYLE,
> >+            NS_ConvertUTF8toUTF16(asciiSpec), aStyleText, aLineNumber, nonce);
> >+      } else {
> >+        csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_STYLE,
> >+            NS_ConvertUTF8toUTF16(asciiSpec), aStyleText, aLineNumber, NS_LITERAL_STRING(""));
> >+      }
> 
> I agree with Blake's comment about the the equivalent script code -- it'd be
> be great to collapse this into a single function-call.
> 
> This is made easier by the fact that you don't actually need to bother with
> the  NS_LITERAL_STRING("") here (aka EmptyString()).  You can just
> unconditionally pass |nonce|, because if foundNonce is false, then |nonce|
> will *be* the empty string.

Done!

Comment 54

[Garrett Robinson [:grobinson]]

(In reply to Blake Kaplan (:mrbkap) from comment #50)
> Comment on attachment 823662 [details] [diff] [review]
> 11.patch
> 
> Review of attachment 823662 [details] [diff] [review]:

I fixed all the nits and reworked the interaction between the outparams of GetAllowsInline{Script,Style} and GetAllowsNonce so it is clearer and commented, addressing comments from both Blake and Daniel.

Comment 55

[Garrett Robinson [:grobinson]]

Attachment: 12.patch

Address all of the previous reviewer's comments on 11.patch, as described in comments above.

Comment 56

[Daniel Holbert [:dholbert]]

Comment on attachment 827126 [details] [diff] [review]
12.patch

>diff --git a/layout/style/nsStyleUtil.cpp b/layout/style/nsStyleUtil.cpp
>+      unsigned short violationType;
>+      reportNonceViolation ? violationType = nsIContentSecurityPolicy::VIOLATION_TYPE_NONCE_STYLE
>+                           : violationType = nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_STYLE;

Best to avoid having complex expressions (with operators and side effects) inside of a ternary operator.

Please simplify to:
 unsigned short violationType = reportNonceViolation ?
   nsIContentSecurityPolicy::VIOLATION_TYPE_NONCE_STYLE :
   nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_STYLE;

>+      csp->LogViolationDetails(violationType, NS_ConvertUTF8toUTF16(asciiSpec),
>+          styleText, aLineNumber, nonce);

Please indent that last line so that "styleText" aligns with "violationType"

(Both of these comments apply to nsScriptLoader, too.)

Comment 57

[Daniel Holbert [:dholbert]]

Comment on attachment 827126 [details] [diff] [review]
12.patch

>+++ b/layout/style/nsStyleUtil.h
>   /*
>    *  Does this principal have a CSP that blocks the application of
[...]
>+   *  @param aContent
>+   *      The <style> element that the caller wants to know whether to honor.
>+   *      Included to check the nonce attribute if one is provided. Allowed to
>+   *      be null, if this is for something other than a <style> element (in
>+   *      which case nonces won't be checked).

Please add an assertion to the beginning of the CSPAllowsInlineStyle impl to verify that this comment is being respected -- i.e. assert that aContent is either null or has Tag() == nsGkAtoms::style.

Comment 58

[Daniel Holbert [:dholbert]]

Comment on attachment 827126 [details] [diff] [review]
12.patch

>+++ b/layout/style/nsStyleUtil.cpp
>+nsStyleUtil::CSPAllowsInlineStyle(nsIContent* aContent,
[...]
>+    rv = csp->GetAllowsInlineStyle(&reportInlineViolation, &allowInlineStyle);
[...]
>+    nsAutoString nonce;
>+    // We can only find a nonce if aContent is provided
>+    bool foundNonce = !!aContent &&
>+      aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);

So I'm not up on the relevant list discussion, but just from looking at the spec, it looks like it still disagrees with this code. It says we should only bother checking the nonce "if 'unsafe-inline' is not in allowed style sources" (i.e. if we're blocking inline styles -- i.e. if allowInlineStyles is false). I'm looking at the current Editor's Draft, which was last updated today (11/4/2013): https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#style-src

In contrast, with your patch, we're checking nonces regardless of whether inline styles are allowed. As you said in comment 39, "a nonce-source overrides unsafe-inline if they are both specified".

Is this a case where we're intentionally diverging from the spec for some reason? Or, is there already a pending spec edit-request to make the spec match your patch's behavior?

Comment 59

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] from comment #58)
> In contrast, with your patch, we're checking nonces regardless of whether
> inline styles are allowed. As you said in comment 39, "a nonce-source
> overrides unsafe-inline if they are both specified".
> 
> Is this a case where we're intentionally diverging from the spec for some
> reason? Or, is there already a pending spec edit-request to make the spec
> match your patch's behavior?

That was based on language in an earlier draft (for script-hash, actually) which has since been changed. I will change the code to match the spec.

Comment 60

[Garrett Robinson [:grobinson]]

Attachment: 13.patch

Fix from dholbert's comments and in-person discussion today.

Comment 61

[Daniel Holbert [:dholbert]]

Comment on attachment 827674 [details] [diff] [review]
13.patch

>+nsStyleUtil::CSPAllowsInlineStyle(nsIContent* aContent,
>+                                  nsIPrincipal* aPrincipal,
[...]
>+  if (aContent != nullptr)
>+    MOZ_ASSERT(aContent->Tag() == nsGkAtoms::style,
>+        "aContent passed to CSPAllowsInlineStyle for an element that is not <style>");

Thanks for adding this assertion! one nit: please move the null-check into the assertion, like so:
 MOZ_ASSERT(!aContent || aContent->Tag() == nsGkAtoms::style,

[I'll look at the rest later; this just jumped out when skimming the patch]

Comment 62

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] from comment #61)
> Comment on attachment 827674 [details] [diff] [review]
> 13.patch
> 
> >+nsStyleUtil::CSPAllowsInlineStyle(nsIContent* aContent,
> >+                                  nsIPrincipal* aPrincipal,
> [...]
> >+  if (aContent != nullptr)
> >+    MOZ_ASSERT(aContent->Tag() == nsGkAtoms::style,
> >+        "aContent passed to CSPAllowsInlineStyle for an element that is not <style>");
> 
> Thanks for adding this assertion! one nit: please move the null-check into
> the assertion, like so:
>  MOZ_ASSERT(!aContent || aContent->Tag() == nsGkAtoms::style,
> 
> [I'll look at the rest later; this just jumped out when skimming the patch]

Done (I wasn't sure if that was equivalent).

Comment 63

[Daniel Holbert [:dholbert]]

Comment on attachment 827674 [details] [diff] [review]
13.patch

>-nsStyleUtil::CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
>+nsStyleUtil::CSPAllowsInlineStyle(nsIContent* aContent,
>+                                  nsIPrincipal* aPrincipal,
[...]
>+  if (aContent != nullptr)
>+    MOZ_ASSERT(aContent->Tag() == nsGkAtoms::style,
>+        "aContent passed to CSPAllowsInlineStyle for an element that is not <style>");

Forgot to mention above: this last line is too long ( > 80 characters).  Insert a linebreak somewhere, e.g.
        "aContent passed to CSPAllowsInlineStyle for "
        "an element that is not <style>");

>     if (reportViolation) {
>       // Inline styles are not allowed by CSP, so report the violation

As noted in comment 51, this comment needs to be made more specific, since it's no longer about inline styles in general.
e.g.
      // This inline style is not allowed [...]
or...
      // Inline styles are not allowed by CSP (and if there was a nonce, it
      // didn't exempt us from that restriction), so report the violation.

>-    if (!inlineOK) {
>+    if (!allowInlineStyle) {
>+        NS_ASSERTION(reportViolation,
>+            "CSP blocked inline style but is not reporting a violation");
>         // The inline style should be blocked.
>         return false;
>     }

While you're here, maybe reduce the indentation in this clause (it's indented 2 spaces too much).

r=me with the above, and with a followup bug filed on adding tests for the things under a), b), etc. in the middle of comment 44 here.

Comment 64

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 827674 [details] [diff] [review]
13.patch

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

Some nits to pick, but this looks good.

::: content/base/src/nsScriptLoader.cpp
@@ +636,5 @@
> +    if (!allowInlineScript) {
> +      foundNonce = scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce);
> +      if (foundNonce) {
> +         /* We can overwrite the outparams from GetAllowsInlineScript because
> +          * if the nonce is correct, then we don't want to report the original

Nit: for consistency with the rest of this file, please use C++-style // comments.

@@ +642,5 @@
> +          * the nonce is incorrect, then we want to return just the specific
> +          * "nonce violation" rather than both a "nonce violation" and
> +          * a generic "inline violation". */
> +        rv = csp->GetAllowsNonce(nonce, nsIContentPolicy::TYPE_SCRIPT,
> +            &reportViolation, &allowInlineScript);

Please line up the & to be under the first character in 'nonce' the line above it.

@@ +663,4 @@
>        }
>  
> +      /* The type of violation to report is determined by whether there was
> +       * a nonce present. */

Here too.

::: dom/workers/RuntimeService.cpp
@@ +706,5 @@
>        NS_NAMED_LITERAL_STRING(scriptSample,
>           "Call to eval() or related function blocked by CSP.");
>        if (mWorkerPrivate->GetReportCSPViolations()) {
>          csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_EVAL,
> +                                 mFileName, scriptSample, mLineNum, NS_LITERAL_STRING(""));

EmptyString()

Comment 65

[Garrett Robinson [:grobinson]]

Attachment: 14.patch

Fix nits from dholbert and mrbkap's most recent comments. r+ from dholbert and mrbkap.

Comment 66

[Garrett Robinson [:grobinson]]

Attachment: 855326-15.patch

Fix typo.

Comment 67

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 828821 [details] [diff] [review]
855326-15.patch

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

r=me on the CSP stuff with these nits.

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +105,5 @@
>    boolean getAllowsInlineStyle(out boolean shouldReportViolations);
>  
>    /**
> +   * Whether this policy recognizes a given nonce
> +

Nit: remove blank line.

::: content/base/src/CSPUtils.jsm
@@ +839,5 @@
>  
>      // make sure the right directive set is used
>      let DIRS = this._specCompliant ? CSPRep.SRC_DIRECTIVES_NEW : CSPRep.SRC_DIRECTIVES_OLD;
>  
>      let contextIsSrcDir = false;

Nit: please rename contextIsSrcDir to be "checkingSrcDir" or something since aContext has different semantics now.

@@ +1377,5 @@
>    }
>  
> +  // check for a nonce-source match
> +  if (R_NONCESRC.test(aStr)) {
> +    if (!CSPPrefObserver.experimentalEnabled) return null;

If I understand correctly, we can't put the if check outside the RE match because of the SOURCE_EXPR RE that's a const.  Since we're matching nonces in the source-expr check, this factory method will be called for nonces (where it would not before implementing nonces).

So yeah, need to do the RE check here.  Bummer.  Maybe toss a comment in here that says "Can't lift out the if check because of the const RE above and matching of whole sources".

@@ +1520,5 @@
>     * @returns
>     *        true if the URI matches a source in this source list.
>     */
>    permits:
> +  function(aSource, aContext) {

Can we shortcut those cases when experimentalEnabled == false to avoid disaster?

::: content/base/src/contentSecurityPolicy.js
@@ +679,5 @@
> +    // with the resource, we cannot determine the nonce. See Bug 612921 and
> +    // Bug 855326.
> +    var possiblePreloadNonceConflict
> +      = (aContentType == cp.TYPE_SCRIPT || aContentType == cp.TYPE_STYLESHEET)
> +      && aContext instanceof Ci.nsIDOMHTMLDocument;

nit: operators on previous line:
var possiblePreloadNonceConflict =
    (blah blah blah) &&
    (blah blah blah);

@@ +719,1 @@
>                  ? cp.ACCEPT : cp.REJECT_SERVER;

Please fix this ternary statement so ? ends the previous line instead of starting one.

Comment 68

[Garrett Robinson [:grobinson]]

Attachment: 16.patch

Fixes from Sid's comment above. r+ from sstamm, dholbert, and mrbkap. Try: https://tbpl.mozilla.org/?tree=Try&rev=d713017d6bf5

Comment 69

[Garrett Robinson [:grobinson]]

Attachment: 855326-17.patch

Try run was green except for the nonce test on B2G, where test fails due to lack of http-on-modify-request observer. This patch disables the tests on B2G.

Comment 70

[Daniel Holbert [:dholbert]]

(Don't forget to file a bug to flesh out the tests, per end of comment 63 / middle of comment 44.)

Comment 71

[Garrett Robinson [:grobinson]]

(In reply to Daniel Holbert [:dholbert] from comment #70)
> (Don't forget to file a bug to flesh out the tests, per end of comment 63 /
> middle of comment 44.)

That's 936097 (depends on this bug) :)

Comment 72

[Daniel Holbert [:dholbert]]

Ah, great - thanks :)

Comment 73

[Daniel Holbert [:dholbert]]

I added the missing "r=mrbkap r=dholbert r=geekboy" to commit message, and pushed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/57213b64023b

Comment 74

[Daniel Holbert [:dholbert]]

Backed out for build bustage:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/c45039455165
with build errors when evaluating a MOZ_ASSERT'ed condition:
> nsStyleLinkElement.cpp:363:41: error: 'Tag' was not declared in this scope
https://tbpl.mozilla.org/php/getParsedLog.php?id=30339897&tree=Mozilla-Inbound

The Try run didn't catch this because it was opt-only.  Garrett, can you fix up the build bustage and give this a Debug+Opt Try run?  (Without that, we can't be sure the assertions are valid.)

Comment 75

[Garrett Robinson [:grobinson]]

Attachment: 18.patch

Fix debug build bustage. Opt+Debug try run: https://tbpl.mozilla.org/?tree=Try&rev=5c5f32925c47

Comment 76

[Daniel Holbert [:dholbert]]

Comment on attachment 829555 [details] [diff] [review]
18.patch

>+++ b/content/base/src/nsStyleLinkElement.cpp
>@@ -355,17 +355,20 @@ nsStyleLinkElement::DoUpdateStyleSheet(n
>+    MOZ_ASSERT(thisContent->NodeInfo()->NameAtom() != nsGkAtoms::link,

Please use
  thisContent->Tag()
here. (It's equivalent to NodeInfo()->NameAtom(), but it's shorter & clearer)

(The build failure happened because you were calling Tag() implicitly on |this|, which wasn't an instance of nsIContent.)

>+++ b/layout/style/nsStyleUtil.cpp
[...]
>-nsStyleUtil::CSPAllowsInlineStyle(nsIPrincipal* aPrincipal,
>+nsStyleUtil::CSPAllowsInlineStyle(nsIContent* aContent,
>+                                  nsIPrincipal* aPrincipal,
[...]
>+  MOZ_ASSERT(!aContent || aContent->NodeInfo()->NameAtom() == nsGkAtoms::style,
>+      "aContent passed to CSPAllowsInlineStyle "
>+      "for an element that is not <style>");

Same here. (This one was actually fine in your last patch; I'd be surprised if you had build issues from this line.)

Comment 77

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #76)
> (The build failure happened because you were calling Tag() implicitly on
> |this|, which wasn't an instance of nsIContent.)

(...which is the fault of my sample-code in comment 43 ;) It was *almost* correct, but I didn't notice the |thisContent| distinction until now.)

Comment 78

[Garrett Robinson [:grobinson]]

Attachment: 19.patch

Use Tag() per dholbert's prior comments. Try: https://tbpl.mozilla.org/?tree=Try&rev=ff25d338bbba

Comment 79

[Daniel Holbert [:dholbert]]

Re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf867ff4215

Comment 80

[Daniel Holbert [:dholbert]]

(that's the patch from comment 78, whose Try run looks good.)

Comment 81

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1bf867ff4215