Compare the SRP-6a evidence messages and the J-PAKE MacTag in constant time over fixed-width encodings rather than with BigInteger.equals, keeping each expected value in the digest's own output form so its length cannot vary with the secret, and sign-extending the J-PAKE tag, which is read signed, incorporating github PR #2406, relates to github #2406.

Author: dghgit

CONTRIBUTORS.html

@@ -594,7 +594,7 @@
 <li>subbudvk &lt;https://github.com/subbudvk&gt; - initial author on S2K parser hardening work for OpenPGP API.</li>
 <li>mkarasik &lt;https://github.com/mkarasik&gt; - initial work on EST server-side key generation (RFC 7030 4.4).</li>
 <li>Bernd Pr&uuml;nster (A-SIT Plus) &lt;bernd.pruenster&#064;a-sit.at&gt; - reported lenient ASN.1 UTCTime/GeneralizedTime parsing accepting structurally malformed content, with fuzzing-derived test cases.</li>
-<li>Naveed Khan &lt;https://github.com/rootvector2&gt; - constant time comparison of membership and confirmation tags in the MLS API. Reported unbounded array allocation in the BKS/UBER keystore load path (OutOfMemoryError DoS from a crafted keystore) and introduction of a capped decompression limit in PGPCompressedData.getDataStream(), both with POC. Original submission creating S/MIME backing temp files with owner-only permissions (PR #2326). Rejecting empty sequences in the X.509 extension parsers whose RFC 5280 syntax is SEQUENCE SIZE (1..MAX) - CRLDistPoint, CertificatePolicies, ExtendedKeyUsage, PolicyMappings, and SubjectDirectoryAttributes (PR #2331). Hardening asn1.cms.SignerInfo decode to reject a non-INTEGER version or non-tagged unsignedAttrs via getInstance rather than leaking a ClassCastException (PR #2342). Fixing an off-by-4 header-length guard in the OpenPGP NotationData signature subpacket parser (PR #2346). Rejecting CR/LF in the S/MIME streaming writer header names and values (SMIMEEnvelopedWriter/SMIMESignedWriter withHeader) to prevent MIME header injection (PR #2348). Adding minimum-length guards to the SM2 decrypt and GOST28147/DSTU7624/DESede/RC2 key-wrap unwrap paths (PR #2359). Guarding the ECDH session-key length in the JCE OpenPGP decryptor (PR #2383). Fixing the identity fast-path type guards in the OER getInstance factories and the transposed isInstance arguments in OEROptional.getObject (PR #2373). Requiring the signature handed to AIMerSigner.verifySignature to be exactly the parameter set's signature size (PR #2401).</li>
+<li>Naveed Khan &lt;https://github.com/rootvector2&gt; - constant time comparison of membership and confirmation tags in the MLS API. Reported unbounded array allocation in the BKS/UBER keystore load path (OutOfMemoryError DoS from a crafted keystore) and introduction of a capped decompression limit in PGPCompressedData.getDataStream(), both with POC. Original submission creating S/MIME backing temp files with owner-only permissions (PR #2326). Rejecting empty sequences in the X.509 extension parsers whose RFC 5280 syntax is SEQUENCE SIZE (1..MAX) - CRLDistPoint, CertificatePolicies, ExtendedKeyUsage, PolicyMappings, and SubjectDirectoryAttributes (PR #2331). Hardening asn1.cms.SignerInfo decode to reject a non-INTEGER version or non-tagged unsignedAttrs via getInstance rather than leaking a ClassCastException (PR #2342). Fixing an off-by-4 header-length guard in the OpenPGP NotationData signature subpacket parser (PR #2346). Rejecting CR/LF in the S/MIME streaming writer header names and values (SMIMEEnvelopedWriter/SMIMESignedWriter withHeader) to prevent MIME header injection (PR #2348). Adding minimum-length guards to the SM2 decrypt and GOST28147/DSTU7624/DESede/RC2 key-wrap unwrap paths (PR #2359). Guarding the ECDH session-key length in the JCE OpenPGP decryptor (PR #2383). Fixing the identity fast-path type guards in the OER getInstance factories and the transposed isInstance arguments in OEROptional.getObject (PR #2373). Requiring the signature handed to AIMerSigner.verifySignature to be exactly the parameter set's signature size (PR #2401). Constant time comparison of the SRP-6a evidence messages (PR #2406).</li>
 <li>suraj0208 &lt;https://github.com/suraj0208&gt; - initial work on auto-detecting private key reader (JcaPrivateKeyReader).</li>
 <li>liamgilligan &lt;https://github.com/liamgilligan&gt; - noticing the BIP-340 step numbering in the BIP340Signer signing comments was incorrect (PR #2340).</li>
 <li>digi-scrypt &lt;https://github.com/digi-scrypt&gt; - disabling DTD and external-entity resolution in KMIPInputStream to close an XXE (local file disclosure / SSRF) exposure in KMIP XML parsing (PR #2315).</li>

