bug 1517337 - make secret overwriting consistent across backends r=jcj
authorDana Keeler <dkeeler@mozilla.com>
Wed, 09 Jan 2019 18:25:46 +0000
changeset 510230 1e674ee50a1d30961768ac175b0cc769603ffb14
parent 510229 699fe9e8f434a750ee3ddce587282ae396ea0db2
child 510231 93ff3f0d95bdfddd415271537c149400e6bc7744
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs1517337
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1517337 - make secret overwriting consistent across backends r=jcj As originally written, the keychain-backed secret storing implementation would not overwrite a secret if prompted to generate or recover one with a label that was already in use. Since libsecret and credential manager both do this by default, this change makes the keychain-backed implementation behave the same way. Differential Revision: https://phabricator.services.mozilla.com/D15697
security/manager/ssl/KeychainSecret.cpp
security/manager/ssl/NSSKeyStore.cpp
security/manager/ssl/OSKeyStore.cpp
security/manager/ssl/ScopedNSSTypes.h
security/manager/ssl/tests/unit/test_oskeystore.js
--- a/security/manager/ssl/KeychainSecret.cpp
+++ b/security/manager/ssl/KeychainSecret.cpp
@@ -64,16 +64,26 @@ nsresult KeychainSecret::StoreSecret(con
   // "account" is the way we differentiate different secrets.
   // By default, secrets stored by the application (Firefox) in this way are not
   // accessible to other applications, so we shouldn't need to worry about
   // unauthorized access or namespace collisions. This will be the case as long
   // as we never set the kSecAttrAccessGroup attribute on the CFDictionary. The
   // platform enforces this restriction using the application-identifier
   // entitlement that each application bundle should have. See
   // https://developer.apple.com/documentation/security/1401659-secitemadd?language=objc#discussion
+
+  // The keychain does not overwrite secrets by default (unlike other backends
+  // like libsecret and credential manager). To be consistent, we first delete
+  // any previously-stored secrets that use the given label.
+  nsresult rv = DeleteSecret(aLabel);
+  if (NS_FAILED(rv)) {
+    MOZ_LOG(gKeychainSecretLog, LogLevel::Debug,
+            ("DeleteSecret before StoreSecret failed"));
+    return rv;
+  }
   const CFStringRef keys[] = {kSecClass, kSecAttrAccount, kSecValueData};
   ScopedCFType<CFStringRef> label(MozillaStringToCFString(aLabel));
   if (!label) {
     MOZ_LOG(gKeychainSecretLog, LogLevel::Debug,
             ("MozillaStringToCFString failed"));
     return NS_ERROR_FAILURE;
   }
   ScopedCFType<CFDataRef> secret(CFDataCreate(
@@ -85,20 +95,16 @@ nsresult KeychainSecret::StoreSecret(con
   }
   const void* values[] = {kSecClassGenericPassword, label.get(), secret.get()};
   static_assert(ArrayLength(keys) == ArrayLength(values),
                 "mismatched SecItemAdd key/value array sizes");
   ScopedCFType<CFDictionaryRef> addDictionary(CFDictionaryCreate(
       nullptr, (const void**)&keys, (const void**)&values, ArrayLength(keys),
       &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
   // https://developer.apple.com/documentation/security/1401659-secitemadd
-  // If we've been asked to store a secret with a label that matches the label
-  // for a preexisting secret, this will return an error without overwriting the
-  // secret (which is the behavior specified by
-  // nsIOSKeyStore.asyncGenerateSecret).
   OSStatus osrv = SecItemAdd(addDictionary.get(), nullptr);
   if (osrv != errSecSuccess) {
     MOZ_LOG(gKeychainSecretLog, LogLevel::Debug,
             ("SecItemAdd failed: %d", osrv));
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
--- a/security/manager/ssl/NSSKeyStore.cpp
+++ b/security/manager/ssl/NSSKeyStore.cpp
@@ -102,16 +102,28 @@ nsresult NSSKeyStore::Unlock() {
 nsresult NSSKeyStore::StoreSecret(const nsACString& aSecret,
                                   const nsACString& aLabel) {
   NS_ENSURE_STATE(mSlot);
   if (NS_FAILED(Unlock())) {
     MOZ_LOG(gNSSKeyStoreLog, LogLevel::Debug, ("Error unlocking NSS key db"));
     return NS_ERROR_FAILURE;
   }
 
+  // It is possible for multiple keys to have the same nickname in NSS. To
+  // prevent the problem of not knowing which key to use in the future, simply
+  // delete all keys with this nickname before storing a new one (if something
+  // else is using our prefix ("org.mozilla.nss.keystore") with the given label,
+  // it may result in breakage).
+  nsresult rv = DeleteSecret(aLabel);
+  if (NS_FAILED(rv)) {
+    MOZ_LOG(gNSSKeyStoreLog, LogLevel::Debug,
+            ("DeleteSecret before StoreSecret failed"));
+    return rv;
+  }
+
   uint8_t* p = BitwiseCast<uint8_t*, const char*>(aSecret.BeginReading());
   UniqueSECItem key(SECITEM_AllocItem(nullptr, nullptr, aSecret.Length()));
   if (!key) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
   key->type = siBuffer;
   memcpy(key->data, p, aSecret.Length());
   key->len = aSecret.Length();
--- a/security/manager/ssl/OSKeyStore.cpp
+++ b/security/manager/ssl/OSKeyStore.cpp
@@ -147,16 +147,19 @@ nsresult OSKeyStore::GenerateSecret(cons
 nsresult OSKeyStore::RecoverSecret(const nsACString& aLabel,
                                    const nsACString& aRecoveryPhrase) {
   NS_ENSURE_STATE(mKs);
   nsAutoCString secret;
   nsresult rv = Base64Decode(aRecoveryPhrase, secret);
   if (NS_FAILED(rv)) {
     return rv;
   }
+  if (secret.Length() != mKs->GetKeyByteLength()) {
+    return NS_ERROR_INVALID_ARG;
+  }
   nsAutoCString label = mLabelPrefix + aLabel;
   rv = mKs->StoreSecret(secret, label);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   return NS_OK;
 }
--- a/security/manager/ssl/ScopedNSSTypes.h
+++ b/security/manager/ssl/ScopedNSSTypes.h
@@ -233,16 +233,28 @@ inline void SECKEYEncryptedPrivateKeyInf
     SECKEYEncryptedPrivateKeyInfo* epki) {
   return SECKEY_DestroyEncryptedPrivateKeyInfo(epki, PR_TRUE);
 }
 
 inline void VFY_DestroyContext_true(VFYContext* ctx) {
   VFY_DestroyContext(ctx, true);
 }
 
+// If this was created via PK11_ListFixedKeysInSlot, we may have a list of keys,
+// in which case we have to free them all (and if not, this will still free the
+// one key).
+inline void FreeOneOrMoreSymKeys(PK11SymKey* keys) {
+  PK11SymKey* next;
+  while (keys) {
+    next = PK11_GetNextSymKey(keys);
+    PK11_FreeSymKey(keys);
+    keys = next;
+  }
+}
+
 }  // namespace internal
 
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueCERTCertificate, CERTCertificate,
                                       CERT_DestroyCertificate)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueCERTCertificateList,
                                       CERTCertificateList,
                                       CERT_DestroyCertificateList)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueCERTCertificatePolicies,
@@ -276,17 +288,17 @@ MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(Un
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePK11GenericObject,
                                       PK11GenericObject,
                                       PK11_DestroyGenericObject)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePK11SlotInfo, PK11SlotInfo,
                                       PK11_FreeSlot)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePK11SlotList, PK11SlotList,
                                       PK11_FreeSlotList)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePK11SymKey, PK11SymKey,
-                                      PK11_FreeSymKey)
+                                      internal::FreeOneOrMoreSymKeys)
 
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePLArenaPool, PLArenaPool,
                                       internal::PORT_FreeArena_false)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePORTString, char, PORT_Free)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePRFileDesc, PRFileDesc, PR_Close)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePRString, char, PR_Free)
 
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueSECAlgorithmID, SECAlgorithmID,
--- a/security/manager/ssl/tests/unit/test_oskeystore.js
+++ b/security/manager/ssl/tests/unit/test_oskeystore.js
@@ -201,8 +201,106 @@ add_task(async function() {
 
   ok(!await keystore.asyncSecretAvailable(LABELS[0]),
      "we didn't recover a secret, so the secret shouldn't be available");
   let recoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
   ok(recoveryPhrase && recoveryPhrase.length > 0,
      "we should be able to re-use that label to generate a new secret");
   await delete_all_secrets();
 });
