8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE #5236

valeriepeng · 2021-08-24T01:33:42Z

Could someone help review this straight forward change? During the interoperability testing with PKCS11 KW/KWP support, it is noticed that SunJCE provider used the wrong block size (AES: 16) when padding is needed for KW mode. With KW, KWP modes, data block size is multiples of 8-byte, so the padding should pad data to multiples of 8 bytes instead of 16. In addition, although PKCS#11 v3.0 states the IV for KWP mode is 4-byte, NSS's implementation would silently ignore the specified IVs. Thus, for max interoperability, it seems safer to change SunJCE provider to always use the same default IV and disallow custom IVs for KWP mode, at least for now. Regression test is enhanced to test more scenarios.

Thanks,
Valerie

Progress

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

Issue

JDK-8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE

Reviewers

Xue-Lei Andrew Fan (@XueleiFan - Reviewer)
Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5236

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

Using diff file

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

…mode for SunJCE Changed the KW and KWP mode impl of SunJCE provider to use 8-byte cipher block size and fixed the IV value for KWP mode.

bridgekeeper · 2021-08-24T01:34:42Z

👋 Welcome back valeriep! 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 · 2021-08-24T01:38:17Z

@valeriepeng 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.

mlbridge · 2021-08-24T01:43:40Z

Webrevs

00: Full (b5d31611)

seanjmullan · 2021-08-30T19:09:09Z

