764937 - CSP : unspecified default-src directive should default to no restrictions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Ian Melven :imelven]

per the spec, when a default-src isn't specified, no restrictions should apply

according to dveditz, when a default-src isn't specified Gecko currently assumes default-src: none

Comment 1

[Marshall Moutenot]

Attachment: Potential Fix?

Y'all, I think this is right from discussions with tanvi and dveditz but it's a lot simpler than I had initially done. Thoughts?

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 649763 [details] [diff] [review]
Potential Fix?

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

You probably want to change CSPRep.makeExplicit() to, if there's no DEFAULT_SRC, set it to *.

::: content/base/src/CSPUtils.jsm
@@ +421,5 @@
>  
>    } // end directive: loop
>  
>    // if makeExplicit fails for any reason, default to default-src 'none'.  This
>    // includes the case where "default-src" is not present.

Remove this sentence that says "this includes the case...", since we only want to fail closed if makeExplicit fails.  makeExplicit should be changed to use "*" if there's no default-src directive.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

It might actually be value to remove CSPRep.makeExplicit() entirely since it's not fully necessary.  I filed bug 780978 for this.

Comment 4

[Ian Melven :imelven]

I don't think Marshall is working on this still.

Comment 5

[Ian Melven :imelven]

This needs to be tested with the 1.0 parser.

Comment 6

[Tanvi Vyas[:tanvi]]

Does the 1.0 parser fix this?

Comment 7

[Ian Melven :imelven]

(In reply to Tanvi Vyas [:tanvi] from comment #6)
> Does the 1.0 parser fix this?

see comment 5. Nobody has tested it yet AFAIK.

Comment 8

[Tanvi Vyas[:tanvi]]

Kailas, Freddy - can you take a look and test this.  If it fixes this, than the CSP 1.0 version of userCSP will work well.  If not, we will be stuck with the same problem.  userCSP won't be very useful to users (it will break pages) if they don't specify a default-src directive.

If this isn't fixed by the CSP 1.0 parser, we should consider adding a hack to userCSP so that we set default-src to * if the user or the website don't specify a default-src.  Kailas, we may have already done something like this, but I can't remember at the moment.

Comment 9

[Ian Melven :imelven]

(In reply to Tanvi Vyas [:tanvi] from comment #8)
> Kailas, Freddy - can you take a look and test this.  If it fixes this, than
> the CSP 1.0 version of userCSP will work well.  If not, we will be stuck
> with the same problem.  userCSP won't be very useful to users (it will break
> pages) if they don't specify a default-src directive.
> 
> If this isn't fixed by the CSP 1.0 parser, we should consider adding a hack
> to userCSP so that we set default-src to * if the user or the website don't
> specify a default-src.  Kailas, we may have already done something like
> this, but I can't remember at the moment.

If this isn't fixed by the CSP 1.0 parser, I will take care of fixing this bug there, also :)

Comment 10

[Kailas]

Tanvi: In userCSP we automatically set default-src to "*" if it is not set by users. Similarly, inferCSP feature also sets default-src to "*". 

I have one question here, why we don't set default-src to "self" and prefer to set it to "*". Setting it to "*" means all other directives which are not specified in CSP policy will also have effect of "*". This might make protection provided by CSP policy weaker. For example, if an attacker injected a script tag (script src="http://attacker.com/mal.js") that loads malicious script from attacker's domain and "script-src" is not specified in CSP policy then setting "default-src" to "*" also makes "script-src" to "*" which makes CSP protection guarantee provided by CSP weaker.

Comment 11

[Kailas]

Attachment: potential patch

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 730057 [details] [diff] [review]
potential patch

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

We won't be taking this patch since old CSP (x-content-security-policy) should not infer 'default-src *' since that's not how we specified it, and the new CSP 1.0 implementation will infer 'default-src *'.

What we need is not a patch that changes the old CSP implementation but a test to check if sending a new 1.0-compliant Content-Security-Policy header without a default-source results in this inference.  Ian mentioned this in comment 5.

::: content/base/src/CSPUtils.jsm
@@ +470,5 @@
>    // if makeExplicit fails for any reason, default to default-src 'none'.  This
>    // includes the case where "default-src" is not present.
>    if (aCSPR.makeExplicit())
>      return aCSPR;
> +  return CSPRep.fromString("default-src *", selfUri);

This conflicts with the comment above it.  If there's a failure in makeExplicit, that probably indicates the policy is invalid and the whole thing should be tossed out (this is not for CSP 1.0, this is for old CSP).

::: content/base/test/unit/test_csputils.js
@@ +412,5 @@
>          do_check_equivalent(cspr._directives[SD[d]],
>                              cspr_allow._directives[SD[d]]);
>        }
> +      // check ensure default-src is * when default-src is not set
> +      cspr = CSPRep.fromString("script-src http://foo.com", URI("http://self.com"));

