Skip to content

Commit

Permalink
Move Mode into ChromeContentVerifierDelegate
Browse files Browse the repository at this point in the history 
Since ContentVerifier only cares if extension should be verified or not,

implementation, and ShouldBeVerified method can just return yes or no.

ContentVerifierDelegate: :Mode could be part of chrome delegate
Change-Id: Id8a851058a72397b4639cf6b28b2a9c6024e97b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1707355
Commit-Queue: Oleg Davydov <burunduk@chromium.org>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#679034}
  • Loading branch information
@burunduk3
burunduk3 authored and Commit Bot committed Jul 19, 2019
1 parent 2772607 commit e840d6e
Show file tree
Hide file tree
Showing 9 changed files with 94 additions and 88 deletions.
90 changes: 48 additions & 42 deletions chrome/browser/extensions/chrome_content_verifier_delegate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ namespace extensions {

namespace {

base::Optional<ContentVerifierDelegate::Mode>& GetModeForTesting() {
static base::NoDestructor<base::Optional<ContentVerifierDelegate::Mode>>
base::Optional<ChromeContentVerifierDelegate::Mode>& GetModeForTesting() {
static base::NoDestructor<base::Optional<ChromeContentVerifierDelegate::Mode>>
testing_mode;
return *testing_mode;
}
Expand All @@ -57,28 +57,29 @@ const char kContentVerificationExperimentName[] =
} // namespace

// static
ContentVerifierDelegate::Mode ChromeContentVerifierDelegate::GetDefaultMode() {
ChromeContentVerifierDelegate::Mode
ChromeContentVerifierDelegate::GetDefaultMode() {
if (GetModeForTesting())
return *GetModeForTesting();

base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();

Mode experiment_value;
#if defined(GOOGLE_CHROME_BUILD)
experiment_value = ContentVerifierDelegate::ENFORCE_STRICT;
experiment_value = ENFORCE_STRICT;
#else
experiment_value = ContentVerifierDelegate::NONE;
experiment_value = NONE;
#endif
const std::string group =
base::FieldTrialList::FindFullName(kContentVerificationExperimentName);
if (group == "EnforceStrict")
experiment_value = ContentVerifierDelegate::ENFORCE_STRICT;
experiment_value = ENFORCE_STRICT;
else if (group == "Enforce")
experiment_value = ContentVerifierDelegate::ENFORCE;
experiment_value = ENFORCE;
else if (group == "Bootstrap")
experiment_value = ContentVerifierDelegate::BOOTSTRAP;
experiment_value = BOOTSTRAP;
else if (group == "None")
experiment_value = ContentVerifierDelegate::NONE;
experiment_value = NONE;

// The field trial value that normally comes from the server can be
// overridden on the command line, which we don't want to allow since
Expand All @@ -91,23 +92,23 @@ ContentVerifierDelegate::Mode ChromeContentVerifierDelegate::GetDefaultMode() {
command_line->GetSwitchValueASCII(::switches::kForceFieldTrials);
if (forced_trials.find(kContentVerificationExperimentName) !=
std::string::npos)
experiment_value = ContentVerifierDelegate::ENFORCE_STRICT;
experiment_value = ChromeContentVerifierDelegate::ENFORCE_STRICT;
}

Mode cmdline_value = NONE;
if (command_line->HasSwitch(::switches::kExtensionContentVerification)) {
std::string switch_value = command_line->GetSwitchValueASCII(
::switches::kExtensionContentVerification);
if (switch_value == ::switches::kExtensionContentVerificationBootstrap)
cmdline_value = ContentVerifierDelegate::BOOTSTRAP;
cmdline_value = BOOTSTRAP;
else if (switch_value == ::switches::kExtensionContentVerificationEnforce)
cmdline_value = ContentVerifierDelegate::ENFORCE;
cmdline_value = ENFORCE;
else if (switch_value ==
::switches::kExtensionContentVerificationEnforceStrict)
cmdline_value = ContentVerifierDelegate::ENFORCE_STRICT;
cmdline_value = ENFORCE_STRICT;
else
// If no value was provided (or the wrong one), just default to enforce.
cmdline_value = ContentVerifierDelegate::ENFORCE;
cmdline_value = ENFORCE;
}

// We don't want to allow the command-line flags to eg disable enforcement
Expand All @@ -134,32 +135,9 @@ ChromeContentVerifierDelegate::ChromeContentVerifierDelegate(
ChromeContentVerifierDelegate::~ChromeContentVerifierDelegate() {
}

ContentVerifierDelegate::Mode ChromeContentVerifierDelegate::ShouldBeVerified(
bool ChromeContentVerifierDelegate::ShouldBeVerified(
const Extension& extension) {
#if defined(OS_CHROMEOS)
if (ExtensionAssetsManagerChromeOS::IsSharedInstall(&extension))
return ContentVerifierDelegate::ENFORCE_STRICT;
#endif

if (!extension.is_extension() && !extension.is_legacy_packaged_app())
return ContentVerifierDelegate::NONE;
if (!Manifest::IsAutoUpdateableLocation(extension.location()))
return ContentVerifierDelegate::NONE;

// Use the InstallVerifier's |IsFromStore| method to avoid discrepancies
// between which extensions are considered in-store.
// See https://crbug.com/766806 for details.
if (!InstallVerifier::IsFromStore(extension)) {
// It's possible that the webstore update url was overridden for testing
// so also consider extensions with the default (production) update url
// to be from the store as well.
if (ManifestURL::GetUpdateURL(&extension) !=
extension_urls::GetDefaultWebstoreUpdateUrl()) {
return ContentVerifierDelegate::NONE;
}
}

return default_mode_;
return GetVerifyMode(extension) != NONE;
}

ContentVerifierKey ChromeContentVerifierDelegate::GetPublicKey() {
Expand Down Expand Up @@ -200,11 +178,11 @@ void ChromeContentVerifierDelegate::VerifyFailed(
if (!extension)
return;

Mode mode = ShouldBeVerified(*extension);
Mode mode = GetVerifyMode(*extension);
// If the failure was due to hashes missing, only "enforce_strict" would
// disable the extension, but not "enforce".
if (reason == ContentVerifyJob::MISSING_ALL_HASHES &&
mode != ContentVerifierDelegate::ENFORCE_STRICT) {
mode != ENFORCE_STRICT) {
return;
}

Expand All @@ -215,7 +193,7 @@ void ChromeContentVerifierDelegate::VerifyFailed(
return;
}
ExtensionService* service = system->extension_service();
if (mode >= ContentVerifierDelegate::ENFORCE) {
if (mode >= ENFORCE) {
if (system->management_policy()->ShouldRepairIfCorrupted(extension)) {
PendingExtensionManager* pending_manager =
service->pending_extension_manager();
Expand Down Expand Up @@ -254,4 +232,32 @@ void ChromeContentVerifierDelegate::Shutdown() {
policy_extension_reinstaller_.reset();
}

ChromeContentVerifierDelegate::Mode
ChromeContentVerifierDelegate::GetVerifyMode(const Extension& extension) {
#if defined(OS_CHROMEOS)
if (ExtensionAssetsManagerChromeOS::IsSharedInstall(&extension))
return ENFORCE_STRICT;
#endif

if (!extension.is_extension() && !extension.is_legacy_packaged_app())
return NONE;
if (!Manifest::IsAutoUpdateableLocation(extension.location()))
return NONE;

// Use the InstallVerifier's |IsFromStore| method to avoid discrepancies
// between which extensions are considered in-store.
// See https://crbug.com/766806 for details.
if (!InstallVerifier::IsFromStore(extension)) {
// It's possible that the webstore update url was overridden for testing
// so also consider extensions with the default (production) update url
// to be from the store as well.
if (ManifestURL::GetUpdateURL(&extension) !=
extension_urls::GetDefaultWebstoreUpdateUrl()) {
return NONE;
}
}

return default_mode_;
}

} // namespace extensions
27 changes: 25 additions & 2 deletions chrome/browser/extensions/chrome_content_verifier_delegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,25 @@ class PolicyExtensionReinstaller;

class ChromeContentVerifierDelegate : public ContentVerifierDelegate {
public:
// Note that it is important for these to appear in increasing "severity"
// order, because we use this to let command line flags increase, but not
// decrease, the mode you're running in compared to the experiment group.
enum Mode {
// Do not try to fetch content hashes if they are missing, and do not
// enforce them if they are present.
NONE = 0,

// If content hashes are missing, try to fetch them, but do not enforce.
BOOTSTRAP,

// If hashes are present, enforce them. If they are missing, try to fetch
// them.
ENFORCE,

// Treat the absence of hashes the same as a verification failure.
ENFORCE_STRICT
};

static Mode GetDefaultMode();
static void SetDefaultModeForTesting(base::Optional<Mode> mode);

Expand All @@ -36,7 +55,7 @@ class ChromeContentVerifierDelegate : public ContentVerifierDelegate {
~ChromeContentVerifierDelegate() override;

// ContentVerifierDelegate:
Mode ShouldBeVerified(const Extension& extension) override;
bool ShouldBeVerified(const Extension& extension) override;
ContentVerifierKey GetPublicKey() override;
GURL GetSignatureFetchUrl(const std::string& extension_id,
const base::Version& version) override;
Expand All @@ -47,8 +66,12 @@ class ChromeContentVerifierDelegate : public ContentVerifierDelegate {
void Shutdown() override;

private:
// Returns what verification mode is appropriate for the given extension, or
// NONE if no verification is needed.
Mode GetVerifyMode(const Extension& extension);

content::BrowserContext* context_;
ContentVerifierDelegate::Mode default_mode_;
Mode default_mode_;

// This maps an extension id to a backoff entry for slowing down
// redownload/reinstall of corrupt policy extensions if it keeps happening
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/content_verifier_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class ContentVerifierTest : public ExtensionBrowserTest {
// Override content verification mode before ExtensionSystemImpl initializes
// ChromeContentVerifierDelegate.
ChromeContentVerifierDelegate::SetDefaultModeForTesting(
ContentVerifierDelegate::ENFORCE);
ChromeContentVerifierDelegate::ENFORCE);

ExtensionBrowserTest::SetUp();
}
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/extensions/content_verifier_hash_fetch_behavior_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,9 @@ class ContentVerifierHashTest
// Override content verification mode before ExtensionSystemImpl initializes
// ChromeContentVerifierDelegate.
ChromeContentVerifierDelegate::SetDefaultModeForTesting(
uses_enforce_strict_mode() ? ContentVerifierDelegate::ENFORCE_STRICT
: ContentVerifierDelegate::ENFORCE);
uses_enforce_strict_mode()
? ChromeContentVerifierDelegate::ENFORCE_STRICT
: ChromeContentVerifierDelegate::ENFORCE);

ExtensionBrowserTest::SetUp();
}
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/extensions/extension_system_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,12 @@ void ExtensionSystemImpl::Shared::Init(bool extensions_enabled) {
// load any extensions.
{
InstallVerifier::Get(profile_)->Init();
ContentVerifierDelegate::Mode mode =
ChromeContentVerifierDelegate::Mode mode =
ChromeContentVerifierDelegate::GetDefaultMode();
#if defined(OS_CHROMEOS)
mode = std::max(mode, ContentVerifierDelegate::BOOTSTRAP);
mode = std::max(mode, ChromeContentVerifierDelegate::BOOTSTRAP);
#endif // defined(OS_CHROMEOS)
if (mode >= ContentVerifierDelegate::BOOTSTRAP)
if (mode >= ChromeContentVerifierDelegate::BOOTSTRAP)
content_verifier_->Start();
info_map()->SetContentVerifier(content_verifier_.get());
#if defined(OS_CHROMEOS)
Expand Down
3 changes: 1 addition & 2 deletions extensions/browser/content_verifier.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -523,8 +523,7 @@ void ContentVerifier::OnExtensionLoaded(
if (shutdown_on_ui_)
return;

ContentVerifierDelegate::Mode mode = delegate_->ShouldBeVerified(*extension);
if (mode != ContentVerifierDelegate::NONE) {
if (delegate_->ShouldBeVerified(*extension)) {
base::PostTaskWithTraits(
FROM_HERE, {content::BrowserThread::IO},
base::BindOnce(&ContentVerifier::OnExtensionLoadedOnIO, this,
Expand Down
5 changes: 2 additions & 3 deletions extensions/browser/content_verifier/test_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,8 @@ void TestContentVerifyJobObserver::JobFinished(
MockContentVerifierDelegate::MockContentVerifierDelegate() = default;
MockContentVerifierDelegate::~MockContentVerifierDelegate() = default;

ContentVerifierDelegate::Mode MockContentVerifierDelegate::ShouldBeVerified(
const Extension& extension) {
return ContentVerifierDelegate::ENFORCE_STRICT;
bool MockContentVerifierDelegate::ShouldBeVerified(const Extension& extension) {
return true;
}

ContentVerifierKey MockContentVerifierDelegate::GetPublicKey() {
Expand Down
3 changes: 1 addition & 2 deletions extensions/browser/content_verifier/test_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ class MockContentVerifierDelegate : public ContentVerifierDelegate {
~MockContentVerifierDelegate() override;

// ContentVerifierDelegate:
ContentVerifierDelegate::Mode ShouldBeVerified(
const Extension& extension) override;
bool ShouldBeVerified(const Extension& extension) override;
ContentVerifierKey GetPublicKey() override;
GURL GetSignatureFetchUrl(const ExtensionId& extension_id,
const base::Version& version) override;
Expand Down
41 changes: 10 additions & 31 deletions extensions/browser/content_verifier_delegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,48 +23,27 @@ class Extension;
// This is an interface for clients that want to use a ContentVerifier.
class ContentVerifierDelegate {
public:
// Note that it is important for these to appear in increasing "severity"
// order, because we use this to let command line flags increase, but not
// decrease, the mode you're running in compared to the experiment group.
enum Mode {
// Do not try to fetch content hashes if they are missing, and do not
// enforce them if they are present.
NONE = 0,

// If content hashes are missing, try to fetch them, but do not enforce.
BOOTSTRAP,

// If hashes are present, enforce them. If they are missing, try to fetch
// them.
ENFORCE,

// Treat the absence of hashes the same as a verification failure.
ENFORCE_STRICT
};

virtual ~ContentVerifierDelegate() {}

// This should return what verification mode is appropriate for the given
// extension, if any.
virtual Mode ShouldBeVerified(const Extension& extension) = 0;
// Returns whether or not resources from |extension| should be verified.
virtual bool ShouldBeVerified(const Extension& extension) = 0;

// Should return the public key to use for validating signatures via the two
// out parameters.
// Returns the public key to use for validating signatures via the two out
// parameters.
virtual ContentVerifierKey GetPublicKey() = 0;

// This should return a URL that can be used to fetch the
// verified_contents.json containing signatures for the given extension
// id/version pair.
// Returns a URL that can be used to fetch the verified_contents.json
// containing signatures for the given extension id/version pair.
virtual GURL GetSignatureFetchUrl(const std::string& extension_id,
const base::Version& version) = 0;

// This should return the set of file paths for images used within the
// browser process. (These may get transcoded during the install process).
// Returns the set of file paths for images used within the browser process.
// (These may get transcoded during the install process).
virtual std::set<base::FilePath> GetBrowserImagePaths(
const extensions::Extension* extension) = 0;

// Called when the content verifier detects that a read of a file inside
// an extension did not match its expected hash.
// Called when the content verifier detects that a read of a file inside an
// extension did not match its expected hash.
virtual void VerifyFailed(const std::string& extension_id,
ContentVerifyJob::FailureReason reason) = 0;

Expand Down

0 comments on commit e840d6e

Please sign in to comment.