8246077: Cloneable test in HmacCore seems questionable

Updated java.security.MessageDigest and Signature classes to return Cloneable obj for Cloneable spi obj

Reviewed-by: weijun, xuelei, coffeys

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

@@ -75,6 +75,7 @@ abstract class HmacCore extends MacSpi implements Cloneable {
             } else {
                 String noCloneProv = md.getProvider().getName();
                 // if no Sun provider, use provider list
+                md = null;
                 Provider[] provs = Security.getProviders();
                 for (Provider p : provs) {
                     try {
@@ -90,6 +91,10 @@ abstract class HmacCore extends MacSpi implements Cloneable {
                         continue;
                     }
                 }
+                if (md == null) {
+                    throw new NoSuchAlgorithmException
+                            ("No Cloneable digest found for " + digestAlgo);
+                }
             }
         }
         this.md = md;

src/java.base/share/classes/java/security/MessageDigest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2020, 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
@@ -131,6 +131,12 @@ public abstract class MessageDigest extends MessageDigestSpi {
         this.algorithm = algorithm;
     }
 
+    // private constructor used only by Delegate class
+    private MessageDigest(String algorithm, Provider p) {
+        this.algorithm = algorithm;
+        this.provider = p;
+    }
+
     /**
      * Returns a MessageDigest object that implements the specified digest
      * algorithm.
@@ -178,10 +184,11 @@ public abstract class MessageDigest extends MessageDigestSpi {
                                              (String)null);
             if (objs[0] instanceof MessageDigest) {
                 md = (MessageDigest)objs[0];
-            } else {
-                md = new Delegate((MessageDigestSpi)objs[0], algorithm);
-            }
                 md.provider = (Provider)objs[1];
+            } else {
+                md = Delegate.of((MessageDigestSpi)objs[0], algorithm,
+                    (Provider) objs[1]);
+            }
 
             if (!skipDebug && pdebug != null) {
                 pdebug.println("MessageDigest." + algorithm +
@@ -245,8 +252,8 @@ public abstract class MessageDigest extends MessageDigestSpi {
             return md;
         } else {
             MessageDigest delegate =
-                new Delegate((MessageDigestSpi)objs[0], algorithm);
+                    Delegate.of((MessageDigestSpi)objs[0], algorithm,
-            delegate.provider = (Provider)objs[1];
+                    (Provider)objs[1]);
             return delegate;
         }
     }
@@ -298,8 +305,8 @@ public abstract class MessageDigest extends MessageDigestSpi {
             return md;
         } else {
             MessageDigest delegate =
-                new Delegate((MessageDigestSpi)objs[0], algorithm);
+                    Delegate.of((MessageDigestSpi)objs[0], algorithm,
-            delegate.provider = (Provider)objs[1];
+                    (Provider)objs[1]);
             return delegate;
         }
     }
@@ -547,8 +554,6 @@ public abstract class MessageDigest extends MessageDigestSpi {
     }
 
 
-
-
     /*
      * The following class allows providers to extend from MessageDigestSpi
      * rather than from MessageDigest. It represents a MessageDigest with an
@@ -563,14 +568,45 @@ public abstract class MessageDigest extends MessageDigestSpi {
      * and its original parent (Object).
      */
 
