1094067 - [CSP] csp.newbackend blocks sources containing ";"

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Torstein]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.111 Safari/537.36 OPR/25.0.1614.68

Steps to reproduce:

After Firefox version 33 was released we have customers that complain about our application. It doesn't load due to a CSP-error reported by Firefox 33. 
This error did not occur in FF versions prior to 33, and this error does not occur in Chrome, IE or Opera.

We checked in the latest FF 34 beta, but the problem occured in this version too.
Searching forums and in bugzilla did not show a similar issue reported.





Actual results:

From console in Web developer tools:
Content Security Policy: The page's settings blocked the loading of a resource at https://someURL/something%3Bjsessionid=...

It appears that the security.csp.newbackend in conjunction with connect-src that contains a ‘;’ cause this error. When disabling the newbackend setting this CSP-error does not appear. 

CSP-directive:
default-src 'none'; style-src 'self'; script-src 'self' 'unsafe-eval'; connect-src 'self' https://someURL/something%3Bjsessionid=...; img-src data:; report-uri /serverpath/a/report-error?cid=8HchhYD1pd8WBdXq; reflected-xss block;



Expected results:

We believe that similar to IE, Chrome and Opera, FF should load the web application even with CSP sources containing URL's with ";"

Comment 1

[Matthias Versen [:Matti]]

Could you provide a public testcase ?

Comment 2

[Torstein]

Thank's for quick reply,

We're working on it :)

Comment 3

[Erik Iter]

I'm one of the parties affected by this and can provide a public test case.

1. Go to:
https://nets-mpi.staging.modirum.com/coffeehouse/checkout
2. Enter the card number:
4800100000000002
3. Click on "Place order"
4. Click on the "BankID" icon
5. You should see the error in web developer saying:
Content Security Policy: The page's settings blocked the loading of a resource at https://nets-acs.staging.modirum.com/mdpayacs/pareq;mdsessionid=... ("connect-src https://csfe-preprod.bankid.no").

Comment 4

[Matthias Versen [:Matti]]

confirming that this is a regression from the new backend:

Last good revision: 464bca437658 (2014-06-26)
First bad revision: be076357691c (2014-06-27)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=464bca437658&tochange=be076357691c

Christoph Kerschbaumer — Bug 925004 - CSP in CPP: flip pref to enable new CSP backend (r=sstamm)

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

Thanks for filing the bug - I am going to look into that tomorrow. Also CC'ing Sid on this one.

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067.patch

Here is what's going on:

1) The Page tries to set the following CSP:

> default-src 'none';
> style-src 'self';
> script-src 'self' 'unsafe-eval';
> connect-src 'self' https://nets-acs.staging.modirum.com/mdpayacs/pareq%3Bmdsessionid=53447CC851226BDA07A64EC8E0F7137E;
> img-src data:;
> report-uri /CentralServerFEJS/a/report-error?cid=wGOlRfmABDBulpQe;
> reflected-xss block;

2) When following the steps described in Comment 3, I get the following three CSP warnings/errors in the console:

> Content Security Policy: Couldn't parse invalid source https://nets-acs.staging.modirum.com/mdpayacs/pareq%3Bmdsessionid=53447CC851226BDA07A64EC8E0F7137E

> Content Security Policy: Not supporting directive 'reflected-xss'. Directive and values will be ignored.

> Content Security Policy: The page's settings blocked the loading of a resource at https://nets-acs.staging.modirum.com/mdpayacs/pareq;mdsessionid=53447CC851226BDA07A64EC8E0F7137E ("connect-src https://csfe-preprod.bankid.no").


3) So our parser currently does not support '%', '=' and some other valid tokens within a path. In fact we should follow path production from RFC 3986, see: http://tools.ietf.org/html/rfc3986#section-3.3

That patch should fix that, Sid, do you agree?

4) By adding 3), our parser recognizes the src correclty, but it's still blocking the load, see console message:

Content Security Policy: The page's settings blocked the loading of a resource at
> https://nets-acs.staging.modirum.com/mdpayacs/pareq;mdsessionid=9C6C5E7F9D6AA0ADC965759CB6745EA5
where the connect-src is (besides self):
> https://nets-acs.staging.modirum.com/mdpayacs/pareq%3bmdsessionid=9c6c5e7f9d6aa0adc965759cb6745ea5


