device/fido/mac: make encoding of the RP ID for metadata storage reversible

This changes the CredentialMetadata::EncodeRpId method to allow an RP ID
to be recovered from the encoding, given the secret key; and it adds a
CredentialMetadata::DecodeRpId method to do so.

This is necessary because we need an effective way to test whether a
given credential in the macOS keychain "belongs" to a given profile
(i.e. was the metadata sealed/encoded under that profile's secret key),
when performing browsing data deletion for that profile.

This was previously impossible: Unsealing the credential ID requires the
correct RP ID, with which the credential id ciphertext is authenticated.
The other two fields (RP ID + user handle, and RP ID) are encoded with
HMAC-SHA-256, so they are not reversible without the RP ID either.

This CL changes the kSecAttrLabel field, which previously stored
HMAC-SHA-256(rp_id), to store a deterministic encryption of rp_id
instead. The cipher is AES-GCM-SIV with a fixed nonce. This makes the
encoding reversible (because you can retrieve the RP ID by decrypting),
while maintaining easy lookup of all credentials for a given RP (because
the encoding is deterministic) and confidentiality (because you need the
key to decrypt).

Bug: 678128
Change-Id: I2e34ca5c7f28a2bd14a953539de6e0ac90568bec
Reviewed-on: https://chromium-review.googlesource.com/1117609
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571422}

crypto/aead.cc

@@ -24,6 +24,9 @@ Aead::Aead(AeadAlgorithm algorithm) : key_(nullptr) {
     case AES_256_GCM:
       aead_ = EVP_aead_aes_256_gcm();
       break;
+    case AES_256_GCM_SIV:
+      aead_ = EVP_aead_aes_256_gcm_siv();
+      break;
   }
 }
 

crypto/aead.h

