CSP change in behavior regards case sensitivity loading resources

RESOLVED FIXED in Firefox 35

Status

()

defect
P3
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: torsteina, Assigned: ckerschb, NeedInfo)

Tracking

({dev-doc-complete, regression, site-compat})

35 Branch
mozilla38
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox35+ fixed, firefox36+ fixed, firefox37+ fixed, firefox38+ fixed, relnote-firefox 35+)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36 OPR/26.0.1656.60

Steps to reproduce:

Trying to login using the Norwegian service Bankid.no on some of the online banks

1. Navigate, with console open, to
https://www.bankid.no/Hjelp-og-nyttige-verktoy/Nyttige-verktoy/BankID-20-uten-Java/
2. Select the  Test din BankID - autentisering link ( Test your BankID - authentication)
3. Wait for the service to load.
This login are used with most banks in Norway in addition to other government sectors.

In Firefox 35, I get an Error with CSP in Console not present in Firefox 34 or relevant browsers ( IE 11 and Chrome 39 )


Actual results:

You get a CSP error in Console due to missmatch in names between resources:
The link above will give you in Console:
Content Security Policy: The page's settings blocked the loading of a resource at https://tools.bankid.no/BankID ("connect-src https://csfe.bankid.no https://tools.bankid.no:443/bankid").


Expected results:

You should be able to load this without this CSP error when I compare to Firefox 34, IE 11 and Chrome 39.

We have checked against a couple of places, and the only difference we find are difference between upper or lowercase on urls.
URL's with all lowercase letters will load with no CSP warning.
(Reporter)

Updated

4 years ago
Severity: normal → major
Component: Untriaged → Security
Priority: -- → P3
(Reporter)

Comment 1

4 years ago
Looking at an earlier CSP issue I reported, I see now I placed this issue in wrong product.
Changing this and sorry for the additional emails
Component: Security → DOM: Security
Product: Firefox → Core

Comment 2

4 years ago
It looks like an issue with the parser.

Regression range:
good=2014-09-25
bad=2014-09-26
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1735ff2bb23e&tochange=9e3d649b80a2

Suspected bug:
Christoph Kerschbaumer — Bug 808292 - CSP: Implement path-level host-source matching
Blocks: 808292
Flags: needinfo?(mozilla)
Keywords: regression
So, first, this is a valid bug. I think we should not perform any normalization when we store a path internally (see patch). A policy of 'connect-src foo.com/bar/abc.js' should *not* allow the loading of 'foo.com/bar/ABC.js' because those are different files in fact.

The spec does *not* explicitly mention case sensitive matching for paths:
http://www.w3.org/TR/CSP2/#source-list-path-patching
but I think that's inferred.

So, in the specific testcase provided in comment 0, the policy that gets applied is:
> default-src 'none';
> style-src 'self';
> script-src 'self' 'unsafe-eval';
> connect-src 'self' https://tools.bankid.no:443/BankID;
> img-src data:;
> report-uri /CentralServerFEJS/a/report-error?cid=9FtSfwa6fgg6Zrbq;
> reflected-xss block;

In our implementation we then normalize 'BankID' and store 'bankid' internally.
Later, in the matching part the uri to load is:
https://tools.bankid.no/BankID 
where our path-matching algorithm returns false because 'BankID' != 'bankid'.

Dan, Sid, I think you are on the same page with me, right?
Flags: needinfo?(mozilla)
Attachment #8550423 - Flags: feedback?(sstamm)
Attachment #8550423 - Flags: feedback?(dveditz)
(Assignee)

Updated

4 years ago
Assignee: nobody → mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8550423 [details] [diff] [review]
bug_1122445_case_error_in_path.patch

Yes! We should be doing an exact match on paths, and we should match exactly what the site told us not some modified form of it (apart from the separate %-encoding issue).

feedback+ on the approach, but the actual patch shouldn't have commented-out code in it.
Attachment #8550423 - Flags: feedback?(dveditz) → feedback+
Comment on attachment 8550423 [details] [diff] [review]
bug_1122445_case_error_in_path.patch

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

Yeah, what Dan said.
Attachment #8550423 - Flags: feedback?(sstamm) → feedback+

Comment 6

4 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
 
> The spec does *not* explicitly mention case sensitive matching for paths:
> http://www.w3.org/TR/CSP2/#source-list-path-patching
> but I think that's inferred.

