use constant-time comparison for srp-6a evidence messages

Author: rootvector2

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

@@ -7,6 +7,7 @@
 import org.bouncycastle.crypto.CryptoServicesRegistrar;
 import org.bouncycastle.crypto.Digest;
 import org.bouncycastle.crypto.params.SRP6GroupParameters;
+import org.bouncycastle.util.Arrays;
 import org.bouncycastle.util.BigIntegers;
 
 /**
@@ -154,7 +155,7 @@ 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))
+        if (Arrays.constantTimeAreEqual(computedM2.toByteArray(), serverM2.toByteArray()))
         {
             this.M2 = serverM2;
             return true;

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

@@ -7,6 +7,7 @@
 import org.bouncycastle.crypto.CryptoServicesRegistrar;
 import org.bouncycastle.crypto.Digest;
 import org.bouncycastle.crypto.params.SRP6GroupParameters;
+import org.bouncycastle.util.Arrays;
 import org.bouncycastle.util.BigIntegers;
 
 /**
@@ -133,7 +134,7 @@ 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))
+        if (Arrays.constantTimeAreEqual(computedM1.toByteArray(), clientM1.toByteArray()))
         {
             this.M1 = clientM1;
             return true;

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,65 @@ 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");
+        }
+    }
+
     private void testClientCatchesBadB(SRP6GroupParameters group)
     {
         byte[] I = "username".getBytes();

docs/releasenotes.html

@@ -51,6 +51,7 @@ <h3>2.1.2 Defects Fixed</h3>
 <li>GOST R 34.10-94 signing (org.bouncycastle.crypto.signers.GOST3410Signer) raised the domain generator to the per-signature nonce k with a bare BigInteger.modPow, whose running time varies with the exponent. Recovering k from that timing yields the private key straight out of the signature equation s = k*m + x*r, so k is now randomised with a random multiple of q before it is raised, exactly as DSASigner already does with its own k. Since the domain parameter a has order q, raising it to a multiple of q gives 1 and the signature is unchanged - the RFC-style known-answer vectors in GOST3410Test still produce the same r and s. Those vectors drive signing from a FixedSecureRandom, so they now carry one further byte for the randomiser to consume, in the same way the DSA signing vectors already do.</li>
 <li>KCCMBlockCipher (DSTU7624-128/256/512 CCM mode) returned the input length rather than 0 from getUpdateOutputSize(int), but like CCMBlockCipher/KGCMBlockCipher it buffers all input until doFinal and produces no output on an update. Through the JCA layer this made the caller-supplied-buffer Cipher.update(input, inOff, inLen, output, outOff) reject a correctly sized output buffer with "javax.crypto.ShortBufferException: output buffer too short for input." when decrypting. getUpdateOutputSize now returns 0, matching the sibling CCM/KGCM modes (github #2354).</li>
 <li>J-PAKE raised values to exponents carrying private material with bare BigInteger.modPow calls, whose running time varies with the exponent: the private ephemerals x1 and x2 in round 1, x2*s and the negated form of it in round 2 and in the keying material - both of which carry the password - and the v behind each Schnorr zero-knowledge proof, which together with the published r would give up x. JPAKEParticipant itself notes that leaking x1 or x2 lets an attacker brute-force the password. All of these exponents are now randomised with a random multiple of q before they are raised. A multiple of q rather than of p-1 is sound here, and much cheaper since the exponents are the size of q: the generator is checked with g^q = 1 when the JPAKEPrimeOrderGroup is built, and each value received from the other participant is checked the same way by validateZeroKnowledgeProof before it is used as a base. JPAKEUtil.calculateA and JPAKEUtil.calculateKeyingMaterial gained overloads taking a SecureRandom, and there is a new JPAKEUtil.calculateGx taking q and a SecureRandom; the existing overloads still work, taking the default from CryptoServicesRegistrar. The three-argument calculateGx has no q to work with, so it blinds with a multiple of p-1 and is deprecated in favour of the new one. The three modPow calls in validateZeroKnowledgeProof are unchanged, since their exponents are all public. The elliptic-curve variant was already routing its private scalars through ECAlgorithms.multiplySecret and needed no change.</li>
+<li>The SRP-6a evidence-message checks compared the authenticator received from the peer against the locally computed one with BigInteger.equals, whose word-by-word magnitude comparison returns as soon as it meets a difference and so runs in a value-dependent time. M1 and M2 are keyed authenticators derived from the shared secret S and arrive from the not-yet-authenticated peer, so that early-out is a MAC-comparison timing oracle against a value the peer is trying to guess. SRP6Server.verifyClientEvidenceMessage and SRP6Client.verifyServerEvidenceMessage - in both the lightweight org.bouncycastle.crypto.agreement.srp package and the org.bouncycastle.tls.crypto.impl.jcajce.srp copies BCTLS carries - now compare with Arrays.constantTimeAreEqual over the fixed encodings, as the sibling J-PAKE MacTag check already does. The accept/reject decision is unchanged for every input, so a correct M1/M2 still verifies and any other value is still rejected.</li>
 <li>The PKIX CertPathBuilder ("PKIX"/"RFC5280"/"RFC3280") matched candidate issuers by subject name only during its depth-first search, so a CertStore containing many self-issued certificates that share a single subject name and never chain to a trust anchor could be explored as a large number of partial paths before the build concluded no chain exists. The builder now bounds the total number of nodes visited per build; the limit is configurable via the org.bouncycastle.x509.max_cert_path_build_nodes system property (default 262144, far above any legitimate build) and, when exceeded, the build fails with a CertPathBuilderException naming the property. This is the builder-side companion to the existing org.bouncycastle.x509.max_policy_nodes bound.</li>
 <li>A group of parse and revocation-handling entry points let an unchecked runtime exception (NullPointerException, ArrayIndexOutOfBoundsException, IllegalStateException or ArithmeticException) escape on empty, content-less or out-of-range input instead of the checked exception each entry point declares - the malformed input was rejected either way, but the leaked type could escape a documented throws contract. Each now fails with its declared type, and well-formed input is unaffected: org.bouncycastle.tsp.cms.CMSTimeStampedData (an empty or truncated stream; the fix also covers its org.bouncycastle.asn1.cms.MetaData and TimeStampDataUtil helpers) and org.bouncycastle.cms.CMSEnvelopedData (an EnvelopedData carrying no encryptedContent) now throw IOException / CMSException rather than NullPointerException; org.bouncycastle.tsp.TimeStampToken rejects a token whose SignerInfo carries no signed attributes with TSPValidationException rather than NullPointerException; org.bouncycastle.cert.cmp.GeneralPKIMessage, org.bouncycastle.est.CSRAttributesResponse, org.bouncycastle.cmc.SimplePKIResponse, org.bouncycastle.openssl.X509TrustedCertificateBlock, org.bouncycastle.tsp.TimeStampRequest, org.bouncycastle.tsp.TimeStampResponse, org.bouncycastle.pkcs.PKCS12PfxPdu and org.bouncycastle.pkcs.PKCS8EncryptedPrivateKeyInfo reject empty / no-content / truncated input with their declared CertIOException / IOException / PKCSIOException rather than a leaked NullPointerException, and org.bouncycastle.cert.crmf.CertificateRequestMessage.hasSigningKeyProofOfPossessionWithPKMAC answers false for an absent or non-signing-key proof-of-possession rather than throwing NullPointerException; org.bouncycastle.crypto.util.OpenSSHPrivateKeyUtil.parsePrivateKeyBlob and org.bouncycastle.math.ec.ECCurve.decodePoint reject an empty (or null) blob / point encoding with IllegalArgumentException rather than ArrayIndexOutOfBoundsException, closing the point-decode path reached by every untrusted-point consumer (EC key parsing, ECDH/ECIES, TLS); and the PKIX revocation code no longer leaks a runtime exception on attacker-controlled CRL/OCSP fields - PKIXCertPathReviewer and X509RevocationChecker bound an out-of-range CRLReason code against their fixed reason table (reporting "unknown" instead of ArrayIndexOutOfBoundsException / ArithmeticException), RFC3280CertPathUtilities tolerates an absent reasons mask on a CRL distribution point, and ProvOcspRevocationChecker tolerates an OCSP response with no nonce extension.</li>
 <li>java.security.AlgorithmParameters.init(byte[]) is contracted to throw IOException on a decoding error, but several BC AlgorithmParameters SPIs (RSA OAEP/PSS, EC, DSA, DH, ElGamal, IES, GOST, and the GCM/CCM parameters of AES, ARIA, LEA and SM4) could leak an unchecked exception. Each affected engineInit(byte[]) and both loadParameters helpers now convert a leaked runtime exception to IOException; well-formed parameters are unaffected.</li>

tls/src/main/java/org/bouncycastle/tls/crypto/impl/jcajce/srp/SRP6Client.java

@@ -6,6 +6,7 @@
 import org.bouncycastle.crypto.CryptoServicesRegistrar;
 import org.bouncycastle.tls.crypto.SRP6Group;
 import org.bouncycastle.tls.crypto.TlsHash;
+import org.bouncycastle.util.Arrays;
 import org.bouncycastle.util.BigIntegers;
 
 /**
@@ -152,7 +153,7 @@ public boolean verifyServerEvidenceMessage(BigInteger serverM2) throws IllegalSt
 
         // Compute the own server evidence message 'M2'
         BigInteger computedM2 = SRP6Util.calculateM2(digest, N, A, M1, S);
-        if (computedM2.equals(serverM2))
+        if (Arrays.constantTimeAreEqual(computedM2.toByteArray(), serverM2.toByteArray()))
         {
             this.M2 = serverM2;
             return true;

tls/src/main/java/org/bouncycastle/tls/crypto/impl/jcajce/srp/SRP6Server.java

@@ -6,6 +6,7 @@
 import org.bouncycastle.crypto.CryptoServicesRegistrar;
 import org.bouncycastle.tls.crypto.SRP6Group;
 import org.bouncycastle.tls.crypto.TlsHash;
+import org.bouncycastle.util.Arrays;
 import org.bouncycastle.util.BigIntegers;
 
 /**
@@ -131,7 +132,7 @@ public boolean verifyClientEvidenceMessage(BigInteger clientM1) throws IllegalSt
 
         // Compute the own client evidence message 'M1'
         BigInteger computedM1 = SRP6Util.calculateM1(digest, N, A, B, S);
-        if (computedM1.equals(clientM1))
+        if (Arrays.constantTimeAreEqual(computedM1.toByteArray(), clientM1.toByteArray()))
         {
             this.M1 = clientM1;
             return true;