587377 - CSP keywords "'self'" and "'none'" are easy to confuse with host names "self" and "none"

Status: RESOLVED FIXED

(Core :: Security, enhancement)

Description

[Devdatta Akhawe [:devd]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.8) Gecko/20100723 Ubuntu/10.04 (lucid) Firefox/3.6.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a3pre) Gecko/20100630 Minefield/3.7a3pre

It is possible for developers to write the policy allow self instead of allow 'self'. Firefox doesn't understand the policy without quotes and drops back to allow 'none'.  This is a very hard thing to debug currently : the report says allow 'none' directive violated which would confuse the developer further.
 




Reproducible: Always

Steps to Reproduce:
1. change a CSP policy to allow self and go to the site
2.
3.

Comment 1

[Devdatta Akhawe [:devd]]

There are multiple options here :
1. Debugging warnings when security.csp.debug was set to true ("Did you mean allow 'self'?")

2. Saying 'parse error' in report sent to report-uri instead of 'allow 'none' violated'. 

3. Changing grammar to accept allow self and allow none;  the quotes  form is tiresome. Note that all other src values always require a schema prefix followed by a ':'. 

I personally like 3.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Hm, I think we should do 1.  We discussed 3 before, but to reduce collisions between keywords and actual host names, it was decided to use punctuation of some sort to identify keywords.

Comment 3

[Devdatta Akhawe [:devd]]

I know but looking at how easy it is to mess up I really think as a matter of usability this should be done.

after checking the spec grammar, it seems the current behavior for allow self is wrong. allow self shouldn't really be considered a parsing error (to cause fall back on allow 'none') - allow self should result in allow http://self being enforced.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Does the policy "allow self" cause a parse error?  I didn't read the spec that way, and I'm pretty sure that our implementation doesn't interpret it that way -- it should do what you say and result in enforcing "allow http://self:80".  If the nightlies do something other than that, it's a bug, and we should fix it.

Comment 5

[Devdatta Akhawe [:devd]]

hmm ... I updated trunk and it doesn't cause a parse error. sorry :)

Anyways - how about changing the violation report (sent to report-uri)  'violated-directive":"allow http://self"' (or whatever the inferred schema is)

this would work for other browsers too - adding an option that gives a warning seems to be a very mozilla specific solution.

Comment 6

[Devdatta Akhawe [:devd]]

( currently it says "violated-directive":"allow self" which can cause confusion.)

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Rephrasing the subject to reflect the actual issue: confusion of 'self' with self and 'none' with none.

I think we could add some helpful hinting debug output for developers.  Dev: is this what you had in mind waaay back when you filed this?

Comment 8

[Devdatta Akhawe [:devd]]

yeah sounds right

Comment 9

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning.patch

Comment 10

[Yeuk Hon Wong [:yeukhon]]

First patch to Firefox component.

I have two issues with my submissions:

1) I want to write tests. I have tested this with a Python server running (https://gist.github.com/yeukhon/7272851) but I checked mochitest and I am not sure how to test error messages. 

2) I was hoping to tell developers exactly which directive(s) is having this issue. But the function I am working with is a method on source expression instance, and I don't think the instance has any knowledge of the directive it belongs to.

Comment 11

[Devdatta Akhawe [:devd]]

As you said, tests would be great. I also think you want localized messages, not just english. I think the tests and patches in Bug 770099 should show you how to write mochitests and localized strings for the patch. Also, I haven't seen this code in a while: this patch works even if the user types in SELF or sElF etc, right?

I wonder if a simpler message would be enough. "Interpreted self as http://self. Did you mean 'self' (with quotes)?"

I agree that getting the exact directive might be difficult. I think it is fine to just print this warning for now: this  case should be rare enough that I don't think the extra coding for getting the directive is justified.

Comment 12

[Devdatta Akhawe [:devd]]

Comment on attachment 827031 [details] [diff] [review]
587377_self_none_warning.patch

Adding mgoodwin for feedback since he worked a lot on improving the web console warnings for CSP.

Comment 13

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning_v2.patch

As stated in the commit message, the test doesn't work unless you refresh the page manually.

see this pastebin for the full execution:
http://pastebin.mozilla.org/3452471

Comment 14

