Added method to pass in digest calculator for checksum when required.…

… Relates to github bcgit#1020. Changed checksum check to constant time.
jervin · Nov 25, 2021 · 2dd5fdf · 2dd5fdf
1 parent 91ce81d
commit 2dd5fdf
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 12 deletions.
diff --git a/pg/src/main/java/org/bouncycastle/openpgp/PGPSecretKey.java b/pg/src/main/java/org/bouncycastle/openpgp/PGPSecretKey.java
@@ -35,7 +35,6 @@
 import org.bouncycastle.openpgp.operator.PBESecretKeyEncryptor;
 import org.bouncycastle.openpgp.operator.PGPContentSignerBuilder;
 import org.bouncycastle.openpgp.operator.PGPDigestCalculator;
-import org.bouncycastle.openpgp.operator.bc.BcPGPDigestCalculatorProvider;
 import org.bouncycastle.util.Arrays;
 
 /**
@@ -493,12 +492,9 @@ private byte[] extractKeyData(
                     boolean useSHA1 = secret.getS2KUsage() == SecretKeyPacket.USAGE_SHA1;
                     byte[] check = checksum(useSHA1 ? decryptorFactory.getChecksumCalculator(HashAlgorithmTags.SHA1) : null, data, (useSHA1) ? data.length - 20 : data.length - 2);
 
-                    for (int i = 0; i != check.length; i++)
+                    if (!Arrays.constantTimeAreEqual(check.length, check, 0, data, data.length - check.length))
                     {
-                        if (check[i] != data[data.length - check.length + i])
-                        {
-                            throw new PGPException("checksum mismatch at " + i + " of " + check.length);
-                        }
+                        throw new PGPException("checksum mismatch at in checksum of " + check.length + " bytes");
                     }
                 }
                 else // version 2 or 3, RSA only.
@@ -780,6 +776,25 @@ public static PGPSecretKey copyWithNewPassword(
         PBESecretKeyDecryptor oldKeyDecryptor,
         PBESecretKeyEncryptor newKeyEncryptor)
         throws PGPException
+    {
+        return copyWithNewPassword(key, oldKeyDecryptor, newKeyEncryptor, null);
+    }
+
+    /**
+     * Return a copy of the passed in secret key, encrypted using a new
+     * password and the passed in algorithm.
+     *
+     * @param key             the PGPSecretKey to be copied.
+     * @param oldKeyDecryptor the current decryptor based on the current password for key.
+     * @param newKeyEncryptor a new encryptor based on a new password for encrypting the secret key material.
+     * @param checksumCalculator digest based checksum calculator for private key data.
+     */
+    public static PGPSecretKey copyWithNewPassword(
+        PGPSecretKey key,
+        PBESecretKeyDecryptor oldKeyDecryptor,
+        PBESecretKeyEncryptor newKeyEncryptor,
+        PGPDigestCalculator checksumCalculator)
+        throws PGPException
     {
         if (key.isPrivateKeyEmpty())
         {
@@ -878,12 +893,23 @@ public static PGPSecretKey copyWithNewPassword(
             {
                 if (s2kUsage == SecretKeyPacket.USAGE_NONE)
                 {
-                    s2kUsage = SecretKeyPacket.USAGE_SHA1;
-                    PGPDigestCalculator checkCalc = new BcPGPDigestCalculatorProvider().get(HashAlgorithmTags.SHA1); //oldKeyDecryptor.getChecksumCalculator(HashAlgorithmTags.SHA1);
+                    if (checksumCalculator != null)
+                    {
+                        if (checksumCalculator.getAlgorithm() != HashAlgorithmTags.SHA1)
+                        {
+                            throw new IllegalArgumentException("only SHA-1 supported for checksums");
+                        }
+                        s2kUsage = SecretKeyPacket.USAGE_SHA1;
 
-                    byte[] check = checksum(checkCalc, rawKeyData, rawKeyData.length);
-                    rawKeyData = Arrays.concatenate(rawKeyData, check);
-                    keyData = newKeyEncryptor.encryptKeyData(rawKeyData, 0, rawKeyData.length);
+                        byte[] check = checksum(checksumCalculator, rawKeyData, rawKeyData.length);
+                        rawKeyData = Arrays.concatenate(rawKeyData, check);
+                        keyData = newKeyEncryptor.encryptKeyData(rawKeyData, 0, rawKeyData.length);
+                    }
+                    else
+                    {
+                        s2kUsage = SecretKeyPacket.USAGE_CHECKSUM;
+                        keyData = newKeyEncryptor.encryptKeyData(rawKeyData, 0, rawKeyData.length);
+                    }
                 }
                 else
                 {

diff --git a/pg/src/test/java/org/bouncycastle/openpgp/test/PGPKeyRingTest.java b/pg/src/test/java/org/bouncycastle/openpgp/test/PGPKeyRingTest.java
@@ -2812,7 +2812,8 @@ private void rewrapTest()
                 pgpKey = PGPSecretKey.copyWithNewPassword(
                                     pgpKey,
                                     null,
-                                    new JcePBESecretKeyEncryptorBuilder(SymmetricKeyAlgorithmTags.CAST5).setProvider("BC").build(newPass));
+                                    new JcePBESecretKeyEncryptorBuilder(SymmetricKeyAlgorithmTags.CAST5).setProvider("BC").build(newPass),
+                                    new BcPGPDigestCalculatorProvider().get(HashAlgorithmTags.SHA1));
                 pgpPriv = PGPSecretKeyRing.insertSecretKey(pgpPriv, pgpKey);
 
                 // this should succeed
@@ -2822,6 +2823,16 @@ private void rewrapTest()
                 {
                     fail("usage/key ID mismatch");
                 }
+
+                pgpKey = PGPSecretKey.copyWithNewPassword(
+                                    pgpKey,
+                                    new JcePBESecretKeyDecryptorBuilder(new BcPGPDigestCalculatorProvider()).setProvider("BC").build(newPass),
+                                    null);
+
+                if (pgpKey.getKeyID() != oldKeyID || pgpKey.getS2KUsage() != SecretKeyPacket.USAGE_NONE)
+                {
+                    fail("usage/key ID mismatch");
+                }
             }
         }
     }