8149802: Signature.verify() doesn't reset the signature object on exception
authorvaleriep
Wed, 28 Sep 2016 03:10:37 +0000
changeset 41205 2867bb7f5b53
parent 41204 c507f3d8c3e4
child 41206 591cd107586c
8149802: Signature.verify() doesn't reset the signature object on exception Summary: Ensure the signature object is always reset after verify() is called. Reviewed-by: xuelei
jdk/src/java.base/share/classes/sun/security/rsa/RSASignature.java
jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11Signature.java
jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/Secmod.java
jdk/src/jdk.crypto.ucrypto/solaris/classes/com/oracle/security/ucrypto/NativeRSASignature.java
jdk/test/java/security/Signature/ResetAfterException.java
--- a/jdk/src/java.base/share/classes/sun/security/rsa/RSASignature.java	Tue Sep 27 18:45:51 2016 -0700
+++ b/jdk/src/java.base/share/classes/sun/security/rsa/RSASignature.java	Wed Sep 28 03:10:37 2016 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2016, 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
@@ -182,14 +182,15 @@
     }
 
     // verify the data and return the result. See JCA doc
+    // should be reset to the state after engineInitVerify call.
     protected boolean engineVerify(byte[] sigBytes) throws SignatureException {
-        if (sigBytes.length != RSACore.getByteLength(publicKey)) {
-            throw new SignatureException("Signature length not correct: got " +
+        try {
+            if (sigBytes.length != RSACore.getByteLength(publicKey)) {
+                throw new SignatureException("Signature length not correct: got " +
                     sigBytes.length + " but was expecting " +
                     RSACore.getByteLength(publicKey));
-        }
-        byte[] digest = getDigestValue();
-        try {
+            }
+            byte[] digest = getDigestValue();
             byte[] decrypted = RSACore.rsa(sigBytes, publicKey);
             byte[] unpadded = padding.unpad(decrypted);
             byte[] decodedDigest = decodeSignature(digestOID, unpadded);
@@ -202,6 +203,8 @@
             return false;
         } catch (IOException e) {
             throw new SignatureException("Signature encoding error", e);
+        } finally {
+            resetDigest();
         }
     }
 
--- a/jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11Signature.java	Tue Sep 27 18:45:51 2016 -0700
+++ b/jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11Signature.java	Wed Sep 28 03:10:37 2016 +0000
@@ -616,8 +616,11 @@
                     return dsaToASN1(signature);
                 }
             }
-        } catch (PKCS11Exception e) {
-            throw new ProviderException(e);
+        } catch (PKCS11Exception pe) {
+            throw new ProviderException(pe);
+        } catch (SignatureException | ProviderException e) {
+            cancelOperation();
+            throw e;
         } finally {
             initialized = false;
             session = token.releaseSession(session);
@@ -669,8 +672,8 @@
                 }
             }
             return true;
-        } catch (PKCS11Exception e) {
-            long errorCode = e.getErrorCode();
+        } catch (PKCS11Exception pe) {
+            long errorCode = pe.getErrorCode();
             if (errorCode == CKR_SIGNATURE_INVALID) {
                 return false;
             }
@@ -682,10 +685,11 @@
             if (errorCode == CKR_DATA_LEN_RANGE) {
                 return false;
             }
-            throw new ProviderException(e);
+            throw new ProviderException(pe);
+        }  catch (SignatureException | ProviderException e) {
+            cancelOperation();
+            throw e;
         } finally {
-            // XXX we should not release the session if we abort above
-            // before calling C_Verify
             initialized = false;
             session = token.releaseSession(session);
         }
--- a/jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/Secmod.java	Tue Sep 27 18:45:51 2016 -0700
+++ b/jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/Secmod.java	Wed Sep 28 03:10:37 2016 +0000
@@ -743,6 +743,7 @@
         Map<Bytes,TrustAttributes> trustMap = new HashMap<Bytes,TrustAttributes>();
         Token token = provider.getToken();
         Session session = null;
+        boolean exceptionOccurred = true;
         try {
             session = token.getOpSession();
             int MAX_NUM = 8192;
@@ -762,8 +763,13 @@
                     // skip put on pkcs11 error
                 }
             }
