Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from

Conversation

@martinuy
Copy link
Contributor

@martinuy martinuy commented Feb 10, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

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

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2510/head:pull/2510
$ git checkout pull/2510

Update a local copy of the PR:
$ git checkout pull/2510
$ git pull https://git.openjdk.java.net/jdk pull/2510/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2510

View PR using the GUI difftool:
$ git pr show -t 2510

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2510.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 10, 2021

👋 Welcome back mbalao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Feb 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 10, 2021

@martinuy The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security label Feb 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 10, 2021

Webrevs

@valeriepeng
Copy link

@valeriepeng valeriepeng commented Feb 17, 2021

I will take a look.
Thanks~

if (padBufferLen != 0) {
// NSS throws up when called with data not in multiple
// of blocks. Try to work around this by holding the
// extra data in padBuffer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@martinuy martinuy Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

char[] tokenLabel = token.tokenInfo.label;
// NSS requires block-sized updates in multi-part operations.
reqBlockUpdates = ((tokenLabel[0] == 'N' && tokenLabel[1] == 'S'
&& tokenLabel[2] == 'S') ? true : false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

// While decrypting with implUpdate, a block-sized buffer with
// encrypted data is always held instead of being unencrypted
// and returned to the caller. This is because the block may
// contain padding bytes, in case it's the last one (unknown
// at this point). In implDoFinal, where we know it's the
// last one, this buffer is unencrypted and unpadded before
// returned to the caller. None of this is necessary for
// encryption: encrypted data can be safely returned upon a
// implUpdate call.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
// update 'padBuffer' if using our own padding impl.
if (paddingObj != null) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -779,10 +814,18 @@ private int implDoFinal(byte[] out, int outOfs, int outLen)
int k = 0;
if (encrypt) {
if (paddingObj != null) {
int startOff = 0;
if (reqBlockUpdates) {
startOff = bytesBuffered;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I checked this with the debugger now. I'll make the change anyways.

@@ -864,7 +907,7 @@ private int implDoFinal(ByteBuffer outBuffer)
if (encrypt) {
if (paddingObj != null) {
int actualPadLen = paddingObj.setPaddingBytes(padBuffer,
requiredOutLen - bytesBuffered);
0, requiredOutLen - bytesBuffered);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the starting offset be 'padBufferLen'?

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, well spotted.. I forgot to synchronize with the byte[] path.

@@ -864,7 +907,7 @@ private int implDoFinal(ByteBuffer outBuffer)
if (encrypt) {
if (paddingObj != null) {
int actualPadLen = paddingObj.setPaddingBytes(padBuffer,
requiredOutLen - bytesBuffered);
0, requiredOutLen - bytesBuffered);
k = token.p11.C_EncryptUpdate(session.id(),
0, padBuffer, 0, actualPadLen,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actualPadLen => actualPadLen + startOfs?

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I forgot to synchronize with the byte[] path. Thanks.

Arrays.fill(plainText, (byte)(inputSize & 0xFF));
ByteBuffer cipherText =
ByteBuffer.allocate(((inputSize / 16 ) + 1) * 16);
byte[] tmp = new byte[16];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. That was probably a vestige of an intermediate version.

if (tmp != null)
cipherText.put(tmp);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

tmp = sunPKCS11cipher.doFinal();
if (tmp != null)
cipherText.put(tmp);

Cipher sunJCECipher = Cipher.getInstance(transformation, "SunJCE");
sunJCECipher.init(Cipher.DECRYPT_MODE, key);
byte[] sunJCEPlain = sunJCECipher.doFinal(cipherText.array());

if (!Arrays.equals(plainText, sunJCEPlain)) {
throw new Exception("Cross-provider cipher test failed.");
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@martinuy martinuy Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@martinuy
Copy link
Contributor Author

@martinuy martinuy commented Apr 6, 2021

@valeriepeng please take a look at my comments in-line and the new proposal here: b47c03e

Thanks,
Martin.-

@valeriepeng
Copy link

@valeriepeng valeriepeng commented Apr 7, 2021

Sure, will take another look. Thanks!
Valerie

valeriepeng
Copy link

valeriepeng commented on b47c03e Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the last block of the current encrypted data?

valeriepeng
Copy link

valeriepeng commented on b47c03e Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nit as above...

martinuy
Copy link
Contributor

martinuy commented on b47c03e Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll replace 'current' with 'last'; and then replace 'last' with 'final' to mean the one that may contain padding bytes

martinuy
Copy link
Contributor

martinuy commented on b47c03e Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@valeriepeng
Copy link

@valeriepeng valeriepeng commented Apr 7, 2021

Rest of changes look good.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@martinuy This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: valeriep

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 206 new commits pushed to the master branch:

  • 76bd313: 8264872: Dependencies: Migrate to PerfData counters
  • 07c8ff4: 8264871: Dependencies: Miscellaneous cleanups in dependencies.cpp
  • 863feab: 8005295: Use mandated information for printing of repeating annotations
  • f26cd2a: 8264997: Remove SystemDictionary::cache_get
  • 9ebc497: 8264765: BreakIterator sees bogus sentence boundary in parenthesized “i.e.” phrase
  • ec31b3a: 8264727: Shenandoah: Remove extraneous whitespace from phase timings report
  • cc54de7: 8264400: (fs) WindowsFileStore equality depends on how the FileStore was constructed
  • 6de0bb2: 8232861: (fc) FileChannel.force fails on WebDAV file systems (macOS)
  • 1ca4abe: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI
  • 06e6b1f: 8259242: Remove ProtectionDomainSet_lock
  • ... and 196 more: https://git.openjdk.java.net/jdk/compare/a1e717f13ec040e4e1490f70ba465b405471e4ff...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Apr 7, 2021
Copy link

@valeriepeng valeriepeng left a comment

Looks fine, thanks~

@martinuy
Copy link
Contributor Author

@martinuy martinuy commented Apr 12, 2021

/integrate

@martinuy martinuy closed this Apr 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 12, 2021

@martinuy Since your change was applied there have been 225 commits pushed to the master branch:

  • d84a7e5: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records
  • 714ae54: 8258788: incorrect response to change in window insets [lanai]
  • f479437: 8265082: test/hotspot/jtreg/gc/g1/TestG1SkipCompaction.java fails validate-source
  • 27f4b27: 8264623: Change to Xcode 12.4 for building on Macos at Oracle
  • 7c20d97: 8265052: Break circular include dependency in objArrayOop.inline.hpp
  • b90ad76: 8233567: [TESTBUG] FocusSubRequestTest.java fails on macos
  • 125184e: 8265012: Shenandoah: Backout JDK-8264718
  • be0d46c: 8262068: Improve G1 Full GC by skipping compaction for regions with high survival ratio
  • f71be8b: 8264954: unified handling for VectorMask object re-materialization during de-optimization
  • 3c9858d: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded
  • ... and 215 more: https://git.openjdk.java.net/jdk/compare/a1e717f13ec040e4e1490f70ba465b405471e4ff...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1ee80e0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@martinuy martinuy deleted the JDK-8261355 branch May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants