Bug 1571555 - Use a blank string in place of the username or password when decryption fails. r=keeler

Don't show the login in about:logins if the username or password cannot be decrypted.

Differential Revision: https://phabricator.services.mozilla.com/D40845

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

browser/components/aboutlogins/tests/browser/browser_masterPassword.js

@@ -69,7 +69,8 @@ add_task(async function test() {
     () =>
       (notification = gBrowser
         .getNotificationBox()
-        .getNotificationWithValue("master-password-login-required"))
+        .getNotificationWithValue("master-password-login-required")),
+    "waiting for master-password-login-required notification"
   );
 
   ok(

security/manager/ssl/SecretDecoderRing.cpp

@@ -26,6 +26,8 @@
 #include "pk11sdr.h"  // For PK11SDR_Encrypt, PK11SDR_Decrypt
 #include "ssl.h"      // For SSL_ClearSessionCache
 
+static mozilla::LazyLogModule gSDRLog("sdrlog");
+
 using namespace mozilla;
 using dom::Promise;
 
@@ -73,8 +75,22 @@ void BackgroundSdrDecryptStrings(const nsTArray<nsCString>& encryptedStrings,
     nsCString plainText;
     rv = sdrService->DecryptString(encryptedString, plainText);
 
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      break;
+    if (NS_FAILED(rv)) {
+      if (rv == NS_ERROR_NOT_AVAILABLE) {
+        // Master Password entry was canceled. Don't keep prompting again.
+        break;
+      }
+
+      // NS_ERROR_ILLEGAL_VALUE or NS_ERROR_FAILURE could be due to bad data for
+      // a single string but we still want to decrypt the others.
+      // Callers of `decryptMany` in crypto-SDR.js assume there will be an
+      // equal number of usernames and passwords so use an empty string to keep
+      // this assumption true.
+      MOZ_LOG(gSDRLog, LogLevel::Warning,
+              ("Couldn't decrypt string: %s", encryptedString.get()));
+      plainTexts.AppendElement(nullptr);
+      rv = NS_OK;
+      continue;
     }
 
     plainTexts.AppendElement(NS_ConvertUTF8toUTF16(plainText));

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

@@ -187,3 +187,60 @@ add_task(async function testAsyncDecryptStrings() {
     );
   }
 });
+
+add_task(async function testAsyncDecryptInvalidStrings() {
+  let sdr = Cc["@mozilla.org/security/sdr;1"].getService(
+    Ci.nsISecretDecoderRing
+  );
+
+  // Test invalid inputs for sdr.asyncDecryptStrings
+  let testCases = [
+    "~bmV0cGxheQ==", // invalid base64 encoding
+    "bmV0cGxheQ==", // valid base64 characters but not encrypted
+    "https://www.example.com", // website address from erroneous migration
+  ];
+
+  let decrypteds = await sdr.asyncDecryptStrings(testCases);
+  equal(
+    decrypteds.length,
+    testCases.length,
+    "each testcase should still return a response"
+  );
+  for (let i = 0; i < decrypteds.length; i++) {
+    let decrypted = decrypteds[i];
+
+    equal(
+      decrypted,
+      "",
+      "decrypted string should be empty when trying to decrypt an invalid input with asyncDecryptStrings"
+    );
+
+    Assert.throws(
+      () => sdr.decryptString(testCases[i]),
+      /NS_ERROR_ILLEGAL_VALUE|NS_ERROR_FAILURE/,
+      `Check testcase would have thrown: ${testCases[i]}`
+    );
+  }
+});
+
+add_task(async function testAsyncDecryptLoggedOut() {
+  // Set a master password.
+  let token = Cc["@mozilla.org/security/pk11tokendb;1"]
+    .getService(Ci.nsIPK11TokenDB)
+    .getInternalKeyToken();
+  token.initPassword("password");
+  token.logoutSimple();
+
+  let sdr = Cc["@mozilla.org/security/sdr;1"].getService(
+    Ci.nsISecretDecoderRing
+  );
+
+  await Assert.rejects(
+    sdr.asyncDecryptStrings(["irrelevant"]),
+    /NS_ERROR_NOT_AVAILABLE/,
+    "Check error is thrown instead of returning empty strings"
+  );
+
+  token.reset();
+  token.initPassword("");
+});

toolkit/components/passwordmgr/crypto-SDR.js

@@ -215,11 +215,14 @@ LoginManagerCrypto_SDR.prototype = {
     return plainText;
   },
 
