1075230 - ASSERTION: Second character needs to be '.' whenever host starts with '*', going to twitter.com

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Whenever I got to https://twitter.com/ I see two of:

###!!! ASSERTION: Second character needs to be '.' whenever host starts with '*': 'mHost[1] == '.'', file /home/dbaron/builds/ssd/mozilla-central/mozilla/content/base/src/nsCSPUtils.cpp, line 330

Thus, either:
 (1) something is wrong in our code, or
 (2) assertions are being misused to report problems with Web content rather than problems in our code

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1075230_twitter_bug.patch

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #0)
> Whenever I got to https://twitter.com/ I see two of:
> 
> ###!!! ASSERTION: Second character needs to be '.' whenever host starts with
> '*': 'mHost[1] == '.'', file
> /home/dbaron/builds/ssd/mozilla-central/mozilla/content/base/src/nsCSPUtils.
> cpp, line 330

David, thanks for bringing this problem to my attention.

The enforced CSP policy of https://twitter.com contains, amongst others, the following sources to be enforced:
> img-src https://* data:;

Interesting is, that 'https://*' is an equivalent for 'https:', which enforces images to be loaded only using https. So far, I haven't thought that someone might might craft a policy using 'https://*' to enforce https schemes rather than just using 'https:'. Nevertheless, it's completely valid syntax and we should fix that.

> Thus, either:
>  (1) something is wrong in our code, or

That is in fact the case, and we have to fix that bug in the csp enforcment part.

>  (2) assertions are being misused to report problems with Web content rather
> than problems in our code

No, assertions are not misused here. The csp parser (nsCSPParser.cpp) correctly parses https://* but the enforcment part in nsCSPUtils.cpp caused a problem. Luckily we used that assertion.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1075230_twitter_bug_test_updates.patch

I know it's not great to integrate that test in the existing tests for path-matching, but it was just the most convenient way and I don't think it's worth adding the overhead of adding additional files just for that test.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

The remaining question, should we uplift the patch to beta?
I would rather say no.

Reason is, our current code works correctly even though we raise an assertion, see:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#330
In the case of https://* where mScheme is "*", we basically create and empty string and check if
the uriHost endsWith with that empty string, which returns true and therefore works correctly.

Sid, any objections?

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8498125 [details] [diff] [review]
bug_1075230_twitter_bug_test_updates.patch

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

Why not TestCSPParser.cpp? This is fine to add to test_csp_path_matching, but really I think it belongs there.
http://mxr.mozilla.org/mozilla-central/source/content/base/test/TestCSPParser.cpp#371

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #4)
> Comment on attachment 8498125 [details] [diff] [review]
> bug_1075230_twitter_bug_test_updates.patch
> 
> Review of attachment 8498125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not TestCSPParser.cpp? This is fine to add to test_csp_path_matching,
> but really I think it belongs there.
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/
> TestCSPParser.cpp#371

The problem is that TestCSPParser.cpp only tests the parser (nsCSPParser) but not the enforcement of a policy (nsCSPUtils) where the assertion was triggered. While we can definitely add a test around here:
http://mxr.mozilla.org/mozilla-central/source/content/base/test/TestCSPParser.cpp#379
we also need a mochitest where we really confirm that csp is enforced correctly. In other words, the assertion is not triggered anymore.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8498124 [details] [diff] [review]
bug_1075230_twitter_bug.patch

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

This concerns me a little bit.  We should be following the spec algorithm as closely as possible:
http://www.w3.org/TR/CSP/#matching

Steps 5-9 say:
5. If the first character of the source expression's host is an U+002A ASTERISK character (*) and the remaining characters, including the leading U+002E FULL STOP character (.), are not a case insensitive match for the rightmost characters of uri-host, then return does not match.
6. If uri-host is not a case insensitive match for the source expression's host, then return does not match.
7. If the source expression does not contain a port and uri-port is not the default port for uri-scheme, then return does not match.
8. If the source expression does contain a port that (a) does not contain an U+002A ASTERISK character (*) and (b) does not represent the same number as uri-port, then return does not match.
9. Otherwise, return does match.

