modify*Key: improve handling of passphrase modification (add tests, too)

OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java

@@ -45,7 +45,7 @@ import java.util.Random;
 public class PgpKeyOperationTest {
 
     static UncachedKeyRing staticRing;
-    static String passphrase;
+    final static String passphrase = genPassphrase();
 
     UncachedKeyRing ring;
     PgpKeyOperation op;
@@ -58,18 +58,6 @@ public class PgpKeyOperationTest {
         Security.insertProviderAt(new BouncyCastleProvider(), 1);
         ShadowLog.stream = System.out;
 
-        {
-            String chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789!@#$%^&*()-_=";
-            Random r = new Random();
-            StringBuilder passbuilder = new StringBuilder();
-            // 20% chance for an empty passphrase
-            for(int i = 0, j = r.nextInt(10) > 2 ? r.nextInt(20) : 0; i < j; i++) {
-                passbuilder.append(chars.charAt(r.nextInt(chars.length())));
-            }
-            passphrase = passbuilder.toString();
-            System.out.println("Passphrase is '" + passphrase + "'");
-        }
-
         SaveKeyringParcel parcel = new SaveKeyringParcel();
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
                 PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER, 0L));
@@ -853,12 +841,79 @@ public class PgpKeyOperationTest {
 
     }
 
+    @Test
+    public void testPassphraseChange() throws Exception {
+
+        // change passphrase to empty
+        parcel.mNewPassphrase = "";
+        UncachedKeyRing modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB);
+
+        Assert.assertEquals("exactly three packets should have been modified (the secret keys)",
+                3, onlyB.size());
+
+        // remember secret key packet with no passphrase for later
+        RawPacket sKeyNoPassphrase = onlyB.get(1);
+        Assert.assertEquals("extracted packet should be a secret subkey",
+                PacketTags.SECRET_SUBKEY, sKeyNoPassphrase.tag);
+
+        // modify keyring, change to non-empty passphrase
+        String otherPassphrase = genPassphrase(true);
+        parcel.mNewPassphrase = otherPassphrase;
+        modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB, "");
+
+        RawPacket sKeyWithPassphrase = onlyB.get(1);
+        Assert.assertEquals("extracted packet should be a secret subkey",
+                PacketTags.SECRET_SUBKEY, sKeyNoPassphrase.tag);
+
+        String otherPassphrase2 = genPassphrase(true);
+        parcel.mNewPassphrase = otherPassphrase2;
+        {
+            // if we replace a secret key with one without passphrase
+            modified = KeyringTestingHelper.removePacket(modified, sKeyNoPassphrase.position);
+            modified = KeyringTestingHelper.injectPacket(modified, sKeyNoPassphrase.buf, sKeyNoPassphrase.position);
+
+            // we should still be able to modify it (and change its passphrase) without errors
+            PgpKeyOperation op = new PgpKeyOperation(null);
+            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(modified.getEncoded(), false, 0);
+            EditKeyResult result = op.modifySecretKeyRing(secretRing, parcel, otherPassphrase);
+            Assert.assertTrue("key modification must succeed", result.success());
+            Assert.assertFalse("log must not contain a warning",
+                    result.getLog().containsWarnings());
+            Assert.assertTrue("log must contain an empty passphrase retry notice",
+                result.getLog().containsType(LogType.MSG_MF_PASSPHRASE_EMPTY_RETRY));
+            modified = result.getRing();
+        }
+
+        {
+            // if we add one subkey with a different passphrase, that should produce a warning but also work
+            modified = KeyringTestingHelper.removePacket(modified, sKeyWithPassphrase.position);
+            modified = KeyringTestingHelper.injectPacket(modified, sKeyWithPassphrase.buf, sKeyWithPassphrase.position);
+
+            PgpKeyOperation op = new PgpKeyOperation(null);
+            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(modified.getEncoded(), false, 0);
+            EditKeyResult result = op.modifySecretKeyRing(secretRing, parcel, otherPassphrase2);
+            Assert.assertTrue("key modification must succeed", result.success());
+            Assert.assertTrue("log must contain a warning",
+                    result.getLog().containsWarnings());
+            Assert.assertTrue("log must contain a failed passphrase change warning",
+                    result.getLog().containsType(LogType.MSG_MF_PASSPHRASE_FAIL));
+        }
+
+    }
 
     private static UncachedKeyRing applyModificationWithChecks(SaveKeyringParcel parcel,
                                                                UncachedKeyRing ring,
                                                                ArrayList<RawPacket> onlyA,
                                                                ArrayList<RawPacket> onlyB) {
-        return applyModificationWithChecks(parcel, ring, onlyA, onlyB, true, true);
+        return applyModificationWithChecks(parcel, ring, onlyA, onlyB, passphrase, true, true);
+    }
+
+    private static UncachedKeyRing applyModificationWithChecks(SaveKeyringParcel parcel,
+                                                               UncachedKeyRing ring,
+                                                               ArrayList<RawPacket> onlyA,
+                                                               ArrayList<RawPacket> onlyB,
+                                                               String passphrase) {
+        return applyModificationWithChecks(parcel, ring, onlyA, onlyB, passphrase, true, true);
     }
 
     // applies a parcel modification while running some integrity checks