[Devdatta Akhawe [:devd]]

Comment on attachment 829093 [details] [diff] [review]
587377_self_none_warning_v2.patch

The basic idea looks good to me. There are likely coding style/guidelines etc that you will need to follow to get this in. I don't have any idea about that; flagging Sid for review.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 829093 [details] [diff] [review]
587377_self_none_warning_v2.patch

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

Your commit message says "There is a mochitests but the test doesn't actually work."  What's this mean?  Are you not finished yet, or is that comment wrong?  There's also a "cd" in your commit message.  You can probably clean up everything except the first line of the commit message.

Please address the style comments.  I'd like to see another patch.

::: browser/devtools/webconsole/test/browser_webconsole_bug_587377_hostname_might_be_keyword.js
@@ +3,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-bug-587377-hostname-might-be-keyword.html";
> +const CSP_WARN_SELF_MSG = "Interpreted self as http://self. Did you mean 'self' (with quotes)?"

Be sure to update this const string if changing the csp.properties value.

::: content/base/src/CSPUtils.jsm
@@ +71,5 @@
>                                  R_KEYWORDSRC.source,  'i');
>  
> +// Keywords without quotes can be used as host name
> +const R_QUOTELESS_KEYWORDS = new RegExp ("^(self|unsafe-inline|unsafe-eval|" + 
> +                                "inline-script|eval-script|none)$", 'i');

Trailing whitespace, please remove it.  Also please line up the strings that are concatenated:

const X = new RegExp ("blah" +
                      "blah", 'i');

