Fix the P11Cipher AES/ECB/PKCS5Padding array index out of range issue #576

taoliult · 2022-09-14T00:06:55Z

Fix the issue https://github.ibm.com/runtimes/backlog/issues/853, when running CipherOutputStream tests in FIPS mode.

This issue happens when using Cipher Algorithm AES as parameter to initialize the P11Cipher, it will by default, initialize the P11Cipher with "AES/ECB/PKCS5Padding”, which failed due to ArrayIndexOutOfBoundsException.

In P11Cipher method “implUpdate()”, when there is no native padding support, like for AES-ECB, it will then go into its own padding implementation. And because the method is called for each byte from testing code ByteArrayInputStream for AES-ECB, so the “inLen = 1”, “padBuffer.length = 16” and “padBufferLen” start from 0 and increase by 1 when each implUpdate() invoke due to the “inLen = 1”. So, “bufCapacity” will from 16-1=15 to 16-16=0, for 16 times loop. And in the problem code, there is one more time when the “padBufferLen == 0”, so totally, there will be 16+1=17 times, which is wrong and cause the issue. For block size 16, there should be multiple of 16 times calls in the below part of codes.

        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;
        }

So, I change the “ if (inLen > bufCapacity)” to “ if (inLen >= bufCapacity)”, the in loop times change to 15, then the totally calls change to 16 for each block size 16 for AES.

It works for the failed test cases. And I also tried using the byte[] directly instead of the ByteArrayInputStream, it also works in the test case. Following are the Grinder tests results after the code fix.

https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/27352/
https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/27353/
https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/27455/
https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/27456/
https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/27457/

And for why in non-FIPS testing, we did not found this issue, that is because in the non-FIPS testing, SunJCE provider will be used instead of the SunPKCS11.

Signed-off-by: Tao Liu [email protected]

pshipton · 2022-09-14T00:13:19Z

@alon-sh pls review

pshipton · 2022-09-14T00:13:53Z

Pls add an IBM copyright to the modified file.

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

keithc-ca · 2022-09-23T19:18:00Z

@alon-sh Can you please share your opinions on this?

alon-sh · 2022-09-24T17:11:48Z

@taoliult i dont understand this fix - if you say the padding buffer is written to 17 times and u changed it so its 16 times. how come was it not failing when trying to buffer 17 times due to the padding buffer being only 16 bytes long? why did it only fail in the final() call?
also, the error in the original issue is
Java.lang.ArrayIndexOutOfBoundsException: Array index out of range: 32
how is that error related to the issue you were pointing out?
please explain how does your change fixes the original issue

alon-sh · 2022-09-28T19:26:24Z

@taoliult any update?

taoliult · 2022-09-29T13:55:18Z

@alon-sh

The test case will read each byte from the ByteArrayInputStream, and the ByteArrayInputStream come from a byte array, which size is 3 times of the blockSize, that is 3*16=48 for AES. And then, it calls the CipherOutputStream’s write and flush method. And when running the CipherOutputStream.write(), it will finally call the method "implUpdate(byte[] in, int inOfs, int inLen, byte[] out, int outOfs, int outLen)" in P11Cipher.

And in implUpdate() method, for AES/ECB/PKCS5Padding, it will go into the below codes, because there is no native padding, it will use the P11Cipher its own padding implementation.

            if (padBufferLen != 0) {                                                          // line 604
                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;
            }

When call the write method for the first byte, padBufferLen == 0, so it will not fall into the above "if" condition. And when call for the second byte, padBufferLen == 1, then it will fall into the above "if" condition.

"inLen" is always 1 because the test code call the write method for each byte.

"padBuffer.length" is 16 because the AES block size is 16.

"padBufferLen" will increase 1 each time when the test code call the write method for each byte. So, it will go from 1 to 16, the 16 times. And when padBufferLen is 16, bufCapacity will be 16-16=0, then the bytes in buffer will be processed and the padBufferLen will be set to 0 again.

"bufCapacity" will from 16-1=15 to 16-16=0, totally 16 times.

And why 16 times, because if (inLen > bufCapacity), and inLen is alway 1 in this test case, bufCapacity will from 15 to 0, 16 times. And then, 1 > 0 is true, so the byte will not be copy into the buffer array, instead, those 16 bytes will be processed.

And, before go into the "if" condition, there is one byte be processed when padBufferLen == 0. So, if there are 17 bytes, all of these 17 bytes will be processed. But if there are 16 bytes, then only the first byte will be processed, all the other 15 bytes will be copy into a buffer array without be processed. And at this time, because the padBufferLen will increase 1 after the above codes, so after the last byte copy into the buffer array, the padBufferLen will become 16.

