Skip to content

Commit

Permalink
8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305
Browse files Browse the repository at this point in the history 
Reviewed-by: mullan, valeriep
  • Loading branch information
Jamil Nimeh committed Dec 8, 2020
1 parent 6ff18e3 commit 500ab45
Show file tree
Hide file tree
Showing 2 changed files with 125 additions and 6 deletions.
13 changes: 9 additions & 4 deletions src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -207,7 +207,7 @@ protected int engineGetOutputSize(int inputLen) {
*/
@Override
protected byte[] engineGetIV() {
return nonce.clone();
return (nonce != null) ? nonce.clone() : null;
}

/**
Expand All @@ -226,11 +226,16 @@ protected byte[] engineGetIV() {
protected AlgorithmParameters engineGetParameters() {
AlgorithmParameters params = null;
if (mode == MODE_AEAD) {
// In a pre-initialized state or any state without a nonce value
// this call should cause a random nonce to be generated, but
// not attached to the object.
byte[] nonceData = (initialized || nonce != null) ? nonce :
createRandomNonce(null);
try {
// Place the 12-byte nonce into a DER-encoded OCTET_STRING
params = AlgorithmParameters.getInstance("ChaCha20-Poly1305");
params.init((new DerValue(
DerValue.tag_OctetString, nonce).toByteArray()));
DerValue.tag_OctetString, nonceData).toByteArray()));
} catch (NoSuchAlgorithmException | IOException exc) {
throw new RuntimeException(exc);
}
Expand Down Expand Up @@ -504,7 +509,7 @@ protected void engineUpdateAAD(ByteBuffer src) {
*
* @return a 12-byte array containing the random nonce.
*/
private byte[] createRandomNonce(SecureRandom random) {
private static byte[] createRandomNonce(SecureRandom random) {
byte[] newNonce = new byte[12];
SecureRandom rand = (random != null) ? random : new SecureRandom();
rand.nextBytes(newNonce);
Expand Down
118 changes: 116 additions & 2 deletions test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/ChaCha20Poly1305ParamTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -23,7 +23,7 @@

/**
* @test
* @bug 8153029
* @bug 8153029 8257769
* @library /test/lib
* @build jdk.test.lib.Convert
* @run main ChaCha20Poly1305ParamTest
Expand All @@ -42,6 +42,10 @@
import java.security.AlgorithmParameters;
import java.security.NoSuchAlgorithmException;
import java.nio.ByteBuffer;
import java.security.InvalidKeyException;
import java.security.MessageDigest;
import java.security.spec.InvalidParameterSpecException;
import javax.crypto.spec.IvParameterSpec;
import jdk.test.lib.Convert;

public class ChaCha20Poly1305ParamTest {
Expand Down Expand Up @@ -232,6 +236,111 @@ public static void main(String args[]) throws Exception {
System.out.println("Caught expected exception: " + ioe);
}

// The next set of tests cover cases where ChaCha20-Poly1305 cipher
// objects have the getParameters() call executed after instantiation
// but before initialization.
System.out.println("*** Test: getParameters before init");
cc20p1305 = Cipher.getInstance("ChaCha20-Poly1305");
AlgorithmParameters algParams = cc20p1305.getParameters();
byte[] preInitNonce = getNonceFromParams(algParams);
// A second pre-init getParameters() call should return a new set of
// random parameters.
AlgorithmParameters algParamsTwo = cc20p1305.getParameters();
byte[] secondNonce = getNonceFromParams(algParamsTwo);
if (MessageDigest.isEqual(preInitNonce, secondNonce)) {
throw new RuntimeException("Unexpected nonce match between " +
"two pre-init getParameters() calls");
}

// Next we will initialize the Cipher object using a form of init
// that doesn't take AlgorithmParameters or AlgorithmParameterSpec.
// The nonce created using the pre-init getParameters() call should
// be overwritten by a freshly generated set of random parameters.
cc20p1305.init(Cipher.ENCRYPT_MODE, DEF_KEY);
AlgorithmParameters postInitAps = cc20p1305.getParameters();
byte[] postInitNonce = getNonceFromParams(postInitAps);
if (MessageDigest.isEqual(preInitNonce, postInitNonce)) {
throw new RuntimeException("Unexpected nonce match between " +
"pre and post-init getParameters() calls");
}
System.out.println("Test Passed");

// After an initialization, subsequent calls to getParameters() should
// return the same parameter value until the next initialization takes
// place.
System.out.println("*** Test: getParameters after init");
AlgorithmParameters postInitApsTwo = cc20p1305.getParameters();
byte[] postInitNonceTwo = getNonceFromParams(postInitApsTwo);
if (!MessageDigest.isEqual(postInitNonce, postInitNonceTwo)) {
throw new RuntimeException("Unexpected nonce mismatch between " +
"two post-init getParameters() calls");
}
System.out.println("Test Passed");

// Test reinitialization use cases.
// First test: instantiate, init(no param), encrypt. Get params
// and attempt to reinit with same parameters. Should fail.
System.out.println("*** Test: Init w/ random nonce, init 2nd time");
cc20p1305 = Cipher.getInstance("ChaCha20-Poly1305");
cc20p1305.init(Cipher.ENCRYPT_MODE, DEF_KEY);
algParams = cc20p1305.getParameters();
preInitNonce = getNonceFromParams(algParams);
// Perform a simple encryption operation
cc20p1305.doFinal(aeadTestList.get(0).input);
try {
// Now try to reinitialize using the same parameters
cc20p1305.init(Cipher.ENCRYPT_MODE, DEF_KEY, algParams);
throw new RuntimeException("Illegal key/nonce reuse");
} catch (InvalidKeyException ike) {
System.out.println("Caught expected exception: " + ike);
}

// Test the reinit guard using an AlgorithmParameterSpec with the
// Same nonce value. This should also be a failure.
try {
cc20p1305.init(Cipher.ENCRYPT_MODE, DEF_KEY,
new IvParameterSpec(preInitNonce));
throw new RuntimeException("Illegal key/nonce reuse");
} catch (InvalidKeyException ike) {
System.out.println("Caught expected exception: " + ike);
}

// Try one more time, this time providing a new 12-byte nonce, which
// should be allowed even if the key is the same.
cc20p1305.init(Cipher.ENCRYPT_MODE, DEF_KEY,
new IvParameterSpec(NONCE_OCTET_STR_12, 2, 12));
System.out.println("Test Passed");

// Reinit test: instantiate, init(no param), getParam, encrypt,
// then init(no param). Should work and the parameters should be
// different after each init.
cc20p1305 = Cipher.getInstance("ChaCha20-Poly1305");
cc20p1305.init(Cipher.ENCRYPT_MODE, DEF_KEY);
byte[] paramInitOne = getNonceFromParams(cc20p1305.getParameters());
// Perform a simple encryption operation
cc20p1305.doFinal(aeadTestList.get(0).input);
// reinit (no params)
cc20p1305.init(Cipher.ENCRYPT_MODE, DEF_KEY);
byte[] paramInitTwo = getNonceFromParams(cc20p1305.getParameters());
if (MessageDigest.isEqual(paramInitOne, paramInitTwo)) {
throw new RuntimeException("Unexpected nonce match between " +
"pre and post-init getParameters() calls");
}
System.out.println("Test Passed");

// Reinit test: instantiate, init(no param), doFinal, then doFinal
// again without intervening init. Should fail due to no-reuse
// protections.
try {
cc20p1305 = Cipher.getInstance("ChaCha20-Poly1305");
cc20p1305.init(Cipher.ENCRYPT_MODE, DEF_KEY);
cc20p1305.doFinal(aeadTestList.get(0).input);
cc20p1305.doFinal(aeadTestList.get(0).input);
throw new RuntimeException("Illegal key/nonce reuse");
} catch (IllegalStateException ise) {
System.out.println("Caught expected exception: " + ise);
}

System.out.println("----- AEAD Tests -----");
for (TestData test : aeadTestList) {
System.out.println("*** Test " + ++testNumber + ": " +
Expand Down Expand Up @@ -374,6 +483,11 @@ private static boolean runAEADTest(TestData testData)
return result;
}

private static byte[] getNonceFromParams(AlgorithmParameters params)
throws InvalidParameterSpecException {
return params.getParameterSpec(IvParameterSpec.class).getIV();
}

/**
* Dump the hex bytes of a buffer into string form.
*
Expand Down

3 comments on commit 500ab45

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

@GoeLin
Copy link
Member

@GoeLin GoeLin commented on 500ab45 Dec 6, 2021

Choose a reason for hiding this comment

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

/backport jdk11u-dev

@openjdk
Copy link

@openjdk openjdk bot commented on 500ab45 Dec 6, 2021

Choose a reason for hiding this comment

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

@GoeLin the backport was successfully created on the branch GoeLin-backport-500ab457 in my personal fork of openjdk/jdk11u-dev. To create a pull request with this backport targeting openjdk/jdk11u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 500ab457 from the openjdk/jdk repository.

The commit being backported was authored by Jamil Nimeh on 8 Dec 2020 and was reviewed by Sean Mullan and Valerie Peng.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk11u-dev:

$ git fetch https://github.com/openjdk-bots/jdk11u-dev GoeLin-backport-500ab457:GoeLin-backport-500ab457
$ git checkout GoeLin-backport-500ab457
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk11u-dev GoeLin-backport-500ab457

Please sign in to comment.