741019 - a trailing slash in a CSP source host causes it to be ignored (per spec), this should be logged with a CSPWarning()

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Ian Melven :imelven]

The character / is disallowed in a host expression in a CSP policy (per the CSP spec).

For example, with this directive :

style-src 'self' http://example.com https://foo-dev.example.com/;

foo-dev.example.com won't be added as an allowed source, and loading a CSS file from it will be denied.

This is a pretty easy mistake to make as I saw today. Right now we just do 
a CSPDebug() if there's an invalid character, see 
http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#1318

There's lots of CSPDebug level output and it's easy to miss this message when you're trying to see why your policy doesn't work, I found today. After some discussion with Sid and Tanvi, it would be nice to make these a CSPWarning() - but not a CSPError() as the spec dictates if a host is invalid, it's dropped and the rest of the valid hosts are kept and CSP processing continues. 

This seems like a good first bug. I would suggest in addition to the CSPDebug() call that's there, doing a CSPWarning() if the last character of the rejected CSP source is a slash, saying that host <hostname-ending-in-slash> will be ignored, due to its ending with a slash.

Comment 1

[Tara]

Hi,

I'm a student at Cal Poly Pomona looking for open source bugs to fix for a project my partner and I are working on. May I be assigned this bug?

Comment 2

[Tara]

Actually, I'm sorry. We are not allowed to work on a bug that is marked as "trivial". Sorry I missed that. 