@@ +1349,5 @@
> +
> +    // Keywords without quotes are valid host names
> +    // Warn developers about this confusion
> +    match = R_QUOTELESS_KEYWORDS.exec(aStr);
> +    if (match) {

Since you're not usign the variable 'match', please use "test()" and just put the test inside the if header:

if (R_QUOTELESS_KEYWORDS.test(aStr)) {
  cspWarn(...);
}

@@ +1352,5 @@
> +    match = R_QUOTELESS_KEYWORDS.exec(aStr);
> +    if (match) {
> +        cspWarn(aCSPRep,
> +                CSPLocalizer.getFormatStr("hostNameMightBeKeyword",
> +                                           [aStr, aStr.toLowerCase()]));        

Indentation should be consistent (two spaces, not four).
Trailing whitespace.  Please remove it.
Also the [aStr should line up with "host on the previous line

@@ +1354,5 @@
> +        cspWarn(aCSPRep,
> +                CSPLocalizer.getFormatStr("hostNameMightBeKeyword",
> +                                           [aStr, aStr.toLowerCase()]));        
> +    };
> + 

Trailing whitespace.  Please remove it.
No semicolon after the if body close brace.

::: dom/locales/en-US/chrome/security/csp.properties
@@ +57,5 @@
>  # eval is a name and should not be localized.
>  scriptFromStringBlocked = An attempt to call JavaScript from a string (by calling a function like eval) has been blocked
> +# LOCALIZATION NOTE (hostNameMightBeKeyword):
> +# %1$S is the host name in question and %2$S is the keyword
> +hostNameMightBeKeyword = Interpreted %1$S as http://%1$S. Did you mean '%2$S' (with quotes)?

This is a bit confusing and needs say what to do, not ask questions.  Maybe go with something like: 
"Interpreting %1$S as a hostname, not a keyword.  If you intended this to be a keyword, use %2$S (wrapped in single quotes)."

Comment 16

[Yeuk Hon Wong [:yeukhon]]

Thanks sid for the thorough review. I will definitely brush up the style.


> > +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-bug-587377-hostname-might-be-keyword.html";
> +const CSP_WARN_SELF_MSG = "Interpreted self as http://self. Did you mean 'self' (with quotes)?"Be sure to update this const string if changing the csp.properties value.

What do you mean by "if changing the csp.properties value"?


> Your commit message says "There is a mochitests but the test doesn't actually work."  What's this mean?  Are you not finished yet, or is that comment wrong? 

Yes. There seems to be something wrong with my test. When I tested my code, it looked like the CSP warning was not caught and I had to refresh the page in order to pass the test. One theory is that the CSP warning was delivered before webconsole was enabled and opened.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

(In reply to yeukhon from comment #16)
> What do you mean by "if changing the csp.properties value"?

If you edit the value in csp.properties (as suggested in my review), you'll need to update the tests to match that value.  There might be a way to import the string bundle into the mochitest so you don't have to edit two strings if you change the language, but I'm not sure.

Comment 18

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning_v3.patch

This patch has a working mochitest browser test, addressed style issue and revised the warning message.

Please let me know what else needs to be fixed and thank you.

Comment 19

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning_v3.patch

Sorry for this spam and invalidated the other one so quickly. 

1. I have added the missing test files. The test now works.
2. Addressed style issue.
3. Revised the warning message.

I hope I have everything in this patch :)

Comment 20

[Devdatta Akhawe [:devd]]

Comment on attachment 830623 [details] [diff] [review]
587377_self_none_warning_v3.patch

The patch looks good to me. I can't review code since I don't have review privilege.

One question: have you considered creating a list of keywords and using that generate the R_KEYWORDSRC and R_QUOTELESS_KEYWORDS? Tomorrow, if CSP adds a new keyword 'foo', we will need updates for both regexes, which seems unnecessary.

Comment 21

[Yeuk Hon Wong [:yeukhon]]

(In reply to Devdatta Akhawe [:devd] from comment #20)
> Comment on attachment 830623 [details] [diff] [review]
> 587377_self_none_warning_v3.patch
> 
> The patch looks good to me. I can't review code since I don't have review
> privilege.
> 
> One question: have you considered creating a list of keywords and using that
> generate the R_KEYWORDSRC and R_QUOTELESS_KEYWORDS? Tomorrow, if CSP adds a
> new keyword 'foo', we will need updates for both regexes, which seems
> unnecessary.

Regarding generating R_QUOTELESS_KEYWORDS: the issue is that in this patch I am also considering deprecated keywords (inline-script and eval-script) as well as 'none'. The 'none' is not a CSP keyword source but just a reserved host keyword that matching nothing. This suggestion will require how the security team feels about it. And the deprecated keywords are checked individually in the codebase, not part of any regexps irrc. Initially I had a dictionary/associated array if you look at 1st patch but I used regex because I try to be consistent. Thoughts? Comments?

Thanks for the review!

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 830623 [details] [diff] [review]
587377_self_none_warning_v3.patch

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

Almost there! But there's one thing:
I'm worried about the string comparison to a string that will get localized.  Can you investigate if it's possible to use CSPLocalizer to generate that const?

(You could also make the test more robust by adding valid hosts and quoted keywords (that wouldn't cause a warning), but I'm not sure that's necessary given this is a relatively simple patch.)

::: browser/devtools/webconsole/test/browser_webconsole_bug_587377_hostname_confusion.js
@@ +7,5 @@
> +
> +// Tests CSP hostname confusion warning is displayed in the Web Console
> +
> +const TEST_HOSTNAME_CONFUSION = "http://example.com/browser/browser/devtools/webconsole/test/test-bug-587377-hostname-confusion.html";
> +const CSP_SELF_WARNING_MSG = "Interpreting self as a hostname, not a keyword. If you intended this to be a keyword, use 'self' (wrapped in single quotes)."

Put a comment here that tells the reader to change this value if csp.properties gets changed.  This test will fail in other localized builds because the strings won't match.  Bonus points if you can use CSPLocalizer to automatically generate this string from csp.properties...

::: content/base/src/CSPUtils.jsm
@@ +1350,5 @@
> +    // Raise a CSP warning in the web console to developer to check his/her intent.
> +    if (R_QUOTELESS_KEYWORDS.test(aStr)) {
> +      cspWarn(aCSPRep,
> +              CSPLocalizer.getFormatStr("hostNameMightBeKeyword",
> +                                        [aStr, aStr.toLowerCase()]));

You can probably get by with two lines instead of three here.  :)

Comment 23

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning_v4.patch

This patch imports CSPUtils.jsm in webconsole/tests/head.js. For documentation purpose, webconsole's head.js already includes its own l10n localizer called WCU_l10n, but I think importing CSPLocalizer from CSPUtils makes the code easier to read.

I have also squashed three lines into two lines like you asked. I have added some comments in the test file to remind myself (and possibly to other contributors) what the heck I was doing in the test.

Two questions for sid:

1) This patch is written based on older parent (a couple weeks old already). I think after review+ I will update my patch to work with the latest moz-central, is that necessary?

2) I just learned from grobinson that "the messages in the web console are a subset of message sent to nsIConsoleService."[1]

So should we determine what kind of tests should go into devtools/webconsole and what goes under content/base/src/test/csp ? This is actually my first contribution so I just went ahead and did this in webconsole test.



[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=918397

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8338210 [details] [diff] [review]
587377_self_none_warning_v4.patch

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

Looks great.

To your questions:

1. There is a little bitrot from the script nonce bug landing and the age of the patch (sorry), but it's trivial to update and we don't need you to ask for another review to correct the bitrot.  
2. Please move the tests to content/base/test/csp before checking this in.  If it is non-trivial to do (major edits or something), please flag me for a follow up, otherwise you're good to go.

Comment 25

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning_v5.patch

Yay! Try access granted. First try here: https://hg.mozilla.org/try/rev/8493ae0f6419

+ Use localizer in test instead of hardcoding test string.
+ move tests to content/base/test/csp

Once positive result has come I will request a check-in.

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8348310 [details] [diff] [review]
587377_self_none_warning_v5.patch

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

You can carry over the r=geekboy since you have an r+ on the previous round.

Comment 27

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning_v6.patch

+ A hackish way to get around some race condition that causes test runner to complain about SimpleTest.finish() has called multiple time. 

+ Added back the waitForExplicitFinish

- B2G mochitest-1 will not pass because of this https://bugzilla.mozilla.org/show_bug.cgi?id=951895#c10

All desktop builds are passing: https://tbpl.mozilla.org/?tree=Try&rev=afa962d88898

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

Now that bug 951895 is fixed, do the tests pass on b2g?

Comment 29

[Sid Stamm [:geekboy or :sstamm]]

Testing it out: https://tbpl.mozilla.org/?tree=Try&rev=5e16f1e3cd2c

Comment 30

[Sid Stamm [:geekboy or :sstamm]]

Looks like it still times out.  Garrett: any chance you've got ideas?

Comment 31

[Yeuk Hon Wong [:yeukhon]]

Sorry guys. I have been super busy these two weeks. 

For reference, this is my try from last week (added dump() to see what was being captured). https://tbpl.mozilla.org/?tree=Try&rev=097ffff77e1a


Now I just ran another build with security.csp.speccompliant set to true and the test is green,

https://tbpl.mozilla.org/?tree=Try&rev=c9e4f047bca3

I will do 2nd build now and if this build becomes green when I get up tomorrow I will do a full build.

Comment 32

[Yeuk Hon Wong [:yeukhon]]

This build https://tbpl.mozilla.org/?tree=Try&rev=c236850d07ba looks good. I am now doing a full build here: https://tbpl.mozilla.org/?tree=Try&rev=e2147e0a6096

Comment 33

[Yeuk Hon Wong [:yeukhon]]

Just realize I have forgotten to run all mochitests. Here is the correct build for all tests: https://tbpl.mozilla.org/?tree=Try&rev=a7450f92ccf5

Comment 34

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning_v7.patch

+ turn security.csp.speccompliant on

Builds are green now.

Comment 35

[Yeuk Hon Wong [:yeukhon]]

Comment on attachment 8360838 [details] [diff] [review]
587377_self_none_warning_v7.patch

Never mind. I have discovered some flaws in this patch, mainly I am using the wrong names for variables. Sorry for the email noise. Removed checkin request.

Comment 36

[Yeuk Hon Wong [:yeukhon]]

Attachment: 587377_self_none_warning_v8.patch

try looks good: https://tbpl.mozilla.org/?tree=Try&rev=32317c904053

+ turn on security.csp.speccompliant on

+ renamed variables

Comment 37

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f71357d7de1

Comment 38

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/1f71357d7de1

Comment 39

[Garrett Robinson [:grobinson]]

I'm guessing the earlier test errors were being caused because earlier patches called SimpleTest.finish before SpecialPowers.postConsoleSentinel. This was fixed in the latest patch.