Ignore insecure parts of CSP in extensions and allow extension to load

Ignore insecure parts of CSP in extensions and allow extension to load

Previously, insecure CSP directive values caused refusal of Chrome to
load the Chrome extension. Now, insecure values are stripped from the
CSP, and a list of detailed warnings is printed at the extensions page.

Renamed ContentSecurityPolicyIsSecure to SanitizeContentSecurityPolicy
and let it return a string (the sanitized CSP) instead of a boolean
that tells whether the CSP was considered secure.

BUG=434773
R=kalman@chromium.org
R=mkwst@chromium.org
TEST=extensions_unittests=ExtensionCSPValidator.*
     unit_tests=ContentSecurityPolicyManifestTest.*:PlatformAppsManifestTest:PlatformAppContentSecurityPolicy

Committed: https://crrev.com/f19335614f1a7f78b76a640aba422b13e51a2391
Cr-Commit-Position: refs/heads/master@{#310191}

[Mike West, 2014-11-23 14:23:41]

I haven't looked at the patch yet, but it's not clear to me that this would be
an improvement. Why is it better to tell the developer that her extension
doesn't work later rather than right away?

[robwu, 2014-11-23 14:33:54]

On 2014/11/23 14:23:41, Mike West wrote:
> I haven't looked at the patch yet, but it's not clear to me that this would be
> an improvement. Why is it better to tell the developer that her extension
> doesn't work later rather than right away?

Because refusal to load the extension when a CSP error occurs hinders
evolvability. When I made the CSP validator a bit stricter w.r.t. wildcard TLDs,
some extensions stopped loading. If we ever want to relax the whitelist, we get
a similar problem: the extensions would not load in older versions of Chrome
because of the strict validator.

These issues are fixed with this patch. And the developer will get a specific
actionable warning instead of a generic hard error that links to the
documentation.

[Mike West, 2014-11-23 16:26:19]

On 2014/11/23 14:33:54, robwu wrote:
> On 2014/11/23 14:23:41, Mike West wrote:
> > I haven't looked at the patch yet, but it's not clear to me that this would
be
> > an improvement. Why is it better to tell the developer that her extension
> > doesn't work later rather than right away?
> 
> Because refusal to load the extension when a CSP error occurs hinders
> evolvability. When I made the CSP validator a bit stricter w.r.t. wildcard
TLDs,
> some extensions stopped loading. If we ever want to relax the whitelist, we
get
> a similar problem: the extensions would not load in older versions of Chrome
> because of the strict validator.
> 
> These issues are fixed with this patch. And the developer will get a specific
> actionable warning instead of a generic hard error that links to the
> documentation.

Hrm. Why can't we just improve the error message we throw when the installation
is rejected?

I won't oppose this change, but I'm not sure I see the value. *shrug* If kalman@
is happy with it, then it's the right thing to do.

[robwu, 2014-11-23 16:34:59]

On 2014/11/23 16:26:19, Mike West wrote:
> On 2014/11/23 14:33:54, robwu wrote:
> > On 2014/11/23 14:23:41, Mike West wrote:
> > > I haven't looked at the patch yet, but it's not clear to me that this
would
> be
> > > an improvement. Why is it better to tell the developer that her extension
> > > doesn't work later rather than right away?
> > 
> > Because refusal to load the extension when a CSP error occurs hinders
> > evolvability. When I made the CSP validator a bit stricter w.r.t. wildcard
> TLDs,
> > some extensions stopped loading. If we ever want to relax the whitelist, we
> get
> > a similar problem: the extensions would not load in older versions of Chrome
> > because of the strict validator.
> > 
> > These issues are fixed with this patch. And the developer will get a
specific
> > actionable warning instead of a generic hard error that links to the
> > documentation.
> 
> Hrm. Why can't we just improve the error message we throw when the
installation
> is rejected?
> 
> I won't oppose this change, but I'm not sure I see the value. *shrug* If
kalman@
> is happy with it, then it's the right thing to do.

Evolvability is the main objective of this patch (see e.g. crbug.com/409952
crbug.com/410045 crbug.com/432227 for the incompatibility issues that were
caused by making the CSP validator stricter). The improved error messages are
just a nice side effect.

kalman@ inspired me to write this CL, see comments 3-5 in
https://codereview.chromium.org/722233004/#msg3:
"why don't we just ignore invalid CSP and apply the default CSP if it doesn't
parse?"

[Mike West, 2014-11-24 11:37:18]

mkwst@chromium.org changed required reviewers:
+ kalman@chromium.org

[Mike West, 2014-11-24 11:37:19]

A few comments off the top. Marking kalman@ as required; he knows the extension
code significantly better than I, and if he's happy with the change, then I'm
happy. :)

