0

Revert "Extensions: Prohibit remote stylesheets by default in MV3."

This reverts commit a9d01743d6.

Reason for revert: We have decided to not disallow remote stylesheets
on a platform level for now.

Original change's description:
> Extensions: Prohibit remote stylesheets by default in MV3.
>
> This CL hardens the requirements for
> "content_security_policy.extension_pages" manifest key in Manifest V3 to
> prohibit remote CSS by disallowing insecure values for "style-src".
> Since this is a breaking change, allow extensions to specify insecure
> (effective) values till M93 and raise an install warning.
>
> Bug: 1016997
> Change-Id: I5cf67166f35d5cf21e819e852aca774a870acf08
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2812500
> Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#872200}

Bug: 1016997
Change-Id: Ibad5b532654f27643cef044b8e0a553c44b4f441
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2845455
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#874974}
This commit is contained in:
Karan Bhatia
2021-04-22 01:48:44 +00:00
committed by Chromium LUCI CQ
parent a33da25ec7
commit 3ba901b7a1
10 changed files with 69 additions and 160 deletions

@ -21,7 +21,6 @@
#include "base/strings/string_split.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "content/public/common/url_constants.h"
#include "extensions/common/constants.h"
#include "extensions/common/error_utils.h"
@ -41,7 +40,6 @@ const char kObjectSrc[] = "object-src";
const char kFrameSrc[] = "frame-src";
const char kChildSrc[] = "child-src";
const char kWorkerSrc[] = "worker-src";
const char kStyleSrc[] = "style-src";
const char kSelfSource[] = "'self'";
const char kNoneSource[] = "'none'";
@ -238,7 +236,7 @@ std::string GetSecureDirectiveValues(
std::string source_lower = base::ToLowerASCII(source_literal);
bool is_secure_csp_token = false;
// We might need to relax this allowlist over time.
// We might need to relax this whitelist over time.
if (source_lower == kSelfSource || source_lower == kNoneSource ||
source_lower == "'wasm-eval'" || source_lower == "blob:" ||
source_lower == "filesystem:" ||
@ -624,8 +622,7 @@ bool ContentSecurityPolicyIsSandboxed(
bool DoesCSPDisallowRemoteCode(const std::string& content_security_policy,
base::StringPiece manifest_key,
std::u16string* error,
std::vector<InstallWarning>& warnings) {
std::u16string* error) {
DCHECK(error);
struct DirectiveMapping {
@ -638,12 +635,13 @@ bool DoesCSPDisallowRemoteCode(const std::string& content_security_policy,
DirectiveMapping script_src_mapping({DirectiveStatus({kScriptSrc})});
DirectiveMapping object_src_mapping({DirectiveStatus({kObjectSrc})});
DirectiveMapping worker_src_mapping({DirectiveStatus({kWorkerSrc})});
DirectiveMapping style_src_mapping({DirectiveStatus({kStyleSrc})});
DirectiveMapping default_src_mapping({DirectiveStatus({kDefaultSrc})});
DirectiveMapping* directive_mappings[] = {
&script_src_mapping, &object_src_mapping, &worker_src_mapping,
&style_src_mapping, &default_src_mapping,
&script_src_mapping,
&object_src_mapping,
&worker_src_mapping,
&default_src_mapping,
};
// Populate |directive_mappings|.
@ -679,9 +677,6 @@ bool DoesCSPDisallowRemoteCode(const std::string& content_security_policy,
// "object-src" fallbacks to "default-src".
fallback_if_necessary(&object_src_mapping, default_src_mapping);
// "style-src" fallbacks to "default-src".
fallback_if_necessary(&style_src_mapping, default_src_mapping);
// "worker-src" fallbacks to "script-src", which might itself fallback to
// "default-src".
fallback_if_necessary(&worker_src_mapping, script_src_mapping);
@ -724,23 +719,8 @@ bool DoesCSPDisallowRemoteCode(const std::string& content_security_policy,
continue;
}
std::u16string insecure_directive_error_or_warning;
if (!is_secure_directive(*mapping, &insecure_directive_error_or_warning)) {
// TODO(crbug.com/1016997): Special accommodation for "style-src": Allow
// extensions to not specify a safe value for "style-src" till M93 but
// raise an install warning.
if (mapping == &style_src_mapping) {
std::string warning =
base::UTF16ToASCII(insecure_directive_error_or_warning);
warning += " This will cause an error beginning M93.";
warnings.push_back(
InstallWarning(std::move(warning), manifest_key.as_string()));
continue;
}
*error = std::move(insecure_directive_error_or_warning);
if (!is_secure_directive(*mapping, error))
return false;
}
DCHECK(mapping->directive);
secure_directives.insert(mapping->directive);

@ -133,8 +133,7 @@ bool ContentSecurityPolicyIsSandboxed(
// If not, populates |error|.
bool DoesCSPDisallowRemoteCode(const std::string& content_security_policy,
base::StringPiece manifest_key,
std::u16string* error,
std::vector<InstallWarning>& warnings);
std::u16string* error);
} // namespace csp_validator

@ -14,31 +14,34 @@
#include "extensions/common/manifest_constants.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace extensions {
namespace {
using extensions::csp_validator::ContentSecurityPolicyIsLegal;
using extensions::csp_validator::GetEffectiveSandoxedPageCSP;
using extensions::csp_validator::SanitizeContentSecurityPolicy;
using extensions::csp_validator::ContentSecurityPolicyIsSandboxed;
using extensions::csp_validator::OPTIONS_NONE;
using extensions::csp_validator::OPTIONS_ALLOW_UNSAFE_EVAL;
using extensions::csp_validator::OPTIONS_ALLOW_INSECURE_OBJECT_SRC;
using extensions::ErrorUtils;
using extensions::InstallWarning;
using extensions::Manifest;
using csp_validator::ContentSecurityPolicyIsLegal;
using csp_validator::ContentSecurityPolicyIsSandboxed;
using csp_validator::CSPParser;
using csp_validator::GetEffectiveSandoxedPageCSP;
using csp_validator::OPTIONS_ALLOW_INSECURE_OBJECT_SRC;
using csp_validator::OPTIONS_ALLOW_UNSAFE_EVAL;
using csp_validator::OPTIONS_NONE;
using csp_validator::SanitizeContentSecurityPolicy;
namespace {
std::string InsecureValueWarning(
const std::string& directive,
const std::string& value,
const std::string& manifest_key = manifest_keys::kContentSecurityPolicy) {
const std::string& manifest_key =
extensions::manifest_keys::kContentSecurityPolicy) {
return ErrorUtils::FormatErrorMessage(
manifest_errors::kInvalidCSPInsecureValueIgnored, manifest_key, value,
directive);
extensions::manifest_errors::kInvalidCSPInsecureValueIgnored,
manifest_key, value, directive);
}
std::string MissingSecureSrcWarning(const std::string& manifest_key,
const std::string& directive) {
return ErrorUtils::FormatErrorMessage(
manifest_errors::kInvalidCSPMissingSecureSrc, manifest_key, directive);
extensions::manifest_errors::kInvalidCSPMissingSecureSrc, manifest_key,
directive);
}
bool CSPEquals(const std::string& csp1, const std::string& csp2) {
@ -59,14 +62,15 @@ struct SanitizedCSPResult {
SanitizedCSPResult SanitizeCSP(const std::string& policy, int options) {
SanitizedCSPResult result;
result.csp = SanitizeContentSecurityPolicy(
policy, manifest_keys::kContentSecurityPolicy, options, &result.warnings);
policy, extensions::manifest_keys::kContentSecurityPolicy, options,
&result.warnings);
return result;
}
SanitizedCSPResult SanitizeSandboxPageCSP(const std::string& policy) {
SanitizedCSPResult result;
result.csp = GetEffectiveSandoxedPageCSP(
policy, manifest_keys::kSandboxedPagesCSP, &result.warnings);
policy, extensions::manifest_keys::kSandboxedPagesCSP, &result.warnings);
return result;
}
@ -153,8 +157,8 @@ TEST(ExtensionCSPValidator, IsLegal) {
TEST(ExtensionCSPValidator, IsSecure) {
auto missing_secure_src_warning = [](const std::string& directive) {
return MissingSecureSrcWarning(manifest_keys::kContentSecurityPolicy,
directive);
return MissingSecureSrcWarning(
extensions::manifest_keys::kContentSecurityPolicy, directive);
};
EXPECT_TRUE(CheckCSP(SanitizeCSP(std::string(), OPTIONS_ALLOW_UNSAFE_EVAL),
@ -473,7 +477,7 @@ TEST(ExtensionCSPValidator, EffectiveSandboxedPageCSP) {
auto insecure_value_warning = [](const std::string& directive,
const std::string& value) {
return InsecureValueWarning(directive, value,
manifest_keys::kSandboxedPagesCSP);
extensions::manifest_keys::kSandboxedPagesCSP);
};
EXPECT_TRUE(CheckCSP(
@ -535,6 +539,7 @@ TEST(ExtensionCSPValidator, EffectiveSandboxedPageCSP) {
insecure_value_warning("child-src", "http://foo.com")));
}
namespace extensions {
namespace csp_validator {
void PrintTo(const CSPParser::Directive& directive, ::std::ostream* os) {
@ -545,9 +550,10 @@ void PrintTo(const CSPParser::Directive& directive, ::std::ostream* os) {
}
} // namespace csp_validator
} // namespace extensions
TEST(ExtensionCSPValidator, ParseCSP) {
using CSPParser = csp_validator::CSPParser;
using CSPParser = extensions::csp_validator::CSPParser;
using DirectiveList = CSPParser::DirectiveList;
struct TestCase {
@ -594,8 +600,8 @@ TEST(ExtensionCSPValidator, DoesCSPDisallowRemoteCode) {
auto insecure_value_error = [kManifestKey](const std::string& directive,
const std::string& value) {
return ErrorUtils::FormatErrorMessage(
manifest_errors::kInvalidCSPInsecureValueError, kManifestKey, value,
directive);
extensions::manifest_errors::kInvalidCSPInsecureValueError,
kManifestKey, value, directive);
};
auto missing_secure_src_error = [kManifestKey](const std::string& directive) {
@ -605,10 +611,9 @@ TEST(ExtensionCSPValidator, DoesCSPDisallowRemoteCode) {
struct {
const char* policy;
std::string expected_error; // Empty if no error expected.
std::string expected_warning; // Empty if no warning expected.
} test_cases[] = {
{"frame-src google.com; default-src yahoo.com; script-src 'self'; "
"worker-src; object-src http://localhost:80 'none'; style-src 'self'",
"worker-src; object-src http://localhost:80 'none'",
""},
{"worker-src http://localhost google.com; script-src; object-src 'self'",
insecure_value_error("worker-src", "google.com")},
@ -622,35 +627,16 @@ TEST(ExtensionCSPValidator, DoesCSPDisallowRemoteCode) {
{"script-src; worker-src 'self'; default-src google.com",
insecure_value_error("object-src", "google.com")},
// "worker-src" falls back to "script-src".
{"script-src 'self'; object-src 'none'; style-src 'none'; default-src "
"google.com",
""},
{"script-src 'self'; object-src 'none'; default-src google.com", ""},
{"script-src 'unsafe-eval'; worker-src; default-src;",
insecure_value_error("script-src", "'unsafe-eval'")},
{"script-src; object-src; style-src 'self';"},
{"script-src; object-src; style-src google.com;", "",
"'dummy_key': Insecure CSP value \"google.com\" in directive "
"'style-src'. This will cause an error beginning M93."},
{"script-src; object-src;", "",
"'dummy_key': CSP directive 'style-src' must be specified (either "
"explicitly, or implicitly via 'default-src') and must allow only "
"secure resources. This will cause an error beginning M93."}};
insecure_value_error("script-src", "'unsafe-eval'")}};
for (const auto& test_case : test_cases) {
SCOPED_TRACE(test_case.policy);
std::u16string error;
std::vector<InstallWarning> warnings;
bool result = csp_validator::DoesCSPDisallowRemoteCode(
test_case.policy, kManifestKey, &error, warnings);
bool result = extensions::csp_validator::DoesCSPDisallowRemoteCode(
test_case.policy, kManifestKey, &error);
EXPECT_EQ(test_case.expected_error.empty(), result);
EXPECT_EQ(base::ASCIIToUTF16(test_case.expected_error), error);
EXPECT_EQ(test_case.expected_warning.empty(), warnings.empty());
if (!warnings.empty()) {
EXPECT_EQ(1u, warnings.size());
EXPECT_EQ(test_case.expected_warning, warnings[0].message);
EXPECT_EQ(kManifestKey, warnings[0].key);
}
}
}
} // namespace extensions

@ -349,7 +349,7 @@ const char kInvalidCSPInsecureValueError[] =
"'*': Insecure CSP value \"*\" in directive '*'.";
const char kInvalidCSPMissingSecureSrc[] =
"'*': CSP directive '*' must be specified (either explicitly, or "
"implicitly via 'default-src') and must allow only secure resources.";
"implicitly via 'default-src') and must whitelist only secure resources.";
const char kInvalidDefaultLocale[] =
"Invalid value for default locale - locale name must be a string.";
const char kInvalidDescription[] =

@ -7,8 +7,6 @@
#include <memory>
#include <utility>
#include "base/check.h"
#include "base/dcheck_is_on.h"
#include "base/no_destructor.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
@ -33,6 +31,9 @@ const char kDefaultContentSecurityPolicy[] =
"script-src 'self' blob: filesystem:; "
"object-src 'self' blob: filesystem:;";
// The default secure CSP to be used in order to prevent remote scripts.
const char kDefaultSecureCSP[] = "script-src 'self'; object-src 'self';";
const char kDefaultSandboxedPageContentSecurityPolicy[] =
"sandbox allow-scripts allow-forms allow-popups allow-modals; "
"script-src 'self' 'unsafe-inline' 'unsafe-eval'; child-src 'self';";
@ -92,9 +93,6 @@ const base::Value* GetManifestPath(const Extension* extension,
const char* GetDefaultExtensionPagesCSP(Extension* extension,
bool secure_only) {
// The default secure CSP used to mitigate against remotely hosted code.
static const char kDefaultSecureCSP[] =
"script-src 'self'; style-src 'self'; object-src 'self';";
if (secure_only)
return kDefaultSecureCSP;
@ -122,12 +120,10 @@ const std::string& CSPInfo::GetExtensionPagesCSP(const Extension* extension) {
// static
const std::string* CSPInfo::GetIsolatedWorldCSP(const Extension& extension) {
if (extension.manifest_version() >= 3) {
const char kDefaultMV3IsolatedWorldCSP[] =
"script-src 'self'; object-src 'self';";
// The isolated world will use its own CSP which blocks remotely hosted
// code.
static const base::NoDestructor<std::string> default_isolated_world_csp(
kDefaultMV3IsolatedWorldCSP);
kDefaultSecureCSP);
return default_isolated_world_csp.get();
}
@ -245,19 +241,17 @@ bool CSPHandler::ParseExtensionPagesCSP(
return false;
}
std::vector<InstallWarning> warnings;
if (secure_only) {
if (!csp_validator::DoesCSPDisallowRemoteCode(
content_security_policy_str, manifest_key, error, warnings)) {
if (!csp_validator::DoesCSPDisallowRemoteCode(content_security_policy_str,
manifest_key, error)) {
return false;
}
extension->AddInstallWarnings(std::move(warnings));
SetExtensionPagesCSP(extension, manifest_key, secure_only,
content_security_policy_str);
return true;
}
std::vector<InstallWarning> warnings;
std::string sanitized_content_security_policy = SanitizeContentSecurityPolicy(
content_security_policy_str, manifest_key.as_string(),
GetValidatorOptions(extension), &warnings);
@ -303,19 +297,16 @@ bool CSPHandler::SetExtensionPagesCSP(Extension* extension,
base::StringPiece manifest_key,
bool secure_only,
std::string content_security_policy) {
#if DCHECK_IS_ON()
if (secure_only) {
std::u16string error;
std::vector<InstallWarning> install_warnings;
CHECK(csp_validator::DoesCSPDisallowRemoteCode(
content_security_policy, manifest_key, &error, install_warnings));
DCHECK(csp_validator::DoesCSPDisallowRemoteCode(content_security_policy,
manifest_key, &error));
} else {
CHECK_EQ(content_security_policy,
SanitizeContentSecurityPolicy(
content_security_policy, manifest_key.as_string(),
GetValidatorOptions(extension), nullptr));
DCHECK_EQ(content_security_policy,
SanitizeContentSecurityPolicy(
content_security_policy, manifest_key.as_string(),
GetValidatorOptions(extension), nullptr));
}
#endif // DCHECK_IS_ON()
extension->SetManifestData(
keys::kContentSecurityPolicy,

@ -28,9 +28,7 @@ const char kDefaultSandboxedPageCSP[] =
const char kDefaultExtensionPagesCSP[] =
"script-src 'self' blob: filesystem:; "
"object-src 'self' blob: filesystem:;";
const char kSecureExtensionPagesCSP[] =
"script-src 'self'; style-src 'self'; object-src 'self';";
const char kSecureIsolatedWorldCSP[] = "script-src 'self'; object-src 'self';";
const char kDefaultSecureCSP[] = "script-src 'self'; object-src 'self';";
} // namespace
@ -126,37 +124,17 @@ TEST_F(CSPInfoUnitTest, CSPStringKey) {
TEST_F(CSPInfoUnitTest, CSPDictionary_ExtensionPages) {
struct {
const char* file_name = nullptr;
const char* csp = nullptr;
const char* expected_warning = nullptr;
} cases[] = {
{"csp_dictionary_valid_1.json", "default-src 'none'"},
{"csp_dictionary_valid_2.json",
"worker-src 'self'; script-src; default-src 'self'"},
{"csp_empty_dictionary_valid.json", kSecureExtensionPagesCSP},
{"csp_unsafe_style_src.json",
"script-src; object-src; default-src google.com",
"'content_security_policy.extension_pages': Insecure CSP value "
"\"google.com\" in directive 'style-src'. This will cause an error "
"beginning M93."},
{"csp_missing_style_src.json", "script-src; object-src;",
"'content_security_policy.extension_pages': CSP directive 'style-src' "
"must be specified (either explicitly, or implicitly via 'default-src') "
"and must allow only secure resources. This will cause an "
"error beginning M93."},
{"csp_safe_style_src.json", "script-src; object-src; style-src"}};
const char* file_name;
const char* csp;
} cases[] = {{"csp_dictionary_valid_1.json", "default-src 'none'"},
{"csp_dictionary_valid_2.json",
"worker-src 'self'; script-src; default-src 'self'"},
{"csp_empty_dictionary_valid.json", kDefaultSecureCSP}};
for (const auto& test_case : cases) {
SCOPED_TRACE(base::StringPrintf("Testing %s.", test_case.file_name));
scoped_refptr<Extension> extension;
if (!test_case.expected_warning) {
extension = LoadAndExpectSuccess(test_case.file_name);
} else {
extension =
LoadAndExpectWarning(test_case.file_name, test_case.expected_warning);
}
scoped_refptr<Extension> extension =
LoadAndExpectSuccess(test_case.file_name);
ASSERT_TRUE(extension.get());
EXPECT_EQ(test_case.csp, CSPInfo::GetExtensionPagesCSP(extension.get()));
}
@ -187,8 +165,7 @@ TEST_F(CSPInfoUnitTest, CSPDictionary_Sandbox) {
const char kCustomSandboxedCSP[] =
"sandbox; script-src 'self'; child-src 'self';";
const char kCustomExtensionPagesCSP[] =
"script-src; object-src; style-src http://localhost;";
const char kCustomExtensionPagesCSP[] = "script-src; object-src;";
struct {
const char* file_name;
@ -196,7 +173,7 @@ TEST_F(CSPInfoUnitTest, CSPDictionary_Sandbox) {
const char* expected_csp;
} success_cases[] = {
{"sandbox_dictionary_1.json", "/test", kCustomSandboxedCSP},
{"sandbox_dictionary_1.json", "/index", kSecureExtensionPagesCSP},
{"sandbox_dictionary_1.json", "/index", kDefaultSecureCSP},
{"sandbox_dictionary_2.json", "/test", kDefaultSandboxedPageCSP},
{"sandbox_dictionary_2.json", "/index", kCustomExtensionPagesCSP},
};
@ -243,11 +220,11 @@ TEST_F(CSPInfoUnitTest, CSPDictionaryMandatoryForV3) {
const std::string* isolated_world_csp =
CSPInfo::GetIsolatedWorldCSP(*extension);
ASSERT_TRUE(isolated_world_csp);
EXPECT_EQ(kSecureIsolatedWorldCSP, *isolated_world_csp);
EXPECT_EQ(kDefaultSecureCSP, *isolated_world_csp);
EXPECT_EQ(kDefaultSandboxedPageCSP,
CSPInfo::GetSandboxContentSecurityPolicy(extension.get()));
EXPECT_EQ(kSecureExtensionPagesCSP,
EXPECT_EQ(kDefaultSecureCSP,
CSPInfo::GetExtensionPagesCSP(extension.get()));
}
}

@ -1,8 +0,0 @@
{
"name": "test",
"version": "0.1",
"manifest_version": 3,
"content_security_policy": {
"extension_pages" : "script-src; object-src;"
}
}

@ -1,8 +0,0 @@
{
"name": "test",
"version": "0.1",
"manifest_version": 3,
"content_security_policy": {
"extension_pages" : "script-src; object-src; style-src"
}
}

@ -1,8 +0,0 @@
{
"name": "test",
"version": "0.1",
"manifest_version": 3,
"content_security_policy": {
"extension_pages" : "script-src; object-src; default-src google.com"
}
}

@ -6,6 +6,6 @@
"pages": ["test"]
},
"content_security_policy" : {
"extension_pages" : "script-src; object-src; style-src http://localhost;"
"extension_pages" : "script-src; object-src;"
}
}