From 194e4d7631fec6c53b887c532666aaa8b6365477 Mon Sep 17 00:00:00 2001 From: Paul Schaub Date: Fri, 10 Sep 2021 20:14:12 +0200 Subject: [PATCH] Automatically 'repair' keys with S2K usage CHECKSUM to use SHA1 when changing passphrases --- .../secretkeyring/SecretKeyRingEditor.java | 28 ++++++++++++++----- ...S2KUsageTest.java => S2KUsageFixTest.java} | 10 ++----- 2 files changed, 24 insertions(+), 14 deletions(-) rename pgpainless-core/src/test/java/org/pgpainless/key/protection/fixes/{EnsureSecureS2KUsageTest.java => S2KUsageFixTest.java} (90%) diff --git a/pgpainless-core/src/main/java/org/pgpainless/key/modification/secretkeyring/SecretKeyRingEditor.java b/pgpainless-core/src/main/java/org/pgpainless/key/modification/secretkeyring/SecretKeyRingEditor.java index 97834e35..d480a55a 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/key/modification/secretkeyring/SecretKeyRingEditor.java +++ b/pgpainless-core/src/main/java/org/pgpainless/key/modification/secretkeyring/SecretKeyRingEditor.java @@ -30,6 +30,7 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.bouncycastle.bcpg.S2K; +import org.bouncycastle.bcpg.SecretKeyPacket; import org.bouncycastle.openpgp.PGPException; import org.bouncycastle.openpgp.PGPKeyPair; import org.bouncycastle.openpgp.PGPKeyRingGenerator; @@ -59,6 +60,7 @@ import org.pgpainless.key.protection.PasswordBasedSecretKeyRingProtector; import org.pgpainless.key.protection.SecretKeyRingProtector; import org.pgpainless.key.protection.UnlockSecretKey; import org.pgpainless.key.protection.UnprotectedKeysProtector; +import org.pgpainless.key.protection.fixes.S2KUsageFix; import org.pgpainless.key.protection.passphrase_provider.SolitaryPassphraseProvider; import org.pgpainless.key.util.KeyRingUtils; import org.pgpainless.key.util.RevocationAttributes; @@ -591,32 +593,44 @@ public class SecretKeyRingEditor implements SecretKeyRingEditorInterface { PGPSecretKeyRing secretKeys, SecretKeyRingProtector oldProtector, SecretKeyRingProtector newProtector) throws PGPException { + List secretKeyList = new ArrayList<>(); if (keyId == null) { // change passphrase of whole key ring - List newlyEncryptedSecretKeys = new ArrayList<>(); Iterator secretKeyIterator = secretKeys.getSecretKeys(); while (secretKeyIterator.hasNext()) { PGPSecretKey secretKey = secretKeyIterator.next(); secretKey = reencryptPrivateKey(secretKey, oldProtector, newProtector); - newlyEncryptedSecretKeys.add(secretKey); + secretKeyList.add(secretKey); } - return new PGPSecretKeyRing(newlyEncryptedSecretKeys); } else { // change passphrase of selected subkey only - List secretKeyList = new ArrayList<>(); Iterator secretKeyIterator = secretKeys.getSecretKeys(); while (secretKeyIterator.hasNext()) { PGPSecretKey secretKey = secretKeyIterator.next(); - if (secretKey.getPublicKey().getKeyID() == keyId) { // Re-encrypt only the selected subkey secretKey = reencryptPrivateKey(secretKey, oldProtector, newProtector); } - secretKeyList.add(secretKey); } - return new PGPSecretKeyRing(secretKeyList); } + + PGPSecretKeyRing newRing = new PGPSecretKeyRing(secretKeyList); + newRing = s2kUsageFixIfNecessary(newRing, newProtector); + return newRing; + } + + private PGPSecretKeyRing s2kUsageFixIfNecessary(PGPSecretKeyRing secretKeys, SecretKeyRingProtector protector) throws PGPException { + boolean hasS2KUsageChecksum = false; + for (PGPSecretKey secKey : secretKeys) { + if (secKey.getS2KUsage() == SecretKeyPacket.USAGE_CHECKSUM) { + hasS2KUsageChecksum = true; + } + } + if (hasS2KUsageChecksum) { + secretKeys = S2KUsageFix.replaceUsageChecksumWithUsageSha1(secretKeys, protector, true); + } + return secretKeys; } private static PGPSecretKey reencryptPrivateKey(PGPSecretKey secretKey, SecretKeyRingProtector oldProtector, SecretKeyRingProtector newProtector) throws PGPException { diff --git a/pgpainless-core/src/test/java/org/pgpainless/key/protection/fixes/EnsureSecureS2KUsageTest.java b/pgpainless-core/src/test/java/org/pgpainless/key/protection/fixes/S2KUsageFixTest.java similarity index 90% rename from pgpainless-core/src/test/java/org/pgpainless/key/protection/fixes/EnsureSecureS2KUsageTest.java rename to pgpainless-core/src/test/java/org/pgpainless/key/protection/fixes/S2KUsageFixTest.java index 9679c61e..1baf3593 100644 --- a/pgpainless-core/src/test/java/org/pgpainless/key/protection/fixes/EnsureSecureS2KUsageTest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/key/protection/fixes/S2KUsageFixTest.java @@ -37,7 +37,7 @@ import org.pgpainless.decryption_verification.DecryptionStream; import org.pgpainless.key.protection.SecretKeyRingProtector; import org.pgpainless.util.Passphrase; -public class EnsureSecureS2KUsageTest { +public class S2KUsageFixTest { private static final String KEY_WITH_USAGE_CHECKSUM = "-----BEGIN PGP PRIVATE KEY BLOCK-----\n" + "Version: PGPainless\n" + @@ -78,11 +78,7 @@ public class EnsureSecureS2KUsageTest { private static final String MESSAGE_PLAIN = "Hello, World!\n"; @Test - public void verifyBouncycastleChangesUnprotectedKeysTo_USAGE_CHECKSUM() throws PGPException, InvalidAlgorithmParameterException, NoSuchAlgorithmException, IOException { - // Bouncycastle unfortunately uses USAGE_CHECKSUM as default S2K usage when setting a passphrase - // on a previously unprotected key. - // This test verifies this hypothesis by creating a fresh, protected key (which uses the recommended USAGE_SHA1), - // unprotecting the key and then again setting a passphrase on it. + public void verifyOutFixInChangePassphraseWorks() throws PGPException, InvalidAlgorithmParameterException, NoSuchAlgorithmException, IOException { PGPSecretKeyRing before = PGPainless.generateKeyRing().modernKeyRing("Alice", "before"); for (PGPSecretKey key : before) { assertEquals(SecretKeyPacket.USAGE_SHA1, key.getS2KUsage()); @@ -103,7 +99,7 @@ public class EnsureSecureS2KUsageTest { .toNewPassphrase(Passphrase.fromPassword("after")) .done(); for (PGPSecretKey key : after) { - assertEquals(SecretKeyPacket.USAGE_CHECKSUM, key.getS2KUsage(), "Looks like BC fixed the default S2K usage. Yay!"); + assertEquals(SecretKeyPacket.USAGE_SHA1, key.getS2KUsage()); } }