check signed-message length in AIMerSigner.verifySignature

Author: rootvector2

core/src/main/java/org/bouncycastle/pqc/crypto/aimer/AIMerSigner.java

@@ -77,6 +77,17 @@ public byte[] generateSignature(byte[] message)
     @Override
     public boolean verifySignature(byte[] message, byte[] signature)
     {
+        // generateSignature returns the message followed by the signature (the
+        // signed-message envelope), so the signature proper starts at
+        // message.length. Reject anything but exactly that envelope before
+        // slicing it: a shorter buffer would throw
+        // ArrayIndexOutOfBoundsException, and a longer one would have its
+        // trailing bytes ignored, so a valid signature with data appended
+        // would still verify.
+        if (signature.length != message.length + params.getSignatureBytes())
+        {
+            return false;
+        }
         byte[] sig = new byte[params.getSignatureBytes()];
         AIMerEngine engine = new AIMerEngine(params);
         System.arraycopy(signature, message.length, sig, 0, params.getSignatureBytes());

core/src/test/java/org/bouncycastle/pqc/crypto/test/AIMerTest.java

@@ -3,6 +3,7 @@
 import java.security.SecureRandom;
 
 import junit.framework.TestCase;
+import org.bouncycastle.crypto.AsymmetricCipherKeyPair;
 import org.bouncycastle.crypto.AsymmetricCipherKeyPairGenerator;
 import org.bouncycastle.crypto.CipherParameters;
 import org.bouncycastle.crypto.Signer;
@@ -14,6 +15,8 @@
 import org.bouncycastle.pqc.crypto.aimer.AIMerPrivateKeyParameters;
 import org.bouncycastle.pqc.crypto.aimer.AIMerPublicKeyParameters;
 import org.bouncycastle.pqc.crypto.aimer.AIMerSigner;
+import org.bouncycastle.util.Arrays;
+import org.bouncycastle.util.Strings;
 
 public class AIMerTest
     extends TestCase
@@ -23,6 +26,7 @@ public static void main(String[] args)
     {
         AIMerTest test = new AIMerTest();
         test.testTestVectors();
+        test.testWrongLengthSignatureRejected();
     }
 
     private static final AIMerParameters[] PARAMETER_SETS = new AIMerParameters[]
@@ -94,4 +98,38 @@ public MessageSigner getMessageSigner()
         long end = System.currentTimeMillis();
         System.out.println("time cost: " + (end - start) + "\n");
     }
+
+    public void testWrongLengthSignatureRejected()
+    {
+        SecureRandom random = new SecureRandom();
+        byte[] message = Strings.toByteArray("AIMer wrong length signature");
+
+        for (int i = 0; i != PARAMETER_SETS.length; i++)
+        {
+            AIMerParameters parameters = PARAMETER_SETS[i];
+
+            AIMerKeyPairGenerator kpGen = new AIMerKeyPairGenerator();
+            kpGen.init(new AIMerKeyGenerationParameters(random, parameters));
+            AsymmetricCipherKeyPair kp = kpGen.generateKeyPair();
+
+            AIMerSigner signer = new AIMerSigner();
+            signer.init(true, kp.getPrivate());
+            byte[] signature = signer.generateSignature(message);
+
+            AIMerSigner verifier = new AIMerSigner();
+            verifier.init(false, kp.getPublic());
+
+            assertEquals(parameters.getName(), message.length + parameters.getSignatureBytes(), signature.length);
+            assertTrue(parameters.getName(), verifier.verifySignature(message, signature));
+
+            // a short buffer must be rejected rather than indexed past its end
+            assertFalse(parameters.getName(), verifier.verifySignature(message, new byte[0]));
+            assertFalse(parameters.getName(), verifier.verifySignature(message, Arrays.copyOf(signature, signature.length - 1)));
+            assertFalse(parameters.getName(), verifier.verifySignature(message, Arrays.copyOf(signature, message.length)));
+
+            // trailing data must not be silently ignored
+            assertFalse(parameters.getName(), verifier.verifySignature(message, Arrays.append(signature, (byte)0)));
+            assertFalse(parameters.getName(), verifier.verifySignature(message, Arrays.concatenate(signature, new byte[16])));
+        }
+    }
 }

docs/releasenotes.html

@@ -135,6 +135,7 @@ <h3>2.1.2 Defects Fixed</h3>
 <li>The BCJSSE provider now only computes active early key share groups for clients offering TLS 1.3+. Previously it was computed for all connections, which was mostly harmless, but could generate misleading log messages (at WARNING level) for servers or pre-TLS1.3 clients where the logged condition was irrelevant (github #2392).</li>
 <li>Initialising a BC signature, cipher or key-agreement service with a private key from another provider whose key material is not accessible - a hardware-backed key whose getModulus() / getX() / getS() / getParams() throws, as an IBM CCA RSAPrivateHWKey does with "Hardware error, function getModulus has no meaning in hardware" - let that provider-specific unchecked UnsupportedOperationException escape a method declared to throw only InvalidKeyException, so an mTLS handshake failed with a raw hardware error rather than the documented type. The key-parameter helpers that examine a key through a java.security.interfaces or javax.crypto.interfaces type a foreign provider may implement - RSAUtil, DSAUtil, ECUtil (its java.security.interfaces.ECPrivateKey branch), both DHUtil copies and ElGamalUtil - now catch a failure to read the key's parameters and raise InvalidKeyException with the original exception chained as its cause. A caller, or a JSSE layer that keys its provider fallback on InvalidKeyException, sees the declared type and a clear message. This does not let BC sign or decrypt with a non-exportable hardware key, which is not possible; it makes the refusal in contract. BC's own keys and any exportable key are unaffected (github #1440).</li>
 <li>org.bouncycastle.operator.DefaultKemEncapsulationLengthProvider.getEncapsulationLength() looked its argument up in a table of the KEMs whose encapsulation lengths are registered - ML-KEM, NTRU, HQC, FrodoKEM and composite ML-KEM - and dereferenced the result without checking it. A CMS RFC 9629 KEMRecipientInfo recipient using any other KEM which does register a key wrapping cipher, BIKE, NTRU+ or SMAUG-T, got as far as the wrap and then failed with a NullPointerException carrying no indication of which algorithm was at fault. The lookup now throws IllegalArgumentException naming the KEM's OID, as the sibling DefaultKemAlgorithmIdentifierFinder does, and the contract is documented on the KemEncapsulationLengthProvider interface (github #2398).</li>
+<li>AIMerSigner.verifySignature (org.bouncycastle.pqc.crypto.aimer) sliced the signature out of the signed-message envelope generateSignature produces - the message followed by the signature - at offset message.length without first checking the envelope was that long, so a truncated or otherwise short signature threw ArrayIndexOutOfBoundsException out of verify rather than returning false, reaching the caller unchecked through Signature.verify() on the BCPQC "AIMer" services; and because only the bytes at that offset were read, a valid signature with data appended still verified, so the accepted encoding was not unique. The envelope is now required to be exactly message.length + the parameter set's signature size before it is read, and anything else is reported as a failed verification, matching the guard the Falcon, Faest, Mayo, Snova and QRUOV signers apply. A correctly formed signature is unaffected.</li>
 </ul>
 
 <h3>2.1.3 Additional Features and Functionality</h3>