1045902 - warn users of CSP reflected-xss directive about Firefox XSS behavior

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Sid Stamm [:geekboy or :sstamm]]

When sites send the reflected-xss directive with their CSP, we should put a warning in the developer/web console about how Firefox interprets it may not line up with the spec.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Currently the csp-parser would log a warning to the console if it encounters the 'reflected-xss' directive during parsing [1]:
> "Couldn't process unknown directive ..."
basically the parser logs the same warning for all unknown directives; every directive not listed here [2].

Everything following 'reflected-xss' will be ignored till we hit the ending ";" for that directive. I am not sure if we want anything else than what we already have.

Or do you want a special warning, because 'reflected-xss' is in the spec, but we do *not* support it? At least not support it as of now.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPParser.cpp#780
[2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.h#78
[2] http://www.w3.org/TR/2014/WD-CSP11-20140211/#reflected-xss

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1045902_wip.patch

That should do what we want for this bug.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1045902_wip.patch

Facepalm - copy/paste error and fixed only one site and not the other - should be: notSupportingDirective :-)

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8465961 [details] [diff] [review]
bug_1045902_wip.patch

Garrett, wanna review this?

Comment 5

[Garrett Robinson [:grobinson]]

Comment on attachment 8465961 [details] [diff] [review]
bug_1045902_wip.patch

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

::: dom/locales/en-US/chrome/security/csp.properties
@@ +66,5 @@
>  # %1$S is the hostname in question and %2$S is the keyword
>  hostNameMightBeKeyword = Interpreting %1$S as a hostname, not a keyword. If you intended this to be a keyword, use '%2$S' (wrapped in single quotes).
> +# LOCALIZATION NOTE (notSupportingDirective):
> +# directive is not supported (e.g. 'reflected-xss')
> +notSupportingDirective = Not supporting directive '%1$S'. Directive and values will be ignored.

A thought: people might wonder *why* we are ignoring reflected-xss, and this messages does not explain that. However, I think it is sufficiently clear and for the average user, and it is usefully generic. People who want to know that should be able to find the answer online (i.e. in this bug).

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1045902_tests.patch

Garrett, I couldn't resist to write a testcase for that as well. Can you have a look? Should be pretty straight forward. Thanks!

Comment 7

[Garrett Robinson [:grobinson]]

Comment on attachment 8472731 [details] [diff] [review]
bug_1045902_tests.patch

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

I think you can get rid of a lot of boilerplate if you use SimpleTest.expectConsoleMessages.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1045902_tests_v2.patch

(In reply to Garrett Robinson [:grobinson] from comment #7)
> Comment on attachment 8472731 [details] [diff] [review]
> bug_1045902_tests.patch
> 
> Review of attachment 8472731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you can get rid of a lot of boilerplate if you use
> SimpleTest.expectConsoleMessages.

In general, I totally agree with you, but it seems that there is a problem related to the 'SENTINEL' on line
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#964
and hence it's not working correctly which causes my testcase to fail.

At the moment I don't want to invest the time to figure out what's causing the problem in detail. Since literally all other tests in devtools use the approach I am using to test console messages, I think you agree that we can accept the test the way it is right now. Can we agree on that?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1045902_v2.patch

Added reviewer to the comment. Carrying over r+ from grobinson.

Comment 10

[Garrett Robinson [:grobinson]]

Comment on attachment 8474761 [details] [diff] [review]
bug_1045902_tests_v2.patch

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

Agreed that you shouldn't waste time on SimpleTest.expectConsoleMessages if it's buggy. Other than the boilerplate, this test is good!

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cffcf3a6c904
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25aeae7a346

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cffcf3a6c904
https://hg.mozilla.org/mozilla-central/rev/d25aeae7a346

Comment 13

[Eric Shepherd [:sheppy]]

I've added dev-doc-needed since it may make sense to add a note somewhere in the CSP docs explaining what the deal is here. If it doesn't, we can remove the keyword.

Comment 14

[Jean-Yves Perrier [:teoli]]

I added a comment in: https://developer.mozilla.org/en-US/Firefox/Releases/34#Security_and_Networking

Comment 15

[u561511]

Is there any update on this? It doesn't appear any documentation has been modified as to why firefox returns this way, only that it does.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jacob Taylor from comment #15)
> Is there any update on this? It doesn't appear any documentation has been
> modified as to why firefox returns this way, only that it does.

We are working on a blogpost to cover that topic. Once posted, I will paste a link to the post within this bug.