From 85b06b68110976ed1ae2d9e30deab38d87f5132c Mon Sep 17 00:00:00 2001 From: Jason Katonica Date: Thu, 8 Feb 2024 11:57:09 -0500 Subject: [PATCH] Avoid overlapping buffers in native ChaCha20 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](https://github.com/eclipse-openj9/openj9/issues/18703). Signed-off-by: Jason Katonica --- .../crypto/provider/NativeChaCha20Cipher.java | 136 ++++++++++++++---- 1 file changed, 110 insertions(+), 26 deletions(-) diff --git a/closed/src/java.base/share/classes/com/sun/crypto/provider/NativeChaCha20Cipher.java b/closed/src/java.base/share/classes/com/sun/crypto/provider/NativeChaCha20Cipher.java index 5d4fec94360..0d568ad7c88 100644 --- a/closed/src/java.base/share/classes/com/sun/crypto/provider/NativeChaCha20Cipher.java +++ b/closed/src/java.base/share/classes/com/sun/crypto/provider/NativeChaCha20Cipher.java @@ -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; @@ -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); } /** @@ -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; @@ -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]; } @@ -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) { @@ -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. * @@ -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 { @@ -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 { @@ -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 { @@ -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; @@ -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) { @@ -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; } }