core/src/main/java/org/bouncycastle/crypto/agreement/jpake/JPAKEUtil.java

@@ -460,6 +460,25 @@ public static BigInteger calculateMacTag(
         BigInteger gx4,
         BigInteger keyingMaterial,
         Digest digest)
+    {
+        return new BigInteger(calculateMacTagEncoded(
+            participantId, partnerParticipantId, gx1, gx2, gx3, gx4, keyingMaterial, digest));
+    }
+
+    /**
+     * As {@link #calculateMacTag}, but leaving the result in the MAC's own fixed-width output form
+     * for {@link #constantTimeEquals}. Read back as a BigInteger the encoding is minimal, so its
+     * length would vary with the value.
+     */
+    private static byte[] calculateMacTagEncoded(
+        String participantId,
+        String partnerParticipantId,
+        BigInteger gx1,
+        BigInteger gx2,
+        BigInteger gx3,
+        BigInteger gx4,
+        BigInteger keyingMaterial,
+        Digest digest)
     {
         byte[] macKey = calculateMacKey(
             keyingMaterial,
@@ -484,8 +503,7 @@ public static BigInteger calculateMacTag(
 
         Arrays.fill(macKey, (byte)0);
 
-        return new BigInteger(macOutput);
-
+        return macOutput;
     }
 
     /**
@@ -537,7 +555,7 @@ public static void validateMacTag(
          *            x1 <-> x3
          *            x2 <-> x4
          */
-        BigInteger expectedMacTag = calculateMacTag(
+        byte[] expectedMacTag = calculateMacTagEncoded(
             partnerParticipantId,
             participantId,
             gx3,
@@ -547,14 +565,56 @@ public static void validateMacTag(
             keyingMaterial,
             digest);
 
-        if (!Arrays.constantTimeAreEqual(expectedMacTag.toByteArray(), partnerMacTag.toByteArray()))
+        if (!constantTimeEquals(expectedMacTag, partnerMacTag))
         {
             throw new CryptoException(
                 "Partner MacTag validation failed. "
                     + "Therefore, the password, MAC, or digest algorithm of each participant does not match.");
         }
     }
 
+    /**
+     * Constant-time comparison of the partner's MacTag against the expected one.
+     * <p>
+     * The expected value is kept in the MAC's own output form so the comparison runs over a fixed
+     * number of bytes; read back as a BigInteger its encoding is minimal, so the length alone would
+     * vary with the secret-derived value. Note calculateMacTag reads the MAC output with the
+     * <i>signed</i> BigInteger(byte[]) constructor, so a MacTag is negative about half the time and
+     * the supplied value has to be encoded two's-complement and sign-extended to match:
+     * BigIntegers.asUnsignedByteArray zero-pads instead, which does not round-trip a negative tag.
+     * A value too wide to be a MacTag cannot match and is rejected before any comparison, a
+     * decision taken purely on what the partner sent.
+     *
+     * @param expectedEnc the locally computed MacTag, as the MAC produced it.
+     * @param supplied the MacTag received from the partner.
+     * @return true if the two are equal.
+     */
+    private static boolean constantTimeEquals(byte[] expectedEnc, BigInteger supplied)
+    {
+        int length = expectedEnc.length;
+
+        if (supplied.bitLength() >= length * 8)
+        {
+            return false;
+        }
+
+        byte[] suppliedEnc = new byte[length];
+        if (supplied.signum() < 0)
+        {
+            Arrays.fill(suppliedEnc, (byte)0xFF);
+        }
+
+        byte[] minimal = supplied.toByteArray();
+        System.arraycopy(minimal, 0, suppliedEnc, length - minimal.length, minimal.length);
+
+        boolean rv = Arrays.constantTimeAreEqual(expectedEnc, suppliedEnc);
+
+        Arrays.fill(minimal, (byte)0);
+        Arrays.fill(suppliedEnc, (byte)0);
+
+        return rv;
+    }
+
     private static void updateDigest(Digest digest, BigInteger bigInteger)
     {
         byte[] byteArray = BigIntegers.asUnsignedByteArray(bigInteger);

core/src/main/java/org/bouncycastle/crypto/agreement/srp/SRP6Client.java

@@ -153,8 +153,8 @@ public boolean verifyServerEvidenceMessage(BigInteger serverM2) throws CryptoExc
         }
 
         // Compute the own server evidence message 'M2'
-        BigInteger computedM2 = SRP6Util.calculateM2(digest, N, A, M1, S);
-        if (computedM2.equals(serverM2))
+        byte[] computedM2 = SRP6Util.calculateM2Encoded(digest, N, A, M1, S);
+        if (SRP6Util.constantTimeEquals(computedM2, serverM2))
         {
             this.M2 = serverM2;
             return true;

core/src/main/java/org/bouncycastle/crypto/agreement/srp/SRP6Server.java

@@ -132,8 +132,8 @@ public boolean verifyClientEvidenceMessage(BigInteger clientM1) throws CryptoExc
         }
 
         // Compute the own client evidence message 'M1'
-        BigInteger computedM1 = SRP6Util.calculateM1(digest, N, A, B, S);
-        if (computedM1.equals(clientM1))
+        byte[] computedM1 = SRP6Util.calculateM1Encoded(digest, N, A, B, S);
+        if (SRP6Util.constantTimeEquals(computedM1, clientM1))
         {
             this.M1 = clientM1;
             return true;

core/src/main/java/org/bouncycastle/crypto/agreement/srp/SRP6Util.java

@@ -5,6 +5,7 @@
 
 import org.bouncycastle.crypto.CryptoException;
 import org.bouncycastle.crypto.Digest;
+import org.bouncycastle.util.Arrays;
 import org.bouncycastle.util.BigIntegers;
 
 public class SRP6Util
@@ -74,8 +75,7 @@ public static BigInteger validatePublicValue(BigInteger N, BigInteger val)
      */
     public static BigInteger calculateM1(Digest digest, BigInteger N, BigInteger A, BigInteger B, BigInteger S)
     {
-        BigInteger M1 = hashPaddedTriplet(digest, N, A, B, S);
-        return M1;
+        return new BigInteger(1, calculateM1Encoded(digest, N, A, B, S));
     }
 
     /**
@@ -91,8 +91,7 @@ public static BigInteger calculateM1(Digest digest, BigInteger N, BigInteger A,
      */
     public static BigInteger calculateM2(Digest digest, BigInteger N, BigInteger A, BigInteger M1, BigInteger S)
     {
-        BigInteger M2 = hashPaddedTriplet(digest, N, A, M1, S);
-        return M2;
+        return new BigInteger(1, calculateM2Encoded(digest, N, A, M1, S));
     }
 
     /**
@@ -114,7 +113,25 @@ public static BigInteger calculateKey(Digest digest, BigInteger N, BigInteger S)
         return new BigInteger(1, output);
     }
 
-    private static BigInteger hashPaddedTriplet(Digest digest, BigInteger N, BigInteger n1, BigInteger n2, BigInteger n3)
+    /**
+     * As {@link #calculateM1(Digest, BigInteger, BigInteger, BigInteger, BigInteger)}, but leaving
+     * the result in the digest's own fixed-width output form for {@link #constantTimeEquals}.
+     */
+    static byte[] calculateM1Encoded(Digest digest, BigInteger N, BigInteger A, BigInteger B, BigInteger S)
+    {
+        return hashPaddedTriplet(digest, N, A, B, S);
+    }
+
+    /**
+     * As {@link #calculateM2(Digest, BigInteger, BigInteger, BigInteger, BigInteger)}, but leaving
+     * the result in the digest's own fixed-width output form for {@link #constantTimeEquals}.
+     */
+    static byte[] calculateM2Encoded(Digest digest, BigInteger N, BigInteger A, BigInteger M1, BigInteger S)
+    {
+        return hashPaddedTriplet(digest, N, A, M1, S);
+    }
+
+    private static byte[] hashPaddedTriplet(Digest digest, BigInteger N, BigInteger n1, BigInteger n2, BigInteger n3)
     {
         int padLength = (N.bitLength() + 7) / 8;
 
@@ -129,7 +146,7 @@ private static BigInteger hashPaddedTriplet(Digest digest, BigInteger N, BigInte
         byte[] output = new byte[digest.getDigestSize()];
         digest.doFinal(output, 0);
 
-        return new BigInteger(1, output);
+        return output;
     }
 
     private static BigInteger hashPaddedPair(Digest digest, BigInteger N, BigInteger n1, BigInteger n2)
@@ -159,4 +176,35 @@ private static byte[] getPadded(BigInteger n, int length)
         }
         return bs;
     }
+
+    /**
+     * Constant-time comparison of an evidence message received from the peer against the locally
+     * computed one.
+     * <p>
+     * The expected value is kept in its raw digest-output form so the comparison runs over a fixed
+     * number of bytes: read back as a BigInteger its encoding is minimal, so the length alone would
+     * vary with the secret-derived value. The supplied value is a non-negative digest output too,
+     * so one that is negative or too large to be one cannot match and is rejected before any
+     * comparison - a decision taken purely on what the peer sent, which reveals nothing.
+     *
+     * @param expectedEnc the locally computed evidence message, as the digest produced it.
+     * @param supplied the evidence message received from the peer.
+     * @return true if the two are equal.
+     */
+    static boolean constantTimeEquals(byte[] expectedEnc, BigInteger supplied)
+    {
+        if (supplied.signum() < 0 || supplied.bitLength() > expectedEnc.length * 8)
+        {
+            return false;
+        }
+
+        byte[] suppliedEnc = BigIntegers.asUnsignedByteArray(expectedEnc.length, supplied);
+
+        boolean rv = Arrays.constantTimeAreEqual(expectedEnc, suppliedEnc);
+
+        Arrays.fill(suppliedEnc, (byte)0);
+
+        return rv;
+    }
+
 }

core/src/test/java/org/bouncycastle/crypto/agreement/test/JPAKEUtilTest.java

@@ -139,6 +139,62 @@ public void testValidateMacTag()
         }
     }
 
