diff options
4 files changed, 143 insertions, 17 deletions
diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java index 00bbafed1..964512617 100644 --- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java +++ b/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(); + } + } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java index 222c47215..cdd2eacc0 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/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) { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java index 8e7dcd4aa..26fe63550 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java +++ b/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), diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index f6c208ca0..5259336ec 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/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> |