8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305

[jnimeh]

This fix corrects a problem where ChaCha20-Poly1305 objects prior to init throw NPE when getParameters() is called. It will now generate parameters containing a random nonce on each pre-init call to getParameters(). Post-initialization calls to the getParameters() method will always return the same set of parameters until the next initialization occurs.

---

Progress

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

Issue

• JDK-8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305

Reviewers

• Sean Mullan (@seanjmullan - Reviewer) ⚠️ Review applies to b9a2448
• Valerie Peng (@valeriepeng - Reviewer) ⚠️ Review applies to 0f74545

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1644/head:pull/1644
$ git checkout pull/1644

[bridgekeeper]

👋 Welcome back jnimeh! 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]

@jnimeh 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]

Webrevs

• 03: Full - Incremental (fc2cb52)
• 02: Full - Incremental (0f74545)
• 01: Full - Incremental (b9a2448)
• 00: Full (56fd18c)

[openjdk]

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

8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305

Reviewed-by: mullan, 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 89 new commits pushed to the master branch:

• 6ff18e3: 8257855: Example SafeVarargsNotApplicableToRecordAccessors breaks test tools/javac/diags/CheckExamples.java
• cef606f: 8253762: JFR: getField(String) should be able to access subfields
• 39b8a2e: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks
• c43c224: 8257796: [TESTBUG] TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on x86_32
• 62c7788: 8257211: C2: Enable call devirtualization during post-parse phase
• 149a02f: 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal
• f92745d: 8257718: LogCompilation: late_inline doesnt work right for JDK 8 logs
• 6937d9f: 8257799: Update JLS cross-references in java.compiler
• a5297bd: 8254939: macOS: unused function 'replicate4_imm'
• 36c0600: 8257805: Add compiler/blackhole tests to tier1
• ... and 79 more: https://git.openjdk.java.net/jdk/compare/d80ae05f617b35bd327e03869284de0c41adb94d...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.

[seanjmullan]

The test should probably have the bugid added to it.

[valeriepeng]

Outside of Cipher.getParameters(), there are other direct references of nonce which may lead to NPE which we should fix also. Inside engineGetIV(), it should check for null nonce before directly calling clone().

[mlbridge]

Mailing list message from Bernd Eckenfels on security-dev:

BTW stupid - somewhat related - question, why does the nonce to be parsed out of a DER blob, shouldn?t there be an getter on the Parameter Spec object? Many protocols would need the raw array, is there a matching spec - or should we add one?

Gruss
Bernd
--
http://bernd.eckenfels.net
________________________________
Von: security-dev <security-dev-retn at openjdk.java.net> im Auftrag von Jamil Nimeh <jnimeh at openjdk.java.net>
Gesendet: Monday, December 7, 2020 9:05:16 PM
An: security-dev at openjdk.java.net <security-dev at openjdk.java.net>
Betreff: Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

On Mon, 7 Dec 2020 19:53:27 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision:

pre-init getParameters nonce data is now a local variable

src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 232:

230: // this call should cause a random nonce to be generated, but
231: // not attached to the object.
232: byte[] nonceData = initialized ? nonce : createRandomNonce(null);

The "initialized" variable is set to false in engineDoFinal() call. So, if users call getParameters() after finish cipher operation, this will return random nonces instead of the one used in previous doFinal operation. Will this be a little un-intuitive?

Unintuitive is a charitable way to put it. After doFinal the Cipher technically isn't in an uninitialized state per the spec, it's supposed to be in the state it would be immediately following init(). So the wrong behavior would happen in this use case. Will fix.

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

PR: https://git.openjdk.java.net/jdk/pull/1644
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20201208/1507b679/attachment-0001.htm>

[mlbridge]

Mailing list message from Jamil Nimeh on security-dev:

Hi Bernd, it's not a stupid question at all.? I think what you might be
looking for is Cipher.getIV()?? In the case of ChaCha20-Poly1305, that
method returns the nonce as a byte array where getParameters() returns
an AlgorithmParameters object where the encoding is consistent with RFC
8103.

But both getParameters() and getIV() were doing the wrong thing
(chucking NPE) when they should've either come up with a random param or
null, respectively when in a pre-initialized state.

--Jamil

On 12/7/2020 5:19 PM, Bernd Eckenfels wrote:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20201207/629fdbc9/attachment.htm>

[mlbridge]

Mailing list message from Jamil Nimeh on security-dev:

Oh, one other thing in case you weren't talking about this in the
context of Cipher and instead were talking about the AlgorithmParameters
object itself.? If that's all you've got in scope one approach is to
call getParameterSpec(IvParameterSpec.class).getIV();? That will give
you the byte array as well.? Now that I think about it, I should
probably do that in my test code because I went and manually parsed the
DER encoding.? :)? Glad you brought it up.

--Jamil

On 12/7/2020 5:39 PM, Jamil Nimeh wrote:

Hi Bernd, it's not a stupid question at all.? I think what you might
be looking for is Cipher.getIV()?? In the case of ChaCha20-Poly1305,
that method returns the nonce as a byte array where getParameters()
returns an AlgorithmParameters object where the encoding is consistent
with RFC 8103.

But both getParameters() and getIV() were doing the wrong thing
(chucking NPE) when they should've either come up with a random param
or null, respectively when in a pre-initialized state.

--Jamil

On 12/7/2020 5:19 PM, Bernd Eckenfels wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20201207/a4cf13f7/attachment.htm>

[jnimeh]

/integrate

[openjdk]

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

• 6ff18e3: 8257855: Example SafeVarargsNotApplicableToRecordAccessors breaks test tools/javac/diags/CheckExamples.java
• cef606f: 8253762: JFR: getField(String) should be able to access subfields
• 39b8a2e: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks
• c43c224: 8257796: [TESTBUG] TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on x86_32
• 62c7788: 8257211: C2: Enable call devirtualization during post-parse phase
• 149a02f: 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal
• f92745d: 8257718: LogCompilation: late_inline doesnt work right for JDK 8 logs
• 6937d9f: 8257799: Update JLS cross-references in java.compiler
• a5297bd: 8254939: macOS: unused function 'replicate4_imm'
• 36c0600: 8257805: Add compiler/blackhole tests to tier1
• ... and 79 more: https://git.openjdk.java.net/jdk/compare/d80ae05f617b35bd327e03869284de0c41adb94d...master

Your commit was automatically rebased without conflicts.

Pushed as commit 500ab45.

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