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

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Martin Balao-2
Hi,

I'd like to propose a fix for JDK-8261355 [1].

The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.

No regressions observed in jdk/sun/security/pkcs11.

Thanks,
Martin.-

--
[1] - https://bugs.openjdk.java.net/browse/JDK-8261355

-------------

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

Changes: https://git.openjdk.java.net/jdk/pull/2510/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2510&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261355
  Stats: 180 lines in 2 files changed: 105 ins; 27 del; 48 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2510.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2510/head:pull/2510

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

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

Valerie Peng-2
On Wed, 10 Feb 2021 15:06:35 GMT, Martin Balao <[hidden email]> wrote:

> Hi,
>
> I'd like to propose a fix for JDK-8261355 [1].
>
> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>
> No regressions observed in jdk/sun/security/pkcs11.
>
> Thanks,
> Martin.-
>
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355

I will take a look.
Thanks~

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

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

Valerie Peng-2
In reply to this post by Martin Balao-2
On Wed, 10 Feb 2021 15:06:35 GMT, Martin Balao <[hidden email]> wrote:

> Hi,
>
> I'd like to propose a fix for JDK-8261355 [1].
>
> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>
> No regressions observed in jdk/sun/security/pkcs11.
>
> Thanks,
> Martin.-
>
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355

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

> 593:                     // NSS throws up when called with data not in multiple
> 594:                     // of blocks. Try to work around this by holding the
> 595:                     // extra data in padBuffer.

Well, I am not sure if other PKCS#11 libraries are like NSS which requires input size to be multiple of blocks for every multi-part encryption/decryption calls. We are paying the cost of buffering non-blocksize data ourselves and the associated byte copying as a result. Oh-well.

With this change, you should also update the implDoFinal() impl which calls paddingObj.setPaddingBytes(byte[], int) for encryption and writes the padding bytes "after" the existing buffered bytes, i.e. padBufferLen. Otherwise, the existing buffered bytes may be overwritten w/ padding bytes and things will fail. The new regression test should cover this scenario also. It currently only tests the changes made to update() calls.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Martin Balao-2
In reply to this post by Martin Balao-2
> Hi,
>
> I'd like to propose a fix for JDK-8261355 [1].
>
> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>
> No regressions observed in jdk/sun/security/pkcs11.
>
> Thanks,
> Martin.-
>
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355

Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:

 - Avoid overriding buffered bytes with padding in the doFinal call.
 - Only do encryption block-size buffering for NSS
 - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

-------------

Changes: https://git.openjdk.java.net/jdk/pull/2510/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2510&range=01
  Stats: 245 lines in 2 files changed: 164 ins; 23 del; 58 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2510.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2510/head:pull/2510

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Martin Balao-2
In reply to this post by Valerie Peng-2
On Tue, 2 Mar 2021 13:16:24 GMT, Valerie Peng <[hidden email]> wrote:

>> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - Avoid overriding buffered bytes with padding in the doFinal call.
>>  - Only do encryption block-size buffering for NSS
>>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 595:
>
>> 593:                     // NSS throws up when called with data not in multiple
>> 594:                     // of blocks. Try to work around this by holding the
>> 595:                     // extra data in padBuffer.
>
> Well, I am not sure if other PKCS#11 libraries are like NSS which requires input size to be multiple of blocks for every multi-part encryption/decryption calls. We are paying the cost of buffering non-blocksize data ourselves and the associated byte copying as a result. Oh-well.
>
> With this change, you should also update the implDoFinal() impl which calls paddingObj.setPaddingBytes(byte[], int) for encryption and writes the padding bytes "after" the existing buffered bytes, i.e. padBufferLen. Otherwise, the existing buffered bytes may be overwritten w/ padding bytes and things will fail. The new regression test should cover this scenario also. It currently only tests the changes made to update() calls.

I've pushed a new proposal to limit the performance impact of Java-side buffering to the NSS library. This adds to the previous conditions: the operation has to be encryption and the mechanism must not have native padding. If we realize in the future that other libraries are affected as well, we can easily extend the scope.

In regards to the implDoFinal bug, well spotted! Fixed in this new proposal and the test has been enhanced to cover not only this case but also different padding sizes and different block numbers.

Branch rebased (today) to the latest master.

Look forward to your comments.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

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

> 263:                 // NSS requires block-sized updates in multi-part operations.
> 264:                 reqBlockUpdates = ((tokenLabel[0] == 'N' && tokenLabel[1] == 'S'
> 265:                         && tokenLabel[2] == 'S') ? true : false);

