1086612 - Malformed CSP frame-src directive disables CSP

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Peksa]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.104 Safari/537.36

Steps to reproduce:

A page with the following CSP header, with a malformed frame-src directive, seems to disable the default policy frame-src 'self'.

Content-Security-Policy: frame-src bankid:/*

And hence allows the following iframe:
<iframe src="bankid://foobar"></iframe>

Test case: https://peks.as/experiments/ffcsp2/

This occurs in Firefox 33.0 Stable, Firefox 34.0 beta, Firefox 36.0a1 (2014-10-21) nightly but not in Firefox 32.0.3, so it seems to be a regression.



Actual results:

The app corresponding to the bankid:// scheme is launched.
The developer console shows a warning:
Content Security Policy: Failed to parse unrecognized source bankid:/*



Expected results:

The iframe should be blocked, the app with the registered custom scheme shouldn't be launched.
The developer console should show both the parse warning and an additional error that the iframe was blocked:
Content Security Policy: Failed to parse unrecognized source bankid:/*
Content Security Policy: The page's settings blocked the loading of a resource at bankid://foobar ("frame-src 'none'").

(or something similar).

The iframe is blocked as expected in both Firefox 32.0.3 and Chrome 38.0.2125.104 m, both on Windows.

Comment 1

[Matthias Versen [:Matti]]

Last good nightly: 2014-06-26
First bad nightly: 2014-06-27

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=464bca437658&tochange=be076357691c

I guess this is :
a8f96ce0c95e	Christoph Kerschbaumer — Bug 925004 - CSP in CPP: flip pref to enable new CSP backend (r=sstamm)

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Interesting, neither the old [1], nor the new spec [2] seem to cover the part about using
> frame-src 'self'
as default.

In my opinion a CSP of
> Content-Security-Policy: frame-src bankid:/*

should log a warning to the console and disregard the directive since it's not holding any valid sources.
Sid, apparently the JS implementation of CSP handled that case differently. Do you know why? Are we missing something in the new CSP implementation? 

[1] http://www.w3.org/TR/CSP/#frame-src
[2] http://www.w3.org/TR/CSP11/#directive-frame-src

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

So here's the deal.  The ABNF for source-list permits an empty list.  That means when we parse any source-list directives, we do not disregard the directive when it has zero valid-syntax sources.

So "frame-src blah:/invalid" would parse the same as "frame-src ", which is an empty set of valid sources but has the "frame-src" policy active.

In detail, the spec says:
http://www.w3.org/TR/CSP/#parsing-1

> 1. If source list (with leading and trailing whitespace stripped) is a 
>    case insensitive match for the string 'none' (including the quotation marks), 
>    return the empty set.
> 2. Let the set of source expressions be the empty set.
> 3. For each token returned by splitting source list on spaces, if the token matches 
>    the grammar for source-expression or ext-host-source, add the token to the set of 
>    source expressions.
> 4. Return the set of source expressions.

Later the spec says:

> A URI matches a source list, if, and only if, the URI matches at 
> least one source expression in the set of source expressions obtained 
> by parsing the source list. Notice that no URIs match an empty set 
> of source expressions, such as the set obtained by parsing the source 
> list 'none'.

So parsing "'none'" gives the empty set, and a source list with no valid tokens also gives the empty set.  Maybe we don't want to actually and explicitly add 'none' as a source to the list, but we should be enforcing frame-src with zero valid sources.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1086612.patch

In case we find a valid directive but can not parse a valid source-expression we fall back to 'none' as the spec suggests.

Please note that I also updated the parser to allow a valid directive with no source-expression in the policy. E.g. a policy of
> script-src
with the empty set also defaults to
> script-src 'none'

There is a test in TestCSPParser.cpp that tests both of these behaviors (no valid source-expression and no source-expression at all).

Further I added a mochitest that confirms a fall back to 'none' in case there is a valid directive but no (valid) source-expression.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1086612_tests.patch

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1086612_parser_test_updates.patch

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8509194 [details] [diff] [review]
bug_1086612.patch

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

::: content/base/src/nsCSPParser.cpp
@@ +838,3 @@
>    if (srcs.Length() == 0) {
> +    nsCSPKeywordSrc *keyword = new nsCSPKeywordSrc(CSP_NONE);
> +    srcs.AppendElement(keyword);

Do we want to warn that the directive's value has zero valid sources?

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8509198 [details] [diff] [review]
bug_1086612_parser_test_updates.patch

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

::: content/base/test/TestCSPParser.cpp
@@ +443,1 @@
>  nsresult TestPoliciesThatLogWarning() {

Do these new tests log a warning, or should you call this function "TestOneSourceDirectives" or something more descriptive?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7)
> Comment on attachment 8509194 [details] [diff] [review]
> bug_1086612.patch
> 
> Review of attachment 8509194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsCSPParser.cpp
> @@ +838,3 @@
> >    if (srcs.Length() == 0) {
> > +    nsCSPKeywordSrc *keyword = new nsCSPKeywordSrc(CSP_NONE);
> > +    srcs.AppendElement(keyword);
> 
> Do we want to warn that the directive's value has zero valid sources?

Oh sorry, I wanted to mention that in the initial patch description, but somehow I forgot. So, when parsing an invalid source the parser already logs a warning to the console. I don't think it's necessary to pollute the console with yet another warning that there is no valid source expression left.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8)
> Comment on attachment 8509198 [details] [diff] [review]
> bug_1086612_parser_test_updates.patch
> 
> Review of attachment 8509198 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/test/TestCSPParser.cpp
> @@ +443,1 @@
> >  nsresult TestPoliciesThatLogWarning() {
> 
> Do these new tests log a warning, or should you call this function
> "TestOneSourceDirectives" or something more descriptive?

All of these tests log a warning, but we can't really inspect them in compiled code tests. To avoid confusion, I will update the name - good point.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5569dfd0d37
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c92d59d2f05
https://hg.mozilla.org/integration/mozilla-inbound/rev/c12e6b5e73d1

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d5569dfd0d37
https://hg.mozilla.org/mozilla-central/rev/2c92d59d2f05
https://hg.mozilla.org/mozilla-central/rev/c12e6b5e73d1

Comment 13

[Peksa]

Verified in nightly 36.0a1 (2014-10-23) on Windows. The frame is blocked as expected:

Content Security Policy: The page's settings blocked the loading of a resource at bankid://foobar ("frame-src 'none'").

Impressive turnaround time, thanks!

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Peksa from comment #13)
> Verified in nightly 36.0a1 (2014-10-23) on Windows. The frame is blocked as
> expected:
> 
> Content Security Policy: The page's settings blocked the loading of a
> resource at bankid://foobar ("frame-src 'none'").
> 
> Impressive turnaround time, thanks!

Thanks for bringing it to our attention!