Don't disable extensions immediately if verification is out of date

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 
@@ skipping 556 matching lines @@
         extension_prefs_->GetAllDelayedInstallInfo());
     UMA_HISTOGRAM_COUNTS_100("Extensions.UpdateOnLoad",
                              delayed_info2->size() - delayed_info->size());
 
     SetReadyAndNotifyListeners();
 
     // TODO(erikkay) this should probably be deferred to a future point
     // rather than running immediately at startup.
     CheckForExternalUpdates();
 
-    InstallVerifier* verifier =
-        extensions::ExtensionSystem::Get(profile_)->install_verifier();
-    if (verifier->NeedsBootstrap())
-      VerifyAllExtensions(true);  // bootstrap=true.
+    MaybeBootstrapVerifier();
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ExtensionService::GarbageCollectExtensions, AsWeakPtr()),
         base::TimeDelta::FromSeconds(kGarbageCollectStartupDelay));
 
     if (extension_prefs_->NeedsStorageGarbageCollection()) {
       GarbageCollectIsolatedStorage();
       extension_prefs_->SetNeedsStorageGarbageCollection(false);
     }
     system_->management_policy()->RegisterProvider(
@@ skipping 13 matching lines @@
        it != all_extensions->end(); ++it) {
     extensions::BlacklistState state =
         extension_prefs_->GetExtensionBlacklistState((*it)->id());
     if (state == extensions::BLACKLISTED_SECURITY_VULNERABILITY ||
         state == extensions::BLACKLISTED_POTENTIALLY_UNWANTED ||
         state == extensions::BLACKLISTED_CWS_POLICY_VIOLATION)
       greylist_.Insert(*it);
   }
 }
 
+void ExtensionService::MaybeBootstrapVerifier() {
+  InstallVerifier* verifier =
+      extensions::ExtensionSystem::Get(profile_)->install_verifier();
+  bool do_bootstrap = false;
+
+  if (verifier->NeedsBootstrap()) {
+    do_bootstrap = true;
+  } else {
+    // If any of the installed extensions have an install time newer than the
+    // signature's timestamp, we need to bootstrap because our signature may
+    // be missing valid extensions.
+    base::Time timestamp = verifier->signature_timestamp();
+    extensions::ExtensionSet::const_iterator i;

[Finnur, 2014/02/12 13:54:57]

nit: Like you, I have a tendency to move large declarations of variables out of
the for loop for readability reasons but I've gotten pushback on that from
reviewers because then you are changing the scope of things, in this case |i|,
to live outside the scope of the for loop. I don't have a strong opinion, I'll
leave it up to you.

[asargent_no_longer_on_chrome, 2014/02/12 16:27:56]

Ah, that's actually a good argument - fixed it.

+    for (i = extensions()->begin(); i != extensions()->end(); ++i) {
+      const Extension& extension = **i;
+      if (verifier->NeedsVerification(extension) &&
+          extension_prefs_->GetInstallTime(extension.id()) >= timestamp) {

[Finnur, 2014/02/12 13:54:57]

You seem to have thought about this problem elsewhere in the code, but just to
be sure: Is this code block robust enough to handle when the system clock
changes manually? For example, if I set my system clock backward and/or
forwards, will I end up in a state where I always/never bootstrap, depending on
what time I've set? (always being better than never, I suppose).

[asargent_no_longer_on_chrome, 2014/02/12 16:27:56]

We have code elsewhere that forces a request for a new signature whenever new
extensions are installed/uninstalled, so you're right in your comment elsewhere
that this code just makes sure during the rollout period that we don't
accidentally disable things for users who have an outdated signature as
discussed in the bug and at the meeting. Eventually we can move to assuming
there's always a valid signature and get rid of this. 

+        do_bootstrap = true;
+        break;
+      }
+    }
+  }
+  if (do_bootstrap)
+    VerifyAllExtensions(true);  // bootstrap=true.

[Finnur, 2014/02/12 13:54:57]

Refresh my memory... Bootstrap means: Fetch the signatures again and verify, not
just fetch signatures and verify on next boot, right?

[asargent_no_longer_on_chrome, 2014/02/12 16:27:56]

Right - inside the callback function  FinishVerifyAllExtensions, if we
successfully got a result back from the server, down at line 683 we proceed to
call CheckManagementPolicy which, because the signature's timestamp will now be
updated, should properly enforce the results we got back from the server and
this time *not* skip any extensions based on their install time inside
InstallVerifier::MustRemainDisabled. 

[Finnur, 2014/02/12 16:56:26]

Good. I was worried that someone could force Chrome to be always in bootstrap
mode and thereby avoiding the enforcement. Seems like you got that covered.

+}
+
 void ExtensionService::VerifyAllExtensions(bool bootstrap) {
   ExtensionIdSet to_add;
   scoped_ptr<ExtensionSet> all_extensions = GenerateInstalledExtensionsSet();
 
   for (ExtensionSet::const_iterator i = all_extensions->begin();
        i != all_extensions->end(); ++i) {
     const Extension& extension = **i;
 
     if (InstallVerifier::NeedsVerification(extension))
       to_add.insert(extension.id());
@@ skipping 2248 matching lines @@
 void ExtensionService::UnloadAllExtensionsInternal() {
   profile_->GetExtensionSpecialStoragePolicy()->RevokeRightsForAllExtensions();
 
   registry_->ClearAll();
   system_->runtime_data()->ClearAll();
 
   // TODO(erikkay) should there be a notification for this?  We can't use
   // EXTENSION_UNLOADED since that implies that the extension has been disabled
   // or uninstalled.
 }