src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java

     */
    @Override
    protected int engineGetBlockSize() {
-        return cipher.getBlockSize();
+        return 8;


Line 186 still says:

* symmetric cipher whose block size must be 128-bit

Should that also be updated?

Line 186 is correct in that the underlying Cipher block size must be 128-bit. However, the KW/KWP processing affected the input size requirement into 8-byte blocks, thus the overall cipher block size is now 8 instead of 16 bytes.

So the block size is always 8 even when there is no padding?

Yes, with KW (no pad), data must be in multiples of 8. When data does not meet this size requirement, an external padding scheme such as PKCS5/7 padding is needed to pad the data to multiples of 8 in order for KW mode to process the data.
As for KWP mode, it internally pads the data to multiples of 8 before starting the internal processing. Thus, no external padding is needed. Are you asking if 8 should be returned for KWP mode due to its internal padding? KWP is like a variant of KW, so it seems to me that it should return the same block size as KW.

Your explanation make sense. I just wanted to make sure we didn't miss something.

Yes, thanks for checking.

seanjmullan · 2021-08-30T19:13:04Z

Has bug been filed against NSS for ignoring the IV?

XueleiFan

I have no further comment, please check Sean's comment before integration.

openjdk · 2021-08-30T21:32:02Z

@valeriepeng 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:

8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE

Reviewed-by: xuelei, mullan

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 86 new commits pushed to the master branch:

02822e1: 8272377: assert preconditions that are ensured when created in add_final_edges
a58cf16: 8272563: assert(is_double_stack() && !is_virtual()) failed: type check
0e14bf7: 8273176: handle latest VS2019 in abstract_vm_version
f1c5e26: 8273206: jdk/jfr/event/gc/collection/TestG1ParallelPhases.java fails after JDK-8159979
e600fe1: 8272618: Unnecessary Attr.visitIdent.noOuterThisPath
2fce7cb: 8272963: Update the java manpage markdown source
18a731a: 8269770: nsk tests should start IOPipe channel before launch debuggee - Debugee.prepareDebugee
9c392d0: 8273197: ProblemList 2 jtools tests due to JDK-8273187
3d657eb: 8262186: Call X509KeyManager.chooseClientAlias once for all key types
c1e0aac: 8273186: Remove leftover comment about sparse remembered set in G1 HeapRegionRemSet
... and 76 more: https://git.openjdk.java.net/jdk/compare/9bc023220fbbb0b6ea1ed1a0ca2aa3848764e8cd...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.

valeriepeng · 2021-08-31T19:55:58Z

Has bug been filed against NSS for ignoring the IV?

I've just filed https://bugzilla.mozilla.org/show_bug.cgi?id=1728419

valeriepeng · 2021-08-31T19:57:20Z

Thanks Sean and Xuelei for the review and feedbacks! If there are additional comments, please let me know. Otherwise, I will proceed with integration tomorrow...

valeriepeng · 2021-09-01T22:16:53Z

/integrate

openjdk · 2021-09-01T22:17:56Z

Going to push as commit 1a5a2b6.
Since your change was applied there have been 91 commits pushed to the master branch:

2f01a6f: 8273157: Add convenience methods to Messager
9689f61: 8272788: Nonleaf ranked locks should not be safepoint_check_never
4ee0dac: 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs
655ea6d: 8270489: Support archived heap objects in EpsilonGC
dacd197: 8273217: Make ParHeapInspectTask _safepoint_check_never
02822e1: 8272377: assert preconditions that are ensured when created in add_final_edges
a58cf16: 8272563: assert(is_double_stack() && !is_virtual()) failed: type check
0e14bf7: 8273176: handle latest VS2019 in abstract_vm_version
f1c5e26: 8273206: jdk/jfr/event/gc/collection/TestG1ParallelPhases.java fails after JDK-8159979
e600fe1: 8272618: Unnecessary Attr.visitIdent.noOuterThisPath
... and 81 more: https://git.openjdk.java.net/jdk/compare/9bc023220fbbb0b6ea1ed1a0ca2aa3848764e8cd...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-01T22:18:11Z

@valeriepeng Pushed as commit 1a5a2b6.

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

8271745: Correct block size for KW,KWP mode and use fixed IV for KWP …

b5d3161

…mode for SunJCE Changed the KW and KWP mode impl of SunJCE provider to use 8-byte cipher block size and fixed the IV value for KWP mode.

openjdk bot added the rfr Pull request is ready for review label Aug 24, 2021

openjdk bot added the security security-dev@openjdk.org label Aug 24, 2021

seanjmullan reviewed Aug 30, 2021

View reviewed changes

XueleiFan approved these changes Aug 30, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 30, 2021

seanjmullan approved these changes Sep 1, 2021

View reviewed changes

openjdk bot closed this Sep 1, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 1, 2021

valeriepeng deleted the JDK-8271745 branch September 1, 2021 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE #5236

8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE #5236

valeriepeng commented Aug 24, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 24, 2021

openjdk bot commented Aug 24, 2021

mlbridge bot commented Aug 24, 2021

seanjmullan Aug 30, 2021

valeriepeng Aug 31, 2021

seanjmullan Aug 31, 2021

valeriepeng Sep 1, 2021

seanjmullan Sep 1, 2021

valeriepeng Sep 1, 2021

seanjmullan commented Aug 30, 2021

XueleiFan left a comment

openjdk bot commented Aug 30, 2021 •

edited

Loading

valeriepeng commented Aug 31, 2021

valeriepeng commented Aug 31, 2021

valeriepeng commented Sep 1, 2021

openjdk bot commented Sep 1, 2021

openjdk bot commented Sep 1, 2021

8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE #5236

8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE #5236

Conversation

valeriepeng commented Aug 24, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 24, 2021

openjdk bot commented Aug 24, 2021

mlbridge bot commented Aug 24, 2021

Webrevs

seanjmullan Aug 30, 2021

Choose a reason for hiding this comment

valeriepeng Aug 31, 2021

Choose a reason for hiding this comment

seanjmullan Aug 31, 2021

Choose a reason for hiding this comment

valeriepeng Sep 1, 2021

Choose a reason for hiding this comment

seanjmullan Sep 1, 2021

Choose a reason for hiding this comment

valeriepeng Sep 1, 2021

Choose a reason for hiding this comment

seanjmullan commented Aug 30, 2021

XueleiFan left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 30, 2021 • edited Loading

valeriepeng commented Aug 31, 2021

valeriepeng commented Aug 31, 2021

valeriepeng commented Sep 1, 2021

openjdk bot commented Sep 1, 2021

openjdk bot commented Sep 1, 2021

valeriepeng commented Aug 24, 2021 •

edited by openjdk bot

Loading

openjdk bot commented Aug 30, 2021 •

edited

Loading