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

RESOLVED FIXED in mozilla24

Status

()

defect
P1
normal
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: imelven, Assigned: geekboy)

Tracking

(Blocks 1 bug)

Trunk
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

7 years ago
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
Reporter

Updated

7 years ago
Blocks: csp-w3c-1.0

Updated

7 years ago
Assignee: nobody → mmoutenot

Comment 1

7 years ago
Posted patch Potential Fix? (obsolete) — Splinter Review
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?
Attachment #649763 - Flags: feedback?(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.
Attachment #649763 - Flags: feedback?(sstamm) → feedback-
It might actually be value to remove CSPRep.makeExplicit() entirely since it's not fully necessary.  I filed bug 780978 for this.

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Keywords: checkin-needed
Reporter

Updated

7 years ago
Depends on: 783049

Updated

7 years ago
Blocks: 783497

Updated

7 years ago
Depends on: 780978
Reporter

Comment 4

6 years ago
I don't think Marshall is working on this still.
Assignee: mmoutenot → nobody
Reporter

Updated

6 years ago
Priority: -- → P1
Reporter

Updated

6 years ago
Assignee: nobody → imelven
Reporter

Updated

6 years ago
Flags: needinfo?(imelven)
Reporter

Comment 5

6 years ago
This needs to be tested with the 1.0 parser.
Flags: needinfo?(imelven)
Reporter

Updated

6 years ago
Flags: needinfo?(imelven)
Does the 1.0 parser fix this?
Reporter

Comment 7

6 years ago
(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.
Flags: needinfo?(imelven)
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.
Reporter

Comment 9

6 years ago
(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

6 years ago
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

6 years ago
Posted patch potential patch (obsolete) — Splinter Review
Attachment #730057 - Flags: review?(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).
Attachment #730057 - Flags: review?(sstamm) → review-
Reporter

Updated

6 years ago
Assignee: imelven → nobody
Assignee

Updated

6 years ago
Duplicate of this bug: 702176
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?
Assignee

Updated

6 years ago
Assignee: nobody → sstamm
Reporter

Comment 15

6 years ago
I haven't tested this with the 1.0 parser yet.
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.
Posted patch proposed patch (obsolete) — Splinter Review
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.
Attachment #649763 - Attachment is obsolete: true
Attachment #730057 - Attachment is obsolete: true
Attachment #755680 - Flags: review?(imelven)
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+.
Attachment #755680 - Flags: review?(imelven) → review+
(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!
Posted patch fixSplinter Review
Thanks, Tanvi.  Here's the updated patch with the parameter name changes.
Attachment #755680 - Attachment is obsolete: true
Attachment #756050 - Flags: review+
Waiting to land this change with bug 780978.
Reporter

Comment 22

6 years ago
Thanks very much for taking this review, Tanvi !
https://hg.mozilla.org/mozilla-central/rev/8e1c229ed6c6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Assignee

Updated

5 years ago
See Also: → 1031259
You need to log in before you can comment on or make changes to this bug.