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
--- 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");
+ }
+ }
+}