-    static class Delegate extends MessageDigest implements MessageDigestSpi2 {
+    private static class Delegate extends MessageDigest
+            implements MessageDigestSpi2 {
+        // use this class for spi objects which implements Cloneable
+        private static final class CloneableDelegate extends Delegate
+                implements Cloneable {
+
+            private CloneableDelegate(MessageDigestSpi digestSpi,
+                    String algorithm, Provider p) {
+                super(digestSpi, algorithm, p);
+            }
+        }
 
         // The provider implementation (delegate)
-        private MessageDigestSpi digestSpi;
+        private final MessageDigestSpi digestSpi;
 
-        // constructor
+        // factory method used by MessageDigest class to create Delegate objs
-        public Delegate(MessageDigestSpi digestSpi, String algorithm) {
+        static Delegate of(MessageDigestSpi digestSpi, String algo,
-            super(algorithm);
+                Provider p) {
+            Objects.requireNonNull(digestSpi);
+            boolean isCloneable = (digestSpi instanceof Cloneable);
+            // Spi impls from SunPKCS11 provider implement Cloneable but their
+            // clone() may throw CloneNotSupportException
+            if (isCloneable && p.getName().startsWith("SunPKCS11") &&
+                    p.getClass().getModule().getName().equals
+                    ("jdk.crypto.cryptoki")) {
+                try {
+                    digestSpi.clone();
+                } catch (CloneNotSupportedException cnse) {
+                    isCloneable = false;
+                }
+            }
+            return (isCloneable? new CloneableDelegate(digestSpi, algo, p) :
+                    new Delegate(digestSpi, algo, p));
+        }
+
+        // private constructor
+        private Delegate(MessageDigestSpi digestSpi, String algorithm,
+                Provider p) {
+            super(algorithm, p);
             this.digestSpi = digestSpi;
         }
 
@@ -582,17 +618,16 @@ public abstract class MessageDigest extends MessageDigestSpi {
          * @throws    CloneNotSupportedException if this is called on a
          * delegate that does not support {@code Cloneable}.
          */
+        @Override
         public Object clone() throws CloneNotSupportedException {
-            if (digestSpi instanceof Cloneable) {
+            if (this instanceof Cloneable) {
-                MessageDigestSpi digestSpiClone =
-                    (MessageDigestSpi)digestSpi.clone();
                 // Because 'algorithm', 'provider', and 'state' are private
                 // members of our supertype, we must perform a cast to
                 // access them.
-                MessageDigest that =
+                MessageDigest that = new CloneableDelegate(
-                    new Delegate(digestSpiClone,
+                        (MessageDigestSpi)digestSpi.clone(),
-                                 ((MessageDigest)this).algorithm);
+                        ((MessageDigest)this).algorithm,
-                that.provider = ((MessageDigest)this).provider;
+                        ((MessageDigest)this).provider);
                 that.state = ((MessageDigest)this).state;
                 return that;
             } else {
@@ -600,22 +635,27 @@ public abstract class MessageDigest extends MessageDigestSpi {
             }
         }
 
+        @Override
         protected int engineGetDigestLength() {
             return digestSpi.engineGetDigestLength();
         }
 
+        @Override
         protected void engineUpdate(byte input) {
             digestSpi.engineUpdate(input);
         }
 
+        @Override
         protected void engineUpdate(byte[] input, int offset, int len) {
             digestSpi.engineUpdate(input, offset, len);
         }
 
+        @Override
         protected void engineUpdate(ByteBuffer input) {
             digestSpi.engineUpdate(input);
         }
 
+        @Override
         public void engineUpdate(SecretKey key) throws InvalidKeyException {
             if (digestSpi instanceof MessageDigestSpi2) {
                 ((MessageDigestSpi2)digestSpi).engineUpdate(key);
@@ -624,15 +664,19 @@ public abstract class MessageDigest extends MessageDigestSpi {
                 ("Digest does not support update of SecretKey object");
             }
         }
+
+        @Override
         protected byte[] engineDigest() {
             return digestSpi.engineDigest();
         }
 
+        @Override
         protected int engineDigest(byte[] buf, int offset, int len)
             throws DigestException {
                 return digestSpi.engineDigest(buf, offset, len);
         }
 
+        @Override
         protected void engineReset() {
             digestSpi.engineReset();
         }

src/java.base/share/classes/java/security/Signature.java

@@ -272,7 +272,7 @@ public abstract class Signature extends SignatureSpi {
         NoSuchAlgorithmException failure;
         do {
             Service s = t.next();
-            if (isSpi(s)) {
+            if (isSpi(s)) { // delayed provider selection
                 return new Delegate(s, t, algorithm);
             } else {
                 // must be a subclass of Signature, disable dynamic selection
@@ -295,7 +295,7 @@ public abstract class Signature extends SignatureSpi {
             sig.algorithm = algorithm;
         } else {
             SignatureSpi spi = (SignatureSpi)instance.impl;
-            sig = new Delegate(spi, algorithm);
+            sig = Delegate.of(spi, algorithm);
         }
         sig.provider = instance.provider;
         return sig;
@@ -464,7 +464,7 @@ public abstract class Signature extends SignatureSpi {
         // check Cipher
         try {
             Cipher c = Cipher.getInstance(RSA_CIPHER, p);
-            return new Delegate(new CipherAdapter(c), RSA_SIGNATURE);
+            return Delegate.of(new CipherAdapter(c), RSA_SIGNATURE);
         } catch (GeneralSecurityException e) {
             // throw Signature style exception message to avoid confusion,
             // but append Cipher exception as cause
@@ -1092,6 +1092,14 @@ public abstract class Signature extends SignatureSpi {
 
     @SuppressWarnings("deprecation")
     private static class Delegate extends Signature {
+        // use this class for spi objects which implements Cloneable
+        private static final class CloneableDelegate extends Delegate
+                implements Cloneable {
+            private CloneableDelegate(SignatureSpi digestSpi,
+                    String algorithm) {
+                super(digestSpi, algorithm);
+            }
+        }
 
         // The provider implementation (delegate)
         // filled in once the provider is selected
@@ -1108,15 +1116,24 @@ public abstract class Signature extends SignatureSpi {
         // null once provider is selected
         private Iterator<Service> serviceIterator;
 
-        // constructor
+        // factory method used by Signature class to create Delegate objs
-        Delegate(SignatureSpi sigSpi, String algorithm) {
+        static Delegate of(SignatureSpi sigSpi, String algorithm) {
+            if (sigSpi instanceof Cloneable) {
+                return new CloneableDelegate(sigSpi, algorithm);
+            } else {
+                return new Delegate(sigSpi, algorithm);
+            }
+        }
+
+        // private constructor
+        private Delegate(SignatureSpi sigSpi, String algorithm) {
             super(algorithm);
             this.sigSpi = sigSpi;
             this.lock = null; // no lock needed
         }
 
-        // used with delayed provider selection
+        // constructor used with delayed provider selection
-        Delegate(Service service,
+        private Delegate(Service service,
                         Iterator<Service> iterator, String algorithm) {
             super(algorithm);
             this.firstService = service;
@@ -1132,15 +1149,16 @@ public abstract class Signature extends SignatureSpi {
          * @throws    CloneNotSupportedException if this is called on a
          * delegate that does not support {@code Cloneable}.
          */
+        @Override
         public Object clone() throws CloneNotSupportedException {
             chooseFirstProvider();
             if (sigSpi instanceof Cloneable) {
-                SignatureSpi sigSpiClone = (SignatureSpi)sigSpi.clone();
                 // Because 'algorithm' and 'provider' are private
                 // members of our supertype, we must perform a cast to
                 // access them.
-                Signature that =
+                Signature that = new CloneableDelegate(
-                    new Delegate(sigSpiClone, ((Signature)this).algorithm);
+                   (SignatureSpi)sigSpi.clone(),
+                   ((Signature)this).algorithm);
                 that.provider = ((Signature)this).provider;
                 return that;
             } else {

src/java.base/share/classes/sun/security/provider/DigestBase.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2020, 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
@@ -233,6 +233,7 @@ abstract class DigestBase extends MessageDigestSpi implements Cloneable {
     public Object clone() throws CloneNotSupportedException {
         DigestBase copy = (DigestBase) super.clone();
         copy.buffer = copy.buffer.clone();
+        copy.oneByte = null;
         return copy;
     }
 

test/jdk/com/sun/crypto/provider/Mac/DigestCloneabilityTest.java

@@ -0,0 +1,106 @@
+/*
+ * Copyright (c) 2020, 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 8246077
+ * @summary Ensure a cloneable digest can be accepted/used by HmacCore
+ */
+import java.security.*;
+import java.security.spec.AlgorithmParameterSpec;
+import javax.crypto.*;
+import javax.crypto.spec.*;
+
+public class DigestCloneabilityTest {
+
+    private static String ALGO = "HmacSHA512";
+
+    public static void main(String[] args) throws Exception {
+        Provider p = new SampleProvider();
+        // make SampleProvider the most preferred, so its digest impl is picked
+        int status = Security.insertProviderAt(p, 1);
+        try {
+            Mac mac = Mac.getInstance(ALGO, "SunJCE");
+            // do a complete mac generation and check if the supplied
+            // digest is used
+            mac.init(new SecretKeySpec(new byte[512>>3], ALGO));
+            mac.update((byte)0x12);
+            byte[] macBytes = mac.doFinal();
+            if (!SampleProvider.CloneableDigest.isUsed) {
+                throw new RuntimeException("Expected Digest impl not used");
+            }
+        } finally {
+            if (status != -1) {
+                Security.removeProvider(p.getName());
+            }
+        }
+        System.out.println("Test Passed");
+    }
+
+    public static class SampleProvider extends Provider {
+
+        public SampleProvider() {
+            super("Sample", "1.0", "test provider");
+            putService(new Provider.Service(this, "MessageDigest", "SHA-512",
+                    "DigestCloneabilityTest$SampleProvider$CloneableDigest",
+                    null, null));
+        }
+        public static class CloneableDigest extends MessageDigestSpi
+                implements Cloneable {
+            private MessageDigest md;
+            static boolean isUsed = false;
+
+            public CloneableDigest() throws NoSuchAlgorithmException {
+                try {
+                    md = MessageDigest.getInstance("SHA-512", "SUN");
+                } catch (NoSuchProviderException nspe) {
+                    // should never happen
+                }
+            }
+
+            public byte[] engineDigest() {
+                isUsed = true;
+                return md.digest();
+            }
+
+            public void engineReset() {
+                isUsed = true;
+                md.reset();
+            }
+
+            public void engineUpdate(byte input) {
+                isUsed = true;
+                md.update(input);
+            }
+
+            public void engineUpdate(byte[] b, int ofs, int len) {
+                isUsed = true;
+                md.update(b, ofs, len);
+            }
+
+            public Object clone() throws CloneNotSupportedException {
+                return this;
+            }
+        }
+    }
+}

test/jdk/java/security/MessageDigest/TestCloneable.java

@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2020, 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 8246077
+ * @summary Make sure that digest spi and the resulting digest impl are
+ * consistent in the impl of Cloneable interface
+ * @run testng TestCloneable
+ */
+import java.security.*;
+import java.util.Objects;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import org.testng.Assert;
+
+public class TestCloneable {
+
+    private static final Class<CloneNotSupportedException> CNSE =
+            CloneNotSupportedException.class;
+
+    @DataProvider
+    public Object[][] testData() {
+        return new Object[][] {
+            { "MD2", "SUN" }, { "MD5", "SUN" }, { "SHA-1", "SUN" },
+            { "SHA-224", "SUN" }, { "SHA-256", "SUN" },
+            { "SHA-384", "SUN" }, { "SHA-512", "SUN" },
+            { "SHA3-224", "SUN" }, { "SHA3-256", "SUN" },
+            { "SHA3-384", "SUN" }, { "SHA3-512", "SUN" }
+        };
+    }
+
+    @Test(dataProvider = "testData")
+    public void test(String algo, String provName)
+            throws NoSuchProviderException, NoSuchAlgorithmException,
+            CloneNotSupportedException {
+        System.out.print("Testing " + algo + " impl from " + provName);
+        Provider p = Security.getProvider(provName);
+        Provider.Service s = p.getService("MessageDigest", algo);
+        Objects.requireNonNull(s);
+        MessageDigestSpi spi = (MessageDigestSpi) s.newInstance(null);
+        MessageDigest md = MessageDigest.getInstance(algo, provName);
+        if (spi instanceof Cloneable) {
+            System.out.println(": Cloneable");
+            Assert.assertTrue(md instanceof Cloneable);
+            MessageDigest md2 = (MessageDigest) md.clone();
+            Assert.assertEquals(md2.getAlgorithm(), algo);
+            Assert.assertEquals(md2.getProvider().getName(), provName);
+            Assert.assertTrue(md2 instanceof Cloneable);
+        } else {
+            System.out.println(": NOT Cloneable");
+            Assert.assertThrows(CNSE, ()->md.clone());
+        }
+        System.out.println("Test Passed");
+    }
+}

test/jdk/java/security/Signature/TestCloneable.java

@@ -0,0 +1,95 @@
+/*
+ * Copyright (c) 2020, 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 8246077
+ * @summary Make sure that signature objects which are cloneable
+ *         implement the Cloneable interface
+ * @run testng TestCloneable
+ */
+import java.security.NoSuchProviderException;
+import java.security.NoSuchAlgorithmException;
+import java.security.Signature;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import org.testng.Assert;
+
+public class TestCloneable {
+
+    private static final Class<CloneNotSupportedException> CNSE =
+            CloneNotSupportedException.class;
+
+    @DataProvider
+    public Object[][] testData() {
+        return new Object[][] {
+            { "SHA1withDSA", "SUN" }, { "NONEwithDSA", "SUN" },
+            { "SHA224withDSA", "SUN" }, { "SHA256withDSA", "SUN" },
+            { "EdDSA", "SunEC" }, { "Ed25519", "SunEC" }, { "Ed448", "SunEC" },
+            { "SHA1withECDSA", "SunEC" }, { "SHA224withECDSA", "SunEC" },
+            { "SHA256withECDSA", "SunEC" }, { "SHA384withECDSA", "SunEC" },
+            { "SHA512withECDSA", "SunEC" }, { "NONEwithECDSA", "SunEC" },
+            { "MD2withRSA", "SunRsaSign" }, { "MD5withRSA", "SunRsaSign" },
+            { "SHA1withRSA", "SunRsaSign" }, { "SHA224withRSA", "SunRsaSign" },
+            { "SHA256withRSA", "SunRsaSign" },
+            { "SHA384withRSA", "SunRsaSign" },
+            { "SHA512withRSA", "SunRsaSign" },
+            { "SHA512/224withRSA", "SunRsaSign" },
+            { "SHA512/256withRSA", "SunRsaSign" },
+            { "RSASSA-PSS", "SunRsaSign" },
+            { "NONEwithRSA", "SunMSCAPI" },
+            { "SHA1withRSA", "SunMSCAPI" }, { "SHA256withRSA", "SunMSCAPI" },
+            { "SHA384withRSA", "SunMSCAPI" }, { "SHA512withRSA", "SunMSCAPI" },
+            { "RSASSA-PSS", "SunMSCAPI" },
+            { "MD5withRSA", "SunMSCAPI" }, { "MD2withRSA", "SunMSCAPI" },
+            { "SHA1withECDSA", "SunMSCAPI" },
+            { "SHA224withECDSA", "SunMSCAPI" },
+            { "SHA256withECDSA", "SunMSCAPI" },
+            { "SHA384withECDSA", "SunMSCAPI" },
+            { "SHA512withECDSA", "SunMSCAPI" }
+        };
+    }
+
+    @Test(dataProvider = "testData")
+    public void test(String algo, String provName)
+            throws NoSuchAlgorithmException, CloneNotSupportedException {
+        System.out.print("Testing " + algo + " impl from " + provName);
+        try {
+            Signature sig = Signature.getInstance(algo, provName);
+            if (sig instanceof Cloneable) {
+                System.out.println(": Cloneable");
+                Signature sig2 = (Signature) sig.clone();
+                Assert.assertEquals(sig2.getAlgorithm(), algo);
+                Assert.assertEquals(sig2.getProvider().getName(), provName);
+                Assert.assertTrue(sig2 instanceof Cloneable);
+            } else {
+                System.out.println(": NOT Cloneable");
+                Assert.assertThrows(CNSE, ()->sig.clone());
+            }
+            System.out.println("Test Passed");
+        } catch (NoSuchProviderException nspe) {
+            // skip testing
+            System.out.println("Skip " + provName + " - not available");
+        }
+    }
+}