From aa1587403d58d2877efd3c310f7f0d44c449dd5b Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Tue, 22 Dec 2020 15:42:55 -0300 Subject: [PATCH 01/15] 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures --- .../share/classes/sun/security/pkcs11/P11Cipher.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java index 470a888cd84b4..107f311ebe173 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java @@ -628,7 +628,7 @@ private int implUpdate(byte[] in, int inOfs, int inLen, throw (ShortBufferException) (new ShortBufferException().initCause(e)); } - reset(false); + reset(true); throw new ProviderException("update() failed", e); } } @@ -746,7 +746,7 @@ private int implUpdate(ByteBuffer inBuffer, ByteBuffer outBuffer) throw (ShortBufferException) (new ShortBufferException().initCause(e)); } - reset(false); + reset(true); throw new ProviderException("update() failed", e); } } @@ -799,7 +799,6 @@ private int implDoFinal(byte[] out, int outOfs, int outLen) } return k; } catch (PKCS11Exception e) { - doCancel = false; handleException(e); throw new ProviderException("doFinal() failed", e); } finally { @@ -884,7 +883,6 @@ private int implDoFinal(ByteBuffer outBuffer) } return k; } catch (PKCS11Exception e) { - doCancel = false; handleException(e); throw new ProviderException("doFinal() failed", e); } finally { From 542b23402116f7eca99c6fd8d7649ac01d9e1fc9 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Mon, 28 Dec 2020 12:49:19 -0300 Subject: [PATCH 02/15] Test for 8258833. --- .../pkcs11/Cipher/CancelMultipart.java | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java diff --git a/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java b/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java new file mode 100644 index 0000000000000..aaa2a56251cca --- /dev/null +++ b/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java @@ -0,0 +1,200 @@ +/* + * Copyright (c) 2020, Red Hat, Inc. + * 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 8258833 + * @library /test/lib .. + * @modules jdk.crypto.cryptoki/sun.security.pkcs11:open + * @run main/othervm CancelMultipart + */ + +import java.lang.reflect.Field; +import java.nio.ByteBuffer; +import java.security.Key; +import java.security.Provider; +import java.security.ProviderException; +import javax.crypto.Cipher; +import javax.crypto.IllegalBlockSizeException; +import javax.crypto.spec.SecretKeySpec; + +public class CancelMultipart extends PKCS11Test { + + private static Provider provider; + private static Key key; + + static { + key = new SecretKeySpec(new byte[16], "AES"); + } + + private static class SessionLeaker { + private LeakOperation op; + private LeakInputType type; + + SessionLeaker(LeakOperation op, LeakInputType type) { + this.op = op; + this.type = type; + } + + private void leakAndTry() throws Exception { + Cipher cipher = op.getCipher(); + try { + type.doOperation(cipher, + (op instanceof LeakDecrypt ? + LeakInputType.DECRYPT_MODE : + LeakInputType.ENCRYPT_MODE)); + throw new Exception("PKCS11Exception expected, invalid block" + + "size"); + } catch (ProviderException | IllegalBlockSizeException e) { + // Exception expected - session returned to the SessionManager + // should be cancelled. That's what will be tested now. + } + + tryCipherInit(); + } + } + + private static interface LeakOperation { + Cipher getCipher() throws Exception; + } + + private static interface LeakInputType { + static int ENCRYPT_MODE = 1; + static int DECRYPT_MODE = 2; + void doOperation(Cipher cipher, int mode) throws Exception; + } + + private static class LeakDecrypt implements LeakOperation { + public Cipher getCipher() throws Exception { + Cipher cipher = Cipher.getInstance( + "AES/ECB/PKCS5Padding", provider); + cipher.init(Cipher.DECRYPT_MODE, key); + return cipher; + } + } + + private static class LeakEncrypt implements LeakOperation { + public Cipher getCipher() throws Exception { + Cipher cipher = Cipher.getInstance( + "AES/ECB/NoPadding", provider); + cipher.init(Cipher.ENCRYPT_MODE, key); + return cipher; + } + } + + private static class LeakByteBuffer implements LeakInputType { + public void doOperation(Cipher cipher, int mode) throws Exception { + if (mode == DECRYPT_MODE) { + cipher.update(ByteBuffer.allocate(1), ByteBuffer.allocate(1)); + cipher.doFinal(ByteBuffer.allocate(0), ByteBuffer.allocate(1)); + } else { + cipher.update(ByteBuffer.allocate(1), ByteBuffer.allocate(2)); + } + } + } + + private static class LeakByteArray implements LeakInputType { + public void doOperation(Cipher cipher, int mode) throws Exception { + if (mode == DECRYPT_MODE) { + cipher.update(new byte[1]); + cipher.doFinal(new byte[1], 0, 0); + } else { + cipher.update(new byte[1]); + } + } + } + + public static void main(String[] args) throws Exception { + main(new CancelMultipart(), args); + } + + @Override + public void main(Provider p) throws Exception { + init(p); + + // Try multiple paths: + + executeTest(new SessionLeaker(new LeakEncrypt(), new LeakByteArray()), + "P11Cipher::implUpdate(byte[], int, int, byte[], int, int)"); + + executeTest(new SessionLeaker(new LeakEncrypt(), new LeakByteBuffer()), + "P11Cipher::implUpdate(ByteBuffer, ByteBuffer)"); + + executeTest(new SessionLeaker(new LeakDecrypt(), new LeakByteArray()), + "P11Cipher::implDoFinal(byte[], int, int)"); + + executeTest(new SessionLeaker(new LeakDecrypt(), new LeakByteBuffer()), + "P11Cipher::implDoFinal(ByteBuffer)"); + + System.out.println("TEST PASS - OK"); + } + + private static void executeTest(SessionLeaker sl, String testName) + throws Exception { + try { + sl.leakAndTry(); + System.out.println(testName + ": OK"); + } catch (Exception e) { + System.out.println(testName + ": FAILED"); + throw e; + } + } + + private static void init(Provider p) throws Exception { + provider = p; + + // The max number of sessions is 2 because, in addition to the + // operation (i.e. PKCS11::getNativeKeyInfo), a session to hold + // the P11Key object is needed. + setMaxSessions(2); + } + + /* + * This method is intended to generate pression on the number of sessions + * to be used from the NSS Software Token, so sessions with (potentially) + * active operations are reused. + */ + private static void setMaxSessions(int maxSessions) throws Exception { + Field tokenField = Class.forName("sun.security.pkcs11.SunPKCS11") + .getDeclaredField("token"); + tokenField.setAccessible(true); + Field sessionManagerField = Class.forName("sun.security.pkcs11.Token") + .getDeclaredField("sessionManager"); + sessionManagerField.setAccessible(true); + Field maxSessionsField = Class.forName("sun.security.pkcs11.SessionManager") + .getDeclaredField("maxSessions"); + maxSessionsField.setAccessible(true); + Object sessionManagerObj = sessionManagerField.get( + tokenField.get(provider)); + maxSessionsField.setInt(sessionManagerObj, maxSessions); + } + + private static void tryCipherInit() throws Exception { + Cipher cipher = Cipher.getInstance("AES/ECB/NoPadding", provider); + cipher.init(Cipher.ENCRYPT_MODE, key); + + // If initialization passes, finish gracefully so other paths can + // be tested under the current max number of sessions. + cipher.doFinal(new byte[16], 0, 0); + } +} From 91363c161630de82852d6db50c7ebdfb949de6f4 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Thu, 7 Jan 2021 17:07:06 -0300 Subject: [PATCH 03/15] Comment describing the CancelMultipart test assertion. --- .../sun/security/pkcs11/Cipher/CancelMultipart.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java b/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java index aaa2a56251cca..de81de6e7c16c 100644 --- a/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java +++ b/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java @@ -191,10 +191,19 @@ private static void setMaxSessions(int maxSessions) throws Exception { private static void tryCipherInit() throws Exception { Cipher cipher = Cipher.getInstance("AES/ECB/NoPadding", provider); + + // A CKR_OPERATION_ACTIVE error may be thrown if a session was + // returned to the Session Manager with an active operation, and + // we try to initialize the Cipher using it. + // + // Given that the maximum number of sessions was forced to 2, we know + // that the session to be used here was already used in a previous + // (failed) operation. Thus, the test asserts that the operation was + // properly cancelled. cipher.init(Cipher.ENCRYPT_MODE, key); // If initialization passes, finish gracefully so other paths can - // be tested under the current max number of sessions. + // be tested under the current maximum number of sessions. cipher.doFinal(new byte[16], 0, 0); } } From 80efdbbcff4075895d68710de7802608ef0c06e9 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Fri, 8 Jan 2021 16:44:03 -0300 Subject: [PATCH 04/15] Cancel Operation should not fail if the operation is not initialized in the token. --- .../share/classes/sun/security/pkcs11/P11Cipher.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java index 107f311ebe173..f3f305b5ffbb5 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java @@ -439,6 +439,13 @@ private void cancelOperation() { token.p11.C_DecryptFinal(session.id(), 0, buffer, 0, bufLen); } } catch (PKCS11Exception e) { + if (e.getErrorCode() == CKR_OPERATION_NOT_INITIALIZED) { + // Cancel Operation may be invoked after an error on a PKCS#11 + // call. If the operation inside the token was already cancelled, + // do not fail here. This is part of a defensive mechanism for + // PKCS#11 libraries that do not strictly follow the standard. + return; + } if (encrypt) { throw new ProviderException("Cancel failed", e); } From 5bf00de0cafca3bd757a71d4db6ff86078f20207 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Fri, 8 Jan 2021 16:56:46 -0300 Subject: [PATCH 05/15] Documentation note explaining why Cancel Operation is not required in P11Signature --- .../share/classes/sun/security/pkcs11/P11Signature.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java index c8616e26e499e..7b276e7a45c4d 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java @@ -654,6 +654,11 @@ protected byte[] engineSign() throws SignatureException { } } } catch (PKCS11Exception pe) { + // As per the PKCS#11 standard, C_Sign and C_SignFinal may only + // keep the operation active on CKR_BUFFER_TOO_SMALL errors. + // However, this type of error is prevented from reaching this + // point at OpenJDK's libj2pkcs11 native library. Thus, doCancel + // can safely be 'false' here. doCancel = false; throw new ProviderException(pe); } catch (SignatureException | ProviderException e) { From 3ccb24d77e9620adc2b24e2e9125d8916f84d574 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Thu, 14 Jan 2021 10:23:34 -0300 Subject: [PATCH 06/15] Documentation note explaining why Cancel Operation is not required in P11PSSSignature --- .../share/classes/sun/security/pkcs11/P11PSSSignature.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java index 14b6110dfb3cf..5ef39252f61f2 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java @@ -662,6 +662,11 @@ protected byte[] engineSign() throws SignatureException { doCancel = false; return signature; } catch (PKCS11Exception pe) { + // As per the PKCS#11 standard, C_Sign and C_SignFinal may only + // keep the operation active on CKR_BUFFER_TOO_SMALL errors. + // However, this type of error is prevented from reaching this + // point at OpenJDK's libj2pkcs11 native library. Thus, doCancel + // can safely be 'false' here. doCancel = false; throw new ProviderException(pe); } catch (ProviderException e) { From 1724fa18d8916385c64e5d6913edcd44205fe4b1 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Thu, 14 Jan 2021 12:18:53 -0300 Subject: [PATCH 07/15] Documentation note explaining why Cancel Operation is not required in P11AEADCipher --- .../sun/security/pkcs11/P11AEADCipher.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java index 82d0dc164f470..b6d9e1934dfa7 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java @@ -350,6 +350,13 @@ private void cancelOperation() { 0, buffer, 0, bufLen); } } catch (PKCS11Exception e) { + if (e.getErrorCode() == CKR_OPERATION_NOT_INITIALIZED) { + // Cancel Operation may be invoked after an error on a PKCS#11 + // call. If the operation inside the token was already cancelled, + // do not fail here. This is part of a defensive mechanism for + // PKCS#11 libraries that do not strictly follow the standard. + return; + } if (encrypt) { throw new ProviderException("Cancel failed", e); } @@ -616,6 +623,12 @@ private int implDoFinal(byte[] in, int inOfs, int inLen, } return k; } catch (PKCS11Exception e) { + // As per the PKCS#11 standard, C_Encrypt and C_Decrypt may only + // keep the operation active on CKR_BUFFER_TOO_SMALL errors or + // successful calls to determine the output length. However, + // these cases are not expected here because the output length + // is checked in the OpenJDK side before making the PKCS#11 call. + // Thus, doCancel can safely be 'false'. doCancel = false; handleException(e); throw new ProviderException("doFinal() failed", e); @@ -702,6 +715,12 @@ private int implDoFinal(ByteBuffer inBuffer, ByteBuffer outBuffer) outBuffer.position(outBuffer.position() + k); return k; } catch (PKCS11Exception e) { + // As per the PKCS#11 standard, C_Encrypt and C_Decrypt may only + // keep the operation active on CKR_BUFFER_TOO_SMALL errors or + // successful calls to determine the output length. However, + // these cases are not expected here because the output length + // is checked in the OpenJDK side before making the PKCS#11 call. + // Thus, doCancel can safely be 'false'. doCancel = false; handleException(e); throw new ProviderException("doFinal() failed", e); From 2a9b7aef34f36419192ca2c99be992b83c06001a Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Thu, 14 Jan 2021 14:42:30 -0300 Subject: [PATCH 08/15] C_DecryptFinal/C_EncryptFinal failures do not need a Cancel Operation; only C_DecryptUpdate/C_EncryptUpdate ones. --- .../share/classes/sun/security/pkcs11/P11Cipher.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java index f3f305b5ffbb5..3789e37b0cce1 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java @@ -851,9 +851,9 @@ private int implDoFinal(ByteBuffer outBuffer) 0, padBuffer, 0, actualPadLen, outAddr, outArray, outOfs, outLen); } + doCancel = false; k += token.p11.C_EncryptFinal(session.id(), outAddr, outArray, (outOfs + k), (outLen - k)); - doCancel = false; } else { // Special handling to match SunJCE provider behavior if (bytesBuffered == 0 && padBufferLen == 0) { @@ -867,18 +867,18 @@ private int implDoFinal(ByteBuffer outBuffer) 0, padBuffer, 0, padBuffer.length); padBufferLen = 0; } + doCancel = false; k += token.p11.C_DecryptFinal(session.id(), 0, padBuffer, k, padBuffer.length - k); - doCancel = false; int actualPadLen = paddingObj.unpad(padBuffer, k); k -= actualPadLen; outArray = padBuffer; outOfs = 0; } else { + doCancel = false; k = token.p11.C_DecryptFinal(session.id(), outAddr, outArray, outOfs, outLen); - doCancel = false; } } if ((!encrypt && paddingObj != null) || From f705b7faf1d074ceac8dc5e6bc4e23be45782c2e Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Fri, 15 Jan 2021 15:14:04 -0300 Subject: [PATCH 09/15] Better error handling in P11PSSSignature Cancel Operation and documentation improvements. --- .../sun/security/pkcs11/P11PSSSignature.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java index 5ef39252f61f2..9e797fba997da 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java @@ -298,6 +298,13 @@ private void cancelOperation() { } } } catch (PKCS11Exception e) { + if (e.getErrorCode() == CKR_OPERATION_NOT_INITIALIZED) { + // Cancel Operation may be invoked after an error on a PKCS#11 + // call. If the operation inside the token was already cancelled, + // do not fail here. This is part of a defensive mechanism for + // PKCS#11 libraries that do not strictly follow the standard. + return; + } if (mode == M_SIGN) { throw new ProviderException("cancel failed", e); } @@ -663,10 +670,10 @@ protected byte[] engineSign() throws SignatureException { return signature; } catch (PKCS11Exception pe) { // As per the PKCS#11 standard, C_Sign and C_SignFinal may only - // keep the operation active on CKR_BUFFER_TOO_SMALL errors. - // However, this type of error is prevented from reaching this - // point at OpenJDK's libj2pkcs11 native library. Thus, doCancel - // can safely be 'false' here. + // keep the operation active on CKR_BUFFER_TOO_SMALL errors or + // successful calls to determine the output length. However, + // these cases are handled at OpenJDK's libj2pkcs11 native + // library. Thus, doCancel can safely be 'false' here. doCancel = false; throw new ProviderException(pe); } catch (ProviderException e) { From 6abecdd2db6f0918a7a7f6da0502a7c2420b2de6 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Fri, 15 Jan 2021 15:20:46 -0300 Subject: [PATCH 10/15] Consistent Cancel Operation behavior across P11-services: do not fail when the operation being cancelled was not initialized. --- .../share/classes/sun/security/pkcs11/P11Mac.java | 7 +++++++ .../share/classes/sun/security/pkcs11/P11Signature.java | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java index 0671ce9dc40e8..b35c0366df7a8 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java @@ -151,6 +151,13 @@ private void cancelOperation() { try { token.p11.C_SignFinal(session.id(), 0); } catch (PKCS11Exception e) { + if (e.getErrorCode() == CKR_OPERATION_NOT_INITIALIZED) { + // Cancel Operation may be invoked after an error on a PKCS#11 + // call. If the operation inside the token was already cancelled, + // do not fail here. This is part of a defensive mechanism for + // PKCS#11 libraries that do not strictly follow the standard. + return; + } throw new ProviderException("Cancel failed", e); } } diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java index 7b276e7a45c4d..10c720fadc15f 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java @@ -314,6 +314,13 @@ private void cancelOperation() { } } } catch (PKCS11Exception e) { + if (e.getErrorCode() == CKR_OPERATION_NOT_INITIALIZED) { + // Cancel Operation may be invoked after an error on a PKCS#11 + // call. If the operation inside the token was already cancelled, + // do not fail here. This is part of a defensive mechanism for + // PKCS#11 libraries that do not strictly follow the standard. + return; + } if (mode == M_VERIFY) { long errorCode = e.getErrorCode(); if ((errorCode == CKR_SIGNATURE_INVALID) || From ee90166eacb2dd05bf4e9ef95472fdaec4fbb7be Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Fri, 15 Jan 2021 15:33:11 -0300 Subject: [PATCH 11/15] More consistent documentation about Cancel Operation in P11-services. --- .../share/classes/sun/security/pkcs11/P11Mac.java | 5 +++++ .../share/classes/sun/security/pkcs11/P11Signature.java | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java index b35c0366df7a8..04d904d52076e 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java @@ -220,6 +220,11 @@ protected byte[] engineDoFinal() { ensureInitialized(); return token.p11.C_SignFinal(session.id(), 0); } catch (PKCS11Exception e) { + // As per the PKCS#11 standard, C_SignFinal may only + // keep the operation active on CKR_BUFFER_TOO_SMALL errors or + // successful calls to determine the output length. However, + // these cases are handled at OpenJDK's libj2pkcs11 native + // library. Thus, doCancel can safely be 'false' here. throw new ProviderException("doFinal() failed", e); } finally { reset(false); diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java index 10c720fadc15f..a52b4a9c49d58 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java @@ -662,10 +662,10 @@ protected byte[] engineSign() throws SignatureException { } } catch (PKCS11Exception pe) { // As per the PKCS#11 standard, C_Sign and C_SignFinal may only - // keep the operation active on CKR_BUFFER_TOO_SMALL errors. - // However, this type of error is prevented from reaching this - // point at OpenJDK's libj2pkcs11 native library. Thus, doCancel - // can safely be 'false' here. + // keep the operation active on CKR_BUFFER_TOO_SMALL errors or + // successful calls to determine the output length. However, + // these cases are handled at OpenJDK's libj2pkcs11 native + // library. Thus, doCancel can safely be 'false' here. doCancel = false; throw new ProviderException(pe); } catch (SignatureException | ProviderException e) { From 0f96ddf1b10806a8d362b291bc25024d22a64626 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Fri, 15 Jan 2021 15:47:34 -0300 Subject: [PATCH 12/15] Minor documentation improvement in P11Mac regarding Cancel Operation --- .../share/classes/sun/security/pkcs11/P11Mac.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java index 04d904d52076e..1cc35ad451ff1 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java @@ -224,7 +224,8 @@ protected byte[] engineDoFinal() { // keep the operation active on CKR_BUFFER_TOO_SMALL errors or // successful calls to determine the output length. However, // these cases are handled at OpenJDK's libj2pkcs11 native - // library. Thus, doCancel can safely be 'false' here. + // library. Thus, P11Mac::reset can be called with a 'false' + // doCancel argument from here. throw new ProviderException("doFinal() failed", e); } finally { reset(false); From 4c892a44944ef4f17da219768549c496917034f9 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Fri, 15 Jan 2021 17:21:43 -0300 Subject: [PATCH 13/15] Removing the encryption-update path in CancelMultipart test as it depends on a know bug to cause a PKCS#11 error. --- .../pkcs11/Cipher/CancelMultipart.java | 24 ++----------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java b/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java index de81de6e7c16c..122e6ad5c3c39 100644 --- a/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java +++ b/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java @@ -62,7 +62,7 @@ private void leakAndTry() throws Exception { type.doOperation(cipher, (op instanceof LeakDecrypt ? LeakInputType.DECRYPT_MODE : - LeakInputType.ENCRYPT_MODE)); + null)); throw new Exception("PKCS11Exception expected, invalid block" + "size"); } catch (ProviderException | IllegalBlockSizeException e) { @@ -79,8 +79,7 @@ private static interface LeakOperation { } private static interface LeakInputType { - static int ENCRYPT_MODE = 1; - static int DECRYPT_MODE = 2; + static int DECRYPT_MODE = 1; void doOperation(Cipher cipher, int mode) throws Exception; } @@ -93,22 +92,11 @@ public Cipher getCipher() throws Exception { } } - private static class LeakEncrypt implements LeakOperation { - public Cipher getCipher() throws Exception { - Cipher cipher = Cipher.getInstance( - "AES/ECB/NoPadding", provider); - cipher.init(Cipher.ENCRYPT_MODE, key); - return cipher; - } - } - private static class LeakByteBuffer implements LeakInputType { public void doOperation(Cipher cipher, int mode) throws Exception { if (mode == DECRYPT_MODE) { cipher.update(ByteBuffer.allocate(1), ByteBuffer.allocate(1)); cipher.doFinal(ByteBuffer.allocate(0), ByteBuffer.allocate(1)); - } else { - cipher.update(ByteBuffer.allocate(1), ByteBuffer.allocate(2)); } } } @@ -118,8 +106,6 @@ public void doOperation(Cipher cipher, int mode) throws Exception { if (mode == DECRYPT_MODE) { cipher.update(new byte[1]); cipher.doFinal(new byte[1], 0, 0); - } else { - cipher.update(new byte[1]); } } } @@ -134,12 +120,6 @@ public void main(Provider p) throws Exception { // Try multiple paths: - executeTest(new SessionLeaker(new LeakEncrypt(), new LeakByteArray()), - "P11Cipher::implUpdate(byte[], int, int, byte[], int, int)"); - - executeTest(new SessionLeaker(new LeakEncrypt(), new LeakByteBuffer()), - "P11Cipher::implUpdate(ByteBuffer, ByteBuffer)"); - executeTest(new SessionLeaker(new LeakDecrypt(), new LeakByteArray()), "P11Cipher::implDoFinal(byte[], int, int)"); From 65dcc0dfe4c92feeb8a4bb95eaaef5375c92cd53 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Wed, 20 Jan 2021 10:08:28 -0300 Subject: [PATCH 14/15] Copyright dates updated to 2021 on modified files --- .../share/classes/sun/security/pkcs11/P11AEADCipher.java | 2 +- .../share/classes/sun/security/pkcs11/P11Mac.java | 2 +- .../share/classes/sun/security/pkcs11/P11PSSSignature.java | 2 +- .../share/classes/sun/security/pkcs11/P11Signature.java | 2 +- test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java index b6d9e1934dfa7..7913d755d4e49 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2021, 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 diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java index 1cc35ad451ff1..29b26651c39fa 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2021, 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 diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java index 9e797fba997da..8221435266834 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2021, 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 diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java index a52b4a9c49d58..f1366b46a8195 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2021, 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 diff --git a/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java b/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java index 122e6ad5c3c39..28f3699050cbb 100644 --- a/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java +++ b/test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, Red Hat, Inc. + * Copyright (c) 2021, Red Hat, Inc. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it From bb52a06491b0c694cab1483084d3bf1f00446415 Mon Sep 17 00:00:00 2001 From: Martin Balao Date: Wed, 20 Jan 2021 10:13:23 -0300 Subject: [PATCH 15/15] Align doCancel pattern in 'P11Cipher::implDoFinal(byte[]..' to 'P11Cipher::implDoFinal(ByteBuffer..'. Better documentation in P11Cipher. Copyright date updated. --- .../sun/security/pkcs11/P11Cipher.java | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java index 3789e37b0cce1..362d46733dc12 100644 --- a/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java +++ b/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2021, 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 @@ -635,6 +635,10 @@ private int implUpdate(byte[] in, int inOfs, int inLen, throw (ShortBufferException) (new ShortBufferException().initCause(e)); } + // Some implementations such as the NSS Software Token do not + // cancel the operation upon a C_EncryptUpdate/C_DecryptUpdate + // failure (as required by the PKCS#11 standard). See JDK-8258833 + // for further information. reset(true); throw new ProviderException("update() failed", e); } @@ -753,6 +757,10 @@ private int implUpdate(ByteBuffer inBuffer, ByteBuffer outBuffer) throw (ShortBufferException) (new ShortBufferException().initCause(e)); } + // Some implementations such as the NSS Software Token do not + // cancel the operation upon a C_EncryptUpdate/C_DecryptUpdate + // failure (as required by the PKCS#11 standard). See JDK-8258833 + // for further information. reset(true); throw new ProviderException("update() failed", e); } @@ -777,9 +785,14 @@ private int implDoFinal(byte[] out, int outOfs, int outLen) 0, padBuffer, 0, actualPadLen, 0, out, outOfs, outLen); } + // Some implementations such as the NSS Software Token do not + // cancel the operation upon a C_EncryptUpdate failure (as + // required by the PKCS#11 standard). Cancel is not needed + // only after this point. See JDK-8258833 for further + // information. + doCancel = false; k += token.p11.C_EncryptFinal(session.id(), 0, out, (outOfs + k), (outLen - k)); - doCancel = false; } else { // Special handling to match SunJCE provider behavior if (bytesBuffered == 0 && padBufferLen == 0) { @@ -791,17 +804,22 @@ private int implDoFinal(byte[] out, int outOfs, int outLen) padBuffer, 0, padBufferLen, 0, padBuffer, 0, padBuffer.length); } + // Some implementations such as the NSS Software Token do not + // cancel the operation upon a C_DecryptUpdate failure (as + // required by the PKCS#11 standard). Cancel is not needed + // only after this point. See JDK-8258833 for further + // information. + doCancel = false; k += token.p11.C_DecryptFinal(session.id(), 0, padBuffer, k, padBuffer.length - k); - doCancel = false; int actualPadLen = paddingObj.unpad(padBuffer, k); k -= actualPadLen; System.arraycopy(padBuffer, 0, out, outOfs, k); } else { + doCancel = false; k = token.p11.C_DecryptFinal(session.id(), 0, out, outOfs, outLen); - doCancel = false; } } return k; @@ -851,6 +869,11 @@ private int implDoFinal(ByteBuffer outBuffer) 0, padBuffer, 0, actualPadLen, outAddr, outArray, outOfs, outLen); } + // Some implementations such as the NSS Software Token do not + // cancel the operation upon a C_EncryptUpdate failure (as + // required by the PKCS#11 standard). Cancel is not needed + // only after this point. See JDK-8258833 for further + // information. doCancel = false; k += token.p11.C_EncryptFinal(session.id(), outAddr, outArray, (outOfs + k), (outLen - k)); @@ -867,6 +890,11 @@ private int implDoFinal(ByteBuffer outBuffer) 0, padBuffer, 0, padBuffer.length); padBufferLen = 0; } + // Some implementations such as the NSS Software Token do not + // cancel the operation upon a C_DecryptUpdate failure (as + // required by the PKCS#11 standard). Cancel is not needed + // only after this point. See JDK-8258833 for further + // information. doCancel = false; k += token.p11.C_DecryptFinal(session.id(), 0, padBuffer, k, padBuffer.length - k);