8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

Reviewed-by: valeriep

Author: martinuy

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java

@@ -70,7 +70,7 @@ final class P11Cipher extends CipherSpi {
     private static interface Padding {
         // ENC: format the specified buffer with padding bytes and return the
         // actual padding length
-        int setPaddingBytes(byte[] paddingBuffer, int padLen);
+        int setPaddingBytes(byte[] paddingBuffer, int startOff, int padLen);
 
         // DEC: return the length of trailing padding bytes given the specified
         // padded data
@@ -91,8 +91,8 @@ private static class PKCS5Padding implements Padding {
             this.blockSize = blockSize;
         }
 
-        public int setPaddingBytes(byte[] paddingBuffer, int padLen) {
-            Arrays.fill(paddingBuffer, 0, padLen, (byte) (padLen & 0x007f));
+        public int setPaddingBytes(byte[] paddingBuffer, int startOff, int padLen) {
+            Arrays.fill(paddingBuffer, startOff, startOff + padLen, (byte) (padLen & 0x007f));
             return padLen;
         }
 
@@ -169,6 +169,14 @@ public int unpad(byte[] paddedData, int len)
     // specification mandates a fixed size of the key
     private int fixedKeySize = -1;
 
+    // Indicates whether the underlying PKCS#11 library requires block-sized
+    // updates during multi-part operations. In such case, we buffer data in
+    // padBuffer up to a block-size. This may be needed only if padding is
+    // applied on the Java side. An example of the previous is when the
+    // CKM_AES_ECB mechanism is used and the PKCS#11 library is NSS. See more
+    // on JDK-8261355.
+    private boolean reqBlockUpdates = false;
+
     P11Cipher(Token token, String algorithm, long mechanism)
             throws PKCS11Exception, NoSuchAlgorithmException {
         super();
@@ -252,6 +260,10 @@ protected void engineSetPadding(String padding)
                 // no native padding support; use our own padding impl
                 paddingObj = new PKCS5Padding(blockSize);
                 padBuffer = new byte[blockSize];
+                char[] tokenLabel = token.tokenInfo.label;
+                // NSS requires block-sized updates in multi-part operations.
+                reqBlockUpdates = ((tokenLabel[0] == 'N' && tokenLabel[1] == 'S'
+                        && tokenLabel[2] == 'S') ? true : false);
             }
         } else {
             throw new NoSuchPaddingException("Unsupported padding " + padding);
@@ -587,46 +599,54 @@ private int implUpdate(byte[] in, int inOfs, int inLen,
         try {
             ensureInitialized();
             int k = 0;
-            if (encrypt) {
-                k = token.p11.C_EncryptUpdate(session.id(), 0, in, inOfs, inLen,
-                        0, out, outOfs, outLen);
-            } else {
-                int newPadBufferLen = 0;
-                if (paddingObj != null) {
-                    if (padBufferLen != 0) {
-                        // NSS throws up when called with data not in multiple
-                        // of blocks. Try to work around this by holding the
-                        // extra data in padBuffer.
-                        if (padBufferLen != padBuffer.length) {
-                            int bufCapacity = padBuffer.length - padBufferLen;
-                            if (inLen > bufCapacity) {
-                                bufferInputBytes(in, inOfs, bufCapacity);
-                                inOfs += bufCapacity;
-                                inLen -= bufCapacity;
-                            } else {
-                                bufferInputBytes(in, inOfs, inLen);
-                                return 0;
-                            }
+            int newPadBufferLen = 0;
+            if (paddingObj != null && (!encrypt || reqBlockUpdates)) {
+                if (padBufferLen != 0) {
+                    if (padBufferLen != padBuffer.length) {
+                        int bufCapacity = padBuffer.length - padBufferLen;
+                        if (inLen > bufCapacity) {
+                            bufferInputBytes(in, inOfs, bufCapacity);
+                            inOfs += bufCapacity;
+                            inLen -= bufCapacity;
+                        } else {
+                            bufferInputBytes(in, inOfs, inLen);
+                            return 0;
                         }
+                    }
+                    if (encrypt) {
+                        k = token.p11.C_EncryptUpdate(session.id(),
+                                0, padBuffer, 0, padBufferLen,
+                                0, out, outOfs, outLen);
+                    } else {
                         k = token.p11.C_DecryptUpdate(session.id(),
                                 0, padBuffer, 0, padBufferLen,
                                 0, out, outOfs, outLen);
-                        padBufferLen = 0;
                     }
-                    newPadBufferLen = inLen & (blockSize - 1);
-                    if (newPadBufferLen == 0) {
-                        newPadBufferLen = padBuffer.length;
-                    }
-                    inLen -= newPadBufferLen;
+                    padBufferLen = 0;
+                }
+                newPadBufferLen = inLen & (blockSize - 1);
+                if (!encrypt && newPadBufferLen == 0) {
+                    // While decrypting with implUpdate, the last encrypted block
+                    // is always held in a buffer. If it's the final one (unknown
+                    // at this point), it may contain padding bytes and need further
+                    // processing. In implDoFinal (where we know it's the final one)
+                    // the buffer is decrypted, unpadded and returned.
+                    newPadBufferLen = padBuffer.length;
                 }
-                if (inLen > 0) {
+                inLen -= newPadBufferLen;
+            }
+            if (inLen > 0) {
+                if (encrypt) {
+                    k += token.p11.C_EncryptUpdate(session.id(), 0, in, inOfs,
+                            inLen, 0, out, (outOfs + k), (outLen - k));
+                } else {
                     k += token.p11.C_DecryptUpdate(session.id(), 0, in, inOfs,
                             inLen, 0, out, (outOfs + k), (outLen - k));
                 }
-                // update 'padBuffer' if using our own padding impl.
-                if (paddingObj != null) {
-                    bufferInputBytes(in, inOfs + inLen, newPadBufferLen);
-                }
+            }
+            // update 'padBuffer' if using our own padding impl.
+            if (paddingObj != null && newPadBufferLen > 0) {
+                bufferInputBytes(in, inOfs + inLen, newPadBufferLen);
             }
             bytesBuffered += (inLen - k);
             return k;
@@ -687,60 +707,62 @@ private int implUpdate(ByteBuffer inBuffer, ByteBuffer outBuffer)
             }
 
             int k = 0;
-            if (encrypt) {
-                if (inAddr == 0 && inArray == null) {
-                    inArray = new byte[inLen];
-                    inBuffer.get(inArray);
-                } else {
-                    inBuffer.position(origPos + inLen);
-                }
-                k = token.p11.C_EncryptUpdate(session.id(),
-                        inAddr, inArray, inOfs, inLen,
-                        outAddr, outArray, outOfs, outLen);
-            } else {
-                int newPadBufferLen = 0;
-                if (paddingObj != null) {
-                    if (padBufferLen != 0) {
-                        // NSS throws up when called with data not in multiple
-                        // of blocks. Try to work around this by holding the
-                        // extra data in padBuffer.
-                        if (padBufferLen != padBuffer.length) {
-                            int bufCapacity = padBuffer.length - padBufferLen;
-                            if (inLen > bufCapacity) {
-                                bufferInputBytes(inBuffer, bufCapacity);
-                                inOfs += bufCapacity;
-                                inLen -= bufCapacity;
-                            } else {
-                                bufferInputBytes(inBuffer, inLen);
-                                return 0;
-                            }
+            int newPadBufferLen = 0;
+            if (paddingObj != null  && (!encrypt || reqBlockUpdates)) {
+                if (padBufferLen != 0) {
+                    if (padBufferLen != padBuffer.length) {
+                        int bufCapacity = padBuffer.length - padBufferLen;
+                        if (inLen > bufCapacity) {
+                            bufferInputBytes(inBuffer, bufCapacity);
+                            inOfs += bufCapacity;
+                            inLen -= bufCapacity;
+                        } else {
+                            bufferInputBytes(inBuffer, inLen);
+                            return 0;
                         }
+                    }
+                    if (encrypt) {
+                        k = token.p11.C_EncryptUpdate(session.id(), 0,
+                                padBuffer, 0, padBufferLen, outAddr, outArray,
+                                outOfs, outLen);
+                    } else {
                         k = token.p11.C_DecryptUpdate(session.id(), 0,
                                 padBuffer, 0, padBufferLen, outAddr, outArray,
                                 outOfs, outLen);
-                        padBufferLen = 0;
-                    }
-                    newPadBufferLen = inLen & (blockSize - 1);
-                    if (newPadBufferLen == 0) {
-                        newPadBufferLen = padBuffer.length;
                     }
-                    inLen -= newPadBufferLen;
+                    padBufferLen = 0;
                 }
-                if (inLen > 0) {
-                    if (inAddr == 0 && inArray == null) {
-                        inArray = new byte[inLen];
-                        inBuffer.get(inArray);
-                    } else {
-                        inBuffer.position(inBuffer.position() + inLen);
-                    }
+                newPadBufferLen = inLen & (blockSize - 1);
+                if (!encrypt && newPadBufferLen == 0) {
+                    // While decrypting with implUpdate, the last encrypted block
+                    // is always held in a buffer. If it's the final one (unknown
+                    // at this point), it may contain padding bytes and need further
+                    // processing. In implDoFinal (where we know it's the final one)
+                    // the buffer is decrypted, unpadded and returned.
+                    newPadBufferLen = padBuffer.length;
+                }
+                inLen -= newPadBufferLen;
+            }
+            if (inLen > 0) {
+                if (inAddr == 0 && inArray == null) {
+                    inArray = new byte[inLen];
+                    inBuffer.get(inArray);
+                } else {
+                    inBuffer.position(inBuffer.position() + inLen);
+                }
+                if (encrypt) {
+                    k += token.p11.C_EncryptUpdate(session.id(), inAddr,
+                            inArray, inOfs, inLen, outAddr, outArray,
+                            (outOfs + k), (outLen - k));
+                } else {
                     k += token.p11.C_DecryptUpdate(session.id(), inAddr,
                             inArray, inOfs, inLen, outAddr, outArray,
                             (outOfs + k), (outLen - k));
                 }
-                // update 'padBuffer' if using our own padding impl.
-                if (paddingObj != null && newPadBufferLen != 0) {
-                    bufferInputBytes(inBuffer, newPadBufferLen);
-                }
+            }
+            // update 'padBuffer' if using our own padding impl.
+            if (paddingObj != null && newPadBufferLen > 0) {
+                bufferInputBytes(inBuffer, newPadBufferLen);
             }
             bytesBuffered += (inLen - k);
             if (!(outBuffer instanceof DirectBuffer) &&
@@ -779,10 +801,14 @@ private int implDoFinal(byte[] out, int outOfs, int outLen)
             int k = 0;
             if (encrypt) {
                 if (paddingObj != null) {
+                    int startOff = 0;
+                    if (reqBlockUpdates) {
+                        startOff = padBufferLen;
+                    }
                     int actualPadLen = paddingObj.setPaddingBytes(padBuffer,
-                            requiredOutLen - bytesBuffered);
+                            startOff, requiredOutLen - bytesBuffered);
                     k = token.p11.C_EncryptUpdate(session.id(),
-                            0, padBuffer, 0, actualPadLen,
+                            0, padBuffer, 0, startOff + actualPadLen,
                             0, out, outOfs, outLen);
                 }
                 // Some implementations such as the NSS Software Token do not
@@ -863,10 +889,14 @@ private int implDoFinal(ByteBuffer outBuffer)
 
             if (encrypt) {
                 if (paddingObj != null) {
+                    int startOff = 0;
+                    if (reqBlockUpdates) {
+                        startOff = padBufferLen;
+                    }
                     int actualPadLen = paddingObj.setPaddingBytes(padBuffer,
-                            requiredOutLen - bytesBuffered);
+                            startOff, requiredOutLen - bytesBuffered);
                     k = token.p11.C_EncryptUpdate(session.id(),
-                            0, padBuffer, 0, actualPadLen,
+                            0, padBuffer, 0, startOff + actualPadLen,
                             outAddr, outArray, outOfs, outLen);
                 }
                 // Some implementations such as the NSS Software Token do not

test/jdk/sun/security/pkcs11/Cipher/EncryptionPadding.java

@@ -0,0 +1,104 @@
+/*
+ * 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
+ * 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 8261355
+ * @library /test/lib ..
+ * @run main/othervm EncryptionPadding
+ */
+
+import java.nio.ByteBuffer;
+import java.security.Key;
+import java.security.Provider;
+import java.util.Arrays;
+import javax.crypto.Cipher;
+import javax.crypto.spec.SecretKeySpec;
+
+public class EncryptionPadding extends PKCS11Test {
+
+    private static String transformation = "AES/ECB/PKCS5Padding";
+    private static Key key = new SecretKeySpec(new byte[16], "AES");
+
+    public static void main(String[] args) throws Exception {
+        main(new EncryptionPadding(), args);
+    }
+
+    @Override
+    public void main(Provider p) throws Exception {
+        testWithInputSize(p, 1);
+        testWithInputSize(p, 15);
+        testWithInputSize(p, 16);
+        testWithInputSize(p, 17);
+        System.out.println("TEST PASS - OK");
+    }
+
+    private static void testWithInputSize(Provider p, int inputSize)
+            throws Exception {
+        testWithInputSize(p, inputSize, false);
+        testWithInputSize(p, inputSize, true);
+    }
+
+    private static void testWithInputSize(Provider p, int inputSize,
+            boolean isByteBuffer) throws Exception {
+        byte[] plainText = new byte[inputSize];
+        Arrays.fill(plainText, (byte)(inputSize & 0xFF));
+        ByteBuffer cipherText =
+                ByteBuffer.allocate(((inputSize / 16 ) + 1) * 16);
+        byte[] tmp;
+
+        Cipher sunPKCS11cipher = Cipher.getInstance(transformation, p);
+        sunPKCS11cipher.init(Cipher.ENCRYPT_MODE, key);
+        for (int i = 0; i < ((inputSize - 1) / 16) + 1; i++) {
+            int updateLength = Math.min(inputSize - (16 * i), 16);
+            if (!isByteBuffer) {
+                tmp = sunPKCS11cipher.update(plainText, i * 16,
+                        updateLength);
+                if (tmp != null) {
+                    cipherText.put(tmp);
+                }
+            } else {
+                ByteBuffer bb = ByteBuffer.allocate(updateLength);
+                bb.put(plainText, i * 16, updateLength);
+                bb.flip();
+                sunPKCS11cipher.update(bb, cipherText);
+            }
+        }
+        if (!isByteBuffer) {
+            tmp = sunPKCS11cipher.doFinal();
+            if (tmp != null) {
+                cipherText.put(tmp);
+            }
+        } else {
+            sunPKCS11cipher.doFinal(ByteBuffer.allocate(0), cipherText);
+        }
+
+        Cipher sunJCECipher = Cipher.getInstance(transformation, "SunJCE");
+        sunJCECipher.init(Cipher.DECRYPT_MODE, key);
+        byte[] sunJCEPlain = sunJCECipher.doFinal(cipherText.array());
+
+        if (!Arrays.equals(plainText, sunJCEPlain)) {
+            throw new Exception("Cross-provider cipher test failed.");
+        }
+    }
+}