From df4cc22ab60f9329e3f56f964177631ecb13830c Mon Sep 17 00:00:00 2001 From: Paul Schaub Date: Sat, 14 Sep 2024 11:51:01 +0200 Subject: [PATCH] Fix unpacking time (unsigned int) from octets for large values --- .../bouncycastle/bcpg/PublicKeyPacket.java | 4 +- .../bouncycastle/bcpg/SignaturePacket.java | 6 +- .../org/bouncycastle/bcpg/StreamUtil.java | 17 ++++- .../java/org/bouncycastle/bcpg/sig/Utils.java | 2 +- .../org/bouncycastle/bcpg/test/AllTests.java | 3 +- .../bcpg/test/TimeEncodingTest.java | 70 +++++++++++++++++++ 6 files changed, 92 insertions(+), 10 deletions(-) create mode 100644 pg/src/test/java/org/bouncycastle/bcpg/test/TimeEncodingTest.java diff --git a/pg/src/main/java/org/bouncycastle/bcpg/PublicKeyPacket.java b/pg/src/main/java/org/bouncycastle/bcpg/PublicKeyPacket.java index 1a83ba0cfd..21db64348b 100644 --- a/pg/src/main/java/org/bouncycastle/bcpg/PublicKeyPacket.java +++ b/pg/src/main/java/org/bouncycastle/bcpg/PublicKeyPacket.java @@ -134,7 +134,7 @@ public class PublicKeyPacket throw new UnsupportedPacketVersionException("Unsupported Public Key Packet version encountered: " + version); } - time = StreamUtil.read4OctetLength(in); + time = StreamUtil.readSeconds(in); if (version == 2 || version == VERSION_3) { @@ -324,7 +324,7 @@ public byte[] getEncodedContents() pOut.write(version); - StreamUtil.writeTime(pOut, time); + StreamUtil.writeSeconds(pOut, time); if (version <= VERSION_3) { diff --git a/pg/src/main/java/org/bouncycastle/bcpg/SignaturePacket.java b/pg/src/main/java/org/bouncycastle/bcpg/SignaturePacket.java index 9b4e405aec..29fd201421 100644 --- a/pg/src/main/java/org/bouncycastle/bcpg/SignaturePacket.java +++ b/pg/src/main/java/org/bouncycastle/bcpg/SignaturePacket.java @@ -26,7 +26,7 @@ public class SignaturePacket private int version; private int signatureType; - private long creationTime; + private long creationTime; // millis private long keyID; private int keyAlgorithm; private int hashAlgorithm; @@ -647,10 +647,8 @@ public void encode( { pOut.write(5); // the length of the next block - long time = creationTime / 1000; - pOut.write(signatureType); - StreamUtil.writeTime(pOut, time); + StreamUtil.writeTime(pOut, creationTime); StreamUtil.writeKeyID(pOut, keyID); diff --git a/pg/src/main/java/org/bouncycastle/bcpg/StreamUtil.java b/pg/src/main/java/org/bouncycastle/bcpg/StreamUtil.java index 3f7df33323..fb0ccc6d90 100644 --- a/pg/src/main/java/org/bouncycastle/bcpg/StreamUtil.java +++ b/pg/src/main/java/org/bouncycastle/bcpg/StreamUtil.java @@ -111,13 +111,26 @@ static long readKeyID(BCPGInputStream in) static void writeTime(BCPGOutputStream pOut, long time) throws IOException { - StreamUtil.write4OctetLength(pOut, (int)time); + StreamUtil.writeSeconds(pOut, time / 1000); } static long readTime(BCPGInputStream in) throws IOException { - return (long)read4OctetLength(in) * 1000L; + long s = readSeconds(in); + return s * 1000L; + } + + static void writeSeconds(BCPGOutputStream pOut, long time) + throws IOException + { + StreamUtil.write4OctetLength(pOut, (int)time); + } + + static long readSeconds(BCPGInputStream in) + throws IOException + { + return ((long)read4OctetLength(in)) & 0xFFFFFFFFL; } static void write2OctetLength(OutputStream pOut, int len) diff --git a/pg/src/main/java/org/bouncycastle/bcpg/sig/Utils.java b/pg/src/main/java/org/bouncycastle/bcpg/sig/Utils.java index 95cbe0cba8..3189049ffd 100644 --- a/pg/src/main/java/org/bouncycastle/bcpg/sig/Utils.java +++ b/pg/src/main/java/org/bouncycastle/bcpg/sig/Utils.java @@ -57,7 +57,7 @@ static long timeFromBytes(byte[] bytes) throw new IllegalStateException("Byte array has unexpected length. Expected length 4, got " + bytes.length); } - return Pack.bigEndianToInt(bytes, 0); + return Pack.bigEndianToInt(bytes, 0) & 0xFFFFFFFFL; // time is unsigned } static byte[] timeToBytes(long t) diff --git a/pg/src/test/java/org/bouncycastle/bcpg/test/AllTests.java b/pg/src/test/java/org/bouncycastle/bcpg/test/AllTests.java index 860b1a10a6..c278aa5871 100644 --- a/pg/src/test/java/org/bouncycastle/bcpg/test/AllTests.java +++ b/pg/src/test/java/org/bouncycastle/bcpg/test/AllTests.java @@ -24,7 +24,8 @@ public void testPacketParsing() new OnePassSignaturePacketTest(), new OpenPgpMessageTest(), new FingerprintUtilTest(), - new EncryptedMessagePacketTest() + new EncryptedMessagePacketTest(), + new TimeEncodingTest() }; for (int i = 0; i != tests.length; i++) diff --git a/pg/src/test/java/org/bouncycastle/bcpg/test/TimeEncodingTest.java b/pg/src/test/java/org/bouncycastle/bcpg/test/TimeEncodingTest.java new file mode 100644 index 0000000000..483d3b5b1b --- /dev/null +++ b/pg/src/test/java/org/bouncycastle/bcpg/test/TimeEncodingTest.java @@ -0,0 +1,70 @@ +package org.bouncycastle.bcpg.test; + +import org.bouncycastle.bcpg.BCPGInputStream; +import org.bouncycastle.bcpg.BCPGOutputStream; +import org.bouncycastle.bcpg.PacketFormat; +import org.bouncycastle.bcpg.PublicKeyPacket; +import org.bouncycastle.bcpg.UnknownBCPGKey; +import org.bouncycastle.bcpg.sig.KeyExpirationTime; +import org.bouncycastle.util.test.SimpleTest; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.Date; + +public class TimeEncodingTest + extends SimpleTest +{ + @Override + public String getName() + { + return "UtilsTest"; + } + + @Override + public void performTest() + throws Exception + { + testRoundtrippingLargeUnsignedInt(); + testKeyWithLargeCreationTime(); + } + + private void testRoundtrippingLargeUnsignedInt() + { + // Integer.MAX_VALUE < large < 0xffffffff + long large = 2523592696L; // fits a 32-bit *unsigned* int, but overflows signed int + // KeyExpirationTime packs the time into 4 octets + KeyExpirationTime kexp = new KeyExpirationTime(false, large); + // getTime() parses the time from 4 octets + isEquals("Roundtripped unsigned int mismatches before packet parser pass", large, kexp.getTime()); + + // To be safe, do an additional packet encode/decode roundtrip + KeyExpirationTime pKexp = new KeyExpirationTime(kexp.isCritical(), kexp.isLongLength(), kexp.getData()); + isEquals("Roundtripped unsigned int mismatches after packet parser pass", large, pKexp.getTime()); + } + + private void testKeyWithLargeCreationTime() + throws IOException + { + long maxSeconds = 0xFFFFFFFEL; // Fits 32 unsigned int, but not signed int + Date maxPGPDate = new Date(maxSeconds * 1000); + UnknownBCPGKey k = new UnknownBCPGKey(1, new byte[]{1}); // dummy + PublicKeyPacket p = new PublicKeyPacket(PublicKeyPacket.VERSION_6, 99, maxPGPDate, k); + isEquals("Key creation time mismatches before encoding", maxPGPDate, p.getTime()); + + ByteArrayOutputStream bOut = new ByteArrayOutputStream(); + BCPGOutputStream pOut = new BCPGOutputStream(bOut, PacketFormat.CURRENT); + p.encode(pOut); + pOut.close(); + ByteArrayInputStream bIn = new ByteArrayInputStream(bOut.toByteArray()); + BCPGInputStream pIn = new BCPGInputStream(bIn); + PublicKeyPacket parsed = (PublicKeyPacket) pIn.readPacket(); + isEquals("Key creation time mismatches after encoding", maxPGPDate, parsed.getTime()); + } + + public static void main(String[] args) + { + runTest(new TimeEncodingTest()); + } +}