-  /*
+  /**
    * Decrypts the specified strings, using the SecretDecoderRing.
    *
-   * Returns a promise which resolves with the the decrypted strings,
-   * or throws/rejects with an error if there was a problem.
+   * @resolve {string[]} The decrypted strings. If a string cannot
+   * be decrypted, the empty string is returned for that instance.
+   * Callers will need to use decrypt() to determine if the encrypted
+   * string is invalid or intentionally empty. Throws/reject with
+   * an error if there was a problem.
    */
   async decryptMany(cipherTexts) {
     if (!Array.isArray(cipherTexts) || !cipherTexts.length) {

toolkit/components/passwordmgr/storage-json.js

@@ -309,12 +309,14 @@ this.LoginManagerStorage_json.prototype = {
     // decrypt entries for caller.
     logins = this._decryptLogins(logins);
 
-    this.log("_getAllLogins: returning", logins.length, "logins.");
+    this.log("getAllLogins: returning", logins.length, "logins.");
     return logins;
   },
 
   /**
-   * Returns an array of nsILoginInfo.
+   * Returns an array of nsILoginInfo. If decryption of a login
+   * fails due to a corrupt entry, the login is not included in
+   * the resulting array.
    *
    * @resolve {nsILoginInfo[]}
    */
@@ -332,6 +334,29 @@ this.LoginManagerStorage_json.prototype = {
 
     let result = [];
     for (let i = 0; i < logins.length; i++) {
+      if (!usernames[i] || !passwords[i]) {
+        // If the username or password is blank it means that decryption may have
+        // failed during decryptMany but we can't differentiate an empty string
+        // value from a failure so we attempt to decrypt again and check the
+        // result.
+        let login = logins[i];
+        try {
+          this._crypto.decrypt(login.username);
+          this._crypto.decrypt(login.password);
+        } catch (e) {
+          // If decryption failed (corrupt entry?), just skip it.
+          // Rethrow other errors (like canceling entry of a master pw)
+          if (e.result == Cr.NS_ERROR_FAILURE) {
+            this.log(
+              "Could not decrypt login:",
+              login.QueryInterface(Ci.nsILoginMetaInfo).guid
+            );
+            continue;
+          }
+          throw e;
+        }
+      }
+
       logins[i].username = usernames[i];
       logins[i].password = passwords[i];
       result.push(logins[i]);

toolkit/components/passwordmgr/storage-mozStorage.js

@@ -439,7 +439,9 @@ LoginManagerStorage_mozStorage.prototype = {
   },
 
   /**
-   * Returns an array of nsILoginInfo.
+   * Returns an array of nsILoginInfo. If decryption of a login
+   * fails due to a corrupt entry, the login is not included in
+   * the resulting array.
    *
    * @resolve {nsILoginInfo[]}
    */
@@ -457,6 +459,29 @@ LoginManagerStorage_mozStorage.prototype = {
 
     let result = [];
     for (let i = 0; i < logins.length; i++) {
+      if (!usernames[i] || !passwords[i]) {
+        // If the username or password is blank it means that decryption may have
+        // failed during decryptMany but we can't differentiate an empty string
+        // value from a failure so we attempt to decrypt again and check the
+        // result.
+        let login = logins[i];
+        try {
+          this._crypto.decrypt(login.username);
+          this._crypto.decrypt(login.password);
+        } catch (e) {
+          // If decryption failed (corrupt entry?), just skip it.
+          // Rethrow other errors (like canceling entry of a master pw)
+          if (e.result == Cr.NS_ERROR_FAILURE) {
+            this.log(
+              "Could not decrypt login:",
+              login.QueryInterface(Ci.nsILoginMetaInfo).guid
+            );
+            continue;
+          }
+          throw e;
+        }
+      }
+
       logins[i].username = usernames[i];
       logins[i].password = passwords[i];
       result.push(logins[i]);

toolkit/components/passwordmgr/test/unit/test_logins_decrypt_failure.js

@@ -26,7 +26,7 @@ function resetMasterPassword() {
 /**
  * Resets the master password after some logins were added to the database.
  */
-add_task(function test_logins_decrypt_failure() {
+add_task(async function test_logins_decrypt_failure() {
   let logins = TestData.loginList();
   for (let loginInfo of logins) {
     Services.logins.addLogin(loginInfo);
@@ -37,6 +37,11 @@ add_task(function test_logins_decrypt_failure() {
 
   // These functions don't see the non-decryptable entries anymore.
   Assert.equal(Services.logins.getAllLogins().length, 0);
+  Assert.equal(
+    (await Services.logins.getAllLoginsAsync()).length,
+    0,
+    "getAllLoginsAsync length"
+  );
   Assert.equal(Services.logins.findLogins("", "", "").length, 0);
   Assert.equal(Services.logins.searchLogins(newPropertyBag()).length, 0);
   Assert.throws(
@@ -56,6 +61,11 @@ add_task(function test_logins_decrypt_failure() {
     Services.logins.addLogin(loginInfo);
   }
   LoginTestUtils.checkLogins(logins);
+  Assert.equal(
+    (await Services.logins.getAllLoginsAsync()).length,
+    logins.length,
+    "getAllLoginsAsync length"
+  );
   Assert.equal(Services.logins.countLogins("", "", ""), logins.length * 2);
 
   // Finding logins doesn't return the non-decryptable duplicates.