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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: geekboy, Assigned: ckerschb)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla34
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CSP 1.1], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
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
Posted patch bug_1045902_wip.patch (obsolete) — Splinter Review
That should do what we want for this bug.
Posted patch bug_1045902_wip.patch (obsolete) — Splinter Review
Facepalm - copy/paste error and fixed only one site and not the other - should be: notSupportingDirective :-)
Attachment #8465960 - Attachment is obsolete: true
Comment on attachment 8465961 [details] [diff] [review]
bug_1045902_wip.patch

Garrett, wanna review this?
Attachment #8465961 - Flags: review?(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).
Attachment #8465961 - Flags: review?(grobinson) → review+
Posted patch bug_1045902_tests.patch (obsolete) — Splinter Review
Garrett, I couldn't resist to write a testcase for that as well. Can you have a look? Should be pretty straight forward. Thanks!
Attachment #8472731 - Flags: review?(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.
Attachment #8472731 - Flags: review?(grobinson) → review-
(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?
Attachment #8472731 - Attachment is obsolete: true
Attachment #8474761 - Flags: review?(grobinson)
Added reviewer to the comment. Carrying over r+ from grobinson.
Attachment #8465961 - Attachment is obsolete: true
Attachment #8474763 - Flags: review+
(Assignee)

Updated

5 years ago
Assignee: grobinson → mozilla
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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!
Attachment #8474761 - Flags: review?(grobinson) → review+
https://hg.mozilla.org/mozilla-central/rev/cffcf3a6c904
https://hg.mozilla.org/mozilla-central/rev/d25aeae7a346
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Keywords: dev-doc-needed

Comment 15

3 years ago
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.
(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.
You need to log in before you can comment on or make changes to this bug.