This makes me think of a bunch of non-nested conditional returns.  (Apologies for pseudocode.)

> // 5
> if (host[0] == '*' && urihost.EndsWith(host.substring(1)))  return false;
> 
> // 6
> if (urihost.EqualsIgnoringCase(host))  return false;
> 
> // 7
> if (!source.hasPort && !IsDefaultForScheme(urischeme, uriport)) return false;
> 
> // 8
> if (source.hasPort && port != '*' && uriport != port) return false;
> 
> // 9
> return true;

Maybe the spec needs more clarification about just "*" hosts, or step 5 should succeed if host.substring(1) is empty string?

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8498125 [details] [diff] [review]
bug_1075230_twitter_bug_test_updates.patch

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> The problem is that TestCSPParser.cpp only tests the parser (nsCSPParser)
> but not the enforcement of a policy (nsCSPUtils) where the assertion was
> triggered. While we can definitely add a test around here:
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/
> TestCSPParser.cpp#379

Makes sense, thanks.

> we also need a mochitest where we really confirm that csp is enforced
> correctly. In other words, the assertion is not triggered anymore.

Got it.  Putting this in test_csp_path_matching.html is fine (I agree, dumb to add another test file for this).  

Please add the new parsing test case to TestCSPPParser.cpp, put a comment in the test_csp_path_matching.html file near this new case (put the bug number there), and r=me.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1075230_twitter_bug_test_updates.patch

Updated both tests, one for the parser, and one for path-matching, where I added a comment including bug number. Carrying over r+.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1075230_twitter_bug.patch

As discussed on IRC, I followed the spec wherever possible and as tight as possible cleaning up unnecessary nested if clauses. Also added comments for the specific parts of the spec and how we enforce it.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8498326 [details] [diff] [review]
bug_1075230_twitter_bug.patch

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

r=me if you fix my spec compliance complaints or file a followup to fix this since it's not closely related to the immediate issue.

::: content/base/src/nsCSPUtils.cpp
@@ +305,5 @@
>    nsAutoCString scheme;
>    nsresult rv = aUri->GetScheme(scheme);
>    NS_ENSURE_SUCCESS(rv, false);
> +  if (!mScheme.IsEmpty() &&
> +      !mScheme.EqualsASCII(scheme.get())) {

This is not quite spec compliant (neither is the current implementation, though).  There's an HTTP/HTTPS matching procedure in 4.4.1: "the scheme of the protected resource’s URI is a case insensitive match for HTTP, and uri-scheme is not a case insensitive match for either HTTP or HTTPS"

This should be something like:
if (mScheme.IsEmpty()) {
  // 4.4.1
  if (selfscheme.EqualsASCII("http") &&
      !scheme.EqualsASCII("http") || !scheme.EqualsASCII("https")) {
    return false;
  }
  // 4.4.2
  if (!selfscheme.EqualsASCII("http") &&
      !selfscheme.EqualsASCII(scheme.get()) {
    return false;
  }
}

(I'm not sure where to get the protected resource's scheme off hand).
    
I noticed the existing implementation doesn't conform to this part of the spec either... please file another bug to fix compliance or find the protected resource's scheme and do it here (either is fine, just not sure how much work it would be to do it here).

@@ +342,5 @@
>      return false;
>    }
>  
> +  // 4.9) Path matching: If there is a path, we have to enforce
> +  // path-level matching, unless the channel got redirected, see:

These (port/path) are out of spec order.  If we fix spec compliance in this bug we should reorder 4.9 and 4.8.  Please either reorder here to be spec compliant or file that followup and one of us can do it there.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> The remaining question, should we uplift the patch to beta?
> I would rather say no.

I think we don't have to rush the fix.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9012ea962e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9536efa6b07b

Also filed bug 1076837, where we can update the source expression matching to follow the spec precisely.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b9012ea962e
https://hg.mozilla.org/mozilla-central/rev/9536efa6b07b