Further review of BLS implementation. Additional tests and public key validation, WipingBuffer

Author: dghgit

core/src/main/java/org/bouncycastle/crypto/generators/BLSKeyPairGenerator.java

@@ -49,7 +49,20 @@ public AsymmetricCipherKeyPair generateKeyPair()
         }
 
         // Draw 32 bytes of IKM from the SecureRandom (the spec's minimum).
-        // Feeding this through KeyGen / HKDF gives a uniform secret in [1, r - 1].
+        // Feeding this through KeyGen / HKDF gives a uniform secret in
+        // [1, r - 1].
+        //
+        // The try/finally wipe of ikm is load-bearing: KeyGen is a pure
+        // deterministic function of (IKM, salt, key_info), and salt /
+        // key_info are fixed public constants for this generator. So
+        // anyone who reads ikm from a heap dump can derive the same sk by
+        // re-running keyGen offline — i.e. ikm is effectively a second
+        // copy of the secret-key bytes. Without the wipe, the bytes
+        // survive until the next GC collects the local stack frame and
+        // the array body; with the wipe the residence window is the body
+        // of the try-block. (The constant-time scalar-mult work at
+        // sign-time would be pointless if a heap inspector could just
+        // read the seed material from a separate file.)
         byte[] ikm = new byte[32];
         try
         {

core/src/main/java/org/bouncycastle/crypto/params/BLSPublicKeyParameters.java

@@ -1,12 +1,32 @@
 package org.bouncycastle.crypto.params;
 
+import org.bouncycastle.crypto.bls.BLS12_381BasicScheme;
 import org.bouncycastle.crypto.bls.BLS12_381Serialization;
 import org.bouncycastle.math.ec.ECPoint;
 
 /**
  * Public-key parameters for a BLS signature scheme. The underlying point
  * lives on the suite-specific G1 curve (BLS12-381 G1 today; further BLS
  * families add curves in the future).
+ * <p>
+ * <b>Constructor invariant.</b> Construction runs the full
+ * draft-irtf-cfrg-bls-signature sec. 2.5 {@code KeyValidate} (on-curve +
+ * prime-order subgroup + non-identity) on the supplied point and rejects
+ * anything that fails. Callers and downstream consumers can therefore
+ * rely on the type itself as the validation boundary — once an instance
+ * exists, the point is a valid BLS public key, and verify-time code
+ * (e.g. {@link org.bouncycastle.crypto.signers.BLSSigner#verifySignature
+ * BLSSigner.verifySignature}) does not need to redo the ~128-bit
+ * GLV-shortened subgroup-membership scalar multiplication on every call.
+ * <p>
+ * The validation cost is the same as a single verify call's previous
+ * pre-pairing keyValidate step, paid once at construction instead of N
+ * times for N verifies under the same key. Callers that deserialize
+ * public keys from untrusted bytes typically run
+ * {@link BLS12_381Serialization#decompressG1 decompressG1} immediately
+ * before constructing this object; that decompression does not subgroup
+ * check, so the constructor is the place where the prime-order check
+ * happens.
  */
 public class BLSPublicKeyParameters
     extends BLSKeyParameters
@@ -16,7 +36,21 @@ public class BLSPublicKeyParameters
     public BLSPublicKeyParameters(BLSParameters params, ECPoint publicPoint)
     {
         super(false, params);
-        this.publicPoint = publicPoint.normalize();
+        if (publicPoint == null)
+        {
+            throw new IllegalArgumentException("publicPoint must not be null");
+        }
+        ECPoint normalised = publicPoint.normalize();
+        // KeyValidate per draft-irtf-cfrg-bls-signature sec. 2.5: rejects
+        // identity, off-curve points (curve equation), and points in
+        // E(Fp) \ G1 (subgroup check). This is the invariant the class
+        // promises downstream — see the class javadoc.
+        if (!BLS12_381BasicScheme.keyValidate(normalised))
+        {
+            throw new IllegalArgumentException(
+                "invalid BLS public key: must be a non-identity point in the prime-order G1 subgroup");
+        }
+        this.publicPoint = normalised;
     }
 
     /**

core/src/main/java/org/bouncycastle/crypto/signers/BLSSigner.java

@@ -1,7 +1,5 @@
 package org.bouncycastle.crypto.signers;
 
-import java.io.ByteArrayOutputStream;
-
 import org.bouncycastle.crypto.CipherParameters;
 import org.bouncycastle.crypto.CryptoException;
 import org.bouncycastle.crypto.Signer;
@@ -118,7 +116,7 @@ public byte[] generateSignature()
         {
             throw new IllegalStateException("BLSSigner not initialised for signing");
         }
-        byte[] msg = buffer.toByteArray();
+        byte[] msg = buffer.snapshot();
         try
         {
             BLS12_381G2HashToCurve h = new BLS12_381G2HashToCurve(dst);
@@ -128,6 +126,11 @@ public byte[] generateSignature()
         }
         finally
         {
+            // Wipe the snapshot copy in addition to the resetting the
+            // backing buffer — hashToCurve has already read every byte
+            // through the SHA-256 expand_message_xmd, so the array is
+            // safe to zero at this point. See WipingBuffer's doc for
+            // why the snapshot is needed at all.
             Arrays.fill(msg, (byte)0);
             reset();
         }
@@ -139,7 +142,6 @@ public boolean verifySignature(byte[] signature)
         {
             throw new IllegalStateException("BLSSigner not initialised for verification");
         }
-        byte[] msg = buffer.toByteArray();
         try
         {
             BLS12_381G2Point sig;
@@ -151,28 +153,32 @@ public boolean verifySignature(byte[] signature)
             {
                 return false;
             }
+
             ECPoint pk = publicKey.getPublicPoint();
-            if (!BLS12_381BasicScheme.keyValidate(pk))
+            if (sig.isInfinity() || !BLS12_381SubgroupCheck.isInG2Subgroup(sig))
             {
                 return false;
             }
-            if (sig.isInfinity() || !BLS12_381SubgroupCheck.isInG2Subgroup(sig))
+            byte[] msg = buffer.snapshot();
+            try
             {
-                return false;
+                BLS12_381G2HashToCurve h = new BLS12_381G2HashToCurve(dst);
+                BLS12_381G2Point q = h.hashToCurve(msg);
+                ECCurve curve = BLS12_381G1.createCurve();
+                ECPoint g1 = BLS12_381G1.getGenerator(curve);
+                // e(g1, sig) == e(pk, H(msg))   <=>   e(g1, sig) * e(-pk, H(msg)) == 1
+                Fp12Element acc = BLS12_381Pairing.multiPair(
+                    new ECPoint[]{g1, pk.negate()},
+                    new BLS12_381G2Point[]{sig, q});
+                return Fp12Element.ONE.equals(acc);
+            }
+            finally
+            {
+                Arrays.fill(msg, (byte)0);
             }
-            BLS12_381G2HashToCurve h = new BLS12_381G2HashToCurve(dst);
-            BLS12_381G2Point q = h.hashToCurve(msg);
-            ECCurve curve = BLS12_381G1.createCurve();
-            ECPoint g1 = BLS12_381G1.getGenerator(curve);
-            // e(g1, sig) == e(pk, H(msg))   <=>   e(g1, sig) * e(-pk, H(msg)) == 1
-            Fp12Element acc = BLS12_381Pairing.multiPair(
-                new ECPoint[]{g1, pk.negate()},
-                new BLS12_381G2Point[]{sig, q});
-            return Fp12Element.ONE.equals(acc);
         }
         finally
         {
-            Arrays.fill(msg, (byte)0);
             reset();
         }
     }
@@ -183,23 +189,127 @@ public void reset()
     }
 
     /**
-     * ByteArrayOutputStream subclass that wipes its internal byte storage
-     * before resetting the count, so message bytes don't linger in the
-     * heap between {@code reset()} and the next GC.
+     * Wipe-aware byte buffer backing the {@link Signer#update update}
+     * contract — bytes accumulate here between {@link #init} /
+     * {@link #reset} cycles.
+     * <p>
+     * <b>Wipe scope</b> (W3 in the review):
+     * <ul>
+     *   <li>On {@link #wipeAndReset}: zero positions {@code 0..count} of
+     *       the current backing array, then reset {@code count}. Called
+     *       from the signer's {@code reset()}.</li>
+     *   <li>On internal capacity growth: zero the old backing array
+     *       before releasing it to GC, so the doubling-resize pattern
+     *       doesn't leave a chain of obsolete buffers each holding a
+     *       prefix of the message.</li>
+     *   <li>On {@link #snapshot}: returns a fresh defensive copy that
+     *       the signer's {@code finally} block zeroes after the
+     *       hash-to-curve consumes it. The copy is unavoidable because
+     *       {@code hashToCurve} takes a {@code byte[]}, not a
+     *       {@code (byte[], offset, length)} triple.</li>
+     * </ul>
+     * <p>
+     * <b>What this does NOT cover.</b> The wipe is best-effort. Message
+     * bytes still flow through the SHA-256 block buffer inside
+     * {@code expand_message_xmd}; the JVM may relocate arrays during GC,
+     * leaving spectral copies behind; and reflection / native debuggers
+     * can observe live bytes anyway. The replacement of the previous
+     * {@link java.io.ByteArrayOutputStream}-derived buffer was driven by
+     * the doubling-resize gap above — the prior class overstated wipe
+     * coverage in its docstring. In typical BLS use the message is not
+     * secret (consensus signing roots, transaction bodies), so a strict
+     * wipe is not load-bearing for the signer; this class minimises
+     * residence as a defence-in-depth measure for callers who choose to
+     * sign sensitive data.
      * <p>
-     * Not thread-safe — {@link BLSSigner} itself follows the usual BC
-     * convention of single-threaded use, so the {@code synchronized}
-     * qualifiers on the inherited {@link ByteArrayOutputStream#write}
-     * methods are not relied on, and {@code wipeAndReset} matches that
-     * contract.
+     * Not thread-safe — the BC {@link Signer} contract is per-instance,
+     * per-thread.
      */
     private static final class WipingBuffer
-        extends ByteArrayOutputStream
     {
+        private byte[] buf = new byte[64];
+        private int count;
+
+        void write(byte b)
+        {
+            ensureCapacity(longSize(count, 1));
+            buf[count++] = b;
+        }
+
+        void write(byte[] in, int off, int len)
+        {
+            if (in == null)
+            {
+                throw new NullPointerException("input array must not be null");
+            }
+            if (off < 0 || len < 0 || ((long)off + (long)len) > (long)in.length)
+            {
+                throw new IndexOutOfBoundsException(
+                    "off=" + off + " len=" + len + " in.length=" + in.length);
+            }
+            ensureCapacity(longSize(count, len));
+            System.arraycopy(in, off, buf, count, len);
+            count += len;
+        }
+
+        /**
+         * @return a fresh copy of bytes {@code 0..count}. The caller is
+         * responsible for wiping the returned array once consumed.
+         */
+        byte[] snapshot()
+        {
+            byte[] out = new byte[count];
+            System.arraycopy(buf, 0, out, 0, count);
+            return out;
+        }
+
         void wipeAndReset()
         {
             Arrays.fill(buf, 0, count, (byte)0);
-            this.count = 0;
+            count = 0;
+        }
+
+        /**
+         * Compute {@code current + delta} as a {@code long} to detect
+         * {@code int} overflow at the {@code count + len} boundary.
+         * Casting through {@code long} avoids the silent wrap-around
+         * that {@code int + int} would produce for ~2GB messages, which
+         * would translate to negative-capacity arguments later.
+         */
+        private static long longSize(int current, int delta)
+        {
+            return (long)current + (long)delta;
+        }
+
+        private void ensureCapacity(long needed)
+        {
+            if (needed <= buf.length)
+            {
+                return;
+            }
+            if (needed > (long)(Integer.MAX_VALUE - 8))
+            {
+                throw new OutOfMemoryError(
+                    "BLSSigner buffer would exceed maximum array size");
+            }
+            int newCap = buf.length;
+            while ((long)newCap < needed)
+            {
+                long doubled = (long)newCap << 1;
+                if (doubled > (long)(Integer.MAX_VALUE - 8))
+                {
+                    newCap = Integer.MAX_VALUE - 8;
+                    break;
+                }
+                newCap = (int)doubled;
+            }
+            byte[] grown = new byte[newCap];
+            System.arraycopy(buf, 0, grown, 0, count);
+            // Wipe BEFORE releasing the old buffer to GC: this is the
+            // load-bearing part of W3. Without it, every doubling-grow
+            // leaves another stale buffer in the heap.
+            Arrays.fill(buf, 0, count, (byte)0);
+            buf = grown;
         }
     }
 }