@@ -866,6 +921,7 @@ public class PgpKeyOperationTest {
                                                                UncachedKeyRing ring,
                                                                ArrayList<RawPacket> onlyA,
                                                                ArrayList<RawPacket> onlyB,
+                                                               String passphrase,
                                                                boolean canonicalize,
                                                                boolean constantCanonicalize) {
 
@@ -980,4 +1036,20 @@ public class PgpKeyOperationTest {
 
     }
 
+    private static String genPassphrase() {
+        return genPassphrase(false);
+    }
+
+    private static String genPassphrase(boolean noEmpty) {
+        String chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789!@#$%^&*()-_=";
+        Random r = new Random();
+        StringBuilder passbuilder = new StringBuilder();
+        // 20% chance for an empty passphrase
+        for(int i = 0, j = noEmpty || r.nextInt(10) > 2 ? r.nextInt(20)+1 : 0; i < j; i++) {
+            passbuilder.append(chars.charAt(r.nextInt(chars.length())));
+        }
+        System.out.println("Generated passphrase: '" + passbuilder.toString() + "'");
+        return passbuilder.toString();
+    }
+
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java

@@ -698,7 +698,7 @@ public class PgpKeyOperation {
                     // Build key encrypter and decrypter based on passphrase
                     PBESecretKeyEncryptor keyEncryptor = new JcePBESecretKeyEncryptorBuilder(
                             PGPEncryptedData.CAST5, sha1Calc)
-                            .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build("".toCharArray());
+                            .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build(passphrase.toCharArray());
 
                     sKey = new PGPSecretKey(keyPair.getPrivateKey(), pKey,
                             sha1Calc, false, keyEncryptor);
@@ -716,6 +716,8 @@ public class PgpKeyOperation {
             if (saveParcel.mNewPassphrase != null) {
                 progress(R.string.progress_modify_passphrase, 90);
                 log.add(LogLevel.INFO, LogType.MSG_MF_PASSPHRASE, indent);
+                indent += 1;
+
                 PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder().build()
                         .get(HashAlgorithmTags.SHA1);
                 PBESecretKeyDecryptor keyDecryptor = new JcePBESecretKeyDecryptorBuilder().setProvider(
@@ -726,7 +728,51 @@ public class PgpKeyOperation {
                         .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build(
                                 saveParcel.mNewPassphrase.toCharArray());
 
-                sKR = PGPSecretKeyRing.copyWithNewPassword(sKR, keyDecryptor, keyEncryptorNew);
+                // noinspection unchecked
+                for (PGPSecretKey sKey : new IterableIterator<PGPSecretKey>(sKR.getSecretKeys())) {
+                    log.add(LogLevel.DEBUG, LogType.MSG_MF_PASSPHRASE_KEY, indent,
+                            PgpKeyHelper.convertKeyIdToHex(sKey.getKeyID()));
+
+                    boolean ok = false;
+
+                    try {
+                        // try to set new passphrase
+                        sKey = PGPSecretKey.copyWithNewPassword(sKey, keyDecryptor, keyEncryptorNew);
+                        ok = true;
+                    } catch (PGPException e) {
+
+                        // if this is the master key, error!
+                        if (sKey.getKeyID() == masterPublicKey.getKeyID()) {
+                            log.add(LogLevel.ERROR, LogType.MSG_MF_ERROR_PASSPHRASE_MASTER, indent+1);
+                            return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null);
+                        }
+
+                        // being in here means decrypt failed, likely due to a bad passphrase try
+                        // again with an empty passphrase, maybe we can salvage this
+                        try {
+                            log.add(LogLevel.DEBUG, LogType.MSG_MF_PASSPHRASE_EMPTY_RETRY, indent+1);
+                            PBESecretKeyDecryptor emptyDecryptor =
+                                new JcePBESecretKeyDecryptorBuilder().setProvider(
+                                    Constants.BOUNCY_CASTLE_PROVIDER_NAME).build("".toCharArray());
+                            sKey = PGPSecretKey.copyWithNewPassword(sKey, emptyDecryptor, keyEncryptorNew);
+                            ok = true;
+                        } catch (PGPException e2) {
+                            // non-fatal but not ok, handled below
+                        }
+                    }
+
+                    if (!ok) {
+                        // for a subkey, it's merely a warning
+                        log.add(LogLevel.WARN, LogType.MSG_MF_PASSPHRASE_FAIL, indent+1,
+                                PgpKeyHelper.convertKeyIdToHex(sKey.getKeyID()));
+                        continue;
+                    }
+
+                    sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey);
+
+                }
+
+                indent -= 1;
             }
 
         } catch (IOException e) {

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java

@@ -364,6 +364,7 @@ public class OperationResultParcel implements Parcelable {
         MSG_MF_ERROR_NOEXIST_PRIMARY (R.string.msg_mf_error_noexist_primary),
         MSG_MF_ERROR_NOEXIST_REVOKE (R.string.msg_mf_error_noexist_revoke),
         MSG_MF_ERROR_NULL_EXPIRY (R.string.msg_mf_error_null_expiry),
+        MSG_MF_ERROR_PASSPHRASE_MASTER(R.string.msg_mf_error_passphrase_master),
         MSG_MF_ERROR_PAST_EXPIRY(R.string.msg_mf_error_past_expiry),
         MSG_MF_ERROR_PGP (R.string.msg_mf_error_pgp),
         MSG_MF_ERROR_REVOKED_PRIMARY (R.string.msg_mf_error_revoked_primary),
@@ -371,6 +372,9 @@ public class OperationResultParcel implements Parcelable {
         MSG_MF_ERROR_SUBKEY_MISSING(R.string.msg_mf_error_subkey_missing),
         MSG_MF_MASTER (R.string.msg_mf_master),
         MSG_MF_PASSPHRASE (R.string.msg_mf_passphrase),
+        MSG_MF_PASSPHRASE_KEY (R.string.msg_mf_passphrase_key),
+        MSG_MF_PASSPHRASE_EMPTY_RETRY (R.string.msg_mf_passphrase_empty_retry),
+        MSG_MF_PASSPHRASE_FAIL (R.string.msg_mf_passphrase_fail),
         MSG_MF_PRIMARY_REPLACE_OLD (R.string.msg_mf_primary_replace_old),
         MSG_MF_PRIMARY_NEW (R.string.msg_mf_primary_new),
         MSG_MF_SUBKEY_CHANGE (R.string.msg_mf_subkey_change),

OpenKeychain/src/main/res/values/strings.xml

@@ -644,10 +644,14 @@
     <string name="msg_mf_error_noexist_revoke">Bad user id for revocation specified!</string>
     <string name="msg_mf_error_revoked_primary">Revoked user ids cannot be primary!</string>
     <string name="msg_mf_error_null_expiry">Expiry time cannot be "same as before" on subkey creation. This is a programming error, please file a bug report!</string>
+    <string name="msg_mf_error_passphrase_master">Fatal error decrypting master key! This is likely a programming error, please file a bug report!</string>
     <string name="msg_mf_error_pgp">PGP internal exception!</string>
     <string name="msg_mf_error_sig">Signature exception!</string>
     <string name="msg_mf_master">Modifying master certifications</string>
-    <string name="msg_mf_passphrase">Changing passphrase</string>
+    <string name="msg_mf_passphrase">Changing passphrase for keyring…</string>
+    <string name="msg_mf_passphrase_key">Changing passphrase for subkey %s</string>
+    <string name="msg_mf_passphrase_empty_retry">Setting new passphrase failed, trying again with empty old passphrase</string>
+    <string name="msg_mf_passphrase_fail">Passphrase for subkey could not be changed! (Does it have a different one from the other keys?)</string>
     <string name="msg_mf_primary_replace_old">Replacing certificate of previous primary user id</string>
     <string name="msg_mf_primary_new">Generating new certificate for new primary user id</string>
     <string name="msg_mf_subkey_change">Modifying subkey %s</string>