And for the error “ArrayIndexOutOfBoundsException: Array index out of range: 32”, it happens when call the setPaddingBytes() method from line 808 to line 95, codes below.

            if (paddingObj != null) {
                int startOff = 0;
                if (reqBlockUpdates) {
                    startOff = padBufferLen;
                }
                int actualPadLen = paddingObj.setPaddingBytes(padBuffer,     // line 808
                        startOff, requiredOutLen - bytesBuffered);
                k = token.p11.C_EncryptUpdate(session.id(),
                        0, padBuffer, 0, startOff + actualPadLen,
                        0, out, outOfs, outLen);
            }


    public int setPaddingBytes(byte[] paddingBuffer, int startOff, int padLen) {
        Arrays.fill(paddingBuffer, startOff, startOff + padLen, (byte) (padLen & 0x007f));     // line 95
        return padLen;
    }

The padBuffer length is 16, startOff is 16, and requiredOutLen - bytesBuffered is 16. And there is an out of range check in the Arrays.fill() method, so if the startOff + requiredOutLen - bytesBuffered > padBuffer length, that mean 16 + 16 > 16 is true, then the out of range error happens.

And why startOff is 16, because “startOff = padBufferLen;” and padBufferLen is 16 when input byte array size is 16.

So, my understanding, when AES block size is 16, that mean, the 16 or the multiple of 16 can be processed. But in the current code, 17 bytes can be processed but not 16.

@alon-sh please advice.

keithc-ca · 2023-01-24T19:24:14Z

Once we agree on a solution here, we will want equivalent changes for

taoliult · 2023-01-25T14:01:14Z

JDK11 Build:
https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/15375/

Grinder tests without enable FIPS:
sanity.jck: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30626/
extended.jck: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30655/
special.jck: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30628/
sanity.openjdk: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30629/
extended.openjdk: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30630/

Grinder tests with enable FIPS:
sanity.jck: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30644/
extended.jck: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30656/
special.jck: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30645/
sanity.openjdk: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30646/
extended.openjdk: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/30653/

keithc-ca · 2023-01-24T19:21:16Z

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

@@ -604,7 +611,7 @@ private int implUpdate(byte[] in, int inOfs, int inLen,
                if (padBufferLen != 0) {
                    if (padBufferLen != padBuffer.length) {
                        int bufCapacity = padBuffer.length - padBufferLen;
-                        if (inLen > bufCapacity) {
+                        if (inLen >= bufCapacity) {


If I understand your explanation of the problem, we have the same issue on line 715 in implUpdate(ByteBuffer inBuffer, ByteBuffer outBuffer).

@keithc-ca Working on it, will update shortly.

taoliult · 2023-02-07T19:25:24Z

Convert this PR back to draft, since need more time to investigate.

Signed-off-by: Tao Liu <[email protected]>

pshipton requested a review from keithc-ca September 14, 2022 00:13

taoliult force-pushed the bugfix branch from e722e6c to f39ceaa Compare September 14, 2022 00:27

keithc-ca requested changes Sep 14, 2022

View reviewed changes

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java Outdated Show resolved Hide resolved

taoliult force-pushed the bugfix branch from 3103ebb to 7ed1d8a Compare September 14, 2022 21:34

keithc-ca approved these changes Sep 15, 2022

View reviewed changes

taoliult force-pushed the bugfix branch 2 times, most recently from fcdcb26 to ca0d683 Compare January 24, 2023 14:46

keithc-ca requested changes Jan 25, 2023

View reviewed changes

taoliult marked this pull request as draft February 7, 2023 19:24

Fix the P11Cipher AES/ECB/PKCS5Padding array index out of range issue

98f91aa

Signed-off-by: Tao Liu <[email protected]>

taoliult force-pushed the bugfix branch from ca0d683 to 98f91aa Compare February 10, 2023 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the P11Cipher AES/ECB/PKCS5Padding array index out of range issue #576

Fix the P11Cipher AES/ECB/PKCS5Padding array index out of range issue #576

taoliult commented Sep 14, 2022 •

edited by pshipton

Loading

pshipton commented Sep 14, 2022

pshipton commented Sep 14, 2022

keithc-ca commented Sep 23, 2022

alon-sh commented Sep 24, 2022

alon-sh commented Sep 28, 2022

taoliult commented Sep 29, 2022

keithc-ca commented Jan 24, 2023

taoliult commented Jan 25, 2023

keithc-ca Jan 24, 2023

taoliult Feb 7, 2023

taoliult commented Feb 7, 2023

Fix the P11Cipher AES/ECB/PKCS5Padding array index out of range issue #576

Are you sure you want to change the base?

Fix the P11Cipher AES/ECB/PKCS5Padding array index out of range issue #576

Conversation

taoliult commented Sep 14, 2022 • edited by pshipton Loading

pshipton commented Sep 14, 2022

pshipton commented Sep 14, 2022

keithc-ca commented Sep 23, 2022

alon-sh commented Sep 24, 2022

alon-sh commented Sep 28, 2022

taoliult commented Sep 29, 2022

keithc-ca commented Jan 24, 2023

taoliult commented Jan 25, 2023

keithc-ca Jan 24, 2023

Choose a reason for hiding this comment

taoliult Feb 7, 2023

Choose a reason for hiding this comment

taoliult commented Feb 7, 2023

taoliult commented Sep 14, 2022 •

edited by pshipton

Loading