core/src/test/java/org/bouncycastle/crypto/hash2curve/test/BLS12_381BasicSchemeTest.java

@@ -1,11 +1,13 @@
 package org.bouncycastle.crypto.hash2curve.test;
 
 import java.math.BigInteger;
+import java.security.SecureRandom;
 
 import junit.framework.TestCase;
 import org.bouncycastle.crypto.bls.BLS12_381BasicScheme;
 import org.bouncycastle.crypto.bls.BLS12_381G1;
 import org.bouncycastle.crypto.bls.BLS12_381G2Point;
+import org.bouncycastle.crypto.bls.BLS12_381SubgroupCheck;
 import org.bouncycastle.math.ec.ECCurve;
 import org.bouncycastle.math.ec.ECPoint;
 import org.bouncycastle.util.Strings;
@@ -133,4 +135,76 @@ public void testVerifyRejectsInfinitySignature()
         assertFalse("infinity signature must not verify",
             BLS12_381BasicScheme.verify(pk, Strings.toUTF8ByteArray("any"), BLS12_381G2Point.INFINITY));
     }
+
+    // ---------------------------------------------------------------------
+    // keyValidate negative-branch coverage (review gap G3).
+    //
+    // The existing testKeyValidateRejectsIdentity exercises the
+    // non-identity guard. keyValidate also rejects null and off-subgroup
+    // points; both should be pinned. (Off-curve rejection is harder to
+    // exercise without bypassing ECCurve.createPoint's on-curve check;
+    // skipped — the on-curve test path is implicitly covered by
+    // testVerifyRejectsTamperedSignature, which constructs sig + G2_gen
+    // and expects rejection.)
+    // ---------------------------------------------------------------------
+
+    public void testKeyValidateRejectsNull()
+    {
+        assertFalse("null public key must be rejected",
+            BLS12_381BasicScheme.keyValidate(null));
+    }
+
+    public void testKeyValidateRejectsOffSubgroup()
+    {
+        // Construct a point on E(Fp) that is NOT in the prime-order G1
+        // subgroup (same approach as
+        // BLSKeyPairGeneratorTest.testPublicKeyParametersRejectsOffSubgroup).
+        // A random curve point lands in G1 with probability ~1/cofactor
+        // (cofactor ~2^126), so a handful of attempts is plenty.
+        SecureRandom rng = new SecureRandom(new byte[]{91});
+        BigInteger p = BLS12_381G1.Q;
+        ECCurve curve = BLS12_381G1.createCurve();
+        for (int attempt = 0; attempt < 16; ++attempt)
+        {
+            BigInteger x = new BigInteger(p.bitLength(), rng).mod(p);
+            BigInteger rhs = x.modPow(BigInteger.valueOf(3), p)
+                .add(BigInteger.valueOf(4)).mod(p);
+            BigInteger y = rhs.modPow(p.add(BigInteger.ONE).shiftRight(2), p);
+            if (!y.multiply(y).mod(p).equals(rhs))
+            {
+                continue;
+            }
+            ECPoint candidate = curve.createPoint(x, y);
+            if (BLS12_381SubgroupCheck.isInG1Subgroup(candidate))
+            {
+                continue;
+            }
+            assertFalse("off-subgroup G1 point must fail keyValidate",
+                BLS12_381BasicScheme.keyValidate(candidate));
+            return;
+        }
+        fail("could not construct an off-subgroup G1 point in 16 attempts");
+    }
+
+    // ---------------------------------------------------------------------
+    // Empty-message round-trip (review gap G8).
+    // Eth2 KAT vectors don't include a sign-and-verify with msg = [],
+    // and the spec-level scheme tests above all use non-empty messages.
+    // expand_message_xmd has explicit handling for empty inputs (the
+    // length-prefix turns empty msg into a non-empty hash input); this
+    // test pins the end-to-end behaviour.
+    // ---------------------------------------------------------------------
+
+    public void testSignVerifyEmptyMessageRoundTrip()
+    {
+        BigInteger sk = BLS12_381BasicScheme.keyGen(ikm32(9), new byte[0]);
+        ECPoint pk = BLS12_381BasicScheme.skToPk(sk);
+        byte[] msg = new byte[0];
+        BLS12_381G2Point sig = BLS12_381BasicScheme.sign(sk, msg);
+        assertTrue("signature over empty message must verify",
+            BLS12_381BasicScheme.verify(pk, msg, sig));
+        // And must NOT verify under a non-empty distinguishing input.
+        assertFalse("empty-msg signature must not verify under a different msg",
+            BLS12_381BasicScheme.verify(pk, new byte[]{1}, sig));
+    }
 }