IIRC, depending on how the impl is registered, engineSetPadding(String) may not always be called. It's probably safer to set this in engineInit(...)?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

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

> 637:                     // returned to the caller. None of this is necessary for
> 638:                     // encryption: encrypted data can be safely returned upon a
> 639:                     // implUpdate call.

nit: all of the "unencrypted" -> "decrypted". I think this is a bit too verbose? Could we trim it down more, e.g. for decrypting with update() calls, up to a block of input is held inside padBuffer as it may contain padding bytes when no more data is supplied when doFinal() is called.

It should be clear that this does not apply for encryption, so there should be no need to state that.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

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

> 652:             }
> 653:             // update 'padBuffer' if using our own padding impl.
> 654:             if (paddingObj != null) {

nit: if (paddingObj != null && newPadBufferLen > 0)?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

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

> 604:                     // NSS throws up when called with data not in multiple
> 605:                     // of blocks. Try to work around this by holding the
> 606:                     // extra data in padBuffer.

nit: The comment looks a little bit strange. This particular block of code is for handling existing buffered data buffered in earlier update() calls. The comment however is more about 'reqBlockUpdates' itself. How about merging this with the comment for 'reqBlockUpdates' field and then changing this comment to what this particular block of code does.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

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

> 817:                     int startOff = 0;
> 818:                     if (reqBlockUpdates) {
> 819:                         startOff = bytesBuffered;

Shouldn't the starting offset be the number of bytes in padBuffer, i.e. padBufferLen?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
On Tue, 30 Mar 2021 22:10:11 GMT, Valerie Peng <[hidden email]> wrote:

>> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - Avoid overriding buffered bytes with padding in the doFinal call.
>>  - Only do encryption block-size buffering for NSS
>>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 819:
>
>> 817:                     int startOff = 0;
>> 818:                     if (reqBlockUpdates) {
>> 819:                         startOff = bytesBuffered;
>
> Shouldn't the starting offset be the number of bytes in padBuffer, i.e. padBufferLen?

Then no need for the assert(...) to check the starting offset value.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

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

> 908:                 if (paddingObj != null) {
> 909:                     int actualPadLen = paddingObj.setPaddingBytes(padBuffer,
> 910:                             0, requiredOutLen - bytesBuffered);

Shouldn't the starting offset be 'padBufferLen'?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

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

> 910:                             0, requiredOutLen - bytesBuffered);
> 911:                     k = token.p11.C_EncryptUpdate(session.id(),
> 912:                             0, padBuffer, 0, actualPadLen,

actualPadLen => actualPadLen + startOfs?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

test/jdk/sun/security/pkcs11/Cipher/EncryptionPadding.java line 68:

> 66:         ByteBuffer cipherText =
> 67:                 ByteBuffer.allocate(((inputSize / 16 ) + 1) * 16);
> 68:         byte[] tmp = new byte[16];

Seems no need to do new byte[] given how it's used.

test/jdk/sun/security/pkcs11/Cipher/EncryptionPadding.java line 78:

> 76:                         updateLength);
> 77:                 if (tmp != null)
> 78:                     cipherText.put(tmp);

nit: either use "{ }" or move cipherText.put() call to the same line as if-check.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Valerie Peng-2
In reply to this post by Martin Balao-2
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao <[hidden email]> wrote:

>> Hi,
>>
>> I'd like to propose a fix for JDK-8261355 [1].
>>
>> The scheme used for holding data and padding while performing encryption operations is almost the same than the existing one for decryption. The only difference is that encryption does not require a block-sized buffer to be always held because there is no need, upon an update call, to determine which bytes are real output for the caller and which are padding -as it's required for decryption-. I added a couple of comments in implUpdate to explain this.
>>
>> No regressions observed in jdk/sun/security/pkcs11.
>>
>> Thanks,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355
>
> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Avoid overriding buffered bytes with padding in the doFinal call.
>  - Only do encryption block-size buffering for NSS
>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding

test/jdk/sun/security/pkcs11/Cipher/EncryptionPadding.java line 97:

> 95:             throw new Exception("Cross-provider cipher test failed.");
> 96:         }
> 97:     }

Why not just use the byte[] forms for the Cipher.doFinal() and simplify this part, i.e. line 86-96?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Martin Balao-2
In reply to this post by Valerie Peng-2
On Tue, 30 Mar 2021 20:51:26 GMT, Valerie Peng <[hidden email]> wrote:

>> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>>
>>  - Avoid overriding buffered bytes with padding in the doFinal call.
>>  - Only do encryption block-size buffering for NSS
>>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 265:
>
>> 263:                 // NSS requires block-sized updates in multi-part operations.
>> 264:                 reqBlockUpdates = ((tokenLabel[0] == 'N' && tokenLabel[1] == 'S'
>> 265:                         && tokenLabel[2] == 'S') ? true : false);
>
> IIRC, depending on how the impl is registered, engineSetPadding(String) may not always be called. It's probably safer to set this in engineInit(...)?

Looks to me that engineSetPadding is always called from the P11Cipher constructor. I thought that was a good location to set the reqBlockUpdates variable because it's next to the paddingObj initialization; which is a pre-requisite for reqBlockUpdates to be used. In other words, if we have no Java-side padding (paddingObj == null), reqBlockUpdates won't be used and we don't even pay the price of setting it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Martin Balao-2
In reply to this post by Valerie Peng-2
On Tue, 30 Mar 2021 21:19:38 GMT, Valerie Peng <[hidden email]> wrote:

>> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - Avoid overriding buffered bytes with padding in the doFinal call.
>>  - Only do encryption block-size buffering for NSS
>>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 639:
>
>> 637:                     // returned to the caller. None of this is necessary for
>> 638:                     // encryption: encrypted data can be safely returned upon a
>> 639:                     // implUpdate call.
>
> nit: all of the "unencrypted" -> "decrypted". I think this is a bit too verbose? Could we trim it down more, e.g. for decrypting with update() calls, up to a block of input is held inside padBuffer as it may contain padding bytes when no more data is supplied when doFinal() is called.
>
> It should be clear that this does not apply for encryption, so there should be no need to state that.

I'll replace "unencrypted" with "decrypted" and remove the comment about this not being necessary for encryption. I've also trimmed and improved my comment a bit: "While decrypting with implUpdate, the current encrypted block is always held in a buffer. If it's the last one (unknown at this point), it may contain padding bytes and need further processing. In implDoFinal (where we know it's the last one) the buffer is decrypted, unpadded and returned.". One comment about your suggestion: it's block-sized, not 'up to a block size'. But sounded a bit confusing to me overall, so if possible I'd stick to something along the lines above.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Martin Balao-2
In reply to this post by Valerie Peng-2
On Tue, 30 Mar 2021 21:24:37 GMT, Valerie Peng <[hidden email]> wrote:

>> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - Avoid overriding buffered bytes with padding in the doFinal call.
>>  - Only do encryption block-size buffering for NSS
>>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 654:
>
>> 652:             }
>> 653:             // update 'padBuffer' if using our own padding impl.
>> 654:             if (paddingObj != null) {
>
> nit: if (paddingObj != null && newPadBufferLen > 0)?

Yes, makes sense. I've replaced the other " newPadBufferLen != 0" with "newPadBufferLen > 0" to be consistent.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Martin Balao-2
In reply to this post by Valerie Peng-2
On Tue, 30 Mar 2021 21:49:57 GMT, Valerie Peng <[hidden email]> wrote:

>> Martin Balao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - Avoid overriding buffered bytes with padding in the doFinal call.
>>  - Only do encryption block-size buffering for NSS
>>  - 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 606:
>
>> 604:                     // NSS throws up when called with data not in multiple
>> 605:                     // of blocks. Try to work around this by holding the
>> 606:                     // extra data in padBuffer.
>
> nit: The comment looks a little bit strange. This particular block of code is for handling existing buffered data buffered in earlier update() calls. The comment however is more about 'reqBlockUpdates' itself. How about merging this with the comment for 'reqBlockUpdates' field and then changing this comment to what this particular block of code does.

Yes, you are right. I merged the comment with the field description. I believe there is not much to say about that block, though. At least there is nothing new there, except that we may buffer for reqBlockUpdates reasons. If you still want a comment there, let me know and I try to figure out something.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]

Martin Balao-2
In reply to this post by Valerie Peng-2
On Tue, 30 Mar 2021 22:12:16 GMT, Valerie Peng <[hidden email]> wrote:

>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 819:
>>
>>> 817:                     int startOff = 0;
>>> 818:                     if (reqBlockUpdates) {
>>> 819:                         startOff = bytesBuffered;
>>
>> Shouldn't the starting offset be the number of bytes in padBuffer, i.e. padBufferLen?
>
> Then no need for the assert(...) to check the starting offset value.

padBufferLen and bytesBuffered look a bit confusing to me. My suspicion is that they have the same value every time we need them. I'll make the change you suggested and check that we have no regressions. If you believe the assertions are trivial, I'll remove them.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2510
12