8312306: Add more Reference.reachabilityFence() calls to the security classes using Cleaner

Reviewed-by: ascarpino

src/java.base/share/classes/com/sun/crypto/provider/DESKey.java

@@ -89,12 +89,12 @@ final class DESKey implements SecretKey {
     public byte[] getEncoded() {
         // Return a copy of the key, rather than a reference,
         // so that the key data cannot be modified from outside
-
+        try {
-        // The key is zeroized by finalize()
+            return key.clone();
-        // The reachability fence ensures finalize() isn't called early
+        } finally {
-        byte[] result = key.clone();
+            // prevent this from being cleaned for the above block
-        Reference.reachabilityFence(this);
+            Reference.reachabilityFence(this);
-        return result;
+        }
     }
 
     public String getAlgorithm() {
@@ -111,25 +111,35 @@ final class DESKey implements SecretKey {
      */
     @Override
     public int hashCode() {
-        return Arrays.hashCode(this.key) ^ "des".hashCode();
+        try {
+            return Arrays.hashCode(this.key) ^ "des".hashCode();
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 
     @Override
     public boolean equals(Object obj) {
-        if (this == obj)
+        try {
-            return true;
+            if (this == obj)
+                return true;
 
-        if (!(obj instanceof SecretKey that))
+            if (!(obj instanceof SecretKey that))
-            return false;
+                return false;
 
-        String thatAlg = that.getAlgorithm();
+            String thatAlg = that.getAlgorithm();
-        if (!(thatAlg.equalsIgnoreCase("DES")))
+            if (!(thatAlg.equalsIgnoreCase("DES")))
-            return false;
+                return false;
 
-        byte[] thatKey = that.getEncoded();
+            byte[] thatKey = that.getEncoded();
-        boolean ret = MessageDigest.isEqual(this.key, thatKey);
+            boolean ret = MessageDigest.isEqual(this.key, thatKey);
-        java.util.Arrays.fill(thatKey, (byte)0x00);
+            java.util.Arrays.fill(thatKey, (byte)0x00);
-        return ret;
+            return ret;
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 
     /**
@@ -141,7 +151,13 @@ final class DESKey implements SecretKey {
          throws java.io.IOException, ClassNotFoundException
     {
         s.defaultReadObject();
-        key = key.clone();
+        byte[] temp = key;
+        key = temp.clone();
+        Arrays.fill(temp, (byte)0x00);
+        // Use the cleaner to zero the key when no longer referenced
+        final byte[] k = this.key;
+        CleanerFactory.cleaner().register(this,
+                () -> java.util.Arrays.fill(k, (byte)0x00));
     }
 
     /**
@@ -154,9 +170,14 @@ final class DESKey implements SecretKey {
      */
     @java.io.Serial
     private Object writeReplace() throws java.io.ObjectStreamException {
-        return new KeyRep(KeyRep.Type.SECRET,
+        try {
+            return new KeyRep(KeyRep.Type.SECRET,
                         getAlgorithm(),
                         getFormat(),
                         key);
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 }

src/java.base/share/classes/com/sun/crypto/provider/DESedeKey.java

@@ -89,11 +89,12 @@ final class DESedeKey implements SecretKey {
     }
 
     public byte[] getEncoded() {
-        // The key is zeroized by finalize()
+        try {
-        // The reachability fence ensures finalize() isn't called early
+            return key.clone();
-        byte[] result = key.clone();
+        } finally {
-        Reference.reachabilityFence(this);
+            // prevent this from being cleaned for the above block
-        return result;
+            Reference.reachabilityFence(this);
+        }
     }
 
     public String getAlgorithm() {
@@ -110,26 +111,36 @@ final class DESedeKey implements SecretKey {
      */
     @Override
     public int hashCode() {
-        return Arrays.hashCode(this.key) ^ "desede".hashCode();
+        try {
+            return Arrays.hashCode(this.key) ^ "desede".hashCode();
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 
     @Override
     public boolean equals(Object obj) {
-        if (this == obj)
+        try {
-            return true;
+            if (this == obj)
+                return true;
 
-        if (!(obj instanceof SecretKey that))
+            if (!(obj instanceof SecretKey that))
-            return false;
+                return false;
 
-        String thatAlg = that.getAlgorithm();
+            String thatAlg = that.getAlgorithm();
-        if (!(thatAlg.equalsIgnoreCase("DESede"))
+            if (!(thatAlg.equalsIgnoreCase("DESede"))
-            && !(thatAlg.equalsIgnoreCase("TripleDES")))
+                && !(thatAlg.equalsIgnoreCase("TripleDES")))
-            return false;
+                return false;
 
-        byte[] thatKey = that.getEncoded();
+            byte[] thatKey = that.getEncoded();
-        boolean ret = MessageDigest.isEqual(this.key, thatKey);
+            boolean ret = MessageDigest.isEqual(this.key, thatKey);
-        java.util.Arrays.fill(thatKey, (byte)0x00);
+            java.util.Arrays.fill(thatKey, (byte)0x00);
-        return ret;
+            return ret;
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 
     /**
@@ -141,7 +152,13 @@ final class DESedeKey implements SecretKey {
          throws java.io.IOException, ClassNotFoundException
     {
         s.defaultReadObject();
-        key = key.clone();
+        byte[] temp = key;
+        this.key = temp.clone();
+        java.util.Arrays.fill(temp, (byte)0x00);
+        // Use the cleaner to zero the key when no longer referenced
+        final byte[] k = this.key;
+        CleanerFactory.cleaner().register(this,
+                () -> java.util.Arrays.fill(k, (byte)0x00));
     }
 
     /**
@@ -154,9 +171,14 @@ final class DESedeKey implements SecretKey {
      */
     @java.io.Serial
     private Object writeReplace() throws java.io.ObjectStreamException {
-        return new KeyRep(KeyRep.Type.SECRET,
+        try {
-                getAlgorithm(),
+            return new KeyRep(KeyRep.Type.SECRET,
-                getFormat(),
+                    getAlgorithm(),
-                key);
+                    getFormat(),
+                    key);
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 }

src/java.base/share/classes/com/sun/crypto/provider/KeyProtector.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023, 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
@@ -129,7 +129,7 @@ final class KeyProtector {
         SecretKey sKey = null;
         PBEWithMD5AndTripleDESCipher cipher;
         try {
-            sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES", false);
+            sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
             // encrypt private key
             cipher = new PBEWithMD5AndTripleDESCipher();
             cipher.engineInit(Cipher.ENCRYPT_MODE, sKey, pbeSpec, null);
@@ -193,7 +193,7 @@ final class KeyProtector {
 
                 // create PBE key from password
                 PBEKeySpec pbeKeySpec = new PBEKeySpec(this.password);
-                sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES", false);
+                sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
                 pbeKeySpec.clearPassword();
 
                 // decrypt private key
@@ -339,7 +339,7 @@ final class KeyProtector {
         SecretKey sKey = null;
         Cipher cipher;
         try {
-            sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES", false);
+            sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
             pbeKeySpec.clearPassword();
 
             // seal key
@@ -366,8 +366,7 @@ final class KeyProtector {
         try {
             // create PBE key from password
             PBEKeySpec pbeKeySpec = new PBEKeySpec(this.password);
-            sKey = new PBEKey(pbeKeySpec,
+            sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
-                    "PBEWithMD5AndTripleDES", false);
             pbeKeySpec.clearPassword();
 
             SealedObjectForKeyProtector soForKeyProtector = null;

src/java.base/share/classes/com/sun/crypto/provider/PBEKey.java

@@ -26,6 +26,7 @@
 package com.sun.crypto.provider;
 
 import java.lang.ref.Reference;
+import java.lang.ref.Cleaner.Cleanable;
 import java.security.MessageDigest;
 import java.security.KeyRep;
 import java.security.spec.InvalidKeySpecException;
@@ -51,13 +52,14 @@ final class PBEKey implements SecretKey {
 
     private String type;
 
+    private transient Cleanable cleanable;
+
     /**
      * Creates a PBE key from a given PBE key specification.
      *
      * @param keytype the given PBE key specification
      */
-    PBEKey(PBEKeySpec keySpec, String keytype, boolean useCleaner)
+    PBEKey(PBEKeySpec keySpec, String keytype) throws InvalidKeySpecException {
-            throws InvalidKeySpecException {
         char[] passwd = keySpec.getPassword();
         if (passwd == null) {
             // Should allow an empty password.
@@ -78,19 +80,18 @@ final class PBEKey implements SecretKey {
         type = keytype;
 
         // Use the cleaner to zero the key when no longer referenced
-        if (useCleaner) {
+        final byte[] k = this.key;
-            final byte[] k = this.key;
+        cleanable = CleanerFactory.cleaner().register(this,
-            CleanerFactory.cleaner().register(this,
+                () -> java.util.Arrays.fill(k, (byte)0x00));
-                () -> Arrays.fill(k, (byte) 0x00));
-        }
     }
 
     public byte[] getEncoded() {
-        // The key is zeroized by finalize()
+        try {
-        // The reachability fence ensures finalize() isn't called early
+            return key.clone();
-        byte[] result = key.clone();
+        } finally {
-        Reference.reachabilityFence(this);
+            // prevent this from being cleaned for the above block
-        return result;
+            Reference.reachabilityFence(this);
+        }
     }
 
     public String getAlgorithm() {
@@ -107,25 +108,40 @@ final class PBEKey implements SecretKey {
      */
     @Override
     public int hashCode() {
-        return Arrays.hashCode(this.key)
+        try {
-                ^ getAlgorithm().toLowerCase(Locale.ENGLISH).hashCode();
+            return Arrays.hashCode(this.key)
+                    ^ getAlgorithm().toLowerCase(Locale.ENGLISH).hashCode();
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 
     @Override
     public boolean equals(Object obj) {
-        if (obj == this)
+        try {
-            return true;
+            if (obj == this)
+                return true;
 
-        if (!(obj instanceof SecretKey that))
+            if (!(obj instanceof SecretKey that))
-            return false;
+                return false;
 
-        if (!(that.getAlgorithm().equalsIgnoreCase(type)))
+            // destroyed keys are considered different
-            return false;
+            if (isDestroyed() || that.isDestroyed()) {
+                return false;
+            }
 
-        byte[] thatEncoded = that.getEncoded();
+            if (!(that.getAlgorithm().equalsIgnoreCase(type)))
-        boolean ret = MessageDigest.isEqual(this.key, thatEncoded);
+                return false;
-        Arrays.fill(thatEncoded, (byte)0x00);
+
-        return ret;
+            byte[] thatEncoded = that.getEncoded();
+            boolean ret = MessageDigest.isEqual(this.key, thatEncoded);
+            Arrays.fill(thatEncoded, (byte)0x00);
+            return ret;
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 
     /**
@@ -134,12 +150,17 @@ final class PBEKey implements SecretKey {
      */
     @Override
     public void destroy() {
-        if (key != null) {
+        if (cleanable != null) {
-            Arrays.fill(key, (byte) 0x00);
+            cleanable.clean();
-            key = null;
+            cleanable = null;
         }
     }
 
+    @Override
+    public boolean isDestroyed() {
+        return (cleanable == null);
+    }
+
     /**
      * readObject is called to restore the state of this key from
      * a stream.
@@ -149,7 +170,13 @@ final class PBEKey implements SecretKey {
          throws java.io.IOException, ClassNotFoundException
     {
         s.defaultReadObject();
-        key = key.clone();
+        byte[] temp = key;
+        key = temp.clone();
+        Arrays.fill(temp, (byte)0x00);
+        // Use cleaner to zero the key when no longer referenced
+        final byte[] k = this.key;
+        cleanable = CleanerFactory.cleaner().register(this,
+                () -> java.util.Arrays.fill(k, (byte)0x00));
     }
 
 
@@ -163,9 +190,14 @@ final class PBEKey implements SecretKey {
      */
     @java.io.Serial
     private Object writeReplace() throws java.io.ObjectStreamException {
-        return new KeyRep(KeyRep.Type.SECRET,
+        try {
-                getAlgorithm(),
+            return new KeyRep(KeyRep.Type.SECRET,
-                getFormat(),
+                    getAlgorithm(),
-                key);
+                    getFormat(),
+                    key);
+        } finally {
+            // prevent this from being cleaned for the above block
+            Reference.reachabilityFence(this);
+        }
     }
 }

src/java.base/share/classes/com/sun/crypto/provider/PBEKeyFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2023, 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
@@ -239,7 +239,7 @@ abstract class PBEKeyFactory extends SecretKeyFactorySpi {
         if (!(keySpec instanceof PBEKeySpec)) {
             throw new InvalidKeySpecException("Invalid key spec");
         }
-        return new PBEKey((PBEKeySpec)keySpec, type, true);
+        return new PBEKey((PBEKeySpec)keySpec, type);
     }
 
     /**

test/jdk/com/sun/crypto/provider/KeyFactory/PBEKeyDestroyTest.java

@@ -0,0 +1,71 @@
+/*
+ * Copyright (c) 2023, 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 8312306
+ * @summary Check the destroy()/isDestroyed() of the PBEKey impl from SunJCE
+ * @library /test/lib
+ * @run testng/othervm PBEKeyDestroyTest
+ */
+import javax.crypto.*;
+import javax.crypto.spec.*;
+import java.nio.charset.StandardCharsets;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class PBEKeyDestroyTest {
+
+    @Test
+    public void test() throws Exception {
+        PBEKeySpec keySpec = new PBEKeySpec("12345678".toCharArray(),
+                "abcdefgh".getBytes(StandardCharsets.UTF_8), 100000, 128 >> 3);
+
+        SecretKeyFactory skf = SecretKeyFactory.getInstance
+                ("PBEWithHmacSHA1AndAES_128", "SunJCE");
+
+        SecretKey key1 = skf.generateSecret(keySpec);
+        SecretKey key2 = skf.generateSecret(keySpec);
+
+        // should be equal
+        Assert.assertFalse(key1.isDestroyed());
+        Assert.assertFalse(key2.isDestroyed());
+        Assert.assertTrue(key1.equals(key2));
+        Assert.assertTrue(key2.equals(key1));
+
+        // destroy key1
+        key1.destroy();
+        Assert.assertTrue(key1.isDestroyed());
+        Assert.assertFalse(key1.equals(key2));
+        Assert.assertFalse(key2.equals(key1));
+
+        // also destroy key2
+        key2.destroy();
+        Assert.assertTrue(key2.isDestroyed());
+        Assert.assertFalse(key1.equals(key2));
+        Assert.assertFalse(key2.equals(key1));
+
+        // call destroy again to make sure no unexpected exceptions
+        key2.destroy();
+    }
+}