(In reply to Tara from comment #1)
> Hi,
> 
> I'm a student at Cal Poly Pomona looking for open source bugs to fix for a
> project my partner and I are working on. May I be assigned this bug?

Comment 3

[Alan Yeo [:alanyeojw]]

Attachment: add if statement to check for trailing slash in host string

Comment 4

[Ian Melven :imelven]

Comment on attachment 611921 [details] [diff] [review]
add if statement to check for trailing slash in host string


this is pretty close but the warning will never happen in the patch as it is.

a url ending in a slash will error out in the previous block :

   // host string must be LDH with dots and stars.
   var invalidChar = aStr.match(/[^a-zA-Z0-9\-\.\*]/);
   if (invalidChar) {
     CSPdebug("Invalid character '" + invalidChar + "' in host " + aStr);
     return null;
   }

and this block won't be reached. you need to check for the slash inside the if (invalidChar) block.

Comment 5

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #4)
> Comment on attachment 611921 [details] [diff] [review]
> add if statement to check for trailing slash in host string
>
> and this block won't be reached. you need to check for the slash inside the
> if (invalidChar) block.

'this block' meaning the one inside the if (trailingSlash) block in your patch.

Comment 6

[Alan Yeo [:alanyeojw]]

Attachment: resubmit patch based on feedback

Comment 7

[Ian Melven :imelven]

Comment on attachment 611970 [details] [diff] [review]
resubmit patch based on feedback

looks good, i would maybe change the comment from // host string must not have trailing slash to // host string with a trailing slash is a possibly common error, do an additional warning in this case 

feedback?'ing to sid - Sid, do we need a mochitest for this ?

Comment 8

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #7)
> Comment on attachment 611970 [details] [diff] [review]
> resubmit patch based on feedback
> 
> looks good, i would maybe change the comment from // host string must not
> have trailing slash to // host string with a trailing slash is a possibly
> common error, do an additional warning in this case 
> 
> feedback?'ing to sid - Sid, do we need a mochitest for this ?

actually after further discussion between Sid, Tanvi and myself, we think the patch should be a little different, sorry for the confusion !

the logic should be : 

1. check for invalid chars
2. if there's an invalid char, do the CSPDebug() that there's an invalid char
3. if there's an invalid char, check that the rest of the host is valid EXCEPT for a trailing slash - in this case issue a warning as well

the difference from your patch is that the check is that the only invalid char is a trailing slash (apart from the slash the rest of the host is valid), instead of there's a invalid character (or many) somewhere in the host AND a trailing slash. 

a fine distinction, but then the warning will only happen when the trailing slash is the only bad char, which is the case we're concerned about. thanks !

Comment 9

[Ian Melven :imelven]

Comment on attachment 611970 [details] [diff] [review]
resubmit patch based on feedback

Removing feedback+ since this needs another pass.

Comment 10

[Ian Melven :imelven]

After discussing with Sid, this could use an xpcshell test instead of a Mochitest. He suggested adding it to the existing CSP xpcshell tests: see http://mxr.mozilla.org/mozilla-central/source/content/base/test/unit/test_csputils.js

Comment 11

[Alan Yeo [:alanyeojw]]

Attachment: resubmit patch

Comment 12

[Alan Yeo [:alanyeojw]]

Attachment: add test for trailing slash in hosts

Comment 13

[Ian Melven :imelven]

Comment on attachment 615159 [details] [diff] [review]
resubmit patch

This looks pretty good with the following caveat : if there is more than one invalid character, with the current regex, only the first invalid character will be returned. In this case though, we only want to display the warning if the slash is the ONLY invalid character in the host and is the last character, which is what your change will do. 

Sid, does this patch look reasonable to you ?

Comment 14

[Ian Melven :imelven]

Comment on attachment 615175 [details] [diff] [review]
add test for trailing slash in hosts

this is a good test that a trailing slash would fail, ideally we would also test that we got the correct warning we want to see in this case - you could override CSPWarning temporarily and check it got called with the right string perhaps ? alternately you could use an nsIConsoleListener, see https://developer.mozilla.org/en/Console_service#Application_developer_point_of_view

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 615175 [details] [diff] [review]
add test for trailing slash in hosts

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

looks good.  What happens if you add two or three slashes?

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 615159 [details] [diff] [review]
resubmit patch

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

Works for me.  I think if there are additional invalid chars or if the slash is in the middle of the string, there are bigger problems in the deployment (the policy writer may not understand the syntax at all).  I'm not convinced we need warnings for those, as the code should already fail out.

Comment 17

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #15)
> Comment on attachment 615175 [details] [diff] [review]
> add test for trailing slash in hosts
> 
> Review of attachment 615175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good.  What happens if you add two or three slashes?

what do you think on the suggestion in comment 14 to check the actual warning ?

Comment 18

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #15)
> Comment on attachment 615175 [details] [diff] [review]
> add test for trailing slash in hosts
> 
> Review of attachment 615175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good.  What happens if you add two or three slashes?

this code will return the first one, which won't be the trailing slash, so the warning won't be displayed in that case.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Ian Melven :imelven from comment #17)
> what do you think on the suggestion in comment 14 to check the actual
> warning ?

Maybe. I'm not sure it's necessary.

(In reply to Ian Melven :imelven from comment #18)
> this code will return the first one, which won't be the trailing slash, so
> the warning won't be displayed in that case.

Hm, the question then is: do we want to prioritize trailing slash warnings or other failures?  I think maybe it doesn't matter, since if someone has a trailing slash *and* other things in the source they have bigger problems.

I was just asking those questions to see if anything exciting would shake out, I think your approach is totally fine.

Comment 20

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #19)
> (In reply to Ian Melven :imelven from comment #17)
> > what do you think on the suggestion in comment 14 to check the actual
> > warning ?
> 
> Maybe. I'm not sure it's necessary.

ok.

> (In reply to Ian Melven :imelven from comment #18)
> > this code will return the first one, which won't be the trailing slash, so
> > the warning won't be displayed in that case.
> 
> Hm, the question then is: do we want to prioritize trailing slash warnings
> or other failures?  I think maybe it doesn't matter, since if someone has a
> trailing slash *and* other things in the source they have bigger problems.

yeah, this is the assumption i was operating under - prioritizing other failures. The trailing slash warning is all about there only being a trailing slash as discussed before, i don't think it should happen in other case.

> I was just asking those questions to see if anything exciting would shake
> out, I think your approach is totally fine.

cool, sounds like this is ready for review.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 615159 [details] [diff] [review]
resubmit patch

Does this do what you expect if you give it a string like "http://www.goo/gle.com"? I.e. is checking indexOf(invalidChar) actually correct with strings other than the obvious one where there's only one slash and it's at the end of the string?

Comment 22

[Tanvi Vyas[:tanvi]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #21)
> Comment on attachment 615159 [details] [diff] [review]
> resubmit patch
> 
> Does this do what you expect if you give it a string like
> "http://www.goo/gle.com"? I.e. is checking indexOf(invalidChar) actually
> correct with strings other than the obvious one where there's only one slash
> and it's at the end of the string?

I don't think this patch will work for http://goo/gle.com/

 var invalidChar = aStr.match(/[^a-zA-Z0-9\-\.\*]/);
will return the first matched character (since its not a global match).  In this case, the first /.

Then we will check if the last character in aStr is a / with  if (invalidChar == '/' && aStr.indexOf(invalidChar) == aStr.length - 1).  Since that will pass, we will give a warning about the trailing /, but we won't provide a CSPdebug message for the first /.

Comment 23

[Tanvi Vyas[:tanvi]]

We could try something like:

  var invalidChar_Array = aStr.match(/[^a-zA-Z0-9\-\.\*]/g);
  if (invalidChar) {
    // host string with a trailing slash is a possible common error,
    // do an additional warning in this case
    if( invalidChar_Array.length == 1 && invalidChar == '/' && aStr.indexOf(invalidChar) == aStr.length - 1) {
      CSPWarning(aStr + " will be ignored due to trailing slash");
      return null;
    } else {
      for(i=0; i<invalidChar_Array.length; i++) { 
          CSPdebug("Invalid character '" + invalidChar_Array[i] + "' in host " + aStr);
      }
      return null;
    }
}

In this case, if you have just a trailing /, you get the CSPWarning.  If you have any other illegal characters, they will all go into their own CSPDebug.  If you have both a trailing slash and other illegal characters, they will all go into their own CSPDebug (there will be no CSPWarning for the trailing slash).

Do you think that is the right behavior?

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 615159 [details] [diff] [review]
resubmit patch

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

I don't think we need to spit out all of the invalid characters in the source, the main focus of this part of the code is to reject strings with *any* invalid chars.  The debug message includes the whole hostname, so it will display all of the characters (valid or invalid) to whomever reads it.

This bug is focused on making sure the common trailing slash case is clear even when CSP debugging isn't enabled.  If we remove the proposed additional "return null" statement, I think we end up in a good state.

::: content/base/src/CSPUtils.jsm
@@ +1318,5 @@
> +    // host string with a trailing slash is a possible common error,
> +    // do an additional warning in this case
> +    if (invalidChar == '/' && aStr.indexOf(invalidChar) == aStr.length - 1) {
> +      CSPWarning(aStr + " will be ignored due to trailing slash");
> +      return null;

I think we can simply remove this return statement.  If we do that, any source with a trailing slash will get the Warning and the following debug message.  Any source with invalid chars but no trailing slash will just get the debug message.  Both will cause a null return.

Comment 25

[Daniel Veditz [:dveditz]]

In the WebApp Security WG face-to-face we talked about this case. Since a much-requested feature is finer-grained control over things like where scripts come from we are going to add language to make these types of hosts valid. Or rather, they wouldn't comply with 1.0 syntax but the error-handling would be specified such that the proposed future syntax gracefully degrades to the expected host.

http://example.com/    -> http://example.com
example.com:8080/path  -> http://example.com:8080     (assuming 'self' is http:)
etc.

Comment 26

[Ian Melven :imelven]

(In reply to Daniel Veditz [:dveditz] from comment #25)
> In the WebApp Security WG face-to-face we talked about this case. Since a
> much-requested feature is finer-grained control over things like where
> scripts come from we are going to add language to make these types of hosts
> valid. Or rather, they wouldn't comply with 1.0 syntax but the
> error-handling would be specified such that the proposed future syntax
> gracefully degrades to the expected host.
> 
> http://example.com/    -> http://example.com
> example.com:8080/path  -> http://example.com:8080     (assuming 'self' is
> http:)
> etc.

so does that make this bug RESOLVED INVALID ? it sounds like it does...

Comment 27

[Tanvi Vyas[:tanvi]]

(In reply to Ian Melven :imelven from comment #26)
> so does that make this bug RESOLVED INVALID ? it sounds like it does...

I'm not sure when we will implement the newly proposed error handling features for CSP 1.0 (I'm actually not sure if they are in the latest working draft or not yet).  So I would say we should continue to fix this bug, especially since the code only needs a one line change (Sid's comment 24) to remove a return.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 615159 [details] [diff] [review]
resubmit patch

spec has changed, this is no longer an error

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 615175 [details] [diff] [review]
add test for trailing slash in hosts

This is no longer an error per spec

Comment 30

[Ian Melven :imelven]

Sounds like this should be RESOLVED INVALID a la comment 26. Tanvi, do you still disagree ?

Comment 31

[Tanvi Vyas[:tanvi]]

(In reply to Ian Melven :imelven from comment #30)
> Sounds like this should be RESOLVED INVALID a la comment 26. Tanvi, do you
> still disagree ?

What does our current implementation do?  This should no longer be an error or a warning, but we should honor a CSP policy that has domains with a trailing /.  Do we?

Comment 32

[Christoph Kerschbaumer [:ckerschb}]

With the re-implementation of CSP in C++ (see bug 925004) this issue got resolved.