+
+// Test that re-using a label overwrites any previously-stored secret.
+add_task(async function() {
+  await delete_all_secrets();
+
+  let keystore = Cc["@mozilla.org/security/oskeystore;1"]
+                   .getService(Ci.nsIOSKeyStore);
+
+  let recoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
+  ok(recoveryPhrase, "A recovery phrase should've been created.");
+
+  let text = new Uint8Array([0x66, 0x6f, 0x6f, 0x66]);
+  let ciphertext = await keystore.asyncEncryptBytes(LABELS[0], text.length, text);
+  ok(ciphertext, "We should have a ciphertext now.");
+
+  let newRecoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
+  ok(newRecoveryPhrase, "A new recovery phrase should've been created.");
+
+  // The new secret replaced the old one so we shouldn't be able to decrypt the ciphertext now.
+  await keystore.asyncDecryptBytes(LABELS[0], ciphertext)
+    .then(() => ok(false, "decrypting without the original key should have failed"))
+    .catch(() => ok(true, "decrypting without the original key failed as expected"));
+
+  await keystore.asyncRecoverSecret(LABELS[0], recoveryPhrase);
+  let plaintext = await keystore.asyncDecryptBytes(LABELS[0], ciphertext);
+  ok(plaintext.toString() == text.toString(),
+     "Decrypted plaintext should be the same as text (once we have the original key again).");
+
+  await delete_all_secrets();
+});
+
+// Test that re-using a label (this time using a recovery phrase) overwrites any previously-stored
+// secret.
+add_task(async function() {
+  await delete_all_secrets();
+
+  let keystore = Cc["@mozilla.org/security/oskeystore;1"]
+                   .getService(Ci.nsIOSKeyStore);
+
+  let recoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
+  ok(recoveryPhrase, "A recovery phrase should've been created.");
+
+  let newRecoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
+  ok(newRecoveryPhrase, "A new recovery phrase should've been created.");
+
+  let text = new Uint8Array([0x66, 0x6f, 0x6f, 0x66]);
+  let ciphertext = await keystore.asyncEncryptBytes(LABELS[0], text.length, text);
+  ok(ciphertext, "We should have a ciphertext now.");
+
+  await keystore.asyncRecoverSecret(LABELS[0], recoveryPhrase);
+
+  // We recovered the old secret, so decrypting ciphertext that had been encrypted with the newer
+  // key should fail.
+  await keystore.asyncDecryptBytes(LABELS[0], ciphertext)
+    .then(() => ok(false, "decrypting without the new key should have failed"))
+    .catch(() => ok(true, "decrypting without the new key failed as expected"));
+
+  await keystore.asyncRecoverSecret(LABELS[0], newRecoveryPhrase);
+  let plaintext = await keystore.asyncDecryptBytes(LABELS[0], ciphertext);
+  ok(plaintext.toString() == text.toString(),
+     "Decrypted plaintext should be the same as text (once we have the new key again).");
+
+  await delete_all_secrets();
+});
+
+// Test that trying to use recovery phrases that are the wrong size fails.
+add_task(async function() {
+  await delete_all_secrets();
+
+  let keystore = Cc["@mozilla.org/security/oskeystore;1"]
+                   .getService(Ci.nsIOSKeyStore);
+
+  await keystore.asyncRecoverSecret(LABELS[0], "")
+    .then(() => ok(false, "'recovering' with an empty key should have failed"))
+    .catch(() => ok(true, "'recovering' with an empty key failed as expected"));
+  ok(!await keystore.asyncSecretAvailable(LABELS[0]),
+     "we didn't recover a secret, so the secret shouldn't be available");
+
+  await keystore.asyncRecoverSecret(LABELS[0], "AAAAAA")
+    .then(() => ok(false, "recovering with a key that is too short should have failed"))
+    .catch(() => ok(true, "recovering with a key that is too short failed as expected"));
+  ok(!await keystore.asyncSecretAvailable(LABELS[0]),
+     "we didn't recover a secret, so the secret shouldn't be available");
+
+  await keystore.asyncRecoverSecret(LABELS[0], "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA")
+    .then(() => ok(false, "recovering with a key that is too long should have failed"))
+    .catch(() => ok(true, "recovering with a key that is too long failed as expected"));
+  ok(!await keystore.asyncSecretAvailable(LABELS[0]),
+     "we didn't recover a secret, so the secret shouldn't be available");
+
+  let recoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
+  ok(recoveryPhrase && recoveryPhrase.length > 0,
+     "we should be able to use that label to generate a new secret");
+  ok(await keystore.asyncSecretAvailable(LABELS[0]),
+     "the generated secret should now be available");
+
+  await delete_all_secrets();
+});