1004703 - ignore 'unsafe-inline' if nonce- or hash-source specified

Status: RESOLVED FIXED

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

Description

[Garrett Robinson [:grobinson]]

Follow-up to https://bugzilla.mozilla.org/show_bug.cgi?id=979580#c14.

Comment 1

[Mike West]

Hey Sid and Dan! Would you mind triaging this as well? GMail is running into issues while trying to migrate away from 'unsafe-inline'.

Comment 2

[Daniel Veditz [:dveditz]]

Spec is here: http://www.w3.org/TR/CSP2/#directive-script-src

  "If 'unsafe-inline' is not in the list of allowed script sources, or if at least one
  nonce-source or hash-source is present in the list of allowed script sources:"

The intention is to let sites start to lock things down with nonce and/or hashes while specifying unsafe-inline so CSP1.0-compliant browsers don't break on the site. It's a transition state.

Note this means that if you use a nonce/hash it's not possible to use javascript: urls or inline event handlers. It's as if the policy didn't have unsafe-inline at all. Probably worth throwing a warning on the web console if we encounter both unsafe-inline and a hash/nonce. It's not an error, but it might be a mistake. ("warning, 'unsafe-inline' is ignored when hash or nonce also used")

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1004703_ignore_unsafe_inline.patch

Hey Sid, what do you think about the approach of ignoring/removing unsafe-inline within script-src? I also included some compiled code tests to prove that this approach works!

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Dan, potentially going ahead and getting this feature out is going to break some webpages, but it's definitely a step into the right direction. I think you feel the same way, right Dan?

Also, please feel free to also look at the approach and leave some comments there!

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8586581 [details] [diff] [review]
bug_1004703_ignore_unsafe_inline.patch

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

I'm reluctant to actually remove the keyword during parsing, since it may make reporting a bit more confusing.  If the violated directive doesn't appear the same as the one the devs wrote and sent with the page, it may be harder for them to debug.

Would it make more sense to keep a flag in the CSPDirective so the keyword gets ignored (but not deleted from any string representation) when permission is checked?

(Specific feedback about this patch below.)

::: dom/security/nsCSPParser.cpp
@@ +578,5 @@
> +    if (mScriptUnsafeInline) {
> +      // log warning to the console
> +      return nullptr;
> +    }
> +    mScriptUnsafeInline = new nsCSPKeywordSrc(CSP_KeywordToEnum(mCurToken));

Do you need to create and cache this?  Why not just use a flag and instantiate a nsCSPKeywordSrc when you need it?

::: dom/security/nsCSPParser.h
@@ +230,5 @@
>  
> +    // cache variables when trying to remove unsafe-inline
> +    bool               mScriptHashOrNonce;  // false, if no hash or nonce is defined
> +    nsCSPBaseSrc*      mScriptUnsafeInline; // cache for fast removal from srcs[].
> +

These names are confusing -- I recommend using something like mHasHashOrNonce and mHasUnsafeInlineKeyword.  Or make the source list queryable for keywords by adding a new HasKeyword function to CSPDirective...  Just some ideas.

::: dom/security/nsCSPUtils.h
@@ +290,5 @@
>       { return mDirective == nsIContentSecurityPolicy::DEFAULT_SRC_DIRECTIVE; }
>  
> +    inline bool isScriptSrcDirective() const
> +     { return mDirective == nsIContentSecurityPolicy::SCRIPT_SRC_DIRECTIVE; }
> +

Why do you need to add this?  Why not just use equals?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1004703_ignore_unsafe_inline.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #5)
> I'm reluctant to actually remove the keyword during parsing, since it may
> make reporting a bit more confusing.  If the violated directive doesn't
> appear the same as the one the devs wrote and sent with the page, it may be
> harder for them to debug.

We have had that discussion before whether we should modify the policy during parsing or not :-) Anyway, in this case you convinced me. Let's keep unsafe-inline in the policy but ignore it when enforcing the policy.

