Closed Bug 909029 Opened 11 years ago Closed 11 years ago

CSP source-lists ignore some source expressions like 'unsafe-inline' when * or 'none' are used (e.g., style-src, script-src)

Categories

(Core :: Security, defect)

23 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: onlinechess, Assigned: grobinson)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

Generate and send the following http header for a page on the server:

Content-Security-Policy: default-src *; style-src * 'unsafe-inline';

Use inline styles on the page, for example:
<div style="padding: 100px;">Fat div</div>

Or:
<style>
.float-right
{
    float: right;
}
</style>



Actual results:

Every inline style attribute is blocked.

Console shows the following:
CSP WARN:  Directive inline style base restriction violated @...

If "report-uri" is added to CSP, the following error is reported:
{"csp-report":{"document-uri":"http://example.com/","referrer":"","blocked-uri":"self","violated-directive":"inline style base restriction","source-file":"http://example.com/","script-sample":"padding: 100px;"}}



Expected results:

Inline style should not been have blocked.
It appears the following does work (inline styles are no longer blocked):

Content-Security-Policy: default-src *; style-src 'self' 'unsafe-inline';

i.e. when * is replaced with 'self' in style-src declaration.


Curiously, the following doesn't work:

Content-Security-Policy: default-src *; style-src * 'self' 'unsafe-inline';

I.e. putting both * and 'self' blocks all inline styles again.
One last thing: all 3 versions of CSP work just fine in Chrome 28/29, as expected.
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Component: DOM: Core & HTML → Security
Component: Security → DOM: Core & HTML
Quick tests:
http://sitecolony.com/csp-test1.pl
http://sitecolony.com/csp-test2.pl
http://sitecolony.com/csp-test3.pl

Currently, only the second test works correctly in Firefox (i.e. shows the green background for the div).

The last test should block the inline styles (for regression testing, FF currently does it correctly):
http://sitecolony.com/csp-test4.pl
Component: DOM: Core & HTML → Security
Flags: needinfo?(sstamm)
Yep, rats.  Looks like it'll also be a problem on trunk. I didn't catch this case before when I reviewed the changes to the CSP parser.

SourceList.fromString() returns early if * is found before 'unsafe-inline'.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#1087

I think "style-src 'none' 'unsafe-inline'" will also fail to allow inline styles due to the same early return pattern just above it on line 1057.  But this is intended due to the ABNF in the spec that says "either source-list or 'none'".  'none' is only allowed to appear alone.
  
http://www.w3.org/TR/CSP/#source-list

But * can appear with other sources like 'unsafe-inline'.

Temporary Fix: If this is indeed the problem (99% sure here), putting 'unsafe-inline' first or at least before * in the directive value should solve the problem.  

We need to remove that return statement.  Also, we need to test for these cases.  I'll take this bug, but I'm cc'ing Garrett in case he is anxious and wants to fix it first, though we're both out of the office most of the coming week.
Blocks: CSP
Flags: needinfo?(sstamm)
OS: Windows 7 → All
QA Contact: sstamm
Hardware: x86_64 → All
Summary: Content-Security-Policy HTTP header is not interpreted correctly (style-src) → CSP source-lists ignore some source expressions like 'unsafe-inline' when * or 'none' are used (e.g., style-src, script-src)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee: nobody → grobinson
Attached patch Patch 1 (obsolete) — Splinter Review
This patch adds a simple Mochitest to test the inline styles failure described in Comment 1, and removes the problematic early return mentioned in Comment 4. Removing this early return causes the test to pass. All other CSP tests pass.
Attachment #803814 - Flags: review?(sstamm)
Comment on attachment 803814 [details] [diff] [review]
Patch 1

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

We should also test a policy like "style-src 'none' 'unsafe-inline'" for what behavior we expect.  I think the spec says the whole directive is invalid, but maybe I'm interpreting it wrong.  We should commit to one direction and test for it.

::: content/base/test/csp/file_CSP_bug909029.html
@@ +2,5 @@
> +<html>
> +  <body>
> +    <p id="inline-styled">This should be green</p>
> +    <style>
> +      p#inline-styled { color:green; }

