Fix unpacking time (unsigned int) from octets for large values

bcgit · Sep 14, 2024 · f4f8638 · f4f8638
1 parent 7352222
commit f4f8638
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 11 deletions.
diff --git 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
@@ -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
@@ -108,16 +108,29 @@ static long readKeyID(BCPGInputStream in)
         return keyID | in.read();
     }
 
-    static void writeTime(BCPGOutputStream pOut, long time)
-        throws IOException
+    static long readSeconds(BCPGInputStream in)
+            throws IOException
+    {
+        return ((long)read4OctetLength(in)) & 0xFFFFFFFFL;
+    }
+
+    static void writeSeconds(BCPGOutputStream pOut, long time)
+            throws IOException
     {
         StreamUtil.write4OctetLength(pOut, (int)time);
     }
 
     static long readTime(BCPGInputStream in)
         throws IOException
     {
-        return (long)read4OctetLength(in) * 1000L;
+        long s = ((long)read4OctetLength(in)) & 0xFFFFFFFFL;
+        return s * 1000L;
+    }
+
+    static void writeTime(BCPGOutputStream pOut, long time)
+            throws IOException
+    {
+        StreamUtil.write4OctetLength(pOut, (int) (time / 1000));
     }
 
     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
@@ -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
@@ -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
@@ -0,0 +1,71 @@
+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.PublicSubkeyPacket;
+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());
+    }
+}