@@ -19,7 +19,7 @@ namespace crypto {
 // This class exposes the AES-128-CTR-HMAC-SHA256 and AES_256_GCM AEAD.
 class CRYPTO_EXPORT Aead {
  public:
-  enum AeadAlgorithm { AES_128_CTR_HMAC_SHA256, AES_256_GCM };
+  enum AeadAlgorithm { AES_128_CTR_HMAC_SHA256, AES_256_GCM, AES_256_GCM_SIV };
 
   explicit Aead(AeadAlgorithm algorithm);
 

crypto/aead_unittest.cc

@@ -10,45 +10,18 @@
 
 namespace {
 
-TEST(AeadTest, SealOpenCtrHmac) {
-  crypto::Aead aead(crypto::Aead::AES_128_CTR_HMAC_SHA256);
-  std::string key(aead.KeyLength(), 0);
-  aead.Init(&key);
-  std::string nonce(aead.NonceLength(), 0);
-  std::string plaintext("this is the plaintext");
-  std::string ad("this is the additional data");
-  std::string ciphertext;
-  EXPECT_TRUE(aead.Seal(plaintext, nonce, ad, &ciphertext));
-  EXPECT_LT(0U, ciphertext.size());
+const crypto::Aead::AeadAlgorithm kAllAlgorithms[]{
+    crypto::Aead::AES_128_CTR_HMAC_SHA256, crypto::Aead::AES_256_GCM,
+    crypto::Aead::AES_256_GCM_SIV,
+};
 
-  std::string decrypted;
-  EXPECT_TRUE(aead.Open(ciphertext, nonce, ad, &decrypted));
+class AeadTest : public testing::TestWithParam<crypto::Aead::AeadAlgorithm> {};
 
-  EXPECT_EQ(plaintext, decrypted);
-}
-
-TEST(AeadTest, SealOpenWrongKeyCtrHmac) {
-  crypto::Aead aead(crypto::Aead::AES_128_CTR_HMAC_SHA256);
-  std::string key(aead.KeyLength(), 0);
-  std::string wrong_key(aead.KeyLength(), 1);
-  aead.Init(&key);
-  crypto::Aead aead_wrong_key(crypto::Aead::AES_128_CTR_HMAC_SHA256);
-  aead_wrong_key.Init(&wrong_key);
-
-  std::string nonce(aead.NonceLength(), 0);
-  std::string plaintext("this is the plaintext");
-  std::string ad("this is the additional data");
-  std::string ciphertext;
-  EXPECT_TRUE(aead.Seal(plaintext, nonce, ad, &ciphertext));
-  EXPECT_LT(0U, ciphertext.size());
-
-  std::string decrypted;
-  EXPECT_FALSE(aead_wrong_key.Open(ciphertext, nonce, ad, &decrypted));
-  EXPECT_EQ(0U, decrypted.size());
-}
+INSTANTIATE_TEST_CASE_P(, AeadTest, testing::ValuesIn(kAllAlgorithms));
 
-TEST(AeadTest, SealOpenGcm) {
-  crypto::Aead aead(crypto::Aead::AES_256_GCM);
+TEST_P(AeadTest, SealOpen) {
+  crypto::Aead::AeadAlgorithm alg = GetParam();
+  crypto::Aead aead(alg);
   std::string key(aead.KeyLength(), 0);
   aead.Init(&key);
   std::string nonce(aead.NonceLength(), 0);
@@ -64,12 +37,13 @@ TEST(AeadTest, SealOpenGcm) {
   EXPECT_EQ(plaintext, decrypted);
 }
 
-TEST(AeadTest, SealOpenWrongKeyGcm) {
-  crypto::Aead aead(crypto::Aead::AES_256_GCM);
+TEST_P(AeadTest, SealOpenWrongKey) {
+  crypto::Aead::AeadAlgorithm alg = GetParam();
+  crypto::Aead aead(alg);
   std::string key(aead.KeyLength(), 0);
   std::string wrong_key(aead.KeyLength(), 1);
   aead.Init(&key);
-  crypto::Aead aead_wrong_key(crypto::Aead::AES_256_GCM);
+  crypto::Aead aead_wrong_key(alg);
   aead_wrong_key.Init(&wrong_key);
 
   std::string nonce(aead.NonceLength(), 0);

device/fido/mac/credential_metadata.cc

@@ -23,29 +23,6 @@ using cbor::CBORWriter;
 using cbor::CBORReader;
 using cbor::CBORValue;
 
-namespace {
-
-enum Algorithm : uint8_t {
-  kAes256Gcm = 0,
-  kHmacSha256 = 1,
-};
-
-// Derive keys from the caller-provided key to avoid using the same key for
-// both algorithms.
-std::string DeriveKey(base::StringPiece in, Algorithm alg) {
-  static constexpr size_t kKeyLength = 32u;
-  std::string key;
-  const uint8_t info = static_cast<uint8_t>(alg);
-  const bool hkdf_init = ::HKDF(
-      reinterpret_cast<uint8_t*>(base::WriteInto(&key, kKeyLength + 1)),
-      kKeyLength, EVP_sha256(), reinterpret_cast<const uint8_t*>(in.data()),
-      in.size(), nullptr /* salt */, 0, &info, 1);
-  DCHECK(hkdf_init);
-  return key;
-}
-
-}  // namespace
-
 // static
 std::string CredentialMetadata::GenerateRandomSecret() {
   static constexpr size_t kSecretSize = 32u;
@@ -126,8 +103,8 @@ base::Optional<std::vector<uint8_t>> CredentialMetadata::SealCredentialId(
   if (!pt) {
     return base::nullopt;
   }
-  base::Optional<std::string> ciphertext =
-      cryptor.Seal(nonce, *pt, MakeAad(rp_id));
+  base::Optional<std::string> ciphertext = cryptor.Seal(
+      CredentialMetadata::Algorithm::kAes256Gcm, nonce, *pt, MakeAad(rp_id));
   if (!ciphertext) {
     return base::nullopt;
   }
@@ -153,7 +130,8 @@ CredentialMetadata::UnsealCredentialId(
   }
 
   base::Optional<std::string> plaintext =
-      cryptor.Unseal(credential_id.subspan(1, kNonceLength),
+      cryptor.Unseal(CredentialMetadata::Algorithm::kAes256Gcm,
+                     credential_id.subspan(1, kNonceLength),
                      credential_id.subspan(1 + kNonceLength), MakeAad(rp_id));
   if (!plaintext) {
     return base::nullopt;
@@ -191,7 +169,38 @@ base::Optional<std::string> CredentialMetadata::EncodeRpIdAndUserId(
 base::Optional<std::string> CredentialMetadata::EncodeRpId(
     const std::string& secret,
     const std::string& rp_id) {
-  return CredentialMetadata(secret).HmacForStorage(rp_id);
+  // Encrypt with a fixed nonce to make the result deterministic while still
+  // allowing the RP ID to be recovered from the ciphertext later.
+  static constexpr std::array<uint8_t, kNonceLength> fixed_zero_nonce = {};
+  base::span<const uint8_t> pt(reinterpret_cast<const uint8_t*>(rp_id.data()),
+                               rp_id.size());
+  std::string empty_ad;
+  // Using AES-GCM with a fixed nonce would break confidentiality, so this uses
+  // AES-GCM-SIV instead.
+  base::Optional<std::string> ct = CredentialMetadata(secret).Seal(
+      CredentialMetadata::Algorithm::kAes256GcmSiv, fixed_zero_nonce, pt,
+      empty_ad);
+  if (!ct) {
+    return base::nullopt;
+  }
+  // The keychain field that stores the encrypted RP ID only accepts NSString
+  // (not NSData), so we HexEncode to ensure the result is UTF-8-decodable.
+  return base::HexEncode(ct->data(), ct->size());
+}
+
+// static
+base::Optional<std::string> CredentialMetadata::DecodeRpId(
+    const std::string& secret,
+    const std::string& ciphertext) {
+  std::vector<uint8_t> ct;
+  if (!base::HexStringToBytes(ciphertext, &ct)) {
+    return base::nullopt;
+  }
+  static constexpr std::array<uint8_t, kNonceLength> fixed_zero_nonce = {};
+  std::string empty_ad;
+  return CredentialMetadata(secret).Unseal(
+      CredentialMetadata::Algorithm::kAes256GcmSiv, fixed_zero_nonce, ct,
+      empty_ad);
 }
 
 // static
@@ -199,12 +208,45 @@ std::string CredentialMetadata::MakeAad(const std::string& rp_id) {
   return std::string(1, kVersion) + rp_id;
 }
 
+// static
+std::string CredentialMetadata::DeriveKey(base::StringPiece secret,
+                                          Algorithm alg) {
+  static constexpr size_t kKeyLength = 32u;
+  std::string key;
+  const uint8_t info = static_cast<uint8_t>(alg);
+  const bool hkdf_init = ::HKDF(
+      reinterpret_cast<uint8_t*>(base::WriteInto(&key, kKeyLength + 1)),
+      kKeyLength, EVP_sha256(), reinterpret_cast<const uint8_t*>(secret.data()),
+      secret.size(), nullptr /* salt */, 0, &info, 1);
+  DCHECK(hkdf_init);
+  return key;
+}
+
+// static
+base::Optional<crypto::Aead::AeadAlgorithm> CredentialMetadata::ToAeadAlgorithm(
+    Algorithm alg) {
+  switch (alg) {
+    case CredentialMetadata::Algorithm::kAes256Gcm:
+      return crypto::Aead::AES_256_GCM;
+    case CredentialMetadata::Algorithm::kAes256GcmSiv:
+      return crypto::Aead::AES_256_GCM_SIV;
+    case CredentialMetadata::Algorithm::kHmacSha256:
+      NOTREACHED() << "invalid AEAD";
+      return base::nullopt;
+  }
+}
+
 base::Optional<std::string> CredentialMetadata::Seal(
+    CredentialMetadata::Algorithm algorithm,
     base::span<const uint8_t> nonce,
     base::span<const uint8_t> plaintext,
     base::StringPiece authenticated_data) const {
-  const std::string key = DeriveKey(secret_, Algorithm::kAes256Gcm);
-  crypto::Aead aead(crypto::Aead::AES_256_GCM);
+  auto opt_aead_algorithm = ToAeadAlgorithm(algorithm);
+  if (!opt_aead_algorithm)
+    return base::nullopt;
+
+  const std::string key = DeriveKey(secret_, algorithm);
+  crypto::Aead aead(*opt_aead_algorithm);
   aead.Init(&key);
   std::string ciphertext;
   if (!aead.Seal(
@@ -219,11 +261,16 @@ base::Optional<std::string> CredentialMetadata::Seal(
 }
 
 base::Optional<std::string> CredentialMetadata::Unseal(
+    CredentialMetadata::Algorithm algorithm,
     base::span<const uint8_t> nonce,
     base::span<const uint8_t> ciphertext,
     base::StringPiece authenticated_data) const {
-  const std::string key = DeriveKey(secret_, Algorithm::kAes256Gcm);
-  crypto::Aead aead(crypto::Aead::AES_256_GCM);
+  auto opt_aead_algorithm = ToAeadAlgorithm(algorithm);
+  if (!opt_aead_algorithm)
+    return base::nullopt;
+
+  const std::string key = DeriveKey(secret_, algorithm);
+  crypto::Aead aead(*opt_aead_algorithm);
   aead.Init(&key);
   std::string plaintext;
   if (!aead.Open(

device/fido/mac/credential_metadata.h

@@ -97,20 +97,40 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CredentialMetadata {
   static base::Optional<std::string> EncodeRpId(const std::string& secret,
                                                 const std::string& rp_id);
 
+  // DecodeRpId attempts to decode a given RP ID from the keychain. This can be
+  // used to test whether a set of credential metadata was created under the
+  // given secret without knowing the RP ID (which would be required to unseal
+  // a credential ID).
+  static base::Optional<std::string> DecodeRpId(const std::string& secret,
+                                                const std::string& ciphertext);
+
  private:
+  enum Algorithm : uint8_t {
+    kAes256Gcm = 0,
+    kHmacSha256 = 1,
+    kAes256GcmSiv = 2,
+  };
   static constexpr uint8_t kVersion = 0x00;
 
   // MakeAad returns the concatenation of |kVersion| and |rp_id|,
   // which is used as the additional authenticated data (AAD) input to the AEAD.
   static std::string MakeAad(const std::string& rp_id);
 
+  // Derives keys from the caller-provided secret to avoid using the same key
+  // for different algorithms.
+  static std::string DeriveKey(base::StringPiece secret, Algorithm alg);
+  static base::Optional<crypto::Aead::AeadAlgorithm> ToAeadAlgorithm(
+      Algorithm alg);
+
   CredentialMetadata(const std::string& secret);
   ~CredentialMetadata();
 
-  base::Optional<std::string> Seal(base::span<const uint8_t> nonce,
+  base::Optional<std::string> Seal(Algorithm alg,
+                                   base::span<const uint8_t> nonce,
                                    base::span<const uint8_t> plaintext,
                                    base::StringPiece authenticated_data) const;
   base::Optional<std::string> Unseal(
+      Algorithm alg,
       base::span<const uint8_t> nonce,
       base::span<const uint8_t> ciphertext,
       base::StringPiece authenticated_data) const;

device/fido/mac/credential_metadata_unittest.cc

@@ -47,6 +47,10 @@ class CredentialMetadataTest : public ::testing::Test {
     return *CredentialMetadata::EncodeRpId(key_, rp_id_);
   }
 
+  std::string DecodeRpId(const std::string& ct) {
+    return *CredentialMetadata::DecodeRpId(key_, ct);
+  }
+
   std::vector<uint8_t> default_id_ = {0, 1, 2, 3};
   std::string rp_id_ = "acme.com";
   std::string key_ = "supersecretsupersecretsupersecre";
@@ -64,7 +68,7 @@ TEST_F(CredentialMetadataTest, CredentialId_IsRandom) {
   EXPECT_NE(SealCredentialId(DefaultUser()), SealCredentialId(DefaultUser()));
 }
 
-TEST_F(CredentialMetadataTest, CredentialId_FailDecrypt) {
+TEST_F(CredentialMetadataTest, CredentialId_FailDecode) {
   const auto id = SealCredentialId(DefaultUser());
   // Flipping a bit in version, nonce, or ciphertext will fail credential
   // decryption.
@@ -97,13 +101,21 @@ TEST_F(CredentialMetadataTest, EncodeRpIdAndUserId) {
 }
 
 TEST_F(CredentialMetadataTest, EncodeRpId) {
-  EXPECT_EQ(64u, EncodeRpId().size());
+  EXPECT_EQ(48u, EncodeRpId().size());
 
   EXPECT_EQ(EncodeRpId(), EncodeRpId());
   EXPECT_NE(EncodeRpId(), *CredentialMetadata::EncodeRpId(key_, "notacme.com"));
   EXPECT_NE(EncodeRpId(), *CredentialMetadata::EncodeRpId(wrong_key_, rp_id_));
 }
 
+TEST_F(CredentialMetadataTest, DecodeRpId) {
+  EXPECT_EQ(rp_id_, DecodeRpId(EncodeRpId()));
+  EXPECT_NE(rp_id_,
+            *CredentialMetadata::DecodeRpId(
+                key_, *CredentialMetadata::EncodeRpId(key_, "notacme.com")));
+  EXPECT_FALSE(CredentialMetadata::DecodeRpId(wrong_key_, EncodeRpId()));
+}
+
 TEST(CredentialMetadata, GenerateRandomSecret) {
   std::string s1 = CredentialMetadata::GenerateRandomSecret();
   EXPECT_EQ(32u, s1.size());

device/fido/mac/operation_base.h

@@ -44,12 +44,12 @@ class API_AVAILABLE(macosx(10.12.2)) OperationBase : public Operation {
  protected:
   // Subclasses must call Init() at the beginning of Run().
   bool Init() {
-    base::Optional<std::string> rp_id =
+    base::Optional<std::string> encoded_rp_id =
         CredentialMetadata::EncodeRpId(metadata_secret(), RpId());
-    if (!rp_id)
+    if (!encoded_rp_id)
       return false;
 
-    encoded_rp_id_ = std::move(*rp_id);
+    encoded_rp_id_ = std::move(*encoded_rp_id);
     return true;
   }