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

--HG--
extra : moz-landing-system : lando

security/manager/ssl/KeychainSecret.cpp

@@ -69,6 +69,16 @@ nsresult KeychainSecret::StoreSecret(const nsACString& aSecret,
   // 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) {
@@ -90,10 +100,6 @@ nsresult KeychainSecret::StoreSecret(const nsACString& aSecret,
       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,

security/manager/ssl/tests/unit/test_oskeystore.js

@@ -206,3 +206,86 @@ add_task(async function() {
      "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 an empty recovery phrase "works" but is essentially a no-op.
+add_task(async function() {
+  await delete_all_secrets();
+
+  let keystore = Cc["@mozilla.org/security/oskeystore;1"]
+                   .getService(Ci.nsIOSKeyStore);
+  await keystore.asyncRecoverSecret(LABELS[0], "");
+
+  ok(!await keystore.asyncSecretAvailable(LABELS[0]),
+     "we didn't really 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");
+  ok(await keystore.asyncSecretAvailable(LABELS[0]),
+     "the generated secret should now be available");
+
+  await delete_all_secrets();
+});