+    /**
+     * The MacTag comparison encodes both sides to the MAC's fixed width so its running time does
+     * not depend on the expected - secret-derived - value. calculateMacTag reads the MAC output
+     * with the <i>signed</i> BigInteger(byte[]) constructor, so a tag is negative about half the
+     * time, and about one tag in 240 has a two's-complement encoding shorter than the MAC. Those
+     * are the tags a zero-padding fixed-width encoding (BigIntegers.asUnsignedByteArray) fails to
+     * round-trip, so enough tags are exercised here to include some, and every one of them has to
+     * validate. A tag too wide to be one is rejected.
+     */
+    public void testValidateMacTagEncoding()
+        throws CryptoException
+    {
+        JPAKEPrimeOrderGroup pg1 = JPAKEPrimeOrderGroups.SUN_JCE_1024;
+
+        Digest digest = SHA256Digest.newInstance();
+
+        BigInteger gx1 = BigInteger.valueOf(11);
+        BigInteger gx2 = BigInteger.valueOf(22);
+        BigInteger gx3 = BigInteger.valueOf(33);
+        BigInteger gx4 = BigInteger.valueOf(44);
+
+        int shortEncodings = 0;
+
+        for (int i = 0; i != 4096; i++)
+        {
+            BigInteger keyingMaterial = BigInteger.valueOf(i + 1);
+
+            BigInteger macTag = JPAKEUtil.calculateMacTag(
+                "participantId", "partnerParticipantId", gx1, gx2, gx3, gx4, keyingMaterial, digest);
+
+            if (macTag.toByteArray().length != digest.getDigestSize())
+            {
+                shortEncodings++;
+            }
+
+            /* the tag the partner computed must always validate */
+            JPAKEUtil.validateMacTag("partnerParticipantId", "participantId",
+                gx3, gx4, gx1, gx2, keyingMaterial, digest, macTag);
+
+            /* and a tag too wide to be one must not */
+            try
+            {
+                JPAKEUtil.validateMacTag("partnerParticipantId", "participantId",
+                    gx3, gx4, gx1, gx2, keyingMaterial, digest,
+                    macTag.add(BigInteger.ONE.shiftLeft(8 * digest.getDigestSize())));
+                fail("oversized MacTag accepted");
+            }
+            catch (CryptoException e)
+            {
+                // pass
+            }
+        }
+
+        assertTrue("no short-encoding MacTags were exercised", shortEncodings > 0);
+    }
+
     public void testValidateNotNull()
     {
         JPAKEUtil.validateNotNull("a", "description");

core/src/test/java/org/bouncycastle/crypto/test/SRP6Test.java

@@ -39,6 +39,7 @@ public void performTest() throws Exception
         rfc5054AppendixBTestVectors();
 
         testMutualVerification(SRP6StandardGroups.rfc5054_1024);
+        testEvidenceMessageVerification(SRP6StandardGroups.rfc5054_1024);
         testClientCatchesBadB(SRP6StandardGroups.rfc5054_1024);
         testServerCatchesBadA(SRP6StandardGroups.rfc5054_1024);
 
@@ -288,6 +289,94 @@ private void testMutualVerification(SRP6GroupParameters group) throws CryptoExce
         }
     }
 