+            exceptionOccurred = false;
         } finally {
-            token.releaseSession(session);
+            if (exceptionOccurred) {
+                token.killSession(session);
+            } else {
+                token.releaseSession(session);
+            }
         }
         return trustMap;
     }
--- a/jdk/src/jdk.crypto.ucrypto/solaris/classes/com/oracle/security/ucrypto/NativeRSASignature.java	Tue Sep 27 18:45:51 2016 -0700
+++ b/jdk/src/jdk.crypto.ucrypto/solaris/classes/com/oracle/security/ucrypto/NativeRSASignature.java	Wed Sep 28 03:10:37 2016 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2016, 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
@@ -258,27 +258,38 @@
 
     @Override
     protected synchronized byte[] engineSign() throws SignatureException {
-        byte[] sig = new byte[sigLength];
-        int rv = doFinal(sig, 0, sigLength);
-        if (rv < 0) {
-            throw new SignatureException(new UcryptoException(-rv));
+        try {
+            byte[] sig = new byte[sigLength];
+            int rv = doFinal(sig, 0, sigLength);
+            if (rv < 0) {
+                throw new SignatureException(new UcryptoException(-rv));
+            }
+            return sig;
+        } finally {
+            // doFinal should already be called, no need to cancel
+            reset(false);
         }
-        return sig;
     }
 
     @Override
     protected synchronized int engineSign(byte[] outbuf, int offset, int len)
         throws SignatureException {
-        if (outbuf == null || (offset < 0) || (outbuf.length < (offset + sigLength))
-            || (len < sigLength)) {
-            throw new SignatureException("Invalid output buffer. offset: " +
-                offset + ". len: " + len + ". sigLength: " + sigLength);
+        boolean doCancel = true;
+        try {
+            if (outbuf == null || (offset < 0) || (outbuf.length < (offset + sigLength))
+                || (len < sigLength)) {
+                throw new SignatureException("Invalid output buffer. offset: " +
+                    offset + ". len: " + len + ". sigLength: " + sigLength);
+            }
+            int rv = doFinal(outbuf, offset, sigLength);
+            doCancel = false;
+            if (rv < 0) {
+                throw new SignatureException(new UcryptoException(-rv));
+            }
+            return sigLength;
+        } finally {
+            reset(doCancel);
         }
-        int rv = doFinal(outbuf, offset, sigLength);
-        if (rv < 0) {
-            throw new SignatureException(new UcryptoException(-rv));
-        }
-        return sigLength;
     }
 
     @Override
@@ -329,19 +340,25 @@
     @Override
     protected synchronized boolean engineVerify(byte[] sigBytes, int sigOfs, int sigLen)
         throws SignatureException {
-        if (sigBytes == null || (sigOfs < 0) || (sigBytes.length < (sigOfs + this.sigLength))
-            || (sigLen != this.sigLength)) {
-            throw new SignatureException("Invalid signature length: got " +
-                sigLen + " but was expecting " + this.sigLength);
-        }
+        boolean doCancel = true;
+        try {
+            if (sigBytes == null || (sigOfs < 0) || (sigBytes.length < (sigOfs + this.sigLength))
+                || (sigLen != this.sigLength)) {
+                throw new SignatureException("Invalid signature length: got " +
+                    sigLen + " but was expecting " + this.sigLength);
+            }
 
-        int rv = doFinal(sigBytes, sigOfs, sigLen);
-        if (rv == 0) {
-            return true;
-        } else {
-            UcryptoProvider.debug("Signature: " + mech + " verification error " +
+            int rv = doFinal(sigBytes, sigOfs, sigLen);
+            doCancel = false;
+            if (rv == 0) {
+                return true;
+            } else {
+                UcryptoProvider.debug("Signature: " + mech + " verification error " +
                              new UcryptoException(-rv).getMessage());
-            return false;
+                return false;
+            }
+        } finally {
+            reset(doCancel);
         }
     }
 
@@ -432,13 +449,9 @@
 
     // returns 0 (success) or negative (ucrypto error occurred)
     private int doFinal(byte[] sigBytes, int sigOfs, int sigLen) {
-        try {
-            ensureInitialized();
-            int k = nativeFinal(pCtxt.id, sign, sigBytes, sigOfs, sigLen);
-            return k;
-        } finally {
-            reset(false);
-        }
+        ensureInitialized();
+        int k = nativeFinal(pCtxt.id, sign, sigBytes, sigOfs, sigLen);
+        return k;
     }
 
     // check and return RSA key size in number of bytes
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/security/Signature/ResetAfterException.java	Wed Sep 28 03:10:37 2016 +0000
@@ -0,0 +1,140 @@
+/*
+ * Copyright (c) 2016, 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 8149802
+ * @summary Ensure that Signature objects are reset after verification errored out.
+ */
+import java.util.Arrays;
+import java.security.*;
+
+public class ResetAfterException {
+
+    public static void main(String[] args) throws Exception {
+
+        byte[] data = "data to be signed".getBytes();
+        byte[] shortBuffer = new byte[2];
+
+        Provider[] provs = Security.getProviders();
+        boolean failed = false;
+
+        for (Provider p : provs) {
+            Signature sig;
+            try {
+                sig = Signature.getInstance("SHA256withRSA", p);
+            } catch (NoSuchAlgorithmException nsae) {
+                // no support, skip
+                continue;
+            }
+
+            boolean res = true;
+            System.out.println("Testing Provider: " + p.getName());
+            KeyPairGenerator keyGen = null;
+            try {
+                // It's possible that some provider, e.g. SunMSCAPI,
+                // doesn't work well with keys from other providers
+                // so we use the same provider to generate key first
+                keyGen = KeyPairGenerator.getInstance("RSA", p);
+            } catch (NoSuchAlgorithmException nsae) {
+                keyGen = KeyPairGenerator.getInstance("RSA");
+            }
+            if (keyGen == null) {
+                throw new RuntimeException("Error: No support for RSA KeyPairGenerator");
+            }
+            keyGen.initialize(1024);
+            KeyPair keyPair = keyGen.generateKeyPair();
+
+            sig.initSign(keyPair.getPrivate());
+            sig.update(data);
+            byte[] signature = sig.sign();
+            // First check signing
+            try {
+                sig.update(data);
+                // sign with short output buffer to cause exception
+                int len = sig.sign(shortBuffer, 0, shortBuffer.length);
+                System.out.println("FAIL: Should throw SE with short buffer");
+                res = false;
+            } catch (SignatureException e) {
+                // expected exception; ignore
+                System.out.println("Expected Ex for short output buffer: " + e);
+            }
+            // Signature object should reset after a failed generation
+            sig.update(data);
+            byte[] signature2 = sig.sign();
+            if (!Arrays.equals(signature, signature2)) {
+                System.out.println("FAIL: Generated different signature");
+                res = false;
+            } else {
+                System.out.println("Generated same signature");
+            }
+
+            // Now, check signature verification
+            sig.initVerify(keyPair.getPublic());
+            sig.update(data);
+            try {
+                // first verify with valid signature bytes
+                res = sig.verify(signature);
+            } catch (SignatureException e) {
+                System.out.println("FAIL: Valid signature rejected");
+                e.printStackTrace();
+                res = false;
+            }
+
+            try {
+                sig.update(data);
+                // verify with short signaure to cause exception
+                if (sig.verify(shortBuffer)) {
+                    System.out.println("FAIL: Invalid signature verified");
+                    res = false;
+                } else {
+                    System.out.println("Invalid signature rejected");
+                }
+            } catch (SignatureException e) {
+                // expected exception; ignore
+                System.out.println("Expected Ex for short output buffer: " + e);
+            }
+            // Signature object should reset after an a failed verification
+            sig.update(data);
+            try {
+                // verify with valid signature bytes again
+                res = sig.verify(signature);
+                if (!res) {
+                    System.out.println("FAIL: Valid signature is rejected");
+                } else {
+                    System.out.println("Valid signature is accepted");
+                }
+            } catch (GeneralSecurityException e) {
+                System.out.println("FAIL: Valid signature is rejected");
+                e.printStackTrace();
+                res = false;
+            }
+            failed |= !res;
+        }
+        if (failed) {
+            throw new RuntimeException("One or more test failed");
+        } else {
+            System.out.println("Test Passed");
+        }
+   }
+}