I would like to have Sid take a look at this too because it seems we are blocking because of a path-mismatch, according to the PR_LOG:
> nsCSPHostSrc::permits, aUri: https://nets-acs.staging.modirum.com/mdpayacs/pareq;mdsessionid=D9CC4BFBA625DE42ACC5F4001616FCF5
> mPath:   /mdpayacs/pareq%3bmdsessionid=a06aa8a3c29dd39cc9fce3bcf0b631ce
> uriPath: /mdpayacs/pareq;mdsessionid=A06AA8A3C29DD39CC9FCE3BCF0B631CE
> nsCSPContext::ShouldLoad, nsIContentPolicy::REJECT_SERVER

Some kind of encoding problem here?

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067_tests.patch

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Torstein from comment #0)

Thanks for filing- there is indeed a bug in our parser.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8523251 [details] [diff] [review]
bug_1094067_tests.patch

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

r=me with comments addressed.

::: dom/base/test/TestCSPParser.cpp
@@ +361,5 @@
>      { "report-uri /examplepath",
>        "report-uri http://www.selfuri.com/examplepath" },
> +      // not including ';', since that's a delimiter
> +    { "connect-src http://test.com/pathIncludingAz19-._~!$&'()*+,=%:@",
> +      "connect-src http://test.com/pathincludingaz19-._~!$&'()*+,=%:@" }

COMMA (",") should not be valid for a source expression -- it's used by the HTTP header syntax to separate multiple header values.

The spec is a bit inconsistent here.  It says that the 'path-part' is a production from RFC 3986 S 3.3, but that allows all sub-delims (including comma and semicolon).  It then goes on to say:

> "Characters like U+003B SEMICOLON (;) and U+002C COMMA (,) 
> cannot appear in source expressions directly: if you’d like 
> to include these characters in a source expression, they 
> must be percent encoded as %3B and %2C respectively."

http://www.w3.org/TR/CSP2/#path-part

That doesn't specify precisely which characters are valid since it says comma is not valid but is allowed by RFC 3986.3.3.  So the spec needs to be fixed.

For now, please do not allow a comma in the path, and make sure the percent is used appropriately in the test (e.g., as a pct-encoded value like "%20").  Please also bring this issue up with the working group and post a link to the mail thread here so we can track the discussion.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8523250 [details] [diff] [review]
bug_1094067.patch

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

This follows the RFC production nicely, but after thinking about it for a while we need to be really careful with semicolons and commas since they have special meaning in CSP.  See comment 9 for my thoughts on the spec's either vagueness or inconsistency around this issue.

I'd like to see another patch.

::: dom/security/nsCSPParser.cpp
@@ +183,5 @@
> +  return (peek(EXCLAMATION) || peek(DOLLAR) || peek(AMPERSAND) ||
> +          peek(SINGLEQUOTE) || peek(OPENBRACE) || peek(CLOSINGBRACE) ||
> +          peek(WILDCARD) || peek(PLUS) || peek(COMMA) ||
> +          peek(SEMICOLON) || peek(EQUALS));
> +}

While atValidSubDelimChar seems to reflect RFC 3986 accurately, I don't think CSP wants to match commas or semicolons.  It might be *okay* to look for them since the parser probably splits on commas and semicolons earlier, but best-case, our path-matching algorithm will never find them in the sources' paths.  Worst-case, the CSP parser will get confused and end the directive parsing in the middle of an RFC 3986-compliant path.

Consider this policy:
"default-src https://a.foo.com/bar;script-src *; script-src 'none'"
How does it parse?  is the first "script-src" part of the path, or is it the beginning of a new directive?

So we should be explicit here.  Don't allow comma or semicolon and put in a comment about why we don't follow the RFC precisely.

@@ +189,5 @@
> +// pct-encoded   = "%"
> +bool
> +nsCSPParser::atValidPctEncodedChar() {
> +  return peek(PERCENT_SIGN);
> +}

This is not correct.  The RFC says:
>   pct-encoded   = "%" HEXDIG HEXDIG

If we want to follow the RFC precisely, we need to enforce the two HEXDIGs too.  I honestly don't think we should follow the RFC precisely since we already have to ignore comma and semicolon, and would prefer to just drop this function and put a comment wherever we look for "%" and mention that this deviates from the RFC.  There's potentially a CSP spec inconsistency here (or at least vagueness) about using the path productions from RFC 3986, which needs to be brought up with the workgroup (see comment 9 in this bug).

Comment 11

