Bug 1673239 - OpenPGP key material should remain locked in memory in failure scenarios. r=mkmelin a=wsmwk
authorKai Engert <kaie@kuix.de>
Fri, 19 Feb 2021 12:57:02 +0200
changeset 40298 0c8606e7f45d805bf12d8e67828c24a343b34f11
parent 40297 b5383265242b0be09bf5a3f00619f8c069b174e7
child 40299 41dfa822c6c0b3cfda385548a394ae42d01249a4
push id161
push userthunderbird@calypsoblue.org
push dateThu, 04 Mar 2021 23:46:16 +0000
treeherdercomm-esr78@41dfa822c6c0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmkmelin, wsmwk
bugs1673239
Bug 1673239 - OpenPGP key material should remain locked in memory in failure scenarios. r=mkmelin a=wsmwk Differential Revision: https://phabricator.services.mozilla.com/D105494
mail/extensions/openpgp/content/modules/RNP.jsm
--- a/mail/extensions/openpgp/content/modules/RNP.jsm
+++ b/mail/extensions/openpgp/content/modules/RNP.jsm
@@ -1505,35 +1505,33 @@ var RNP = {
     }
 
     if (expireSeconds != 0) {
       if (RNPLib.rnp_op_generate_set_expiration(genOp, expireSeconds)) {
         throw new Error("rnp_op_generate_set_expiration sub failed");
       }
     }
 
-    let lockFailure = false;
+    let unlocked = false;
     try {
       if (passphrase != null && passphrase.length != 0) {
         if (RNPLib.rnp_key_unlock(primaryKey, passphrase)) {
           throw new Error("rnp_key_unlock failed");
         }
+        unlocked = true;
       }
 
       if (RNPLib.rnp_op_generate_execute(genOp)) {
         throw new Error("rnp_op_generate_execute sub failed");
       }
     } finally {
-      if (RNPLib.rnp_key_lock(primaryKey)) {
-        lockFailure = true;
+      if (unlocked) {
+        RNPLib.rnp_key_lock(primaryKey);
       }
     }
-    if (lockFailure) {
-      throw new Error("rnp_key_lock failed");
-    }
 
     RNPLib.rnp_op_generate_destroy(genOp);
     RNPLib.rnp_key_handle_destroy(primaryKey);
 
     await PgpSqliteDb2.acceptAsPersonalKey(newKeyFingerprint);
 
     return newKeyId;
   },