Mostly, re-reading the validator class makes me sad that I haven't moved CSP
parsing up to //content yet. :(

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:151: csp_parts.back() += ";";
Assigning to the result of a method call is iffy. Does `push_back(';')` not do
what you need?

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:230:
std::move(default_src_csp_warnings.begin(),
We can't use `std::move` or `std::back_inserter` yet. See the "library features"
section at the bottom of https://chromium-cpp.appspot.com/.

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:232: std::back_inserter(csp_warnings));
Don't you also need to inject the default `script-src` and `object-src` bits?

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:233: }
I don't see whether you're verifying that the `default-src` is secure...

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:253: for (std::vector<std::string>::iterator
it = csp_warnings.begin();
You can use a C++11 for loop and 'auto' here for clarity.

[robwu, 2014-11-24 12:19:47]

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:151: csp_parts.back() += ";";
On 2014/11/24 11:37:19, Mike West wrote:
> Assigning to the result of a method call is iffy. Does `push_back(';')` not do
> what you need?

Done.

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:230:
std::move(default_src_csp_warnings.begin(),
On 2014/11/24 11:37:19, Mike West wrote:
> We can't use `std::move` or `std::back_inserter` yet. See the "library
features"
> section at the bottom of https://chromium-cpp.appspot.com/.

Done. Replaced with insert.

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:232: std::back_inserter(csp_warnings));
On 2014/11/24 11:37:19, Mike West wrote:
> Don't you also need to inject the default `script-src` and `object-src` bits?

No, they are inferred from default-src.

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:233: }
On 2014/11/24 11:37:19, Mike West wrote:
> I don't see whether you're verifying that the `default-src` is secure...

GetSecureDirectiveValues (via UpdateStatus) ensures that all output values are
secure. If a directive is set but none of the values are secure, then the
directive would be empty ("default-src;") which is equivalent to "default-src
'none';", which is secure (see comment at line 148).

