Skip to content

Commit

Permalink
Avoid overlapping buffers in native ChaCha20
Browse files Browse the repository at this point in the history 
When using the ChaCha20 algorothms and an input
and output buffer overlaps we should ensure that
a copy of the input buffer is made before
encrypting or decrypting. This ensures that
OpenSSL is able to process this data. OpenSSL
does not allow for overlapping input and output
buffers when performing operations on data for the
ChaCha20 algorithm.

The values returned when getting the output
size for a crypto operation were also found to be
incorrect. This update matches the logic that is
within the ChaCha20Cipher.java file for
calculating the output sizes.

Encoded key material was found to be left in
memory under the right conditions. This memory
should be zeroed to avoid a copy of the key from
residing in memory for longer then necessary.
This addition was noticed when comparing the
NativeChaCha20Cipher class to the ChaCha20Cipher
class. The method getEncodedKey now will zero out
the copy of the key before throwing an exception.

Each of these changes fix the two failing tests
reported in issue [18703](eclipse-openj9/openj9#18703).

Signed-off-by: Jason Katonica <katonica@us.ibm.com>
  • Loading branch information
@jasonkatonica
jasonkatonica committed Feb 16, 2024
1 parent a6ce176 commit 85b06b6
Showing 1 changed file with 110 additions and 26 deletions.
136 changes: 110 additions & 26 deletions closed/src/java.base/share/classes/com/sun/crypto/provider/NativeChaCha20Cipher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
*/
/*
* ===========================================================================
* (c) Copyright IBM Corp. 2018, 2023 All Rights Reserved
* (c) Copyright IBM Corp. 2018, 2024 All Rights Reserved
* ===========================================================================
*/
package com.sun.crypto.provider;
Expand Down Expand Up @@ -199,20 +199,7 @@ protected int engineGetBlockSize() {
*/
@Override
protected int engineGetOutputSize(int inputLen) {
int outLen = 0;

if (mode == MODE_NONE) {
outLen = inputLen;
} else if (mode == MODE_AEAD) {
if (direction == Cipher.ENCRYPT_MODE) {
outLen = Math.addExact(inputLen, TAG_LENGTH);
} else {
outLen = Integer.max(inputLen, TAG_LENGTH) - TAG_LENGTH;
outLen = Math.addExact(outLen, engine.getCipherBufferLength());
}
}

return outLen;
return engine.getOutputSize(inputLen, true);
}

/**
Expand Down Expand Up @@ -659,6 +646,9 @@ private static byte[] getEncodedKey(Key key) throws InvalidKeyException {
}
byte[] encodedKey = key.getEncoded();
if (encodedKey == null || encodedKey.length != 32) {
if (encodedKey != null) {
Arrays.fill(encodedKey, (byte)0);
}
throw new InvalidKeyException("Key length must be 256 bits");
}
return encodedKey;
Expand All @@ -677,11 +667,11 @@ private static byte[] getEncodedKey(Key key) throws InvalidKeyException {
*/
@Override
protected byte[] engineUpdate(byte[] in, int inOfs, int inLen) {
byte[] out = new byte[inLen];
byte[] out = new byte[engine.getOutputSize(inLen, false)];
try {
int size = engine.doUpdate(in, inOfs, inLen, out, 0);
// Special case for EngineAEADDec, doUpdate only buffers the input
// So the output array must be empty since no encryption happened yet
// so the output array must be empty since no encryption has happened yet.
if (size == 0) {
return new byte[0];
}
Expand Down Expand Up @@ -740,7 +730,7 @@ protected int engineUpdate(byte[] in, int inOfs, int inLen,
@Override
protected byte[] engineDoFinal(byte[] in, int inOfs, int inLen)
throws AEADBadTagException {
byte[] output = new byte[engineGetOutputSize(inLen)];
byte[] output = new byte[engine.getOutputSize(inLen, true)];
try {
engine.doFinal(in, inOfs, inLen, output, 0);
} catch (ShortBufferException | KeyException exc) {
Expand Down Expand Up @@ -864,6 +854,17 @@ private static byte[] intToLittleEndian (long i) {
* Interface for the underlying processing engines for ChaCha20
*/
interface ChaChaEngine {
/**
* Size an output buffer based on the input and where applicable
* the current state of the engine in a multipart operation.
*
* @param inLength the input length.
* @param isFinal true if this is invoked from a doFinal call.
*
* @return the recommended size for the output buffer.
*/
int getOutputSize(int inLength, boolean isFinal);

/**
* Perform a multi-part update for ChaCha20.
*
Expand Down Expand Up @@ -910,12 +911,49 @@ int doFinal(byte[] in, int inOff, int inLen, byte[] out, int outOff)
* @return the number of unprocessed bytes left.
*/
int getCipherBufferLength();

/**
* Determines if two arrays have any overlapping memory.
*
* @param input The input array.
* @param inputStart The index of the input arrays start.
* @param inputEnd The index of the input arrays end.
* @param output The output array.
* @param outputStart The index of the output arrays start.
* @param outputEnd The index of the output arrays end.
* @return true if any memory overlaps, false otherwise.
*/
static boolean arraysOverlap(byte[] input, int inputStart, int inputEnd, byte[] output, int outputStart, int outputEnd) {

// Check if there is potential overlap if input and output buffers have same address.
if (input != output) {
return false;
}

// If the start of input is anywhere in the range of the output array, there is an overlap.
if ((outputStart <= inputStart) && (inputStart < outputEnd)) {
return true;
}

// If the start of output is anywhere in the range of the input array, there is an overlap.
if ((inputStart <= outputStart) && (outputStart < inputEnd)) {
return true;
}

return false; // Memory does not overlap, return false.
}
}

private final class EngineStreamOnly implements ChaChaEngine {

EngineStreamOnly() { }

@Override
public int getOutputSize(int inLength, boolean isFinal) {
// The isFinal parameter is not relevant in this kind of engine.
return inLength;
}

@Override
public synchronized int doUpdate(byte[] in, int inOff, int inLen, byte[] out,
int outOff) throws ShortBufferException, KeyException {
Expand All @@ -931,10 +969,25 @@ public synchronized int doUpdate(byte[] in, int inOff, int inLen, byte[] out,
throw new ShortBufferException("Output buffer too small");
}

Objects.checkFromIndexSize(inOff, inLen, in.length);
int ret = nativeCrypto.ChaCha20Update(context, in, inOff, inLen, out, outOff, /*aadArray*/ null, /*aadArray.length*/ 0);
if (ret == -1) {
throw new ProviderException("Error in Native ChaCha20Cipher");
if (in != null) {
Objects.checkFromIndexSize(inOff, inLen, in.length);

// Check if there is any overlap with the input and output
// buffers. If so, create a temporary copy of the input such
// that it does not overlap while performing an update computation
// within the openssl library.
int ret = -1;
if (ChaChaEngine.arraysOverlap(in, inOff, inOff + inLen, out, outOff, outOff + getOutputSize(inLen, false))) {
byte[] newInArray = Arrays.copyOfRange(in, inOff, inOff + inLen);
ret = nativeCrypto.ChaCha20Update(context, newInArray, 0, inLen, out, outOff, /*aadArray*/ null, /*aadArray.length*/ 0);
Arrays.fill(newInArray, (byte)0);
} else {
ret = nativeCrypto.ChaCha20Update(context, in, inOff, inLen, out, outOff, /*aadArray*/ null, /*aadArray.length*/ 0);
}

if (ret == -1) {
throw new ProviderException("Error in Native ChaCha20Cipher");
}
}
return inLen;
} else {
Expand Down Expand Up @@ -965,6 +1018,11 @@ private final class EngineAEADEnc implements ChaChaEngine {
counter = 1;
}

@Override
public int getOutputSize(int inLength, boolean isFinal) {
return (isFinal ? Math.addExact(inLength, TAG_LENGTH) : inLength);
}

@Override
public synchronized int doUpdate(byte[] in, int inOff, int inLen, byte[] out,
int outOff) throws ShortBufferException, KeyException {
Expand All @@ -987,11 +1045,22 @@ public synchronized int doUpdate(byte[] in, int inOff, int inLen, byte[] out,

Objects.checkFromIndexSize(inOff, inLen, in.length);

byte aadArray[] = aadBuf.toByteArray();
byte[] aadArray = aadBuf.toByteArray();
aadBuf.reset();
int ret = nativeCrypto.ChaCha20Update(context, in, inOff, inLen, out, outOff, aadArray, aadArray.length);
// Check if there is any overlap with the input and output
// buffers. If so, create a temporary copy of the input such
// that it does not overlap while performing an update computation
// within the openssl library.
int ret = -1;
if (ChaChaEngine.arraysOverlap(in, inOff, inOff + inLen, out, outOff, outOff + getOutputSize(inLen, false))) {
byte[] newInArray = Arrays.copyOfRange(in, inOff, inOff + inLen);
ret = nativeCrypto.ChaCha20Update(context, newInArray, 0, inLen, out, outOff, aadArray, aadArray.length);
Arrays.fill(newInArray, (byte)0);
} else {
ret = nativeCrypto.ChaCha20Update(context, in, inOff, inLen, out, outOff, aadArray, aadArray.length);
}
if (ret == -1) {
throw new ProviderException("Error in Native CipherBlockChaining");
throw new ProviderException("Error in Native ChaCha20Cipher");
}

return inLen;
Expand Down Expand Up @@ -1039,6 +1108,18 @@ private final class EngineAEADDec implements ChaChaEngine {
tag = new byte[TAG_LENGTH];
}

@Override
public int getOutputSize(int inLen, boolean isFinal) {
// If we are performing a decrypt-update we should always return
// zero length since we cannot return any data until the tag has
// been consumed and verified. CipherSpi.engineGetOutputSize will
// always set isFinal to true to get the required output buffer
// size.
return (isFinal ?
Integer.max(Math.addExact((inLen - TAG_LENGTH),
getCipherBufferLength()), 0) : 0);
}

@Override
public int doUpdate(byte[] in, int inOff, int inLen, byte[] out,
int outOff) {
Expand Down Expand Up @@ -1121,7 +1202,10 @@ public synchronized int doFinal(byte[] in, int inOff, int inLen, byte[] out,

@Override
public int getCipherBufferLength() {
return cipherBuf.size();
if (cipherBuf != null) {
return cipherBuf.size();
}
return 0;
}
}

Expand Down

0 comments on commit 85b06b6

Please sign in to comment.