JCE: fix DH KeyAgreement, pad DH secrets to primeLen with leading zeros and add test

Author: cconlon

src/main/java/com/wolfssl/provider/jce/WolfCryptKeyAgreement.java

@@ -235,6 +235,7 @@ protected int engineGenerateSecret(byte[] sharedSecret, int offset)
         throws IllegalStateException, ShortBufferException {
 
         byte tmp[] = null;
+        int returnLen = 0;
 
         if (this.state != EngineState.WC_PUBKEY_DONE)
             throw new IllegalStateException(
@@ -248,32 +249,56 @@ protected int engineGenerateSecret(byte[] sharedSecret, int offset)
         switch (this.type) {
             case WC_DH:
 
-                if ((sharedSecret.length - offset) < this.primeLen) {
-                    throw new ShortBufferException(
-                        "Input buffer too small when generating " +
-                        "shared secret");
-                }
-
                 /* public key has been stored inside this.dh already */
                 tmp = this.dh.makeSharedSecret();
                 if (tmp == null) {
                     throw new RuntimeException("Error when creating DH " +
                             "shared secret");
                 }
 
-                if ((sharedSecret.length - offset) < tmp.length) {
+                /* DH shared secrets can vary in length depending on if they
+                 * are padded or not at the beginning with zero bytes to make
+                 * a total output size matching the prime length.
+                 *
+                 * Native wolfCrypt does not prepend zero bytes to DH shared
+                 * secrets, following RFC 5246 (8.1.2) which instructs to
+                 * strip leading zero bytes.
+                 *
+                 * Sun KeyAgreement DH implementations as of after Java 8
+                 * prepend zero bytes if total length is not equal to prime
+                 * length. This was changed with OpenJDK bug fix JDK-7146728.
+                 *
+                 * BouncyCastle also behaves the same way, prepending zero
+                 * bytes if total secret size is not prime length. This
+                 * follows RFC 2631 (2.1.2).
+                 *
+                 * To match Sun and BC behavior, we pad the secret to primeLen
+                 * by prepending zeros for both generateSecret() methods.
+                 */
+                byte[] paddedSecret = new byte[this.primeLen];
+                Arrays.fill(paddedSecret, (byte)0);
+                System.arraycopy(tmp, 0, paddedSecret,
+                    paddedSecret.length - tmp.length, tmp.length);
+
+                if ((sharedSecret.length - offset) < paddedSecret.length) {
                     zeroArray(tmp);
+                    zeroArray(paddedSecret);
                     throw new ShortBufferException(
                         "Output buffer too small when generating " +
                         "DH shared secret");
                 }
 
-                /* copy array back to output offset */
-                System.arraycopy(tmp, 0, sharedSecret, offset, tmp.length);
+                /* copy padded array back to output offset */
+                System.arraycopy(paddedSecret, 0, sharedSecret, offset,
+                    paddedSecret.length);
+
+                returnLen = this.primeLen;
 
                 /* reset state, using same private info and alg params */
                 this.state = EngineState.WC_PRIVKEY_DONE;
 
+                zeroArray(paddedSecret);
+
                 break;
 
             case WC_ECDH:
@@ -294,6 +319,8 @@ protected int engineGenerateSecret(byte[] sharedSecret, int offset)
                 /* copy array back to output ofset */
                 System.arraycopy(tmp, 0, sharedSecret, offset, tmp.length);
 
+                returnLen = tmp.length;
+
                 /* reset state, using same private info and alg params */
                 byte[] priv = this.ecPrivate.exportPrivate();
                 if (priv == null) {
@@ -313,15 +340,11 @@ protected int engineGenerateSecret(byte[] sharedSecret, int offset)
                 break;
         };
 
-        if (tmp != null) {
-
-            log("generated secret, len: " + tmp.length);
+        log("generated secret, len: " + returnLen);
 
-            zeroArray(tmp);
-            return tmp.length;
-        }
+        zeroArray(tmp);
 
-        return 0;
+        return returnLen;
     }
 
     @Override

src/test/java/com/wolfssl/provider/jce/test/WolfCryptKeyAgreementTest.java

@@ -1011,6 +1011,83 @@ public void testECDHGenerateSecretKeyForDES()
         }
     }
 
+    @Test
+    public void testDHKeyAgreementPadding()
+        throws NoSuchProviderException, NoSuchAlgorithmException,
+               InvalidParameterSpecException, InvalidKeyException,
+               InvalidAlgorithmParameterException,
+               ShortBufferException {
+
+        /* This test verifies that DH shared secrets are properly padded to
+         * the prime length when using generateSecret(byte[], int). This
+         * matches the behavior of SunJCE after Java 8 (JDK-7146728) and
+         * prevents regressions related to padding. Both generateSecret()
+         * methods pad to primeLen. */
+
+        /* Skip in FIPS mode. FIPS 186-4 only allows 1024, 2048, and
+         * 3072-bit DH parameter generation */
+        if (Fips.enabled) {
+            return;
+        }
+
+        /* Generate 2048-bit DH parameters */
+        AlgorithmParameterGenerator paramGen =
+            AlgorithmParameterGenerator.getInstance("DH");
+        paramGen.init(2048);
+
+        AlgorithmParameters params = paramGen.generateParameters();
+        DHParameterSpec dhParams =
+            (DHParameterSpec)params.getParameterSpec(DHParameterSpec.class);
+
+        /* Prime length should be 256 bytes for 2048-bit DH */
+        int primeLen = dhParams.getP().toByteArray().length;
+        if (dhParams.getP().toByteArray()[0] == 0x00) {
+            primeLen--;
+        }
+
+        /* Initialize key pair generator */
+        KeyPairGenerator keyGen =
+            KeyPairGenerator.getInstance("DH", "wolfJCE");
+        keyGen.initialize(dhParams, secureRandom);
+
+        KeyAgreement alice = KeyAgreement.getInstance("DH", "wolfJCE");
+        KeyAgreement bob = KeyAgreement.getInstance("DH", "wolfJCE");
+
+        /* Run multiple iterations to ensure consistent padding behavior */
+        for (int i = 0; i < 100; i++) {
+            byte[] aliceSecret = new byte[primeLen];
+            byte[] bobSecret = new byte[primeLen];
+
+            /* Fill buffers with different stale data to ensure padding
+             * overwrites any existing data */
+            Arrays.fill(aliceSecret, (byte)'a');
+            Arrays.fill(bobSecret, (byte)'b');
+
+            /* Generate new key pairs for this iteration */
+            KeyPair aliceKeyPair = keyGen.generateKeyPair();
+            KeyPair bobKeyPair = keyGen.generateKeyPair();
+
+            /* Perform key agreement */
+            alice.init(aliceKeyPair.getPrivate());
+            alice.doPhase(bobKeyPair.getPublic(), true);
+            int aliceLen = alice.generateSecret(aliceSecret, 0);
+
+            bob.init(bobKeyPair.getPrivate());
+            bob.doPhase(aliceKeyPair.getPublic(), true);
+            int bobLen = bob.generateSecret(bobSecret, 0);
+
+            /* Both secrets should be exactly primeLen bytes (always padded) */
+            assertEquals("Alice's secret length should equal prime length",
+                primeLen, aliceLen);
+            assertEquals("Bob's secret length should equal prime length",
+                primeLen, bobLen);
+
+            /* Both secrets should be identical, including padding */
+            assertArrayEquals("Alice and Bob should generate identical " +
+                "secrets at iteration " + i, aliceSecret, bobSecret);
+        }
+    }
+
     @Test
     public void testThreadedKeyAgreement()
         throws InterruptedException, NoSuchAlgorithmException {