Add TPM firmware update mode "cleanup"

Edge cases have been discovered where the TPM firmware update
completes successfully after interruption, but leaves the TPM in a
state where the SRK is still vulnerable. This adds support for a new
"cleanup" mode that is available when the system flags a vulnerable
SRK on an updated system.

BUG=chromium:872746
TEST=Extended existing unit tests.

Change-Id: I4fd5dab72961d9cac4b985b688ebff6740317111
Reviewed-on: https://chromium-review.googlesource.com/1172685
Commit-Queue: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583207}

chrome/browser/chromeos/login/screens/reset_screen.cc

@@ -75,6 +75,9 @@ void StartTPMFirmwareUpdate(
     case tpm_firmware_update::Mode::kPreserveDeviceState:
       mode_string = "preserve_stateful";
       break;
+    case tpm_firmware_update::Mode::kCleanup:
+      mode_string = "cleanup";
+      break;
   }
 
   if (mode_string.empty()) {

chrome/browser/chromeos/tpm_firmware_update.cc

@@ -31,7 +31,7 @@ namespace tpm_firmware_update {
 
 namespace {
 
-// Checks whether |kSettingsKeyAllowPowerwash| is set to true in |settings|.
+// Decodes a |settings| dictionary into a set of allowed update modes.
 std::set<Mode> GetModesFromSetting(const base::Value* settings) {
   std::set<Mode> modes;
   if (!settings)
@@ -87,7 +87,11 @@ std::unique_ptr<base::DictionaryValue> DecodeSettingsProto(
 // away all the gory threading details.
 class AvailabilityChecker {
  public:
-  using ResponseCallback = base::OnceCallback<void(bool)>;
+  struct Status {
+    bool update_available = false;
+    bool srk_vulnerable_roca = false;
+  };
+  using ResponseCallback = base::OnceCallback<void(const Status&)>;
 
   ~AvailabilityChecker() { Cancel(); }
 
@@ -126,9 +130,19 @@ class AvailabilityChecker {
     return update_location_file;
   }
 
-  static bool IsUpdateAvailable() {
+  static bool CheckAvailabilityStatus(Status* status) {
     int64_t size;
-    return base::GetFileSize(GetUpdateLocationFilePath(), &size) && size;
+    if (!base::GetFileSize(GetUpdateLocationFilePath(), &size)) {
+      // File doesn't exist or error - can't determine availability status.
+      return false;
+    }
+    status->update_available = size > 0;
+    base::FilePath srk_vulnerable_roca_file;
+    CHECK(base::PathService::Get(
+        chrome::FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_SRK_VULNERABLE_ROCA,
+        &srk_vulnerable_roca_file));
+    status->srk_vulnerable_roca = base::PathExists(srk_vulnerable_roca_file);
+    return true;
   }
 
   static void StartOnBackgroundThread(
@@ -144,18 +158,18 @@ class AvailabilityChecker {
       base::WeakPtr<AvailabilityChecker> checker,
       const base::FilePath& target,
       bool error) {
-    bool available = IsUpdateAvailable();
-    if (available || error) {
+    Status status;
+    if (CheckAvailabilityStatus(&status) || error) {
       origin_task_runner->PostTask(
           FROM_HERE,
-          base::BindOnce(&AvailabilityChecker::Resolve, checker, available));
+          base::BindOnce(&AvailabilityChecker::Resolve, checker, status));
     }
   }
 
-  void Resolve(bool available) {
+  void Resolve(const Status& status) {
     Cancel();
     if (callback_) {
-      std::move(callback_).Run(available);
+      std::move(callback_).Run(status);
     }
   }
 
@@ -173,10 +187,13 @@ class AvailabilityChecker {
     // this function terminates. Thus, the final check needs to run independent
     // of |this| and takes |callback_| ownership.
     if (callback_) {
-      base::PostTaskAndReplyWithResult(
-          background_task_runner_.get(), FROM_HERE,
-          base::BindOnce(&AvailabilityChecker::IsUpdateAvailable),
-          std::move(callback_));
+      base::PostTaskAndReplyWithResult(background_task_runner_.get(), FROM_HERE,
+                                       base::BindOnce([]() {
+                                         Status status;
+                                         CheckAvailabilityStatus(&status);
+                                         return status;
+                                       }),
+                                       std::move(callback_));
     }
   }
 
@@ -254,10 +271,25 @@ void GetAvailableUpdateModes(
       base::BindOnce(
           [](std::set<Mode> modes,
              base::OnceCallback<void(const std::set<Mode>&)> callback,
-             bool available) {
-            std::move(callback).Run(available ? modes : std::set<Mode>());
+             const AvailabilityChecker::Status& status) {
+            DCHECK_LT(0U, modes.size());
+            DCHECK_EQ(0U, modes.count(Mode::kCleanup));
+            if (status.update_available) {
+              std::move(callback).Run(modes);
+              return;
+            }
+
+            // If there is no update, but the SRK is vulnerable, allow cleanup
+            // to take place. Note that at least one allowed actual mode is
+            // allowed, which is taken to imply cleanup is also allowed.
+            if (status.srk_vulnerable_roca) {
+              std::move(callback).Run(std::set<Mode>({Mode::kCleanup}));
+              return;
+            }
+
+            std::move(callback).Run(std::set<Mode>());
           },
-          std::move(modes), callback),
+          std::move(modes), std::move(callback)),
       timeout);
 }
 

chrome/browser/chromeos/tpm_firmware_update.h

@@ -32,6 +32,9 @@ enum class Mode : int {
   kPowerwash = 1,
   // Device-state preserving update flow. Destroys all user data.
   kPreserveDeviceState = 2,
+  // Force clear TPM after successful update, useful to flush out vulnerable
+  // SRK that might be left behind.
+  kCleanup = 3,
 };
 
 // Settings dictionary key constants.

chrome/browser/chromeos/tpm_firmware_update_unittest.cc

@@ -43,7 +43,8 @@ class TPMFirmwareUpdateModesTest : public testing::Test {
  public:
   enum class Availability {
     kPending,
-    kUnavailble,
+    kUnavailable,
+    kUnavailableROCAVulnerable,
     kAvailable,
   };
 
@@ -55,9 +56,16 @@ class TPMFirmwareUpdateModesTest : public testing::Test {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     base::FilePath update_location_path =
         temp_dir_.GetPath().AppendASCII("tpm_firmware_update_location");
-    path_override_ = std::make_unique<base::ScopedPathOverride>(
+    path_override_location_ = std::make_unique<base::ScopedPathOverride>(
         chrome::FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_LOCATION,
         update_location_path, update_location_path.IsAbsolute(), false);
+    base::FilePath srk_vulnerable_roca_path = temp_dir_.GetPath().AppendASCII(
+        "tpm_firmware_update_srk_vulnerable_roca");
+    path_override_srk_vulnerable_roca_ =
+        std::make_unique<base::ScopedPathOverride>(
+            chrome::FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_SRK_VULNERABLE_ROCA,
+            srk_vulnerable_roca_path, srk_vulnerable_roca_path.IsAbsolute(),
+            false);
     SetUpdateAvailability(Availability::kAvailable);
     callback_ = base::BindOnce(&TPMFirmwareUpdateModesTest::RecordResponse,
                                base::Unretained(this));
@@ -69,6 +77,22 @@ class TPMFirmwareUpdateModesTest : public testing::Test {
   }
 
   void SetUpdateAvailability(Availability availability) {
+    base::FilePath srk_vulnerable_roca_path;
+    ASSERT_TRUE(base::PathService::Get(
+        chrome::FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_SRK_VULNERABLE_ROCA,
+        &srk_vulnerable_roca_path));
+    switch (availability) {
+      case Availability::kPending:
+      case Availability::kUnavailable:
+        base::DeleteFile(srk_vulnerable_roca_path, false);
+        break;
+      case Availability::kAvailable:
+      case Availability::kUnavailableROCAVulnerable:
+        ASSERT_TRUE(base::ImportantFileWriter::WriteFileAtomically(
+            srk_vulnerable_roca_path, ""));
+        break;
+    }
+
     base::FilePath update_location_path;
     ASSERT_TRUE(base::PathService::Get(
         chrome::FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_LOCATION,
@@ -77,7 +101,8 @@ class TPMFirmwareUpdateModesTest : public testing::Test {
       case Availability::kPending:
         base::DeleteFile(update_location_path, false);
         break;
-      case Availability::kUnavailble:
+      case Availability::kUnavailable:
+      case Availability::kUnavailableROCAVulnerable:
         ASSERT_TRUE(base::ImportantFileWriter::WriteFileAtomically(
             update_location_path, ""));
         break;
@@ -91,7 +116,8 @@ class TPMFirmwareUpdateModesTest : public testing::Test {
 
   std::unique_ptr<base::test::ScopedFeatureList> feature_list_;
   base::ScopedTempDir temp_dir_;
-  std::unique_ptr<base::ScopedPathOverride> path_override_;
+  std::unique_ptr<base::ScopedPathOverride> path_override_location_;
+  std::unique_ptr<base::ScopedPathOverride> path_override_srk_vulnerable_roca_;
   base::test::ScopedTaskEnvironment scoped_task_environment_{
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME};
   ScopedCrosSettingsTestHelper cros_settings_test_helper_;
@@ -163,6 +189,22 @@ TEST_F(TPMFirmwareUpdateModesTest, AvailableAfterWaiting) {
   EXPECT_FALSE(callback_received_);
 }
 
+TEST_F(TPMFirmwareUpdateModesTest, NoUpdateVulnerableSRK) {
+  SetUpdateAvailability(Availability::kUnavailableROCAVulnerable);
+  GetAvailableUpdateModes(std::move(callback_), base::TimeDelta());
+  scoped_task_environment_.RunUntilIdle();
+  EXPECT_TRUE(callback_received_);
+  EXPECT_EQ(std::set<Mode>{Mode::kCleanup}, callback_modes_);
+}
+
+TEST_F(TPMFirmwareUpdateModesTest, NoUpdateNonVulnerableSRK) {
+  SetUpdateAvailability(Availability::kUnavailable);
+  GetAvailableUpdateModes(std::move(callback_), base::TimeDelta());
+  scoped_task_environment_.RunUntilIdle();
+  EXPECT_TRUE(callback_received_);
+  EXPECT_EQ(std::set<Mode>(), callback_modes_);
+}
+
 TEST_F(TPMFirmwareUpdateModesTest, Timeout) {
   SetUpdateAvailability(Availability::kPending);
   GetAvailableUpdateModes(std::move(callback_),
@@ -251,5 +293,14 @@ TEST_F(TPMFirmwareUpdateModesEnterpriseTest,
   EXPECT_EQ(std::set<Mode>({Mode::kPreserveDeviceState}), callback_modes_);
 }
 
+TEST_F(TPMFirmwareUpdateModesEnterpriseTest, VulnerableSRK) {
+  SetUpdateAvailability(Availability::kUnavailableROCAVulnerable);
+  SetPolicy({Mode::kPreserveDeviceState});
+  GetAvailableUpdateModes(std::move(callback_), base::TimeDelta());
+  scoped_task_environment_.RunUntilIdle();
+  EXPECT_TRUE(callback_received_);
+  EXPECT_EQ(std::set<Mode>({Mode::kCleanup}), callback_modes_);
+}
+
 }  // namespace tpm_firmware_update
 }  // namespace chromeos

chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc

@@ -415,12 +415,17 @@ void CoreOobeHandler::HandleToggleResetScreen() {
     // purpose of installing a TPM firmware update.
     tpm_firmware_update::GetAvailableUpdateModes(
         base::BindOnce([](const std::set<tpm_firmware_update::Mode>& modes) {
-          if (modes.count(tpm_firmware_update::Mode::kPowerwash) > 0) {
+          using tpm_firmware_update::Mode;
+          for (Mode mode : {Mode::kPowerwash, Mode::kCleanup}) {
+            if (modes.count(mode) == 0)
+              continue;
+
             // Force the TPM firmware update option to be enabled.
             g_browser_process->local_state()->SetInteger(
                 prefs::kFactoryResetTPMFirmwareUpdateMode,
-                static_cast<int>(tpm_firmware_update::Mode::kPowerwash));
+                static_cast<int>(mode));
             LaunchResetScreen();
+            return;
           }
         }),
         base::TimeDelta());

chrome/browser/ui/webui/settings/browser_lifetime_handler.cc

@@ -30,7 +30,8 @@ void TriggerTPMFirmwareUpdate(
   using chromeos::tpm_firmware_update::Mode;
 
   // Decide which update mode to use.
-  for (Mode mode : {Mode::kPreserveDeviceState, Mode::kPowerwash}) {
+  for (Mode mode :
+       {Mode::kPreserveDeviceState, Mode::kPowerwash, Mode::kCleanup}) {
     if (available_modes.count(mode) == 0) {
       continue;
     }

chrome/common/chrome_paths.cc

@@ -73,6 +73,8 @@ const base::FilePath::CharType kChromeOSComponentFlash[] = FILE_PATH_LITERAL(
     "/run/imageloader/PepperFlashPlayer/libpepflashplayer.so");
 const base::FilePath::CharType kChromeOSTPMFirmwareUpdateLocation[] =
     FILE_PATH_LITERAL("/run/tpm_firmware_update_location");
+const base::FilePath::CharType kChromeOSTPMFirmwareUpdateSRKVulnerableROCA[] =
+    FILE_PATH_LITERAL("/run/tpm_firmware_update_srk_vulnerable_roca");
 #endif  // defined(OS_CHROMEOS)
 
 static base::LazyInstance<base::FilePath>::DestructorAtExit
@@ -585,6 +587,10 @@ bool PathProvider(int key, base::FilePath* result) {
       cur = base::FilePath(kChromeOSTPMFirmwareUpdateLocation);
       create_dir = false;
       break;
+    case chrome::FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_SRK_VULNERABLE_ROCA:
+      cur = base::FilePath(kChromeOSTPMFirmwareUpdateSRKVulnerableROCA);
+      create_dir = false;
+      break;
 #endif  // defined(OS_CHROMEOS)
 
     default:

chrome/common/chrome_paths.h

@@ -136,9 +136,13 @@ enum {
   DIR_CHILD_USERS_DEFAULT_APPS,    // Directory where installer places .crx
                                    // files to be installed when child user
                                    // session starts.
-  FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_LOCATION,  // File containing the location
-                                                // of the updated TPM firmware
-                                                // binary in the file system.
+
+  // File containing the location of the updated TPM firmware binary in the file
+  // system.
+  FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_LOCATION,
+
+  // Flag file indicating SRK ROCA vulnerability status.
+  FILE_CHROME_OS_TPM_FIRMWARE_UPDATE_SRK_VULNERABLE_ROCA,
 #endif  // defined(OS_CHROMEOS)
   PATH_END
 };