I rewrote the patch; Still keeping a cache pointer for the KeywordSrc though, which allows us to just set the ignore-flag within that object (that's also where the ignore flag semantically belongs in my opinion), leaving all other enforcing parts untouched. I am pretty sure you agree.

Also logging a warning the console in case we are going to ignore unsafe-inline because a hash or nonce is specified within script-src.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1004703_ignore_unsafe_inline_tests.patch

Mochitests making sure we ignore unsafe-inline within script-src if hash or nonce is specified. Please keep in mind that that's only the case for script-src, not for default-src. Added a testcase for default-src as well to make sure everything is correct there as well.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1004703_ignore_unsafe_inline_tests.patch

Left a debug statement in the old patch :-( - here is an updated one.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8587168 [details] [diff] [review]
bug_1004703_ignore_unsafe_inline_tests.patch

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

::: dom/base/test/csp/test_ignore_unsafe_inline.html
@@ +18,5 @@
> + * and make sure 'unsafe-inline' is ignored within script-src if hash-source
> + * or nonce-source is specified.
> + */
> +
> +var DEFAULT_POLICY = "default-src 'none'; script-src ";

Please rename this to POLICY_PREFIX or something (it's not a whole policy since the script-src directive is empty).

@@ +25,5 @@
> +  {
> +    policy: DEFAULT_POLICY + "'unsafe-inline'",
> +    description: "'unsafe-inline' allows all scripts to execute",
> +    file: "file_ignore_unsafe_inline.html",
> +    result: "abcd",

Please document the format of result's value somewhere (it's not obvious on a quick scan of the test).  Also, what if the inline scripts run in parallel and complete out of order?  Maybe in the verification (below) you should sort the contents of the testdiv.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8587165 [details] [diff] [review]
bug_1004703_ignore_unsafe_inline.patch

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

This looks good.  My one request is to find a better name for "ignore" in nsCSPUtils::nsCSPKeywordSource.  It's kind of awkward.  Maybe "superseded", "invalidated", or "inactive".  Either way, you should fully document what it means in lots of comments everywhere it's used so it's clear that the reason it's "inactive" is because of another CSP source making it irrelevant.  You document it a bit, but we should be sure it's clear what this thing is for.

::: dom/locales/en-US/chrome/security/csp.properties
@@ +33,5 @@
> +# %1$S defines the duplicate src
> +ignoringDuplicateSrc = Ignoring duplicate source %1$S
> +# LOCALIZATION NOTE (ignoringSrcWithinScriptSrc):
> +# %1$S is the ignored src
> +ignoringSrcWithinScriptSrc = Ignoring "%1$S" within script-src: nonce-source or hash-source specified

You should add that script-src should not be localized.

::: dom/security/nsCSPParser.h
@@ +229,5 @@
>      nsTArray<nsString> mCurDir;
>  
> +    // cache variables to ignore unsafe-inline if hash or nonce is specified
> +    bool               mHasHashOrNonce; // false, if no hash or nonce is defined
> +    nsCSPKeywordSrc*   mUnsafeInlineKeywordSrc; // null, otherwise toggle ignore flag

Maybe add to your comment that this is nsCSPKeywordSrc* so it can be flagged as "ignore" if we find a nonce or hash.

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Dan, potentially going ahead and getting this feature out is going to break
> some webpages, but it's definitely a step into the right direction. I think
> you feel the same way, right Dan?

I still agree with myself in comment 2 :-)

Breaking sites would be sad, but if the spec needs to be changed we will only find that out by implementing the spec'd behavior.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeece72a1d99
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ff6cf84d340

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eeece72a1d99
https://hg.mozilla.org/mozilla-central/rev/6ff6cf84d340

Comment 14

[Devdatta Akhawe [:devd]]

hmm .. anyone know why Firefox is blocking script execution on https://hackpad.com right now?

Comment 15

[Devdatta Akhawe [:devd]]

(sorry .. pressed save too soon and now I feel doubly bad about spamming everyone)

The nonce matches the policy in the header; the code works fine on Chrome; but for some reason, the "ignoring unsafe-inline" has meant that the script code blocks with nonce in them aren't executed. I am kinda confused and was wondering if anyone knew if we were doing something wrong that wasn't allowed in the spec.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Dev: is it actually blocking the script, or only reporting that it's blocking the script? We have a bug on file that we're over-reporting (we report when inline discovered, but it's already sent when we check the nonce/hash and go ahead and allow it anyway).

Comment 17

[Devdatta Akhawe [:devd]]

If you see the console log, it will say "clientVars" not defined. This is defined in an inline script block with a nonce. So, it does look like the script isn't executed.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Devdatta Akhawe [:devd] from comment #15)
> (sorry .. pressed save too soon and now I feel doubly bad about spamming
> everyone)
> 
> The nonce matches the policy in the header; the code works fine on Chrome;
> but for some reason, the "ignoring unsafe-inline" has meant that the script
> code blocks with nonce in them aren't executed. I am kinda confused and was
> wondering if anyone knew if we were doing something wrong that wasn't
> allowed in the spec.

Dave, I am taking this problem into a new Bug. Please follow the conversation over in Bug 1198422