Noob question: do the specs need to be revised to mention this clarification?
[Tracking Requested - why for this release]: Breaking sites.
Duplicate of this bug: 1122653
Thanks for checking Boris, I will make sure to add/update tests.
(In reply to Loic from comment #6)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
>  
> > The spec does *not* explicitly mention case sensitive matching for paths:
> > http://www.w3.org/TR/CSP2/#source-list-path-patching
> > but I think that's inferred.
> 
> Noob question: do the specs need to be revised to mention this clarification?

I don't think the spec needs to be updated. Now that I think about it, it's pretty obvious how the behavior should be for path matching.
OS: Windows 7 → All
Hardware: x86_64 → All
OS: All → Windows 7
Hardware: All → x86_64
OS: Windows 7 → All
Hardware: x86_64 → All
Attachment #8550423 - Attachment is obsolete: true
Attachment #8552648 - Flags: review?(sstamm)
Comment on attachment 8552648 [details] [diff] [review]
bug_1122445_case_error_in_path.patch

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

There's probably other places where we need to stop mutating case.

Schemes:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#336
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#505

Ports:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#513 (numbers don't have a case)

All the downcase conversions should be in the comparing functions where we intend to do case-insensitive compares (not paths).  For example:
> return mScheme.EqualsASCII(scheme.get());
should be something like
> return mScheme.LowerCaseEqualsASCII(scheme.ToLowerCase().get());
http://mxr.mozilla.org/mozilla-release/source/content/base/src/nsCSPUtils.cpp#268

::: dom/security/nsCSPUtils.cpp
@@ -516,5 @@
>  void
>  nsCSPHostSrc::appendPath(const nsAString& aPath)
>  {
>    mPath.Append(aPath);
> -  ToLowerCase(mPath);

Why did we ever do this in the first place?
Attachment #8552648 - Flags: review?(sstamm) → review-
Comment on attachment 8552649 [details] [diff] [review]
bug_1122445_case_error_in_path_test_updates.patch

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

::: dom/base/test/csp/test_csp_path_matching.html
@@ +64,5 @@
>    ["blocked", "test1.example.com:8888/tests/dom/base/test/csp/file_csp_path_matching.py"],
> +
> +  // case sensitive matching of paths and files
> +  ["blocked", "test1.example.com/tests/dom/base/test/CSP/?foo=val"],
> +  ["blocked", "test1.example.com/tests/dom/base/test/csp/FILE_csp_path_matching.js?foo=val"],

Add a couple cases for mixed-case host matching and/or schemes.  Make sure those are case-insensitive matched.
Attachment #8552649 - Flags: review?(sstamm) → review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14)
> All the downcase conversions should be in the comparing functions where we
> intend to do case-insensitive compares (not paths).  For example:
> > return mScheme.EqualsASCII(scheme.get());
> should be something like
> > return mScheme.LowerCaseEqualsASCII(scheme.ToLowerCase().get());
> http://mxr.mozilla.org/mozilla-release/source/content/base/src/nsCSPUtils.
> cpp#268

You know what, let's do this in a different bug.  It's a different problem (potentially affects developers debugging a policy) and is not as important as fixing the path issue.

Just remove the ToLowerCase() in ::setPort() and stop downcasing the paths.  That should be sufficient for this bug.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14)
> There's probably other places where we need to stop mutating case.

I don't think so, because nsIURI normalizes scheme and host but not the path.
> Schemes:
> http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#336
> http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#505
> 
> Ports:
> http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.
> cpp#513 (numbers don't have a case)

Ok, I fixed this one - shouldn't make a difference because calling ToLowerCase on a number has no effect, but ok, no need to perform any operation that has no effect on the outcome.
 
> All the downcase conversions should be in the comparing functions where we
> intend to do case-insensitive compares (not paths).  For example:
> > return mScheme.EqualsASCII(scheme.get());
> should be something like
> > return mScheme.LowerCaseEqualsASCII(scheme.ToLowerCase().get());
> http://mxr.mozilla.org/mozilla-release/source/content/base/src/nsCSPUtils.
> cpp#268

I am not sure about that, but we can discuss that in a different bug. I think we should normalize the CSP as early as possible. But as I said, I am open for discussion, but not in this bug which needs to be uplifted.

> ::: dom/security/nsCSPUtils.cpp
> @@ -516,5 @@
> >  void
> >  nsCSPHostSrc::appendPath(const nsAString& aPath)
> >  {
> >    mPath.Append(aPath);
> > -  ToLowerCase(mPath);
> 
> Why did we ever do this in the first place?

Misconception, for some reason I had the impression that nsIURI normalizes paths as well. Now that I think about it - didn't make sense in the first place.
Attachment #8552648 - Attachment is obsolete: true
Attachment #8552685 - Flags: review?(sstamm)
Added more tests covering upper and lowercase matching for schemes and hosts. Carrying over r+ from sid.
Attachment #8552649 - Attachment is obsolete: true
Attachment #8552686 - Flags: review+
Comment on attachment 8552685 [details] [diff] [review]
bug_1122445_case_error_in_path.patch

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

Thanks, Chris.
Attachment #8552685 - Flags: review?(sstamm) → review+
Oh yes, we have to update that compiled code test:
https://treeherder.mozilla.org/logviewer.html#?job_id=5725391&repo=mozilla-inbound

Here we go!
Attachment #8552686 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
Attachment #8552746 - Flags: review?(sstamm)
Attachment #8552746 - Flags: review?(sstamm) → review+
Christoph, could you fill the uplift requests to aurora, beta & release? thanks
Flags: needinfo?(mozilla)
Comment on attachment 8552685 [details] [diff] [review]
bug_1122445_case_error_in_path.patch

Approval Request Comment
[Feature/regressing bug #]:
Path level host source matching for CSP - Bug 808292.

[User impact if declined]:
Web pages that define a CSP relying on uppercases within in the path part, get false positives. CSP blocks resources to be loaded where it shouldn't.

[Describe test coverage new/current, TreeHerder]:
We landed tests with the fix in this patch - on mc at the moment.

[Risks and why]:
Risk of uplift is very low since it only affects pages deploying CSP and in addition relying on the path part.

[String/UUID change made/needed]:
no
Flags: needinfo?(mozilla)
Attachment #8552685 - Flags: approval-mozilla-release?
Attachment #8552685 - Flags: approval-mozilla-beta?
Attachment #8552685 - Flags: approval-mozilla-aurora?
Attachment #8552685 - Flags: approval-mozilla-release?
Attachment #8552685 - Flags: approval-mozilla-release+
Attachment #8552685 - Flags: approval-mozilla-beta?
Attachment #8552685 - Flags: approval-mozilla-beta+
Attachment #8552685 - Flags: approval-mozilla-aurora?
Attachment #8552685 - Flags: approval-mozilla-aurora+
I couldn't reproduce the initial issue using the website from URL.

All Firefox versions with or without the fix (Firefox 35.0, Firefox 35.0.1, Nightly 38.0a1 2015-01-22 and 2015-01-23) in all platforms (Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5) have the same CSP error in the console:
Content Security Policy: Not supporting directive 'reflected-xss'. Directive and values will be ignored.

Other pages from that websites, such as "Test din nettleser" have similar errors:
Content Security Policy: The page's settings blocked the loading of a resource at self ("script-src *").

In case the website was modified meanwhile, is there another link we could use to test this fix? Thank you!
Flags: needinfo?(mozilla)

Comment 29

4 years ago
Probabli because the webmaster changed https://tools.bankid.no/BankID to https://tools.bankid.no/bankid
(In reply to Petruta Rasa [QA] [:petruta] from comment #28)
> I couldn't reproduce the initial issue using the website from URL.
> 
> All Firefox versions with or without the fix (Firefox 35.0, Firefox 35.0.1,
> Nightly 38.0a1 2015-01-22 and 2015-01-23) in all platforms (Ubuntu 12.04 LTS
> 32-bit, Windows 7 64-bit and Mac OS X 10.9.5) have the same CSP error in the
> console:
> Content Security Policy: Not supporting directive 'reflected-xss'. Directive
> and values will be ignored.

That warning message is intended to be there because FF is not supporting reflected-xss. That's not a bug.

> Other pages from that websites, such as "Test din nettleser" have similar
> errors:
> Content Security Policy: The page's settings blocked the loading of a
> resource at self ("script-src *").

Hard to evaluate at this point, I would need more context to see what the actual error is here.

> In case the website was modified meanwhile, is there another link we could
> use to test this fix? Thank you!

I think Loic pointent out in Comment 29 that the website was modified. I see there is a duplicate bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1122653#c1
I don't know what website this is, but that *might* help to reproduce.

You definitely see the error if you run the attached testcase to this bug.
If there is no other way to reproduce and you need a page to verify I could set up a test up on my homepage.
Flags: needinfo?(mozilla)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> > In case the website was modified meanwhile, is there another link we could
> > use to test this fix? Thank you!
> 
> I think Loic pointent out in Comment 29 that the website was modified. I see
> there is a duplicate bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1122653#c1
> I don't know what website this is, but that *might* help to reproduce.

I already checked the duplicate bug, but unfortunately there's no url provided there.

> You definitely see the error if you run the attached testcase to this bug.

The pages https://www.bankid.no/Hjelp-og-nyttige-verktoy/Nyttige-verktoy/BankID-20-uten-Java/ (from comment 0) and https://www.bankid.no/Hjelp-og-nyttige-verktoy/Nyttige-verktoy/Test-din-BankID-20-uten-Java---autentisering/ (from bugzilla's URL field) no longer have the error. Are you referring to other testcase?

> If there is no other way to reproduce and you need a page to verify I could
> set up a test up on my homepage.

If there's no other known url that had the issue, we would appreciate a page where we could test this. Thanks!
(Reporter)

Comment 32

4 years ago
Thank you for fixing and getting this out in 35.0.1.

Unfortunately I do not have a other known URL for keeping as reference link for later checks.
Many of the sites with the original problem, have changed their path's to avoid the issue.

Comment 34

4 years ago
Could you test with a clean profile, please.
vhttps://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

If the issue is still here, file a new bug, please.
Flags: needinfo?(runar)
You need to log in before you can comment on or make changes to this bug.