We probably don't want this in the old CSP implementation, as I said above, but only in CSP 1.0 (the new parser and non-prefixed header).

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

My hunch is that both the spec compliant and pre 1.0 versions of the CSP parser default-src is 'none' if it's not present.

I don't see unit tests for this one, and this makes me think it:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#749

Ian: can you tell me if you tested the behavior of a spec-compliant policy without a default-src or if I should fix it and write a test for that?

Comment 15

[Ian Melven :imelven]

I haven't tested this with the 1.0 parser yet.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

So I confirmed that the 1.0 parser doesn't "fail open" (absence of default-src means 'none').  The right thing to do here is to remove makeExplicit() and rework the "permits" logic to enforce the right defaults.  That's bug 780978 and I think we should fix this bug before ripping out makeExplicit.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

This changes the CSP 1.0 spec-compliant parser to use "default-src *" when default-src is not specified.  It's intended to fix this bug temporarily until we can remove makeExplicit() in bug 780978 and move the inference logic into permits().  That's part of a bigger effort to get rid of policy intersecting and allow multiple policies in parallel, but we need this fix now.

Comment 18

[Tanvi Vyas[:tanvi]]

Comment on attachment 755680 [details] [diff] [review]
proposed patch

+      if(!this._specCompliant) {
+        this.warn(CSPLocalizer.getStr("allowOrDefaultSrcRequired"));
+        return false;
+      }
+
+      // bug 780978 will remove these lines and needs to make sure that
+      // CSPRep.permits() enforces this logic: prefixed headers default to
+      // "disallowed" and unprefixed headers default to "allowed" when
+      // neither specific -src nor default-src directive is provided.

I don't quite agree with this.  In both the prefixed and unprefixed header, I think a missing default-src should map to default-src *.  Since the current behavior is terrible (IMHO), why not change the behavior for the prefixed header too?

+      function testPermits(cspObj, aUri, aContext) {
+        return cspObj.shouldLoad(aContext, aUri, null, null, null, null)
+               == Ci.nsIContentPolicy.ACCEPT;
+      };
+
aContext should probably be called something else (aContentType?) since aContext is used as the 4th parameter to shouldLoad() and means something else.  However, aContext in testPermits() is already used elsewhere in this test file, so it's better to keep it consistent.

This looks pretty good to me.  Ian is gone for a bit, so I am doing the review in his absence.  Giving this an r+.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Tanvi Vyas [:tanvi] from comment #18)
> I don't quite agree with this.  In both the prefixed and unprefixed header,
> I think a missing default-src should map to default-src *.  Since the
> current behavior is terrible (IMHO), why not change the behavior for the
> prefixed header too?

When we were developing X-Content-Security-Policy, a key tenet was that it "fails closed" and that a default-src was required.  Failure to have the default-src (or allow directive) was an invalid policy and perhaps an attack, so CSP should "fail closed".

https://wiki.mozilla.org/Security/CSP/Specification#Directives

We don't have a good reason to change the old semantics now, instead we should encourage sites to start using the new, standardized feature.  It's more work to change it and it will perhaps introduce unexpected behavior on the sites using X-Content-Security-Policy.

> +      function testPermits(cspObj, aUri, aContext) {
> aContext should probably be called something else (aContentType?) since
> aContext is used as the 4th parameter to shouldLoad() and means something
> else.  However, aContext in testPermits() is already used elsewhere in this
> test file, so it's better to keep it consistent.

Yeah, good catch (was a terrible param name in the old test code).  Thanks!

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

Thanks, Tanvi.  Here's the updated patch with the parameter name changes.

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

Waiting to land this change with bug 780978.

Comment 22

[Ian Melven :imelven]

Thanks very much for taking this review, Tanvi !

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1c229ed6c6

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8e1c229ed6c6