[Erik Iter]

Can you propose a workaround for this, if the semicolon won't be allowed or the solution might take time due to the inconsistency of the RFC 3986?

I tried replacing the semicolon with the question mark (i.e. /mdpayacs/pareq?mdsessionid=...) when passing the URL and got the same error. I don't cherish the thought of changing it in any case, might break the session handling badly somewhere else in our code.

Comment 12

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067.patch

(In reply to Erik Iter from comment #11)
> Can you propose a workaround for this, if the semicolon won't be allowed or
> the solution might take time due to the inconsistency of the RFC 3986?

I can't really propose a workaround, but after looking at the spec again I realized what we have to do.
Before parsing source-expressions we have to perform a percent-decoding of the expression.

Your policy is indeed valid and the attached patch fixes the issue in our csp parser.
I verified that the provided testcase from Comment 3 works correctly and also added tests to our testsuite.


(In reply to Sid Stamm [:geekboy or :sstamm] from comment #10)
> Comment on attachment 8523250 [details] [diff] [review]
> This follows the RFC production nicely, but after thinking about it for a
> while we need to be really careful with semicolons and commas since they
> have special meaning in CSP.  See comment 9 for my thoughts on the spec's
> either vagueness or inconsistency around this issue.

In the attached patch I am still following the RFC production, but since we perform the necessary percent-decoding before parsing source-expressions (see: http://www.w3.org/TR/CSP2/#source-list-parsing) the semicolon and comma should actually be allowed in the path (atValidSubDelimChar()).

> ::: dom/security/nsCSPParser.cpp
> @@ +183,5 @@
> > +  return (peek(EXCLAMATION) || peek(DOLLAR) || peek(AMPERSAND) ||
> > +          peek(SINGLEQUOTE) || peek(OPENBRACE) || peek(CLOSINGBRACE) ||
> > +          peek(WILDCARD) || peek(PLUS) || peek(COMMA) ||
> > +          peek(SEMICOLON) || peek(EQUALS));
> > +}
> 
> While atValidSubDelimChar seems to reflect RFC 3986 accurately, I don't
> think CSP wants to match commas or semicolons.  It might be *okay* to look
> for them since the parser probably splits on commas and semicolons earlier,
> but best-case, our path-matching algorithm will never find them in the
> sources' paths.  Worst-case, the CSP parser will get confused and end the
> directive parsing in the middle of an RFC 3986-compliant path.

So the tokenizer separates different directives by using the semicolon as delimiter and the comma separates different headers, but if properly percent encoded the semicolon and comma can still be used as part of a path in a source expression.

> Consider this policy:
> "default-src https://a.foo.com/bar;script-src *; script-src 'none'"
> How does it parse?  is the first "script-src" part of the path, or is it the
> beginning of a new directive?

In that case we would end up with:
* default-src https://a.foo.com/bar and
* script-src *
where the second script-src will be ignored by the parser since it already parsed a valid script-src directive.

> @@ +189,5 @@
> > +// pct-encoded   = "%"
> > +bool
> > +nsCSPParser::atValidPctEncodedChar() {
> > +  return peek(PERCENT_SIGN);
> > +}
> 
> This is not correct.  The RFC says:
> >   pct-encoded   = "%" HEXDIG HEXDIG
> 
> If we want to follow the RFC precisely, we need to enforce the two HEXDIGs
> too. 

Agreed, I refactored that part. For reasons explained above I actually completely removed that method since we percent-decode source expressions before we actually start parsing. I left a comment in the code as well.

> There's potentially a CSP spec inconsistency
> here (or at least vagueness) about using the path productions from RFC 3986,
> which needs to be brought up with the workgroup (see comment 9 in this bug).

After starring at the spec for a while I don't actually think it's inconsistent.
At first, I was slightly concerned about hash and nonce, that we would incorrectly replace valid chars when doing the decoding, like '%'. But turns out both can not contain '%'.
See: http://www.w3.org/TR/CSP2/#nonce-value
Currently however, our parser is not enforcing a valid base64-value for hash and nonce, we should actually be more restrictive disallow certain characters within a hash/nonce. We should file a follow up and get that fixed as well.

Even though I don't like duplicating code in general, I think it's reasonable that CSP is hosting it's own decoding algorithm given that there is no good API to call JavaScript's decode function.

One final note, initially I wanted to do the percent-decoding in the tokenzier, but I think it semantically belongs into the parser. The downside of that is, that we have to revisit individual characters instead of decoding while we generate tokens. Basically we do two passes over source-expressions instead of one. Since CSP policies are usually short anyway, that shouldn't give us a performance hit though.

Comment 13

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067_tests.patch

I updated the compiled code tests to include valid percent-encoded source expressions and also allowed sub-delims. Further a test where the '%' sign is not followed by valid hexdigs. In such a case the parser does not allow parsing because the '%' itself is not allowed within the RFC production. I don't think we need a mochitest in addition to those tests because the path-matching still works correctly and was not touched by that change.

Carrying over r+, but of course leaving you the option to veto.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Before parsing source-expressions we have to perform a percent-decoding of
> the expression.

I think we want to do the opposite.  I think we want to fully parse it, and _then_ decode the pct-encoded path, or we will end up with semicolons and commas in places we don't want (and at times where they're not relevant).

> > There's potentially a CSP spec inconsistency
> > here (or at least vagueness) about using the path productions from RFC 3986,
> > which needs to be brought up with the workgroup (see comment 9 in this bug).
> 
> After starring at the spec for a while I don't actually think it's
> inconsistent.

Here's what I read in the spec:
* http://www.w3.org/TR/CSP2/#path-part - says the path-part is produced from RFC 3986 S 3.3.
* RFC 3986 S 3.3 says ; and , are valid characters
* This suggests you can use ; and , (without pct-encoding) in a path production

But you can't.  The note in http://www.w3.org/TR/CSP2/#source-list-parsing says they must be pct-encoded.

The spec should be updated to make this clear.  A path-part should be a pct-encoded production from RFC 3986 S 3.3, not just the production from the RFC.  Right now it says two different things to me.

> One final note, initially I wanted to do the percent-decoding in the
> tokenzier, but I think it semantically belongs into the parser. The downside
> of that is, that we have to revisit individual characters instead of
> decoding while we generate tokens. Basically we do two passes over
> source-expressions instead of one. 

I think we should percent-decode paths _only_ as the last step of the path parsing to line up closer with the procedure in the spec (that says to decode when matching, not when parsing).
http://www.w3.org/TR/CSP2/#match-source-expression -> see Section 4.2.2(4.9)

All the parser/tokenizer has to do is look ahead two chars when identifying a % in a path to validate that they are HEXDIG.  That's pretty quick, and not two whole passes.  We _will_ have to two-pass the paths to do the actual decoding after we extract the path out of the raw policy, but that should be okay because not only are policies short, but paths are not always present in a policy.

Comment 15

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067.patch

As discussed over vidyo we only perform the pct-decoding on paths instead of all source-espressions. Also we are not modifying the original policy, but rather creating a copy of the subPath, pct-decode that copy and append that decoded sub path to the hostSrc object.

Comment 16

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8525506 [details] [diff] [review]
bug_1094067.patch

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

::: dom/security/nsCSPParser.cpp
@@ +240,5 @@
>            peek(UNDERLINE) || peek(TILDE));
>  }
>  
> +// sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
> +//                 / "*" / "+" / "," / ";" / "="

