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

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: yeukhon, Assigned: yeukhon)

Tracking

(Blocks 1 bug)

Trunk
mozilla29
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Posted file 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
Blocks: CSP
Component: Untriaged → Security
Product: Firefox → Core
I would definitely argue that Firefox should follow the spec here, especially since the other browsers allow case insensitive directive names.
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();
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → yeukhon
Status: NEW → ASSIGNED
+ unbittrot patch
Attachment #8333665 - Attachment is obsolete: true
Attachment #8361833 - Flags: review?(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 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.
Attachment #8361833 - Flags: review?(sstamm)
Summary: CSP directive names should be case-insensitive → CSP directives and source expressions should do case-insensitive matching and comparison
Posted patch csp_case_insentitive_v2.patch (obsolete) — Splinter Review
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.
Attachment #8361833 - Attachment is obsolete: true
Attachment #8361885 - Flags: feedback?(sstamm)
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 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;
}
Attachment #8361885 - Flags: feedback?(sstamm)
Posted patch csp_case_insentitive_v3.patch (obsolete) — Splinter Review
+ 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(..)
Attachment #8361885 - Attachment is obsolete: true
Attachment #8361940 - Flags: review?(sstamm)
+ Replace all toUpperCase to toLowerCase (and change literals)

+ Compact style fix

Building try:  https://tbpl.mozilla.org/?tree=Try&rev=a2c54a3e87de
Attachment #8361940 - Attachment is obsolete: true
Attachment #8361940 - Flags: review?(sstamm)
Attachment #8361953 - Flags: review?(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.
Attachment #8361953 - Flags: review?(sstamm) → review+
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.
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
Flags: in-testsuite+
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/c04f78be70b8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.