Please make the color value explicit (use rgb(0, 128, 0)) in case the built-in "green" becomes redefined for some dumb reason.

::: content/base/test/csp/test_CSP_bug909029.html
@@ +19,5 @@
> +function checkStyles() {
> +  console.log("in checkStyles");
> +  var testframe = document.getElementById('testframe');
> +
> +  is(getElementColorById(testframe, 'inline-styled'), green, "Inline styles should be allowed!");

We should probably also test that this policy doesn't forget about * and unsafe-inline is limited to styles:

* an inline script (blocked)
* an external css (allowed)
* an external image or something (default-src should allow it)

For the inline script: instead of an observer you can probably just make another target element and change its color via script.
Attachment #803814 - Flags: review?(sstamm) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
Addresses comments on previous patch.

I tested "style-src 'none' 'unsafe-inline'" manually on FF and Chrome. In both cases, 'none' is ignored unless it appears by itself, which is the behavior defined by the spec. I did not add a test for that in this patch since this bug did not report issues with 'none', and the behavior has been and continues to be correct.
Attachment #803814 - Attachment is obsolete: true
Attachment #803885 - Flags: review?(sstamm)
I recommend adding the "style-src 'none' 'unsafe-inline'" test anyway so that it's really clear that's what we expect and so we don't accidentally change this behavior.
Attached patch Patch 3 (obsolete) — Splinter Review
Adds a test for "style-src: 'none' 'unsafe-inline'".

My interpretation of the expected behavior is that, unless 'none' appears by itself, it is invalid and should be ignored. The rest of the style-src directive is interpreted normally. This is what Chrome does as well.
Attachment #803885 - Attachment is obsolete: true
Attachment #803885 - Flags: review?(sstamm)
Attachment #804783 - Flags: review?(sstamm)
Comment on attachment 804783 [details] [diff] [review]
Patch 3

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

This uses an observer, so you'll have to disable it on B2G in b2g.json.  

r=me if you disable the test on b2g,  replace "green", and consider my other suggestions (just advisory).  Please be sure to run this through try before landing, just in case.

::: content/base/test/csp/file_CSP_bug909029_none.html
@@ +1,5 @@
> +<!doctype html>
> +<html>
> +  <head>
> +    <link rel='stylesheet' type='text/css'
> +          href='file_CSP.sjs?testid=none_style&type=text/css' />

maybe put a comment here that indicates how this will load an empty stylesheet and noticed by the observer/examiner as either blocked or loaded.  Took me a while to figure out that the content of this "file" is empty and it has no visual effect on the page when loaded.

@@ +11,5 @@
> +      p#inline-style { color:rgb(0, 128, 0); }
> +    </style>
> +    <script>
> +      // Use inline script to set a style attribute
> +      document.getElementById("inline-script").style.color = "green";

Please use rgb(0,128,0) instead of "green" here for consistency.

::: content/base/test/csp/file_CSP_bug909029_star.html
@@ +11,5 @@
> +      p#inline-style { color:rgb(0, 128, 0); }
> +    </style>
> +    <script>
> +      // Use inline script to set a style attribute
> +      document.getElementById("inline-script").style.color = "green";

Please use rgb(0,128,0) instead of "green" here for consistency.

::: content/base/test/csp/test_CSP_bug909029.html
@@ +101,5 @@
> +}
> +
> +function waitForFinish() {
> +  // Wait for observer tests to finish
> +  setTimeout(function() {

setTimeout is used here since the inline stuff may not have finished by the time the content loads are blocked, right?  If that's not the case, could you just put this check inside the observer?
Attachment #804783 - Flags: review?(sstamm) → review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #10)
> Comment on attachment 804783 [details] [diff] [review]
> Patch 3
> 
> Review of attachment 804783 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: content/base/test/csp/file_CSP_bug909029_none.html
> @@ +1,5 @@
> > +<!doctype html>
> > +<html>
> > +  <head>
> > +    <link rel='stylesheet' type='text/css'
> > +          href='file_CSP.sjs?testid=none_style&type=text/css' />
> 
> maybe put a comment here that indicates how this will load an empty
> stylesheet and noticed by the observer/examiner as either blocked or loaded.
> Took me a while to figure out that the content of this "file" is empty and
> it has no visual effect on the page when loaded.
> 

This is just a mock resource load, using the same script that test_CSP.html uses. I think the confusion here arises from the fact that the results of the inline style tests are visible. Most tests have any "visible" elements wrapped in a <div id="content" style="display:none">, but the CSP tests can't do that because they a) load iframes and b) need to check computed styles. Firefox does not compute styles for iframes that are children of a "display:none" element, so the convention in the CSP tests has been to just put the iframes outside of that div and make them visible.

I think the best solution would actually be to wrap these elements in a <div style="visiblity:hidden">. This maintains the convention used by all other tests, but Firefox will still compute styles on the hidden iframes so the CSP tests won't fail incorrectly.

> ::: content/base/test/csp/test_CSP_bug909029.html
> @@ +101,5 @@
> > +}
> > +
> > +function waitForFinish() {
> > +  // Wait for observer tests to finish
> > +  setTimeout(function() {
> 
> setTimeout is used here since the inline stuff may not have finished by the
> time the content loads are blocked, right?  If that's not the case, could
> you just put this check inside the observer?

It's the other way around - the inline tests are triggered on the load event of the iframe, so we know that when they are called the iframe has been loaded and its styles computed. It's the resource loads and subsequent observer notifications that I'm waiting for. I agree that it's best to avoid setTimeout whenever possible, though, and I'm rewriting the test now so it's not needed.
Attached patch 4.patch (obsolete) — Splinter Review
Addresses comments from previous review. I rewrote the tests so they no longer use setTimeout. I also added an entry to block this test from running on b2g. Re-flagging for review because the test changes were substantial.
Attachment #804783 - Attachment is obsolete: true
Attachment #808064 - Flags: review?(sstamm)
Comment on attachment 808064 [details] [diff] [review]
4.patch

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

r=me.  Might wanna run this through try to make sure the new test passes on all platforms.

::: content/base/test/csp/file_CSP_bug909029_none.html
@@ +1,5 @@
> +<!doctype html>
> +<html>
> +  <head>
> +    <link rel='stylesheet' type='text/css'
> +          href='file_CSP.sjs?testid=noneExternalStylesBlocked&type=text/css' />

Even though this is just a mock resource load, please put a comment in here to that effect (so I don't go look up the source for file_CSP.sjs because it's irrelevant).

::: content/base/test/csp/test_CSP_bug909029.html
@@ +57,5 @@
> +        window.testResult(testid,
> +                          /Blocked/.test(testid),
> +                          "resource blocked by CSP");
> +      } catch(e) {
> +      }

What would throw here?  Please explicitly handle expected exceptions, or turn them into test failures (if they are indeed a failure)

@@ +83,5 @@
> +
> +  // if any test is incomplete, keep waiting
> +  for (var v in window.tests)
> +    if(tests[v] == -1)
> +      return;

I think ECMAScript 6 lets you shrink this to:

if (window.tests.some(v => v == -1))
  return;

Just an option, clearly not necessary.
Attachment #808064 - Flags: review?(sstamm) → review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #13)

> ::: content/base/test/csp/test_CSP_bug909029.html
> @@ +57,5 @@
> > +        window.testResult(testid,
> > +                          /Blocked/.test(testid),
> > +                          "resource blocked by CSP");
> > +      } catch(e) {
> > +      }
> 
> What would throw here?  Please explicitly handle expected exceptions, or
> turn them into test failures (if they are indeed a failure)

The subject of csp-on-violate-policy is either an nsIURI (of the resource that violated the policy) or an nsISupportsCString (text of the violation report, only used for inline violation). In these tests, we use the observed effects of scripts on CSSOM to detect if inline scripts run or not, and so we never care in the case where the subject is an nsISupportsCString. The desired behavior here is simply to ignore the subject, which is what we do. I'm adding a comment to clarify this, but have not added any code to the catch clause because nothing is needed.
Attached patch 5.patchSplinter Review
r+ from sstamm. Try: https://tbpl.mozilla.org/?tree=Try&rev=3a187dd80076
Attachment #808064 - Attachment is obsolete: true
Attachment #821322 - Flags: review+
try looks good
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae8e54262630
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: