From c4d7b188040c3933b0aaeb089a5a057885e88167 Mon Sep 17 00:00:00 2001
From: Will Harris <wfh@chromium.org>
Date: Thu, 12 Dec 2024 12:38:02 -0800
Subject: [PATCH] Add ability for App-Bound to signal need for reencryption

App-Bound decrypt now returns an additional success code if it
determines that re-encryption might be needed for decrypted
data e.g. the key has been rotated.

If this success code is seen by the client code then an
EncryptData call is made on the same COM thread as the Decrypt
with the Microsoft::WRL::ComPtr held active to ensure the
service stays alive for both calls.

The newly encrypted data is returned to the caller and the
caller should then persist the newly encrypted data.

The App-Bound key provider is updated to persist this data to
the pref in this case.

Tests are added to verify all this behavior including adding
a 'fake reencrypt' switch to the elevated service that can be
used during testing. New tests are created for the App-Bound
key provider as these did not exist previously.

Metrics are also added to measure the success and frequency of
the re-encrypt operations.

Finally, the whole re-encrypt is placed behind a disabled by
default feature flag to ensure this can be rolled out safely.

BUG=383157187,382234536

Change-Id: Ia599fde9e83cc66e4daaff637ba16b60580a67cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6084659
Commit-Queue: Will Harris <wfh@chromium.org>
Reviewed-by: Xiaoling Bao <xiaolingbao@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1395573}
---

diff --git a/chrome/browser/os_crypt/app_bound_encryption_provider_win.cc b/chrome/browser/os_crypt/app_bound_encryption_provider_win.cc
index 23934ec..4a7dada 100644
--- a/chrome/browser/os_crypt/app_bound_encryption_provider_win.cc
+++ b/chrome/browser/os_crypt/app_bound_encryption_provider_win.cc
@@ -5,8 +5,11 @@
 #include "chrome/browser/os_crypt/app_bound_encryption_provider_win.h"
 
 #include <optional>
+#include <string>
+#include <tuple>
 
 #include "base/base64.h"
+#include "base/containers/span.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
@@ -38,6 +41,9 @@
 // OSCryptAsync to identify that data has been encrypted with this key.
 constexpr char kAppBoundDataPrefix[] = "v20";
 
+constexpr ProtectionLevel kCurrentProtectionLevel =
+    ProtectionLevel::PROTECTION_PATH_VALIDATION;
+
 }  // namespace
 
 AppBoundEncryptionProviderWin::AppBoundEncryptionProviderWin(
@@ -53,15 +59,13 @@
 
 class AppBoundEncryptionProviderWin::COMWorker {
  public:
-  std::optional<const std::vector<uint8_t>> EncryptKey(
-      const std::vector<uint8_t>& decrypted_key) {
+  OptionalReadOnlyKeyData EncryptKey(ReadOnlyKeyData& decrypted_key) {
     std::string plaintext_string(decrypted_key.begin(), decrypted_key.end());
     std::string ciphertext;
     DWORD last_error;
 
     HRESULT res = os_crypt::EncryptAppBoundString(
-        ProtectionLevel::PROTECTION_PATH_VALIDATION, plaintext_string,
-        ciphertext, last_error);
+        kCurrentProtectionLevel, plaintext_string, ciphertext, last_error);
 
     base::UmaHistogramSparse("OSCrypt.AppBoundProvider.Encrypt.ResultCode",
                              res);
@@ -75,17 +79,19 @@
       return std::nullopt;
     }
 
-    return std::vector<uint8_t>(ciphertext.cbegin(), ciphertext.cend());
+    return ReadOnlyKeyData(ciphertext.cbegin(), ciphertext.cend());
   }
 
-  std::optional<const std::vector<uint8_t>> DecryptKey(
-      const std::vector<uint8_t>& encrypted_key) {
+  std::optional<std::tuple<ReadWriteKeyData, OptionalReadOnlyKeyData>>
+  DecryptKey(ReadOnlyKeyData& encrypted_key) {
     DWORD last_error;
     std::string encrypted_key_string(encrypted_key.begin(),
                                      encrypted_key.end());
     std::string decrypted_key_string;
+    std::optional<std::string> maybe_new_ciphertext;
     HRESULT res = os_crypt::DecryptAppBoundString(
-        encrypted_key_string, decrypted_key_string, last_error);
+        encrypted_key_string, decrypted_key_string, kCurrentProtectionLevel,
+        maybe_new_ciphertext, last_error);
 
     base::UmaHistogramSparse("OSCrypt.AppBoundProvider.Decrypt.ResultCode",
                              res);
@@ -100,11 +106,18 @@
     }
 
     // Copy data to a vector.
-    std::vector<uint8_t> data(decrypted_key_string.cbegin(),
-                              decrypted_key_string.cend());
+    ReadWriteKeyData data(decrypted_key_string.cbegin(),
+                          decrypted_key_string.cend());
     ::SecureZeroMemory(decrypted_key_string.data(),
                        decrypted_key_string.size());
-    return data;
+
+    OptionalReadOnlyKeyData maybe_new_ciphertext_data;
+    if (maybe_new_ciphertext) {
+      maybe_new_ciphertext_data.emplace(maybe_new_ciphertext->cbegin(),
+                                        maybe_new_ciphertext->cend());
+    }
+    return std::make_tuple(std::move(data),
+                           std::move(maybe_new_ciphertext_data));
   }
 };
 
@@ -135,8 +148,9 @@
     // There is a key, perform the decryption on the background worker.
     com_worker_.AsyncCall(&AppBoundEncryptionProviderWin::COMWorker::DecryptKey)
         .WithArgs(std::move(encrypted_key_data.value()))
-        .Then(base::BindOnce(&AppBoundEncryptionProviderWin::ReplyWithKey,
-                             std::move(callback)));
+        .Then(
+            base::BindOnce(&AppBoundEncryptionProviderWin::StoreAndReplyWithKey,
+                           weak_factory_.GetWeakPtr(), std::move(callback)));
     return;
   }
 
@@ -152,15 +166,15 @@
   const auto random_key = crypto::RandBytesAsVector(
       os_crypt_async::Encryptor::Key::kAES256GCMKeySize);
   // Take a copy of the key. This will be returned as the unencrypted key for
-  // the provider, once the encryption operation is complete.
-  std::vector<uint8_t> decrypted_key(random_key.cbegin(), random_key.cend());
+  // the provider, once the encryption operation is complete. This key is
+  // securely cleared later on in `StoreAndReplyWithKey`.
+  ReadWriteKeyData decrypted_key(random_key.cbegin(), random_key.cend());
   // Perform the encryption on the background worker.
   com_worker_.AsyncCall(&AppBoundEncryptionProviderWin::COMWorker::EncryptKey)
       .WithArgs(std::move(random_key))
-      .Then(base::BindOnce(
-          &AppBoundEncryptionProviderWin::StoreEncryptedKeyAndReply,
-          weak_factory_.GetWeakPtr(), std::move(decrypted_key),
-          std::move(callback)));
+      .Then(base::BindOnce(&AppBoundEncryptionProviderWin::HandleEncryptedKey,
+                           weak_factory_.GetWeakPtr(), std::move(decrypted_key),
+                           std::move(callback)));
 }
 
 bool AppBoundEncryptionProviderWin::UseForEncryption() {
@@ -173,7 +187,7 @@
   return false;
 }
 
-base::expected<std::vector<uint8_t>,
+base::expected<AppBoundEncryptionProviderWin::ReadWriteKeyData,
                AppBoundEncryptionProviderWin::KeyRetrievalStatus>
 AppBoundEncryptionProviderWin::RetrieveEncryptedKey() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -184,7 +198,7 @@
   const std::string base64_encrypted_key =
       local_state_->GetString(kEncryptedKeyPrefName);
 
-  std::optional<std::vector<uint8_t>> encrypted_key_with_header =
+  std::optional<ReadWriteKeyData> encrypted_key_with_header =
       base::Base64Decode(base64_encrypted_key);
 
   if (!encrypted_key_with_header) {
@@ -198,49 +212,60 @@
   }
 
   // Trim off the key prefix.
-  return std::vector<uint8_t>(
+  return ReadWriteKeyData(
       encrypted_key_with_header->cbegin() + sizeof(kCryptAppBoundKeyPrefix),
       encrypted_key_with_header->cend());
 }
 
-void AppBoundEncryptionProviderWin::StoreEncryptedKeyAndReply(
-    const std::vector<uint8_t>& decrypted_key,
-    KeyCallback callback,
-    const std::optional<std::vector<uint8_t>>& encrypted_key) {
+void AppBoundEncryptionProviderWin::StoreKey(
+    base::span<const uint8_t> encrypted_key) {

diff --git a/chrome/browser/os_crypt/app_bound_encryption_win_browsertest.cc b/chrome/browser/os_crypt/app_bound_encryption_win_browsertest.cc
index c601ae26..9382aee 100644
--- a/chrome/browser/os_crypt/app_bound_encryption_win_browsertest.cc
+++ b/chrome/browser/os_crypt/app_bound_encryption_win_browsertest.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/os_crypt/app_bound_encryption_win.h"
 
 #include <optional>
+#include <string>
 
 #include "base/command_line.h"
 #include "base/containers/span.h"
@@ -19,12 +20,15 @@
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/process/process_info.h"
+#include "base/strings/strcat.h"
 #include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/test_future.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/browser_features.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/os_crypt/app_bound_encryption_provider_win.h"
 #include "chrome/browser/os_crypt/test_support.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
@@ -38,7 +42,13 @@
 #include "components/policy/core/common/mock_configuration_policy_provider.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/policy_constants.h"
+#include "components/prefs/mock_pref_change_callback.h"
+#include "components/prefs/pref_store.h"
+#include "components/prefs/testing_pref_service.h"
 #include "content/public/test/browser_test.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using testing::_;
 
 namespace os_crypt {
 
@@ -94,16 +104,12 @@
     InProcessBrowserTest::SetUp();
   }
 
-  void TearDown() override {
-    InProcessBrowserTest::TearDown();
-  }
-
   base::HistogramTester histogram_tester_;
+  std::optional<base::ScopedClosureRunner> maybe_uninstall_service_;
+  ScopedLogGrabber log_grabber_;
 
  private:
-  ScopedLogGrabber log_grabber_;
   install_static::ScopedInstallDetails scoped_install_details_;
-  std::optional<base::ScopedClosureRunner> maybe_uninstall_service_;
 };
 
 // Test App-Bound is supported for tests.
@@ -126,8 +132,11 @@
   ASSERT_HRESULT_SUCCEEDED(hr);
 
   std::string returned_plaintext;
-  hr = DecryptAppBoundString(ciphertext, returned_plaintext, last_error);
-
+  std::optional<std::string> maybe_new_ciphertext;
+  hr = DecryptAppBoundString(ciphertext, returned_plaintext,
+                             ProtectionLevel::PROTECTION_PATH_VALIDATION,
+                             maybe_new_ciphertext, last_error);
+  EXPECT_FALSE(maybe_new_ciphertext);
   ASSERT_HRESULT_SUCCEEDED(hr);
   EXPECT_EQ(plaintext, returned_plaintext);
 }
@@ -138,8 +147,12 @@
   std::string ciphertext("invalidciphertext");
   std::string returned_plaintext;
   DWORD last_error = 0;
+  std::optional<std::string> maybe_new_ciphertext;
   const HRESULT hr =
-      DecryptAppBoundString(ciphertext, returned_plaintext, last_error);
+      DecryptAppBoundString(ciphertext, returned_plaintext,
+                            ProtectionLevel::PROTECTION_PATH_VALIDATION,
+                            maybe_new_ciphertext, last_error);
+  EXPECT_FALSE(maybe_new_ciphertext);
   EXPECT_EQ(elevation_service::Elevator::kErrorCouldNotDecryptWithSystemContext,
             hr);
 }
@@ -349,13 +362,163 @@
     ::testing::Values(
         /*policy::key::kApplicationBoundEncryptionEnabled=*/std::nullopt));
 
+class AppBoundEncryptionWinReencryptTest
+    : public AppBoundEncryptionWinTest,
+      public ::testing::WithParamInterface<
+          std::tuple</*fake_reencrypt*/ bool, /*enable_feature*/ bool>> {
+ public:
+  AppBoundEncryptionWinReencryptTest() {
+    feature_list_.InitWithFeatureState(features::kAppBoundDataReencrypt,
+                                       std::get<1>(GetParam()));
+  }
+
+ protected:
+  // Re-encrypt should only happen if both the feature is enabled, and the
+  // service is faking the re-encryption signal.
+  static bool ExpectReencrypt() {
+    return std::get<0>(GetParam()) && std::get<1>(GetParam());
+  }
+  void SetUp() override {
+    if (base::GetCurrentProcessIntegrityLevel() != base::HIGH_INTEGRITY) {
+      GTEST_SKIP() << "Elevation is required for this test.";
+    }
+    maybe_uninstall_service_ =
+        InstallService(log_grabber_, std::get<0>(GetParam()));
+    EXPECT_TRUE(maybe_uninstall_service_.has_value());
+    // Note do not call SetUp from AppBoundEncryptionWinTest, call to
+    // InProcessBrowserTest.
+    InProcessBrowserTest::SetUp();
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+// Test the basic interface to Encrypt and Decrypt data.
+IN_PROC_BROWSER_TEST_P(AppBoundEncryptionWinReencryptTest, EncryptDecrypt) {
+  ASSERT_TRUE(install_static::IsSystemInstall());
+  const std::string plaintext("plaintext");
+  std::string ciphertext;
+  DWORD last_error;
+  base::HistogramTester histograms;
+  HRESULT hr =
+      EncryptAppBoundString(ProtectionLevel::PROTECTION_PATH_VALIDATION,
+                            plaintext, ciphertext, last_error);
+
+  ASSERT_HRESULT_SUCCEEDED(hr);
+
+  std::string returned_plaintext;
+  std::optional<std::string> maybe_new_ciphertext;
+  hr = DecryptAppBoundString(ciphertext, returned_plaintext,
+                             ProtectionLevel::PROTECTION_PATH_VALIDATION,
+                             maybe_new_ciphertext, last_error);
+  ASSERT_HRESULT_SUCCEEDED(hr);
+  EXPECT_EQ(plaintext, returned_plaintext);
+
+  if (ExpectReencrypt()) {
+    histograms.ExpectUniqueSample("OSCrypt.AppBound.ReEncrypt.ResultCode", S_OK,
+                                  1u);
+    ASSERT_TRUE(maybe_new_ciphertext);
+
+    std::optional<std::string> even_newer_ciphertext;
+    // Verify that the new replacement ciphertext returned can still be
+    // decrypted.
+    hr = DecryptAppBoundString(*maybe_new_ciphertext, returned_plaintext,
+                               ProtectionLevel::PROTECTION_PATH_VALIDATION,
+                               even_newer_ciphertext, last_error);
+    ASSERT_HRESULT_SUCCEEDED(hr);
+    EXPECT_EQ(plaintext, returned_plaintext);
+  } else {
+    histograms.ExpectTotalCount("OSCrypt.AppBound.ReEncrypt.ResultCode", 0);
+    ASSERT_FALSE(maybe_new_ciphertext);
+  }
+  histograms.ExpectTotalCount("OSCrypt.AppBound.ReEncrypt.ResultLastError", 0);
+}
+
+// This could be a unit test, but it needs the service installed to work, so
+// makes sense for it to be here alongside the other app-bound encryption tests.
+IN_PROC_BROWSER_TEST_P(AppBoundEncryptionWinReencryptTest, KeyProviderTest) {
+  const char* kPrefName = "os_crypt.app_bound_encrypted_key";
+  ASSERT_TRUE(install_static::IsSystemInstall());
+
+  TestingPrefServiceSimple prefs;
+  MockPrefChangeCallback observer(&prefs);
+  PrefChangeRegistrar registrar;
+  registrar.Init(&prefs);
+  registrar.Add(kPrefName, observer.GetCallback());
+  // The first time the GetKey is called, the provider should generate a random
+  // key, encrypt it with app-bound, then persist the encrypted key to store.
+  EXPECT_CALL(observer, OnPreferenceChanged(_)).Times(1);
+
+  os_crypt_async::AppBoundEncryptionProviderWin::RegisterLocalPrefs(
+      prefs.registry());
+
+  // `Key` has no public constructor and is move-only so use a std::optional as
+  // a handy container.
+  std::optional<os_crypt_async::Encryptor::Key> encryption_key;
+  std::string encrypted_key;
+  {
+    os_crypt_async::AppBoundEncryptionProviderWin provider(
+        &prefs, /*use_for_encryption=*/true);
+    base::test::TestFuture<const std::string&,
+                           std::optional<os_crypt_async::Encryptor::Key>>
+        future;
+    provider.GetKey(future.GetCallback());
+    auto [tag, key] = future.Take();
+    EXPECT_EQ(tag, "v20");
+    ASSERT_TRUE(key);
+    encryption_key.emplace(std::move(*key));
+    encrypted_key = prefs.GetString(kPrefName);
+    EXPECT_FALSE(encrypted_key.empty());
+  }
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
+
+  // The second time the GetKey is called, the provider should retrieve the key
+  // from store then perform a decryption via app-bound. If re-encryption is
+  // specified then a re-encryption call is made and a second write should
+  // happen to the store with the new encrypted key.
+  EXPECT_CALL(observer, OnPreferenceChanged(_))
+      .Times(ExpectReencrypt() ? 1 : 0);
+  {
+    os_crypt_async::AppBoundEncryptionProviderWin provider(
+        &prefs, /*use_for_encryption=*/true);
+    base::test::TestFuture<const std::string&,
+                           std::optional<os_crypt_async::Encryptor::Key>>
+        future;
+    provider.GetKey(future.GetCallback());
+    const auto& [_, key] = future.Get();
+    ASSERT_TRUE(key);
+    // The key returned should be the same as it's been decrypted from the
+    // store, regardless of whether it's been re-encrypted or not.
+    EXPECT_EQ(*key, *encryption_key);
+
+    if (ExpectReencrypt()) {
+      // Re-encryption should always change the encrypted value, because the
+      // underlying encryption schemes use random IVs, nonces or salts.
+      EXPECT_NE(prefs.GetString(kPrefName), encrypted_key);
+    } else {
+      EXPECT_EQ(prefs.GetString(kPrefName), encrypted_key);
+    }
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    ,
+    AppBoundEncryptionWinReencryptTest,
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()),
+    [](const auto& info) {
+      return base::StrCat(
+          {std::get<0>(info.param) ? "FakeReencrypt" : "NoFakeReencrypt",
+           std::get<1>(info.param) ? "FeatureOn" : "FeatureOff"});
+    });
+
 class AppBoundEncryptionWinTestFeatureMaybeDisabled
     : public AppBoundEncryptionWinTest,
       public ::testing::WithParamInterface</*feature enabled*/ bool> {
  public:
   AppBoundEncryptionWinTestFeatureMaybeDisabled() {
     feature_list_.InitWithFeatureState(
-        features::kUseAppBoundEncryptionProviderForEncryption, GetParam());
+        ::features::kUseAppBoundEncryptionProviderForEncryption, GetParam());
   }
 
  private: