Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v4]
On Thu, 8 Apr 2021 21:52:43 GMT, Martin Balao 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 incrementally with one additional > commit since the last revision: > > Minor comment enhancement Looks fine, thanks~ - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v4]
> 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 incrementally with one additional commit since the last revision: Minor comment enhancement - Changes: - all: https://git.openjdk.java.net/jdk/pull/2510/files - new: https://git.openjdk.java.net/jdk/pull/2510/files/b47c03ed..fd299156 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2510&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2510&range=02-03 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v3]
On Tue, 6 Apr 2021 18:30:31 GMT, Martin Balao 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 incrementally with one additional > commit since the last revision: > > Bug fixes and improvements as discussed in the PR Marked as reviewed by valeriep (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
On Wed, 7 Apr 2021 16:42:53 GMT, Valerie Peng wrote: >> @valeriepeng please take a look at my comments in-line and the new proposal >> here: >> https://github.com/openjdk/jdk/pull/2510/commits/b47c03edff1f48b925a67203102385470ac1afdc >> >> Thanks, >> Martin.- > > Sure, will take another look. Thanks! > Valerie Rest of changes look good. - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 6 Apr 2021 14:26:00 GMT, Martin Balao wrote: >> 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. Ok. - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
On Tue, 6 Apr 2021 18:29:56 GMT, Martin Balao wrote: >> I will take a look. >> Thanks~ > > @valeriepeng please take a look at my comments in-line and the new proposal > here: > https://github.com/openjdk/jdk/pull/2510/commits/b47c03edff1f48b925a67203102385470ac1afdc > > Thanks, > Martin.- Sure, will take another look. Thanks! Valerie - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
On Wed, 17 Feb 2021 12:22:55 GMT, Valerie Peng 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~ @valeriepeng please take a look at my comments in-line and the new proposal here: https://github.com/openjdk/jdk/pull/2510/commits/b47c03edff1f48b925a67203102385470ac1afdc Thanks, Martin.- - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v3]
> 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 incrementally with one additional commit since the last revision: Bug fixes and improvements as discussed in the PR - Changes: - all: https://git.openjdk.java.net/jdk/pull/2510/files - new: https://git.openjdk.java.net/jdk/pull/2510/files/dd81ec9c..b47c03ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2510&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2510&range=01-02 Stats: 53 lines in 2 files changed: 11 ins; 18 del; 24 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 6 Apr 2021 16:56:49 GMT, Martin Balao wrote:
>> 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?
>
> We are accumulating cipher text in the cipherText local variable while doing
> updates, both for the 'update(byte[]...' and 'update(ByteBuffer...' cases.
> The last call to doFinal will return the last block of cipher text, which we
> need to append. In regards to Cipher::doFinal, we are using the byte[] form
> of it. Please let me know if I'm not understanding your comment correctly.
I've just realized that the test is not exercising the 'doFinal(ByteBuffer..'
path. Thus, why it did not catch the previous sync bugs. I'll fix that.
-
PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Wed, 31 Mar 2021 00:16:28 GMT, Valerie Peng 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 > 912: > >> 910: 0, requiredOutLen - bytesBuffered); >> 911: k = token.p11.C_EncryptUpdate(session.id(), >> 912: 0, padBuffer, 0, actualPadLen, > > actualPadLen => actualPadLen + startOfs? Yes, I forgot to synchronize with the byte[] path. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Wed, 31 Mar 2021 00:07:14 GMT, Valerie Peng 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
> 910:
>
>> 908: if (paddingObj != null) {
>> 909: int actualPadLen =
>> paddingObj.setPaddingBytes(padBuffer,
>> 910: 0, requiredOutLen - bytesBuffered);
>
> Shouldn't the starting offset be 'padBufferLen'?
Oh, yes, well spotted.. I forgot to synchronize with the byte[] path.
-
PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Wed, 31 Mar 2021 17:00:26 GMT, Valerie Peng 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
>
> 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?
We are accumulating cipher text in the cipherText local variable while doing
updates, both for the 'update(byte[]...' and 'update(ByteBuffer...' cases. The
last call to doFinal will return the last block of cipher text, which we need
to append. In regards to Cipher::doFinal, we are using the byte[] form of it.
Please let me know if I'm not understanding your comment correctly.
-
PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Wed, 31 Mar 2021 16:51:31 GMT, Valerie Peng 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
>
> 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.
Right. That was probably a vestige of an intermediate version.
> 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.
Ok
-
PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 30 Mar 2021 22:12:16 GMT, Valerie Peng 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 30 Mar 2021 21:49:57 GMT, Valerie Peng 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 30 Mar 2021 21:24:37 GMT, Valerie Peng 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 30 Mar 2021 21:19:38 GMT, Valerie Peng 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 30 Mar 2021 20:51:26 GMT, Valerie Peng 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 30 Mar 2021 22:10:11 GMT, Valerie Peng 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Thu, 25 Mar 2021 22:13:42 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 2 Mar 2021 13:16:24 GMT, Valerie Peng 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
> 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
On Wed, 10 Feb 2021 15:06:35 GMT, Martin Balao 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
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
On Wed, 10 Feb 2021 15:06:35 GMT, Martin Balao 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
RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
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
