From 213297ab8b555bff8c9cd5608e49f5c5227cacf4 Mon Sep 17 00:00:00 2001 From: Jill Kleiber Date: Wed, 24 Jul 2024 22:47:01 +0000 Subject: [PATCH] pr-1716-improve-PGPPadding --- .../org/bouncycastle/bcpg/PaddingPacket.java | 4 + .../org/bouncycastle/openpgp/PGPKeyRing.java | 4 + .../org/bouncycastle/openpgp/PGPPadding.java | 93 ++++++++++++- .../openpgp/PGPPublicKeyRing.java | 14 +- .../openpgp/PGPSecretKeyRing.java | 14 +- .../openpgp/test/PGPPaddingTest.java | 128 ++++++++++++++++++ 6 files changed, 250 insertions(+), 7 deletions(-) create mode 100644 pg/src/test/java/org/bouncycastle/openpgp/test/PGPPaddingTest.java diff --git a/pg/src/main/java/org/bouncycastle/bcpg/PaddingPacket.java b/pg/src/main/java/org/bouncycastle/bcpg/PaddingPacket.java index bd45514606..6f3c128bf7 100644 --- a/pg/src/main/java/org/bouncycastle/bcpg/PaddingPacket.java +++ b/pg/src/main/java/org/bouncycastle/bcpg/PaddingPacket.java @@ -39,6 +39,10 @@ public PaddingPacket(int octetLen, SecureRandom random) private static byte[] randomBytes(int octetCount, SecureRandom random) { + if (octetCount <= 0) + { + throw new IllegalArgumentException("Octet count MUST NOT be 0 nor negative."); + } byte[] bytes = new byte[octetCount]; random.nextBytes(bytes); return bytes; diff --git a/pg/src/main/java/org/bouncycastle/openpgp/PGPKeyRing.java b/pg/src/main/java/org/bouncycastle/openpgp/PGPKeyRing.java index c3373e89b7..fb12d5c11d 100644 --- a/pg/src/main/java/org/bouncycastle/openpgp/PGPKeyRing.java +++ b/pg/src/main/java/org/bouncycastle/openpgp/PGPKeyRing.java @@ -10,6 +10,7 @@ import org.bouncycastle.bcpg.BCPGInputStream; import org.bouncycastle.bcpg.Packet; +import org.bouncycastle.bcpg.PacketFormat; import org.bouncycastle.bcpg.PacketTags; import org.bouncycastle.bcpg.SignaturePacket; import org.bouncycastle.bcpg.TrustPacket; @@ -144,6 +145,9 @@ public abstract void encode(OutputStream outStream) public abstract byte[] getEncoded() throws IOException; + public abstract byte[] getEncoded(PacketFormat format) + throws IOException; + private static boolean isUserTag(int tag) { switch (tag) diff --git a/pg/src/main/java/org/bouncycastle/openpgp/PGPPadding.java b/pg/src/main/java/org/bouncycastle/openpgp/PGPPadding.java index 7661f38837..8582a247a0 100644 --- a/pg/src/main/java/org/bouncycastle/openpgp/PGPPadding.java +++ b/pg/src/main/java/org/bouncycastle/openpgp/PGPPadding.java @@ -1,10 +1,16 @@ package org.bouncycastle.openpgp; +import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.OutputStream; +import java.security.SecureRandom; import org.bouncycastle.bcpg.BCPGInputStream; +import org.bouncycastle.bcpg.BCPGOutputStream; import org.bouncycastle.bcpg.Packet; +import org.bouncycastle.bcpg.PacketFormat; import org.bouncycastle.bcpg.PaddingPacket; +import org.bouncycastle.crypto.CryptoServicesRegistrar; /** * The PGPPadding contains random data, and can be used to defend against traffic analysis on version 2 SEIPD messages @@ -16,10 +22,25 @@ public class PGPPadding { private PaddingPacket p; + /** + * Minimum random padding length in octets. + * Chosen totally arbitrarily. + */ + public static final int MIN_PADDING_LEN = 16; + + /** + * Maximum random padding length. + * Chosen somewhat arbitrarily, as SSH also uses max 255 bytes for random padding. + * + * @see + * rfc4253 - Binary Packet Protocol + */ + public static final int MAX_PADDING_LEN = 255; + /** * Default constructor. * - * @param in + * @param in packet input stream * @throws IOException */ public PGPPadding( @@ -34,8 +55,78 @@ public PGPPadding( p = (PaddingPacket)packet; } + /** + * Generate a new, random {@link PGPPadding} object. + * The padding consists of n random bytes, where n is a number between (inclusive) {@link #MIN_PADDING_LEN} + * and {@link #MAX_PADDING_LEN}. + */ + public PGPPadding() + { + this(CryptoServicesRegistrar.getSecureRandom()); + } + + /** + * Generate a new, random {@link PGPPadding} object. + * The padding consists of n random bytes, where n is a number between (inclusive) {@link #MIN_PADDING_LEN} + * and {@link #MAX_PADDING_LEN}. + * + * @param random random number generator instance + */ + public PGPPadding(SecureRandom random) + { + this(MIN_PADDING_LEN + random.nextInt(MAX_PADDING_LEN - MIN_PADDING_LEN + 1), random); + } + + /** + * Generate a new, random {@link PGPPadding} object. + * The padding consists of 
len
random bytes. + */ + public PGPPadding(int len) + { + this(len, CryptoServicesRegistrar.getSecureRandom()); + } + + /** + * Generate a new, random {@link PGPPadding} object. + * The padding consists of 
len
random bytes. + * + * @param len number of random octets + * @param random random number generator instance + */ + public PGPPadding(int len, SecureRandom random) + { + this.p = new PaddingPacket(len, random); + } + + /** + * Return the padding octets as a byte array. + * @return padding octets + */ public byte[] getPadding() { return p.getPadding(); } + + public void encode(OutputStream outStream) + throws IOException + { + BCPGOutputStream pOut = BCPGOutputStream.wrap(outStream); + p.encode(pOut); + } + + public byte[] getEncoded() + throws IOException + { + return getEncoded(PacketFormat.ROUNDTRIP); + } + + public byte[] getEncoded(PacketFormat format) + throws IOException + { + ByteArrayOutputStream bOut = new ByteArrayOutputStream(); + BCPGOutputStream pOut = new BCPGOutputStream(bOut, format); + encode(pOut); + pOut.close(); + return bOut.toByteArray(); + } } diff --git a/pg/src/main/java/org/bouncycastle/openpgp/PGPPublicKeyRing.java b/pg/src/main/java/org/bouncycastle/openpgp/PGPPublicKeyRing.java index 123a0c8955..9cd941e224 100644 --- a/pg/src/main/java/org/bouncycastle/openpgp/PGPPublicKeyRing.java +++ b/pg/src/main/java/org/bouncycastle/openpgp/PGPPublicKeyRing.java @@ -17,7 +17,9 @@ import org.bouncycastle.bcpg.ArmoredInputException; import org.bouncycastle.bcpg.BCPGInputStream; +import org.bouncycastle.bcpg.BCPGOutputStream; import org.bouncycastle.bcpg.Packet; +import org.bouncycastle.bcpg.PacketFormat; import org.bouncycastle.bcpg.PacketTags; import org.bouncycastle.bcpg.PublicKeyPacket; import org.bouncycastle.bcpg.TrustPacket; @@ -238,10 +240,16 @@ public Iterator iterator() public byte[] getEncoded() throws IOException { - ByteArrayOutputStream bOut = new ByteArrayOutputStream(); - - this.encode(bOut); + return getEncoded(PacketFormat.ROUNDTRIP); + } + @Override + public byte[] getEncoded(PacketFormat format) throws IOException + { + ByteArrayOutputStream bOut = new ByteArrayOutputStream(); + BCPGOutputStream pOut = new BCPGOutputStream(bOut, format); + this.encode(pOut); + pOut.close(); return bOut.toByteArray(); } diff --git a/pg/src/main/java/org/bouncycastle/openpgp/PGPSecretKeyRing.java b/pg/src/main/java/org/bouncycastle/openpgp/PGPSecretKeyRing.java index cf06c1c188..d9427c26c3 100644 --- a/pg/src/main/java/org/bouncycastle/openpgp/PGPSecretKeyRing.java +++ b/pg/src/main/java/org/bouncycastle/openpgp/PGPSecretKeyRing.java @@ -15,6 +15,8 @@ import org.bouncycastle.bcpg.ArmoredInputException; import org.bouncycastle.bcpg.BCPGInputStream; +import org.bouncycastle.bcpg.BCPGOutputStream; +import org.bouncycastle.bcpg.PacketFormat; import org.bouncycastle.bcpg.PacketTags; import org.bouncycastle.bcpg.PublicSubkeyPacket; import org.bouncycastle.bcpg.SecretKeyPacket; @@ -389,10 +391,16 @@ public int size() public byte[] getEncoded() throws IOException { - ByteArrayOutputStream bOut = new ByteArrayOutputStream(); - - this.encode(bOut); + return getEncoded(PacketFormat.ROUNDTRIP); + } + @Override + public byte[] getEncoded(PacketFormat format) throws IOException + { + ByteArrayOutputStream bOut = new ByteArrayOutputStream(); + BCPGOutputStream pOut = new BCPGOutputStream(bOut, format); + this.encode(pOut); + pOut.close(); return bOut.toByteArray(); } diff --git a/pg/src/test/java/org/bouncycastle/openpgp/test/PGPPaddingTest.java b/pg/src/test/java/org/bouncycastle/openpgp/test/PGPPaddingTest.java new file mode 100644 index 0000000000..7de0f2a07c --- /dev/null +++ b/pg/src/test/java/org/bouncycastle/openpgp/test/PGPPaddingTest.java @@ -0,0 +1,128 @@ +package org.bouncycastle.openpgp.test; + +import org.bouncycastle.bcpg.ArmoredInputStream; +import org.bouncycastle.bcpg.ArmoredOutputStream; +import org.bouncycastle.bcpg.BCPGInputStream; +import org.bouncycastle.bcpg.BCPGOutputStream; +import org.bouncycastle.bcpg.HashAlgorithmTags; +import org.bouncycastle.bcpg.PacketFormat; +import org.bouncycastle.bcpg.PublicKeyAlgorithmTags; +import org.bouncycastle.crypto.AsymmetricCipherKeyPair; +import org.bouncycastle.crypto.CryptoServicesRegistrar; +import org.bouncycastle.crypto.generators.Ed25519KeyPairGenerator; +import org.bouncycastle.crypto.generators.X25519KeyPairGenerator; +import org.bouncycastle.crypto.params.Ed25519KeyGenerationParameters; +import org.bouncycastle.crypto.params.X25519KeyGenerationParameters; +import org.bouncycastle.openpgp.PGPException; +import org.bouncycastle.openpgp.PGPKeyPair; +import org.bouncycastle.openpgp.PGPPadding; +import org.bouncycastle.openpgp.PGPPublicKeyRing; +import org.bouncycastle.openpgp.PGPSecretKey; +import org.bouncycastle.openpgp.operator.PGPDigestCalculator; +import org.bouncycastle.openpgp.operator.bc.BcKeyFingerprintCalculator; +import org.bouncycastle.openpgp.operator.bc.BcPGPDigestCalculatorProvider; +import org.bouncycastle.openpgp.operator.bc.BcPGPKeyPair; +import org.bouncycastle.util.test.SimpleTest; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.Arrays; +import java.util.Date; + +public class PGPPaddingTest + extends SimpleTest +{ + @Override + public String getName() + { + return "PGPPaddingTest"; + } + + @Override + public void performTest() + throws Exception + { + randomPaddingIsInBounds(); + fixedLenPaddingIsCorrectLength(); + negativePaddingLengthThrows(); + zeroPaddingLengthThrows(); + + parsePaddedCertificate(); + } + + private void randomPaddingIsInBounds() + { + for (int i = 0; i < 10; i++) + { + PGPPadding padding = new PGPPadding(); + int len = padding.getPadding().length; + isTrue("Padding length exceeds bounds. Min: " + PGPPadding.MIN_PADDING_LEN + + ", Max: " + PGPPadding.MAX_PADDING_LEN + ", Actual: " + len , + len >= PGPPadding.MIN_PADDING_LEN && len <= PGPPadding.MAX_PADDING_LEN); + } + } + + private void fixedLenPaddingIsCorrectLength() + { + PGPPadding padding = new PGPPadding(42); + isEquals("Padding length mismatch", 42, padding.getPadding().length); + } + + private void negativePaddingLengthThrows() + { + testException(null, "IllegalArgumentException", () -> new PGPPadding(-1)); + } + + private void zeroPaddingLengthThrows() + { + testException(null, "IllegalArgumentException", () -> new PGPPadding(0)); + } + + private void parsePaddedCertificate() + throws PGPException, IOException + { + PGPDigestCalculator digestCalc = new BcPGPDigestCalculatorProvider().get(HashAlgorithmTags.SHA1); + + Date creationTime = new Date(1000 * (new Date().getTime() / 1000)); + Ed25519KeyPairGenerator edGen = new Ed25519KeyPairGenerator(); + edGen.init(new Ed25519KeyGenerationParameters(CryptoServicesRegistrar.getSecureRandom())); + AsymmetricCipherKeyPair edPair = edGen.generateKeyPair(); + + X25519KeyPairGenerator xGen = new X25519KeyPairGenerator(); + xGen.init(new X25519KeyGenerationParameters(CryptoServicesRegistrar.getSecureRandom())); + AsymmetricCipherKeyPair xPair = xGen.generateKeyPair(); + + PGPKeyPair primaryKeyPair = new BcPGPKeyPair(PublicKeyAlgorithmTags.Ed25519, edPair, creationTime); + PGPKeyPair subKeyPair = new BcPGPKeyPair(PublicKeyAlgorithmTags.X25519, xPair, creationTime); + + PGPSecretKey secretPrimaryKey = new PGPSecretKey(primaryKeyPair.getPrivateKey(), primaryKeyPair.getPublicKey(), digestCalc, true, null); + PGPSecretKey secretSubKey = new PGPSecretKey(subKeyPair.getPrivateKey(), subKeyPair.getPublicKey(), digestCalc, false, null); + + PGPPublicKeyRing certificate = new PGPPublicKeyRing(Arrays.asList(secretPrimaryKey.getPublicKey(), secretSubKey.getPublicKey())); + PGPPadding padding = new PGPPadding(); + + ByteArrayOutputStream bOut = new ByteArrayOutputStream(); + ArmoredOutputStream aOut = ArmoredOutputStream.builder().clearHeaders().build(bOut); + BCPGOutputStream pOut = new BCPGOutputStream(aOut, PacketFormat.CURRENT); + certificate.encode(pOut); + padding.encode(pOut); + + pOut.close(); + aOut.close(); + + ByteArrayInputStream bIn = new ByteArrayInputStream(bOut.toByteArray()); + ArmoredInputStream aIn = new ArmoredInputStream(bIn); + BCPGInputStream pIn = new BCPGInputStream(aIn); + + PGPPublicKeyRing parsed = new PGPPublicKeyRing(pIn, new BcKeyFingerprintCalculator()); + isTrue(org.bouncycastle.util.Arrays.areEqual( + certificate.getEncoded(PacketFormat.CURRENT), + parsed.getEncoded(PacketFormat.CURRENT))); + } + + public static void main(String[] args) + { + runTest(new PGPPaddingTest()); + } +} \ No newline at end of file