@@ -1808,32 +1806,43 @@ var RNP = {
       if (impKey.isNull()) {
         throw new Error("cannot get key handle for imported key: " + k.fpr);
       }
 
       let unableToUnprotectId = null;
 
       if (k.secretAvailable) {
         while (!userFlags.canceled) {
+          // After we unprotect a key, immediately re-protect it with
+          // the new passphrase. If a failure occurrs at some point,
+          // all keys remain protected in memory.
+
           let rv = RNPLib.rnp_key_unprotect(impKey, recentPass);
-
           if (rv == 0) {
+            if (RNPLib.rnp_key_protect(impKey, newPass, null, null, null, 0)) {
+              throw new Error("rnp_key_protect failed");
+            }
+
             let sub_count = new ctypes.size_t();
             if (RNPLib.rnp_key_get_subkey_count(impKey, sub_count.address())) {
               throw new Error("rnp_key_get_subkey_count failed");
             }
             for (let i = 0; i < sub_count.value && !unableToUnprotectId; i++) {
               let sub_handle = new RNPLib.rnp_key_handle_t();
               if (
                 RNPLib.rnp_key_get_subkey_at(impKey, i, sub_handle.address())
               ) {
                 throw new Error("rnp_key_get_subkey_at failed");
               }
               if (RNPLib.rnp_key_unprotect(sub_handle, recentPass)) {
                 unableToUnprotectId = RNP.getKeyIDFromHandle(sub_handle);
+              } else if (
+                RNPLib.rnp_key_protect(sub_handle, newPass, null, null, null, 0)
+              ) {
+                throw new Error("rnp_key_protect failed");
               }
               RNPLib.rnp_key_handle_destroy(sub_handle);
             }
             break;
           }
 
           if (unableToUnprotectId) {
             break;
@@ -1954,25 +1963,16 @@ var RNP = {
             input_from_memory,
             importFlags,
             null
           )
         ) {
           throw new Error("rnp_import_keys failed");
         }
 
-        let impKey2 = await this.getKeyHandleByIdentifier(
-          RNPLib.ffi,
-          "0x" + k.fpr
-        );
-        if (k.secretAvailable) {
-          this.protectKeyWithSubKeys(impKey2, newPass);
-        }
-        RNPLib.rnp_key_handle_destroy(impKey2);
-
         result.importedKeys.push("0x" + k.id);
 
         RNPLib.rnp_input_destroy(input_from_memory);
         RNPLib.rnp_output_destroy(output_to_memory);
         RNPLib.rnp_key_handle_destroy(impKey);
 
         // For acceptance "undecided", we don't store it, because that's
         // the default if no value is stored.
@@ -2813,22 +2813,22 @@ var RNP = {
       if (RNPLib.rnp_output_to_memory(output_to_memory.address(), 0)) {
         throw new Error("rnp_output_to_memory failed");
       }
 
       if (RNPLib.rnp_key_unlock(expKey, internalPassword)) {
         throw new Error("rnp_key_unlock failed");
       }
 
-      if (RNPLib.rnp_key_export(expKey, output_to_memory, exportFlags)) {
-        throw new Error("rnp_key_export failed");
-      }
-
-      if (RNPLib.rnp_key_lock(expKey)) {
-        throw new Error("rnp_key_unlock failed");
+      try {
+        if (RNPLib.rnp_key_export(expKey, output_to_memory, exportFlags)) {
+          throw new Error("rnp_key_export failed");
+        }
+      } finally {
+        RNPLib.rnp_key_lock(expKey);
       }
 
       let result_buf = new ctypes.uint8_t.ptr();
       let result_len = new ctypes.size_t();
       if (
         RNPLib.rnp_output_memory_get_buf(
           output_to_memory,
           result_buf.address(),
@@ -2859,49 +2859,57 @@ var RNP = {
       }
 
       let tempKey = await this.getKeyHandleByIdentifier(tempFFI, fprStr);
 
       if (RNPLib.rnp_key_unlock(tempKey, internalPassword)) {
         throw new Error("rnp_key_unlock failed");
       }
 
-      if (
-        RNPLib.rnp_key_protect(tempKey, backupPassword, null, null, null, 0)
-      ) {
-        throw new Error("rnp_key_protect failed");
+      try {
+        if (
+          RNPLib.rnp_key_protect(tempKey, backupPassword, null, null, null, 0)
+        ) {
+          throw new Error("rnp_key_protect failed");
+        }
+      } finally {
+        RNPLib.rnp_key_lock(tempKey);
       }
 
       let sub_count = new ctypes.size_t();
       if (RNPLib.rnp_key_get_subkey_count(tempKey, sub_count.address())) {
         throw new Error("rnp_key_get_subkey_count failed");
       }
       for (let i = 0; i < sub_count.value; i++) {
         let sub_handle = new RNPLib.rnp_key_handle_t();
         if (RNPLib.rnp_key_get_subkey_at(tempKey, i, sub_handle.address())) {
           throw new Error("rnp_key_get_subkey_at failed");
         }
 
         if (RNPLib.rnp_key_unlock(sub_handle, internalPassword)) {
           throw new Error("rnp_key_unlock failed");
         }
 
-        if (
-          RNPLib.rnp_key_protect(
-            sub_handle,
-            backupPassword,
-            null,
-            null,
-            null,
-            0
-          )
-        ) {
-          throw new Error("rnp_key_protect failed");
+        try {
+          if (
+            RNPLib.rnp_key_protect(
+              sub_handle,
+              backupPassword,
+              null,
+              null,
+              null,
+              0
+            )
+          ) {
+            throw new Error("rnp_key_protect failed");
+          }
+        } finally {
+          RNPLib.rnp_key_lock(sub_handle);
+          RNPLib.rnp_key_handle_destroy(sub_handle);
         }
-        RNPLib.rnp_key_handle_destroy(sub_handle);
       }
 
       if (RNPLib.rnp_key_export(tempKey, out_binary, exportFlags)) {
         throw new Error("rnp_key_export failed");
       }
       RNPLib.rnp_key_handle_destroy(tempKey);
 
       RNPLib.rnp_input_destroy(input_from_memory);
@@ -3050,51 +3058,46 @@ var RNP = {
   // Will change the expiration date of all given keys to newExpiry.
   // fingerprintArray is an array, containing fingerprints, both
   // primary key fingerprints and subkey fingerprints are allowed.
   // Currently, this function assumes that for any subkey that is
   // being changeed, the respective primary key is contained in the
   // array, too. If it isn't, the function will fail, because the
   // primary key must be unlocked, before changing a subkey works.
   async changeExpirationDate(fingerprintArray, newExpiry) {
-    let handles = [];
+    let pass = await OpenPGPMasterpass.retrieveOpenPGPPassword();
 
     for (let fingerprint of fingerprintArray) {
       let handle = this.getKeyHandleByKeyIdOrFingerprint(
         RNPLib.ffi,
         "0x" + fingerprint
       );
 
       if (handle.isNull()) {
         return false;
       }
-      handles.push(handle);
-    }
-
-    for (let handle of handles) {
-      let pass = await OpenPGPMasterpass.retrieveOpenPGPPassword();
-      if (RNPLib.rnp_key_unlock(handle, pass)) {
-        throw new Error("rnp_key_unlock failed");
+
+      let unlocked = false;
+      try {
+        if (RNPLib.rnp_key_unlock(handle, pass)) {
+          throw new Error("rnp_key_unlock failed");
+        }
+        unlocked = true;
+
+        if (RNPLib.rnp_key_set_expiration(handle, newExpiry)) {
+          throw new Error("rnp_key_set_expiration failed");
+        }
+      } finally {
+        if (unlocked) {
+          RNPLib.rnp_key_lock(handle);
+        }
+        RNPLib.rnp_key_handle_destroy(handle);
       }
     }
 
-    for (let handle of handles) {
-      if (RNPLib.rnp_key_set_expiration(handle, newExpiry)) {
-        throw new Error("rnp_key_set_expiration failed");
-      }
-    }
-
-    for (let handle of handles) {
-      if (RNPLib.rnp_key_lock(handle)) {
-        throw new Error("rnp_key_lock failed");
-      }
-
-      RNPLib.rnp_key_handle_destroy(handle);
-    }
-
     await this.saveKeyRings();
     return true;
   },
 
   getAutocryptKeyB64(primaryKeyId, subKeyId, uidString) {
     let primHandle = this.getKeyHandleByKeyIdOrFingerprint(
       RNPLib.ffi,
       primaryKeyId