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

classic Classic list List threaded Threaded
3 messages Options
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