https://codereview.chromium.org/747403002/diff/1/extensions/common/csp_valida...
extensions/common/csp_validator.cc:253: for (std::vector<std::string>::iterator
it = csp_warnings.begin();
On 2014/11/24 11:37:19, Mike West wrote:
> You can use a C++11 for loop and 'auto' here for clarity.

Done.

[not at google - send to devlin, 2014-11-24 18:26:07]

kalman@chromium.org changed reviewers:
+ raymes@chromium.org, sammc@chromium.org

[not at google - send to devlin, 2014-11-24 18:26:08]

I'll stop reviewing there. Be aware of this CL:

https://codereview.chromium.org/754713002/

you're going to stomp all over each other. It's hard to review these in
isolation.

https://codereview.chromium.org/747403002/diff/20001/extensions/common/csp_va...
File extensions/common/csp_validator.h (right):

https://codereview.chromium.org/747403002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:33: // in |sanitized_csp|.
Concise is nice:
If |sanitized_csp| is non-NULL it will be set to |policy| with the insecure
values removed.

And maybe it should be "safe_policy".

[robwu, 2014-11-24 18:33:11]

On 2014/11/24 18:26:08, kalman wrote:
> I'll stop reviewing there. Be aware of this CL:
> 
> https://codereview.chromium.org/754713002/
> 
> you're going to stomp all over each other. It's hard to review these in
> isolation.

That other change looks easier and does not conflict with my CL, so I'll wait
until that patch lands, then rebase and ping you again for review. Thanks for
bringing this one to my attention.

[robwu, 2014-11-26 09:14:01]

kalman@
I've rebased this CL onto the other CL, could you resume the review?

[not at google - send to devlin, 2014-12-01 19:19:31]

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:110: csp_parts.push_back(directive_name);
Only pass const references. If you need to modify a reference, pass it as a
pointer instead.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:140: }
I think this would be slightly easier to read if you used a bool "safe"
variable, setting to true rather than continuing. That would mean
- instead of continue, "safe = true"
- at the end, it would be 
if (safe) {
  csp_parts->push_back(source);
} else if (csp_warnings) {
  csp_warnings->push_back(...);
}

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:157: std::vector<std::string>& csp_parts,
Same comment re reference vs pointer.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:188: std::string* sanitized_csp,
I'm worried that the way the code has been modified allows silently allowing an
insecure CSP, if callers are not careful. That is, if the caller passes in a
NULL |sanitized_csp| and |warnings| -- perfectly normal, if they don't care -
but |policy| was "sanitized" and this method ends up returning true - bad things
could happen.