+    /**
+     * Exercises the evidence-message (M1/M2) verification path, which is where the received
+     * keyed authenticators are checked. A correct M1/M2 must be accepted and a tampered one
+     * rejected, whether the tampering leaves the encoded length the same or shortens it.
+     */
+    private void testEvidenceMessageVerification(SRP6GroupParameters group) throws CryptoException
+    {
+        byte[] I = "username".getBytes();
+        byte[] P = "password".getBytes();
+        byte[] s = new byte[16];
+        random.nextBytes(s);
+
+        SRP6VerifierGenerator gen = new SRP6VerifierGenerator();
+        gen.init(group, SHA256Digest.newInstance());
+        BigInteger v = gen.generateVerifier(s, I, P);
+
+        SRP6Client client = new SRP6Client();
+        client.init(group, SHA256Digest.newInstance(), random);
+
+        SRP6Server server = new SRP6Server();
+        server.init(group, v, SHA256Digest.newInstance(), random);
+
+        BigInteger A = client.generateClientCredentials(s, I, P);
+        BigInteger B = server.generateServerCredentials();
+
+        client.calculateSecret(B);
+        server.calculateSecret(A);
+
+        BigInteger clientM1 = client.calculateClientEvidenceMessage();
+
+        if (!server.verifyClientEvidenceMessage(clientM1))
+        {
+            fail("server rejected the correct client evidence message M1");
+        }
+        if (server.verifyClientEvidenceMessage(clientM1.add(BigInteger.valueOf(1))))
+        {
+            fail("server accepted a tampered client evidence message M1");
+        }
+        if (server.verifyClientEvidenceMessage(clientM1.shiftRight(8)))
+        {
+            fail("server accepted a truncated client evidence message M1");
+        }
+
+        BigInteger serverM2 = server.calculateServerEvidenceMessage();
+
+        if (!client.verifyServerEvidenceMessage(serverM2))
+        {
+            fail("client rejected the correct server evidence message M2");
+        }
+        if (client.verifyServerEvidenceMessage(serverM2.add(BigInteger.valueOf(1))))
+        {
+            fail("client accepted a tampered server evidence message M2");
+        }
+        if (client.verifyServerEvidenceMessage(serverM2.shiftRight(8)))
+        {
+            fail("client accepted a truncated server evidence message M2");
+        }
+
+        // M1/M2 are digest outputs read as non-negative integers, so a negative value, or one too
+        // large to be a digest output, is rejected rather than reaching the fixed-width encoding
+        if (server.verifyClientEvidenceMessage(clientM1.negate()))
+        {
+            fail("server accepted a negative client evidence message M1");
+        }
+        if (server.verifyClientEvidenceMessage(clientM1.shiftLeft(256)))
+        {
+            fail("server accepted an oversized client evidence message M1");
+        }
+        if (client.verifyServerEvidenceMessage(serverM2.negate()))
+        {
+            fail("client accepted a negative server evidence message M2");
+        }
+        if (client.verifyServerEvidenceMessage(serverM2.shiftLeft(256)))
+        {
+            fail("client accepted an oversized server evidence message M2");
+        }
+
+        // ...and the correct values still verify afterwards
+        if (!server.verifyClientEvidenceMessage(clientM1))
+        {
+            fail("server rejected the correct client evidence message M1 on retry");
+        }
+        if (!client.verifyServerEvidenceMessage(serverM2))
+        {
+            fail("client rejected the correct server evidence message M2 on retry");
+        }
+    }
+
     private void testClientCatchesBadB(SRP6GroupParameters group)
     {
         byte[] I = "username".getBytes();