Automatically 'repair' keys with S2K usage CHECKSUM to use SHA1 when changing passphrases

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<PGPSecretKey> secretKeyList = new ArrayList<>();
         if (keyId == null) {
             // change passphrase of whole key ring
-            List<PGPSecretKey> newlyEncryptedSecretKeys = new ArrayList<>();
             Iterator<PGPSecretKey> 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<PGPSecretKey> secretKeyList = new ArrayList<>();
             Iterator<PGPSecretKey> 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 {

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 {
+    public void verifyOutFixInChangePassphraseWorks() 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.
         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());
         }
     }