Renaming this method to EvaluateContentSecurityPolicy would help, or even
SanitizeContentSecurityPolicy since that's basically what it does now. I don't
even know why it needs to return a bool at all, it could just return the
sanitized string (caller can detect whether it's the same as the old string).

I'm not sure how much of this is a reasonable change to make at this stage.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:198: std::vector<std::string> csp_parts;
sane_csp? (basically what this is generating).

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:200: std::vector<std::string>
default_src_csp_warnings;
Can you just directly pass around |warnings|?

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:254: for (auto csp_warning : csp_warnings) {
To avoid string copy I think you need const auto&

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
File extensions/common/csp_validator.h (right):

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.h:29: OPTIONS_ALLOW_INSECURE_OBJECT_SRC = 1 <<
1,
This intentation change doesn't look necessary. clang-format shouldn't have done
it, and if it did it looks like a bug.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.h:33: // for use in the extension system.
, and optionally and when possible outputs a modified version of |policy| which
does.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
File extensions/common/csp_validator_unittest.cc (right):

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator_unittest.cc:80:
EXPECT_EQ(MissingSecureSrcWarning("object-src"), warnings[2].message);
Some helper functions would help all throughout this file. For example, if you
had:

testing::AssertionResult CheckWarnings(std::vector<> expected, std::vector<>
actual) {
  if (expected.size() != actual.size())
    return testing::AssertionFailure() << ...;
  for (size_t i = 0; i < expected.size(); ++i) {
    if (expected[i] != actual[i])
      return testing::AssertionFailure() << ...;
  }
  return testing::AssertionSuccess();
}

then, provide some helper overrides to make that syntactically palatable:

testing::AssertionResult CheckWarnings(std::vector<>, actual1) {
  return CheckWarnings(std::vector<>(1, actual1));
}
testing::AssertionResult CheckWarnings(std::vector<>, actual1, actual2) {
  std::vector actual<>(1, actual1);
  actual.push_back(actual2);
  return CheckWarnings(actual);
}

some variation on that.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/manife...
File extensions/common/manifest_handlers/csp_info.cc (right):

https://codereview.chromium.org/747403002/diff/40001/extensions/common/manife...
extensions/common/manifest_handlers/csp_info.cc:112:
CHECK(ContentSecurityPolicyIsSecure(content_security_policy,
Indeed I find these changes hard to reason about because the semantics of
ContentSecurityPolicyIsSecure has changed.

[robwu, 2014-12-02 23:42:09]

I've fixed the small stylistic issues that you mentioned, in the next patch set
I'm going to rename ContentSecurityPolicyIsSecure to
SanitizeContentSecurityPolicy, change its return value and refactor the tests.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:110: csp_parts.push_back(directive_name);
On 2014/12/01 19:19:31, kalman wrote:
> Only pass const references. If you need to modify a reference, pass it as a
> pointer instead.

Done.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:140: }
On 2014/12/01 19:19:31, kalman wrote:
> I think this would be slightly easier to read if you used a bool "safe"
> variable, setting to true rather than continuing. That would mean
> - instead of continue, "safe = true"
> - at the end, it would be 
> if (safe) {
>   csp_parts->push_back(source);
> } else if (csp_warnings) {
>   csp_warnings->push_back(...);
> }

Done.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:157: std::vector<std::string>& csp_parts,
On 2014/12/01 19:19:31, kalman wrote:
> Same comment re reference vs pointer.

Done.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:188: std::string* sanitized_csp,
On 2014/12/01 19:19:31, kalman wrote:
> I'm worried that the way the code has been modified allows silently allowing
an
> insecure CSP, if callers are not careful. That is, if the caller passes in a
> NULL |sanitized_csp| and |warnings| -- perfectly normal, if they don't care -
> but |policy| was "sanitized" and this method ends up returning true - bad
things
> could happen. Renaming this method to EvaluateContentSecurityPolicy would
help, or even
> SanitizeContentSecurityPolicy since that's basically what it does now.

SanitizeContentSecurityPolicy sgtm. I'll do that in a separate patch set to make
review easier (search & replace + fix spacing & comments).

> I don't even know why it needs to return a bool at all, it could just return
the
> sanitized string (caller can detect whether it's the same as the old string).

The "sanitized CSP" does not need to be equal to the input CSP. E.g.
"default-src<SP><SP>'self'" becomes "default-src<SP>'self';" (note: a space
disappeared and a semicolon was added).
This method is only used in two places, we can drop the bool and use CHECK_EQ
instead of CHECK.

> 
> I'm not sure how much of this is a reasonable change to make at this stage.

Perfectly reasonable.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:198: std::vector<std::string> csp_parts;
On 2014/12/01 19:19:31, kalman wrote:
> sane_csp? (basically what this is generating).

Done. (sane_csp_parts)

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:200: std::vector<std::string>
default_src_csp_warnings;
On 2014/12/01 19:19:31, kalman wrote:
> Can you just directly pass around |warnings|?

Instead of csp_warnings? Yes, especially after I drop the boolean return value.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:254: for (auto csp_warning : csp_warnings) {
On 2014/12/01 19:19:31, kalman wrote:
> To avoid string copy I think you need const auto&

Done.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
File extensions/common/csp_validator.h (right):

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.h:29: OPTIONS_ALLOW_INSECURE_OBJECT_SRC = 1 <<
1,
On 2014/12/01 19:19:31, kalman wrote:
> This intentation change doesn't look necessary. clang-format shouldn't have
done
> it, and if it did it looks like a bug.

Undone (I did the change because vim highlighted the indention).

[not at google - send to devlin, 2014-12-03 20:33:05]

This ... mostly looks fine I guess? However I'm not comfortable for this to be
committed without Mike (West) having a look.

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/747403002/diff/40001/extensions/common/csp_va...
extensions/common/csp_validator.cc:200: std::vector<std::string>
default_src_csp_warnings;
On 2014/12/02 23:42:08, robwu wrote:
> On 2014/12/01 19:19:31, kalman wrote:
> > Can you just directly pass around |warnings|?
> 
> Instead of csp_warnings? Yes, especially after I drop the boolean return
value.

Yes that is what I meant; don't even declare a separate |csp_warnings| variable.

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
File extensions/common/csp_validator.cc (left):

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
extensions/common/csp_validator.cc:109: bool
HasOnlySecureTokens(base::StringTokenizer& tokenizer,
This method should have always taken a pointer not a reference.

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
extensions/common/csp_validator.cc:253: for (auto& csp_warning : csp_warnings) {
const auto&

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
File extensions/common/csp_validator.h (right):

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
extensions/common/csp_validator.h:46: // in |sanitized_csp|.
If you're going to rename this to "SanitizeCSP" or whatever, then sanitized_csp
shouldn't be [documented, not treated as] an optional parameter.

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
extensions/common/csp_validator.h:48: // Returns whether |policy| meets the
minimum security requirements.
This happens iff there were no warnings, right? You could also mention that.

[robwu, 2014-12-09 18:38:51]

Mike: PTAL

I have renamed ContentSecurityPolicyIsSecure to SanitizeContentSecurityPolicy.
The method now returns a string instead of a boolean.

The validator tests have also been refactored, but the test cases themselves
have not changed relative to the previous patch set, except for the addition of
";" at the end of some CSP strings for tests where the output is expected to be
equal to the input (because the sanitized CSP always ends with a ";").

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
File extensions/common/csp_validator.cc (left):

https://codereview.chromium.org/747403002/diff/60001/extensions/common/csp_va...
extensions/common/csp_validator.cc:109: bool
HasOnlySecureTokens(base::StringTokenizer& tokenizer,
On 2014/12/03 20:33:05, kalman wrote:
> This method should have always taken a pointer not a reference.

Done.

[Mike West, 2014-12-15 08:43:22]

The new implementation LGTM (sorry for the delay, I was out sick most of last
week). I hope we can throw away the vast majority of this parsing code by moving
CSP primitives up to //content. That's going to take a little while, however...

Please wait for kalman@ to sign off officially before landing.

[not at google - send to devlin, 2014-12-15 22:55:56]

lgtm, thanks Mike and Rob.

[robwu, 2014-12-25 23:28:52]

raymes, I have rebased because of your CL at
https://codereview.chromium.org/760513003.
Could you double-check whether my rebase/merge went correctly?

This diff between patch set 5 and 6 should have the same behavior as your CL:

https://codereview.chromium.org/747403002/diff2/80001:100001/extensions/commo...

The most notable novelty in my resolution of the rebase/merge conflict is that I
validate plugin-types upfront instead of afterwards. PTAL and happy holidays!

[raymes, 2015-01-05 01:50:51]

lgtm

Thanks! I don't see any problems with this. Note that now we're parsing the
CSP twice, which isn't an issue for me as long as kalman@ is fine with it.

On Fri Dec 26 2014 at 10:28:54 AM <rob@robwu.nl> wrote:

> raymes, I have rebased because of your CL at
> https://codereview.chromium.org/760513003.
> Could you double-check whether my rebase/merge went correctly?
>
> This diff between patch set 5 and 6 should have the same behavior as your
> CL:
>
> https://codereview.chromium.org/747403002/diff2/80001:100001
> /extensions/common/csp_validator.cc
>
> The most notable novelty in my resolution of the rebase/merge conflict is
> that I
> validate plugin-types upfront instead of afterwards. PTAL and happy
> holidays!
>
> https://codereview.chromium.org/747403002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[raymes, 2015-01-05 01:51:14]

lgtm

[robwu, 2015-01-06 00:48:49]

On 2015/01/05 01:50:51, raymes wrote:
> lgtm
> 
> Thanks! I don't see any problems with this. Note that now we're parsing the
> CSP twice, which isn't an issue for me as long as kalman@ is fine with it.

Ben, please confirm whether you're okay with the change in patch set 6.

[not at google - send to devlin, 2015-01-06 21:59:52]

lgtm

[robwu, 2015-01-06 22:04:42]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-01-06 22:05:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/747403002/140001

[commit-bot: I haz the power, 2015-01-07 00:39:03]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-01-07 00:40:03]

Patchset 8 (id:??) landed as
https://crrev.com/f19335614f1a7f78b76a640aba422b13e51a2391
Cr-Commit-Position: refs/heads/master@{#310191}