938652 - CSP directives and source expressions should do case-insensitive matching and comparison

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Yeuk Hon Wong [:yeukhon]]

Attachment: server.py

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131111154639

Steps to reproduce:

I raised this question on webappsec and #security [1]

I first noticed this when I was implementing a validator in Python. Chrome and Opera allows directive names be case-insensitive. 

Example: DEFAULT-SRC 'self';  is a valid CSP policy.
See attachment: launch the demo site on port 9999.

On #security, dchan pointed out that the ABNF spec [2] states the following:

>       ABNF strings are case insensitive and the character set for these
      strings is US-ASCII.

Also, I believe the security engineering team is planning to write CSP parser in C++ in the future. The fix to this bug is trivial. The cost-benefit at this point is debatable given all the tutorials I have read always spell directive-names in lower-case. Yet, the spec references the rule.

Should we enforce this in Firefox right now to confront with the spec? If we should, I will submit a patch to enforce case-insensitive rule.

[1]: http://lists.w3.org/Archives/Public/public-webappsec/2013Oct/0101.html
[2]: http://www.w3.org/TR/CSP/#normative-references

Comment 1

[Ian Melven :imelven]

I would definitely argue that Firefox should follow the spec here, especially since the other browsers allow case insensitive directive names.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Should be an easy fix by converting all directive names to lower case when parsing:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#297
and
http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#555

- var dirname = dir.split(/\s+/)[0];
+ var dirname = dir.split(/\s+/)[0].toLowerCase();

Comment 3

[Yeuk Hon Wong [:yeukhon]]

Attachment: bug_938652_csp_directive_case_insensitive_v1.patch

Yes the fix is very trivial; thanks for looking at the source code, sid.

In addition to the changes proposed, a quick unit test is added in the patch. Following is the test output;

> 0:07.63 TEST-PASS | /home/vagrant/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/content/base/test/unit/test_csputils.js | [test_bug938652_directive_case_insensitive : 926] true == true
>  0:07.63
>  0:07.63 TEST-PASS | /home/vagrant/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/content/base/test/unit/test_csputils.js | [test_bug938652_directive_case_insensitive : 927] false == false

I also check the false condition asserting that only the self policy is enforced (I hope I didn't misinterpret CSPRep.fromString).


Finally, there are several places in CSPUtil.jsm where we are deliberately checking case-insensitive. E.g:

>  // check for 'self' (case insensitive)
>  if (aStr.toUpperCase() === "'SELF'") {

I think it is also safe to assume we can just use .toLowerCase() at the point we get the list of source expressions. But this is a separate ticket and might be unnecessary at this point. Let me know, low priority bug like this is easy for newbie like me to fix.

Comment 4

[Yeuk Hon Wong [:yeukhon]]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=e6ebd0151367

Comment 5

[Yeuk Hon Wong [:yeukhon]]

Attachment: bug_938652_csp_directive_case_insensitive_v2.patch

+ unbittrot patch

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

The spec is a little ambiguous about case sensitivity.  There are a few places where it's clear enough ('none', 'self', schemes, hosts) but does not specifically define what case insensitive means.  I think for the purpose of CSP toLowerCase on the whole policy should be sufficient (CSP seems limited to ASCII), but that needs to be clarified in the spec.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8361833 [details] [diff] [review]
bug_938652_csp_directive_case_insensitive_v2.patch

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

Aside from case-insensitive directive names, we should probably ensure the following things are also matched/equals() case insensitively:
* keywords
* hostnames
* schemes

One option is to downcase the whole policy, but that may have unintended side-effects.  Keywords are already matched case insensitively and stored.  This patch makes directives match case-insensitively.  But to do this completely, you should probably update CSPSource.equals() and CSPHost.equals() to compare hosts and schemes case-insensitively.

Comment 8

[Yeuk Hon Wong [:yeukhon]]

Attachment: csp_case_insentitive_v2.patch

Probably best to get a preliminary review/feedback on this? I probably should add new tests to demonstrate case-insensitive checks. My local build looks good with tests. Thanks.

Comment 9

[Yeuk Hon Wong [:yeukhon]]

Ah sorry, this does not include the previous patch, since I am not qfolding the two right now. I hope that's not a big issue for feedback.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8361885 [details] [diff] [review]
csp_case_insentitive_v2.patch

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

You probably also want to edit CSPSource.equals so the schemes are compared case insensitively (CSPUtils.jsm:1577).

While you're editing the file, could you go through and change all the toUpperCase() calls to be toLowerCase() so that we're consistent?  (You'll have to change the string literals to be lower case too.)

::: content/base/src/CSPUtils.jsm
@@ +1070,5 @@
>      // XXX (sid): I think we can make this more efficient
>      var sortfn = function(a,b) {
> +      _a = a.toString().toLowerCase();
> +      _b = b.toString().toLowerCase();
> +      return _a > _b;

you can just do this all in one line:
return a.toString.toLowerCase() > b.toString.toLowerCase();

@@ +1737,2 @@
>          return false;
> +      }

Same here, you can simplify:

if (this._segments[i].toLowerCase() !=
    that._segments[i].toLowerCase()) {
  return false;
}

Comment 11

[Yeuk Hon Wong [:yeukhon]]

Attachment: csp_case_insentitive_v3.patch

+ qfold with previous patch, so it contains all the fixes we want to have

+ expanded the unit tests by testing CSPHost, CSPSource and CSPSource list equality comparison, and CSPRep.permits(..)

Comment 12

[Yeuk Hon Wong [:yeukhon]]

Attachment: csp_case_insentitive_v4.patch

+ Replace all toUpperCase to toLowerCase (and change literals)

+ Compact style fix

Building try:  https://tbpl.mozilla.org/?tree=Try&rev=a2c54a3e87de

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8361953 [details] [diff] [review]
csp_case_insentitive_v4.patch

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

r=me  

I noticed your try run only did mochitest-1 (not xpcshell tests, which is what you're editing in this patch) so I ran the content/base/test/unit XPCShell tests and the CSP mochitests locally on linux64 and they seemed fine.

Comment 14

[Yeuk Hon Wong [:yeukhon]]

Thanks sid. I have already ran a full try last week.
https://tbpl.mozilla.org/?tree=Try&rev=c2234fb0b766

All green. I will request a check-in after fixing some style and mispell.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Why wait?  I minorly tweaked some comments and fixed the upcase oops in test_csputils.js.

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c04f78be70b8

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c04f78be70b8