1530445 - make CSP_IsQuotelessKeyword be more correct

Status: NEW

(Core :: DOM: Security, enhancement, P5)

Description

[Nathan Froyd [:froydnj]]

CSP_IsQuotelessKeyword's inner loop looks like:

https://searchfox.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#313-321

  nsAutoString keyword;
  for (uint32_t i = 0; i < CSP_LAST_KEYWORD_VALUE; i++) {
    // skipping the leading ' and trimming the trailing '
    keyword.AssignASCII(gCSPUTF8Keywords[i] + 1);
    keyword.Trim("'", false, true);
    if (lowerKey.Equals(keyword)) {
      return true;
    }
  }

which could be improved significantly, but that's a separate bug. The keywords look like:

https://searchfox.org/mozilla-central/source/dom/security/nsCSPUtils.h#113-121

Notice that CSP_REQUIRE_SRI_FOR is not single-quoted, so the current code is subtly wrong in trying to match equire-sri-for. Is this expected behavior? (Matching CSP_NONCE here also seems weird, since the keyword looks more like a prefix...?)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Nathan Froyd [:froydnj] from comment #0)

CSP_IsQuotelessKeyword's inner loop looks like:

https://searchfox.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#313-321

  nsAutoString keyword;
  for (uint32_t i = 0; i < CSP_LAST_KEYWORD_VALUE; i++) {
    // skipping the leading ' and trimming the trailing '
    keyword.AssignASCII(gCSPUTF8Keywords[i] + 1);
    keyword.Trim("'", false, true);
    if (lowerKey.Equals(keyword)) {
      return true;
    }
  }

which could be improved significantly, but that's a separate bug. The keywords look like:

https://searchfox.org/mozilla-central/source/dom/security/nsCSPUtils.h#113-121

Notice that CSP_REQUIRE_SRI_FOR is not single-quoted, so the current code is subtly wrong in trying to match equire-sri-for. Is this expected behavior? (Matching CSP_NONCE here also seems weird, since the keyword looks more like a prefix...?)

Right, both seem slightly wrong, in particular:

• CSP_NONCE should be moved after CSP_HASH, because as you mentioned correctly it's a PREFIX and not an actual keyword. So we could save the string comparision when looping, because we only loop to CSP_LAST_KEYWORD_VALUE.

• CSP_REQUIRE_SRI_FOR, should be surrounded by single quotes, but that would also require to update nsCSPParser - so probably too much effort for this bug. Generally the working group is discussing removing require-sri-for completely, hence I wouldn't invest any time in it. For the purpose of optimizations, I guess you could also move it after CSP_LAST_KEYWORD_VALUE, that way it would still be a valid keyword but we wouldn't loop through it and hence save the string comparison.

Thanks for looking into those optimizations!

Comment 2

[Daniel Veditz [:dveditz]]

require-src-for is a directive, not a quoted keyword. e.g.: require-src-for script;

We're leaning toward pulling it out of the spec, though.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #2)

require-src-for is a directive, not a quoted keyword. e.g.: require-src-for script;

We're leaning toward pulling it out of the spec, though.

There is Bug 1386214 to remove require-sri-for - we should actually do that.