From 7dd816e35628774376b7ad3e68411d4e1e3f62b6 Mon Sep 17 00:00:00 2001 From: Kevin Driver Date: Tue, 1 Jul 2025 11:44:20 -0500 Subject: [PATCH 1/3] JDK-8303613: fixing this bug, but hopefully also setting a precedent for a larger refactor --- .../security/ec/ed/EdDSAPrivateKeyImpl.java | 55 +++++++- .../sun/security/pkcs/NamedPKCS8Key.java | 2 +- .../classes/sun/security/pkcs/PKCS8Key.java | 121 ++++++++++++++---- .../EdDSAPrivateKeyDestroyTest.java | 85 ++++++++++++ 4 files changed, 230 insertions(+), 33 deletions(-) create mode 100644 test/jdk/com/sun/crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java diff --git a/src/java.base/share/classes/sun/security/ec/ed/EdDSAPrivateKeyImpl.java b/src/java.base/share/classes/sun/security/ec/ed/EdDSAPrivateKeyImpl.java index 10b19c1ce534a..4928962528252 100644 --- a/src/java.base/share/classes/sun/security/ec/ed/EdDSAPrivateKeyImpl.java +++ b/src/java.base/share/classes/sun/security/ec/ed/EdDSAPrivateKeyImpl.java @@ -25,17 +25,22 @@ package sun.security.ec.ed; +import jdk.internal.ref.CleanerFactory; +import sun.security.pkcs.PKCS8Key; +import sun.security.util.DerInputStream; +import sun.security.util.DerValue; +import sun.security.x509.AlgorithmId; + import java.io.IOException; import java.io.InvalidObjectException; import java.io.ObjectInputStream; +import java.lang.ref.Cleaner.Cleanable; +import java.lang.ref.Reference; import java.security.InvalidKeyException; import java.security.interfaces.EdECPrivateKey; -import java.util.Optional; import java.security.spec.NamedParameterSpec; - -import sun.security.pkcs.PKCS8Key; -import sun.security.x509.AlgorithmId; -import sun.security.util.*; +import java.util.Arrays; +import java.util.Optional; public final class EdDSAPrivateKeyImpl extends PKCS8Key implements EdECPrivateKey { @@ -47,6 +52,8 @@ public final class EdDSAPrivateKeyImpl private final NamedParameterSpec paramSpec; private byte[] h; + private transient Cleanable cleanable; + EdDSAPrivateKeyImpl(EdDSAParameters params, byte[] h) throws InvalidKeyException { @@ -61,6 +68,11 @@ public final class EdDSAPrivateKeyImpl val.clear(); } checkLength(params); + + final byte[] lh = this.h; + cleanable = CleanerFactory.cleaner().register(this, + () -> Arrays.fill(lh, + (byte) 0x00)); } EdDSAPrivateKeyImpl(byte[] encoded) throws InvalidKeyException { @@ -77,6 +89,29 @@ public final class EdDSAPrivateKeyImpl throw new InvalidKeyException(ex); } checkLength(params); + + final byte[] lh = this.h; + cleanable = CleanerFactory.cleaner().register(this, + () -> Arrays.fill(lh, + (byte) 0x00)); + } + + /** + * Clears the internal copy of the key. + * + */ + @Override + public void destroy() { + super.destroy(); + if (cleanable != null) { + cleanable.clean(); + cleanable = null; + } + } + + @Override + public boolean isDestroyed() { + return (cleanable == null); } void checkLength(EdDSAParameters params) throws InvalidKeyException { @@ -88,7 +123,15 @@ void checkLength(EdDSAParameters params) throws InvalidKeyException { } public byte[] getKey() { - return h.clone(); + try { + if (isDestroyed()) { + throw new IllegalStateException("Key is destroyed"); + } + return h.clone(); + } finally { + // prevent this from being cleaned for the above block + Reference.reachabilityFence(this); + } } @Override diff --git a/src/java.base/share/classes/sun/security/pkcs/NamedPKCS8Key.java b/src/java.base/share/classes/sun/security/pkcs/NamedPKCS8Key.java index a748433da875c..67d939755f610 100644 --- a/src/java.base/share/classes/sun/security/pkcs/NamedPKCS8Key.java +++ b/src/java.base/share/classes/sun/security/pkcs/NamedPKCS8Key.java @@ -127,7 +127,7 @@ private void readObject(ObjectInputStream stream) } @Override - public void destroy() throws DestroyFailedException { + public void destroy() { Arrays.fill(rawBytes, (byte)0); Arrays.fill(privKeyMaterial, (byte)0); if (encodedKey != null) { diff --git a/src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java b/src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java index b7cc5e7057f53..bd4ece2a28403 100644 --- a/src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java +++ b/src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java @@ -26,13 +26,26 @@ package sun.security.pkcs; import jdk.internal.access.SharedSecrets; -import sun.security.util.*; +import jdk.internal.ref.CleanerFactory; +import sun.security.util.DerOutputStream; +import sun.security.util.DerValue; +import sun.security.util.InternalPrivateKey; import sun.security.x509.AlgorithmId; import sun.security.x509.X509Key; +import javax.security.auth.Destroyable; import java.io.IOException; import java.io.ObjectInputStream; -import java.security.*; +import java.lang.ref.Cleaner.Cleanable; +import java.lang.ref.Reference; +import java.security.InvalidKeyException; +import java.security.Key; +import java.security.KeyFactory; +import java.security.KeyRep; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.security.PrivateKey; +import java.security.Provider; import java.security.spec.InvalidKeySpecException; import java.security.spec.PKCS8EncodedKeySpec; import java.util.Arrays; @@ -82,6 +95,8 @@ public class PKCS8Key implements PrivateKey, InternalPrivateKey { public static final int V1 = 0; public static final int V2 = 1; + private transient Cleanable cleanable; + /** * Default constructor. Constructors in subclasses that create a new key * from its components require this. These constructors must initialize @@ -96,12 +111,22 @@ protected PKCS8Key() { } * * This method is also used by {@link #parseKey} to create a raw key. */ + @SuppressWarnings("this-escape") public PKCS8Key(byte[] input) throws InvalidKeyException { try { decode(new DerValue(input)); } catch (IOException e) { throw new InvalidKeyException("Unable to decode key", e); } + final byte[] eK = this.encodedKey; + final byte[] pK = this.privKeyMaterial; + // this-escape is suppressed because the cleaner needs to hold a reference to the object + cleanable = CleanerFactory.cleaner().register(this, + () -> { + if(eK != null) + Arrays.fill(eK, (byte) 0x00); + Arrays.fill(pK, (byte) 0x00); + }); } private PKCS8Key(byte[] privEncoding, byte[] pubEncoding) @@ -111,6 +136,23 @@ private PKCS8Key(byte[] privEncoding, byte[] pubEncoding) version = V2; } + /** + * Clears the internal copy of the key. + * + */ + @Override + public void destroy() { + if (cleanable != null) { + cleanable.clean(); + cleanable = null; + } + } + + @Override + public boolean isDestroyed() { + return (cleanable == null); + } + public int getVersion() { return version; } @@ -256,8 +298,16 @@ public AlgorithmId getAlgorithmId () { * or {@code null} if an encoding error occurs. */ public byte[] getEncoded() { - byte[] b = getEncodedInternal(); - return (b != null) ? b.clone() : null; + try { + if (isDestroyed()) { + throw new IllegalStateException("key is destroyed"); + } + byte[] b = getEncodedInternal(); + return (b != null) ? b.clone() : null; + } finally { + // prevent this from being cleaned for the above block + Reference.reachabilityFence(this); + } } /** @@ -373,28 +423,45 @@ private void readObject(ObjectInputStream stream) throws IOException { */ @Override public boolean equals(Object object) { - if (this == object) { - return true; - } - if (object instanceof PKCS8Key) { - // time-constant comparison - return MessageDigest.isEqual( - getEncodedInternal(), - ((PKCS8Key)object).getEncodedInternal()); - } else if (object instanceof Key) { - // time-constant comparison - byte[] otherEncoded = ((Key)object).getEncoded(); - try { + try { + if (this == object) { + return true; + } + if (object instanceof PKCS8Key pkcs8Key) { + // destroyed keys are considered different + if (isDestroyed() || pkcs8Key.isDestroyed()) { + return false; + } + // time-constant comparison return MessageDigest.isEqual( getEncodedInternal(), - otherEncoded); - } finally { - if (otherEncoded != null) { - Arrays.fill(otherEncoded, (byte) 0); + pkcs8Key.getEncodedInternal()); + } else if (object instanceof Key keyHandle) { + + if (keyHandle instanceof Destroyable destKeyHandle) { + // destroyed keys are considered different + if (isDestroyed() || destKeyHandle.isDestroyed()) { + return false; + } + } + + // time-constant comparison + byte[] otherEncoded = ((Key) object).getEncoded(); + try { + return MessageDigest.isEqual( + getEncodedInternal(), + otherEncoded); + } finally { + if (otherEncoded != null) { + Arrays.fill(otherEncoded, (byte) 0); + } } } + return false; + } finally { + // prevent this from being cleaned for the above block + Reference.reachabilityFence(this); } - return false; } /** @@ -403,13 +470,15 @@ public boolean equals(Object object) { */ @Override public int hashCode() { - return Arrays.hashCode(getEncodedInternal()); + try { + return Arrays.hashCode(getEncodedInternal()); + } finally { + // prevent this from being cleaned for the above block + Reference.reachabilityFence(this); + } } public void clear() { - if (encodedKey != null) { - Arrays.fill(encodedKey, (byte)0); - } - Arrays.fill(privKeyMaterial, (byte)0); + destroy(); } } diff --git a/test/jdk/com/sun/crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java b/test/jdk/com/sun/crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java new file mode 100644 index 0000000000000..5d7e9beeb7ddc --- /dev/null +++ b/test/jdk/com/sun/crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java @@ -0,0 +1,85 @@ +/* + * Copyright (c) 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 + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8303613 + * @summary Check the destroy()/isDestroyed() of the EdDSA impl from SunEC + * @library /test/lib + * @run testng/othervm EdDSAPrivateKeyDestroyTest + */ + +import org.testng.Assert; +import org.testng.annotations.Test; + +import java.security.KeyFactory; +import java.security.KeyPair; +import java.security.KeyPairGenerator; +import java.security.PrivateKey; +import java.security.spec.KeySpec; +import java.security.spec.PKCS8EncodedKeySpec; + +public class EdDSAPrivateKeyDestroyTest { + @Test + public void test() throws Exception { + KeyPairGenerator edDSAGen = + KeyPairGenerator.getInstance("EdDSA", "SunEC"); + + KeyPair kp = edDSAGen.generateKeyPair(); + + PrivateKey priv1 = kp.getPrivate(); + + KeySpec priv2Spec = new PKCS8EncodedKeySpec(priv1.getEncoded()); + KeyFactory edDSAFac2 = KeyFactory.getInstance("EdDSA", "SunEC"); + PrivateKey priv2 = edDSAFac2.generatePrivate(priv2Spec); + + + // should be equal + Assert.assertFalse(priv1.isDestroyed()); + Assert.assertFalse(priv2.isDestroyed()); + Assert.assertEquals(priv2, priv1); + Assert.assertEquals(priv1, priv2); + + System.out.println("Past sanity checks"); + + // destroy key1 + priv1.destroy(); + Assert.assertTrue(priv1.isDestroyed()); + Assert.assertNotEquals(priv2, priv1); + Assert.assertNotEquals(priv1, priv2); + + System.out.println("Past destroy priv1"); + + // also destroy key2 + priv2.destroy(); + Assert.assertTrue(priv2.isDestroyed()); + Assert.assertNotEquals(priv2, priv1); + Assert.assertNotEquals(priv1, priv2); + + System.out.println("Past destroy priv2"); + + // call destroy again to make sure no unexpected exceptions + priv2.destroy(); + + } +} From e77a41e68de251a454edcb0f9a5fd830921b7b66 Mon Sep 17 00:00:00 2001 From: Kevin Driver Date: Tue, 1 Jul 2025 12:13:59 -0500 Subject: [PATCH 2/3] add hashCode to test --- .../crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/jdk/com/sun/crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java b/test/jdk/com/sun/crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java index 5d7e9beeb7ddc..c09da0745ce13 100644 --- a/test/jdk/com/sun/crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java +++ b/test/jdk/com/sun/crypto/provider/KeyFactory/EdDSAPrivateKeyDestroyTest.java @@ -59,6 +59,7 @@ public void test() throws Exception { Assert.assertFalse(priv2.isDestroyed()); Assert.assertEquals(priv2, priv1); Assert.assertEquals(priv1, priv2); + Assert.assertEquals(priv2.hashCode(), priv1.hashCode()); System.out.println("Past sanity checks"); From 55426464fe1847e774d732c25afde6f439b29a77 Mon Sep 17 00:00:00 2001 From: Kevin Driver Date: Tue, 1 Jul 2025 15:42:07 -0500 Subject: [PATCH 3/3] capitalize for consistency with other classes adding similar changes --- src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java b/src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java index bd4ece2a28403..c8db99710c1b0 100644 --- a/src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java +++ b/src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java @@ -300,7 +300,7 @@ public AlgorithmId getAlgorithmId () { public byte[] getEncoded() { try { if (isDestroyed()) { - throw new IllegalStateException("key is destroyed"); + throw new IllegalStateException("Key is destroyed"); } byte[] b = getEncodedInternal(); return (b != null) ? b.clone() : null;