8368694: PKCS11-NSS generic keys generated by DH have leading zeroes stripped

Reviewed-by: valeriep

Author: djelinski

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyAgreement.java

@@ -200,6 +200,7 @@ protected byte[] engineGenerateSecret() throws IllegalStateException {
             CK_ATTRIBUTE[] attributes = new CK_ATTRIBUTE[] {
                 new CK_ATTRIBUTE(CKA_CLASS, CKO_SECRET_KEY),
                 new CK_ATTRIBUTE(CKA_KEY_TYPE, CKK_GENERIC_SECRET),
+                new CK_ATTRIBUTE(CKA_VALUE_LEN, secretLen),
             };
             attributes = token.getAttributes
                 (O_GENERATE, CKO_SECRET_KEY, CKK_GENERIC_SECRET, attributes);
@@ -213,22 +214,11 @@ protected byte[] engineGenerateSecret() throws IllegalStateException {
             token.p11.C_GetAttributeValue(session.id(), keyID, attributes);
             byte[] secret = attributes[0].getByteArray();
             token.p11.C_DestroyObject(session.id(), keyID);
-            // Some vendors, e.g. NSS, trim off the leading 0x00 byte(s) from
-            // the generated secret. Thus, we need to check the secret length
-            // and trim/pad it so the returned value has the same length as
-            // the modulus size
-            if (secret.length == secretLen) {
-                return secret;
-            } else {
-                if (secret.length > secretLen) {
-                    // Shouldn't happen; but check just in case
-                    throw new ProviderException("generated secret is out-of-range");
-                }
-                byte[] newSecret = new byte[secretLen];
-                System.arraycopy(secret, 0, newSecret, secretLen - secret.length,
-                    secret.length);
-                return newSecret;
+            if (secret.length != secretLen) {
+                // Shouldn't happen; but check just in case
+                throw new ProviderException("generated secret is out-of-range");
             }
+            return secret;
         } catch (PKCS11Exception e) {
             throw new ProviderException("Could not derive key", e);
         } finally {
@@ -321,10 +311,20 @@ private SecretKey nativeGenerateSecret(String algorithm)
         long privKeyID = privateKey.getKeyID();
         try {
             session = token.getObjSession();
-            CK_ATTRIBUTE[] attributes = new CK_ATTRIBUTE[] {
-                new CK_ATTRIBUTE(CKA_CLASS, CKO_SECRET_KEY),
-                new CK_ATTRIBUTE(CKA_KEY_TYPE, keyType),
-            };
+            CK_ATTRIBUTE[] attributes;
+            if ("TlsPremasterSecret".equalsIgnoreCase(algorithm)) {
+                attributes = new CK_ATTRIBUTE[]{
+                        new CK_ATTRIBUTE(CKA_CLASS, CKO_SECRET_KEY),
+                        new CK_ATTRIBUTE(CKA_KEY_TYPE, keyType),
+                };
+            } else {
+                // keep the leading zeroes
+                attributes = new CK_ATTRIBUTE[]{
+                        new CK_ATTRIBUTE(CKA_CLASS, CKO_SECRET_KEY),
+                        new CK_ATTRIBUTE(CKA_KEY_TYPE, keyType),
+                        new CK_ATTRIBUTE(CKA_VALUE_LEN, secretLen),
+                };
+            }
             attributes = token.getAttributes
                 (O_GENERATE, CKO_SECRET_KEY, keyType, attributes);
             long keyID = token.p11.C_DeriveKey(session.id(),
@@ -337,19 +337,6 @@ private SecretKey nativeGenerateSecret(String algorithm)
             int keyLen = (int)lenAttributes[0].getLong();
             SecretKey key = P11Key.secretKey
                         (session, keyID, algorithm, keyLen << 3, attributes);
-            if ("RAW".equals(key.getFormat())
-                    && algorithm.equalsIgnoreCase("TlsPremasterSecret")) {
-                // Workaround for Solaris bug 6318543.
-                // Strip leading zeroes ourselves if possible (key not sensitive).
-                // This should be removed once the Solaris fix is available
-                // as here we always retrieve the CKA_VALUE even for tokens
-                // that do not have that bug.
-                byte[] keyBytes = key.getEncoded();
-                byte[] newBytes = KeyUtil.trimZeroes(keyBytes);
-                if (keyBytes != newBytes) {
-                    key = new SecretKeySpec(newBytes, algorithm);
-                }
-            }
             return key;
         } catch (PKCS11Exception e) {
             throw new InvalidKeyException("Could not derive key", e);

test/jdk/com/sun/crypto/provider/TLS/TestLeadingZeroes.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 8014618
+ * @bug 8014618 8368694
  * @summary Need to strip leading zeros in TlsPremasterSecret of DHKeyAgreement
  * @author Pasi Eronen
  */
@@ -88,6 +88,26 @@ private void run() throws Exception {
             throw new Exception("First byte is not zero as expected");
         }
 
+        // generate generic shared secret
+        aliceKeyAgree.init(alicePrivKey);
+        aliceKeyAgree.doPhase(bobPubKey, true);
+        byte[] genericSecret =
+            aliceKeyAgree.generateSecret("Generic").getEncoded();
+        System.out.println("generic secret:\n" + HEX_FORMATTER.formatHex(genericSecret));
+
+        // verify that leading zero is present
+        if (genericSecret.length != 256) {
+            throw new Exception("Unexpected generic secret length");
+        }
+        if (genericSecret[0] != 0) {
+            throw new Exception("First byte is not zero as expected");
+        }
+        for (int i = 0; i < genericSecret.length; i++) {
+            if (genericSecret[i] != sharedSecret[i]) {
+                throw new Exception("Shared secrets differ");
+            }
+        }
+
         // now, test TLS premaster secret
         aliceKeyAgree.init(alicePrivKey);
         aliceKeyAgree.doPhase(bobPubKey, true);

test/jdk/sun/security/pkcs11/tls/TestLeadingZeroesP11.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 8014618
+ * @bug 8014618 8368694
  * @summary Need to strip leading zeros in TlsPremasterSecret of DHKeyAgreement
  * @library /test/lib ..
  * @author Pasi Eronen
@@ -87,6 +87,26 @@ public void main(Provider p) throws Exception {
             throw new Exception("First byte is not zero as expected");
         }
 
+        // generate generic shared secret
+        aliceKeyAgree.init(alicePrivKey);
+        aliceKeyAgree.doPhase(bobPubKey, true);
+        byte[] genericSecret =
+                aliceKeyAgree.generateSecret("Generic").getEncoded();
+        System.out.println("generic secret:\n" + HEX.formatHex(genericSecret));
+
+        // verify that leading zero is present
+        if (genericSecret.length != 128) {
+            throw new Exception("Unexpected generic secret length");
+        }
+        if (genericSecret[0] != 0) {
+            throw new Exception("First byte is not zero as expected");
+        }
+        for (int i = 0; i < genericSecret.length; i++) {
+            if (genericSecret[i] != sharedSecret[i]) {
+                throw new Exception("Shared secrets differ");
+            }
+        }
+
         // now, test TLS premaster secret
         aliceKeyAgree.init(alicePrivKey);
         aliceKeyAgree.doPhase(bobPubKey, true);