Oh Sid, I forgot to ask - do you want me to take out comma and semicolon here?

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> do you want me to take out comma and semicolon
> here?

Yes, but don't remove those chars from the comment.  Instead add something like 'since CSP tokenizes on ";" and "," earlier in the parsing, they won't match here.  To prevent erroneous parsing, we will explicitly reject them here since they should all be removed.  Any remaining semicolons or commas should still be pct-encoded.'
Then of course, don't match on those two chars in the body of the function.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8525506 [details] [diff] [review]
bug_1094067.patch

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

I think we can simplify this a bit and clean it up.

::: dom/security/nsCSPParser.cpp
@@ +182,5 @@
> +  // helper function that should not be visible outside the functions scope
> +  struct local
> +  {
> +    static char16_t convertHexDig(char16_t aHexDig) {
> +      if (aHexDig >= '0' && aHexDig <= '9') {

I think you can use isdigit(aHexDig) for this.

@@ +186,5 @@
> +      if (aHexDig >= '0' && aHexDig <= '9') {
> +        return aHexDig - '0';
> +      }
> +      if (aHexDig >= 'A' && aHexDig <= 'F') {
> +        return aHexDig - '7';

This is confusing.  Why not return aHexDig - 'A' + 10?  And what happens with lowercase chars a-f?

@@ +188,5 @@
> +      }
> +      if (aHexDig >= 'A' && aHexDig <= 'F') {
> +        return aHexDig - '7';
> +      }
> +      return aHexDig - 'W';

Why is this here?

@@ +210,5 @@
> +    hexDig1 = cur;
> +    hexDig1++;
> +
> +    hexDig2 = hexDig1;
> +    hexDig2++;

This logic is confusing.  Why not just:
hexDig1 = cur + 1;
hexDig2 = cur + 2;

@@ +225,5 @@
> +
> +    // finally, we have to decode and append
> +    // the percent-encoded value
> +    outDecStr.Append(local::convertHexDig(*hexDig1) * 16 +
> +                     local::convertHexDig(*hexDig2));

You can also do << 4 instead of * 16 (though at this small scale I doubt the performance will matter).

Since you will have the hexDig two at a time for conversion, you should just make convertHexDig take both digits and convert two at a time.  You've already validated their contents, so the local function can assume they're valid hexdigs; and although the scoping trick is neat, you could do away with the function entirely if you take two args since it's only ever used this once.

@@ +247,5 @@
> +{
> +  return (peek(EXCLAMATION) || peek(DOLLAR) || peek(AMPERSAND) ||
> +          peek(SINGLEQUOTE) || peek(OPENBRACE) || peek(CLOSINGBRACE) ||
> +          peek(WILDCARD) || peek(PLUS) || peek(COMMA) ||
> +          peek(SEMICOLON) || peek(EQUALS));

Remove peek(SEMICOLON) and peek(COMMA) here to help us catch early decoding (where ; and , appear after the initial tokenization).

Bonus brownie points for logging warning when encountering these chars.

@@ +260,5 @@
> +  }
> +  advance();
> +  if (!peek(isValidHexDig)) {
> +    return false;
> +  }

"atValidPctEncodedChar()" sounds like a predicate and not a consuming function.  Do you really want to consume and check at the same time?

I don't think we should advance past % when we run into invalid tokens after %.  This could leave us in an inconsistent state.  For example, "https://foo.com/%$" will parse just fine because we advance past the % in atValidPctEncodedChar(), return false from it, then check atValidSubDelimChar, which sees the $ in mCurChar and returns true.

So either change atValidPctEncodedChar() to never consume and add the advancing logic elsewhere, or rename it and be careful to rewind before returning false (or don't advance until confident it's a valid pct-encoded).

@@ +424,5 @@
>    }
>    if (atEndOfPath()) {
>      // one slash right after host [port] is also considered a path, e.g.
>      // www.example.com/ should result in www.example.com/
> +    // please note that we do not have to perform any pct-decoding here.

Why not?  Please explain.

Comment 19

[Eirik Øverby]

Hi,

I'm a colleague of Erik Iter. The solution that triggers this bug is part of the Norwegian National Digital ID scheme - BankID - and is intended to be a replacement for a Java applet-based solution that has been in use for the last ~10 years.

How are the chances of getting this fix in *before* applet support is removed/disabled? The current workaround we've implemented involves downgrading to the old applet based solution for FF33/34.

Wbr
/Eirik

Comment 20

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067.patch

Thanks for the review Sid, I simplified the patch - here is a new one, but there are a couple things I haven't incorporated:

> ... you could do away
> with the function entirely if you take two args since it's only ever used
> this once.

Indeed we could, but I think it makes the code cleaner if we factor the conversion of the hexdigs out into convertHexDig().
If we inline both cases, the code seems harder to follow in my opinion. If we make convertHexDig() take two arguments we would yet again have to factor out the hex conversion, hence I would rather like to keep it the way it is in this patch.

> "atValidPctEncodedChar()" sounds like a predicate and not a consuming
> function.  Do you really want to consume and check at the same time?
> 
> I don't think we should advance past % when we run into invalid tokens after
> %.  This could leave us in an inconsistent state.  For example,
> "https://foo.com/%$" will parse just fine because we advance past the % in
> atValidPctEncodedChar(), return false from it, then check
> atValidSubDelimChar, which sees the $ in mCurChar and returns true.

That is a good point. However there is no easy way to clean that up in a nice way, because we want to make use of the method peek() that relies on internal state (mCurChar). So we would have to work around that. Also I don't like backtracking, I would rather go only forward in the parser and avoid backtracking copmletely. So I think we should stick with what we have, I added comments in the code to pinpoint that atValidPathChar() might consume characters. I also moved atValidPctEncodedChar() at the very end of the statement, so in case it returns false, we pass the error up the callstack. So we don't end up in an invalid state in our parser. Further, I added your testscase ("https://foo.com/%$") into TestCSPParser as well. Finally I renamed atValidPctEncodedChar() to advanceIfValidPctEncodedChar().

Comment 21

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067_tests.patch

Added another testcase ("https://foo.com/%$") which Sid suggested.
Carry over r+ from sstamm.

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8531377 [details] [diff] [review]
bug_1094067.patch

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

While I'd prefer backtracking or 3-char peek (look-ahead) to this implementation, with enough comments it should be easy enough to understand what's going on.  I just worry a little bit that this part of the code will be modified in the future and pct-decoding will be an abusable way to get the parser to do something we don't want.

r=me, but I strongly recommend doing some look-ahead or backtracking to make the parser's state and function naming conventions more consistent.

::: dom/security/nsCSPParser.cpp
@@ +390,5 @@
>        // to aCspHost, e.g; "http://www.example.com/path1/path2" then the
>        // first part is "/path1", second part "/path2"
>        resetCurValue();
>      }
> +    // note: advanceIfValidPctEncodedChar within atValidPathChar might consume chars

Please add a note about why this matters and how we handle the case.

Comment 23

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1080987.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #22)

> While I'd prefer backtracking or 3-char peek (look-ahead) to this
> implementation, with enough comments it should be easy enough to understand
> what's going on.  I just worry a little bit that this part of the code will
> be modified in the future and pct-decoding will be an abusable way to get
> the parser to do something we don't want.
> 
> r=me, but I strongly recommend doing some look-ahead or backtracking to make
> the parser's state and function naming conventions more consistent.

Hey Sid, thanks for your input. Even though we end up having a little bit of duplicate code because we can not make efficient use of peek() and advance() you convinced me that having a look-ahead is probably the safer option. I added comments around and updated advanceIfValidPctEncodedChar() to only advance after the fact we have verified a valid pct-encoded char. Sorry for all the review-spam, but I think now we end up having what we want.

Besides adding comments, I only updated advanceIfValidPctEncodedChar() and also simplified atValidPathChar() again, everything else in the patch is unchanged. Thanks!

Comment 24

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067.patch

Oops - wrong patch - here we go.

Comment 25

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8533329 [details] [diff] [review]
bug_1094067.patch

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

I think it can be a little cleaner.  r=me with these changes.

::: dom/security/nsCSPParser.cpp
@@ +279,5 @@
> +  // must be followed by a HEXDIG
> +  if ((pctCurChar  >= mEndChar) || !isValidHexDig(*pctCurChar)) {
> +    return false;
> +  }
> +  unused << *pctCurChar++;

This feels awkward, why are you using unused <<?  Can't you just do *(pctCurChar + x) etc, instead of mucking with the pointer using ++?

(My goal is to make this as readable as possible.)

 if ((pctCurChar + 2) >= mEndChar) {
   return false; // string too short.
 }

 if (PERCENT_SIGN != *pctCurChar ||
     !isValidHexDig(*(pctCurChar+1)) ||
     !isValidHexDig(*(pctCurChar+2))) {
   return false; // not pct-encoded
 }

@@ +303,5 @@
> +  // path, but the parser does not advance() to the next char until we have
> +  // verified the actual contents of a valid pct-encoded char.
> +  return (atValidUnreservedChar() ||
> +          atValidSubDelimChar() ||
> +          advanceIfValidPctEncodedChar() ||

Since you're doing look-ahead, why not separate isValidPctEncodedChar() from the advancing operation?  That makes this predicate function "atValidPathChar" a pure peek and not an advancing operation, so it can more widely and safely be used.  Then, at the call-site, you can advance one or three characters depending on the result from atValidPathChar() and peek(PERCENT_SIGN).

Comment 26

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1094067.patch

Thanks Sid! Incorporated your suggestions - here we go!

Comment 27

[Christoph Kerschbaumer [:ckerschb}]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80de5f55ca1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5866fd3c075c

Comment 28

[Christoph Kerschbaumer [:ckerschb}]

Actually, we are just modifying an existing test - not adding a new one - no need for in-testsuite+ - clearing that flag.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80de5f55ca1e
https://hg.mozilla.org/mozilla-central/rev/5866fd3c075c