Skip to content

Commit

Permalink
8298865: Excessive memory allocation in CipherOutputStream AEAD decry…
Browse files Browse the repository at this point in the history
…ption

Reviewed-by: valeriep, ascarpino
  • Loading branch information
djelinski committed Dec 20, 2022
1 parent dd15d30 commit 36de61c
Show file tree
Hide file tree
Showing 3 changed files with 159 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ public class CipherInputStream extends FilterInputStream {
* The ostart and ofinish indices are reset to 0.
*
* If obuffer is null/zero-sized, do not allocate a new buffer.
* This reduces allocation for AEAD ciphers that never return data from update
* This reduces allocation for authenticated decryption
* that never returns data from update
*
* @param inLen the input length (in bytes)
*/
Expand Down Expand Up @@ -152,7 +153,7 @@ private int getMoreData() throws IOException {
ofinish = cipher.doFinal(obuffer, 0);
} else {
obuffer = cipher.doFinal();
ofinish = obuffer.length;
ofinish = (obuffer != null) ? obuffer.length : 0;
}
} catch (IllegalBlockSizeException | BadPaddingException
| ShortBufferException e) {
Expand All @@ -167,7 +168,7 @@ private int getMoreData() throws IOException {
ensureCapacity(readin);
try {
// initial obuffer is assigned by update/doFinal;
// for AEAD ciphers, obuffer is always null or zero-length here
// for AEAD decryption, obuffer is always null or zero-length here
if (obuffer != null && obuffer.length > 0) {
ofinish = cipher.update(ibuffer, 0, readin, obuffer, ostart);
} else {
Expand Down
37 changes: 33 additions & 4 deletions src/java.base/share/classes/javax/crypto/CipherOutputStream.java
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,18 @@ public class CipherOutputStream extends FilterOutputStream {
* Ensure obuffer is big enough for the next update or doFinal
* operation, given the input length {@code inLen} (in bytes)
*
* If obuffer is null/zero-sized, do not allocate a new buffer.
* This reduces allocation for authenticated decryption
* that never returns data from update
*
* @param inLen the input length (in bytes)
*/
private void ensureCapacity(int inLen) {
if (obuffer == null || obuffer.length == 0) {
return;
}
int minLen = cipher.getOutputSize(inLen);
if (obuffer == null || obuffer.length < minLen) {
if (obuffer.length < minLen) {
obuffer = new byte[minLen];
}
}
Expand Down Expand Up @@ -144,7 +151,15 @@ public void write(int b) throws IOException {
ibuffer[0] = (byte) b;
ensureCapacity(1);
try {
int ostored = cipher.update(ibuffer, 0, 1, obuffer);
// initial obuffer is assigned by update/doFinal;
// for AEAD decryption, obuffer is always null or zero-length here
int ostored;
if (obuffer != null && obuffer.length > 0) {
ostored = cipher.update(ibuffer, 0, 1, obuffer);
} else {
obuffer = cipher.update(ibuffer, 0, 1);
ostored = (obuffer != null) ? obuffer.length : 0;
}
if (ostored > 0) {
output.write(obuffer, 0, ostored);
}
Expand Down Expand Up @@ -186,7 +201,15 @@ public void write(byte[] b) throws IOException {
public void write(byte[] b, int off, int len) throws IOException {
ensureCapacity(len);
try {
int ostored = cipher.update(b, off, len, obuffer);
// initial obuffer is assigned by update/doFinal;
// for AEAD decryption, obuffer is always null or zero-length here
int ostored;
if (obuffer != null && obuffer.length > 0) {
ostored = cipher.update(b, off, len, obuffer);
} else {
obuffer = cipher.update(b, off, len);
ostored = (obuffer != null) ? obuffer.length : 0;
}
if (ostored > 0) {
output.write(obuffer, 0, ostored);
}
Expand Down Expand Up @@ -241,7 +264,13 @@ public void close() throws IOException {
closed = true;
ensureCapacity(0);
try {
int ostored = cipher.doFinal(obuffer, 0);
int ostored;
if (obuffer != null && obuffer.length > 0) {
ostored = cipher.doFinal(obuffer, 0);
} else {
obuffer = cipher.doFinal();
ostored = (obuffer != null) ? obuffer.length : 0;
}
if (ostored > 0) {
output.write(obuffer, 0, ostored);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/*
* Copyright (c) 2022, 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
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.bench.javax.crypto.full;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Setup;

import javax.crypto.Cipher;
import javax.crypto.CipherOutputStream;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.SecretKeySpec;
import java.io.ByteArrayOutputStream;
import java.io.OutputStream;

/**
* This performance test runs AES/GCM encryption and decryption using CipherOutputStream.
*
* This test rotates the IV and creates a new GCMParameterSpec for each encrypt
* benchmark operation
*/

public class AESGCMCipherOutputStream extends CryptoBase {

@Param({"128"})
private int keyLength;

@Param({"16384", "1048576"})
private int dataSize;

byte[] encryptedData;
byte[] in;
ByteArrayOutputStream out;
private Cipher encryptCipher;
private Cipher decryptCipher;
SecretKeySpec ks;
GCMParameterSpec gcm_spec;
byte[] iv;

private static final int IV_BUFFER_SIZE = 32;
private static final int IV_MODULO = IV_BUFFER_SIZE - 16;
int iv_index = 0;

private int next_iv_index() {
int r = iv_index;
iv_index = (iv_index + 1) % IV_MODULO;
return r;
}

@Setup
public void setup() throws Exception {
setupProvider();

// Setup key material
byte[] keystring = fillSecureRandom(new byte[keyLength / 8]);
ks = new SecretKeySpec(keystring, "AES");
iv = fillSecureRandom(new byte[IV_BUFFER_SIZE]);
gcm_spec = new GCMParameterSpec(96, iv, next_iv_index(), 16);

// Setup Cipher classes
encryptCipher = makeCipher(prov, "AES/GCM/NoPadding");
encryptCipher.init(Cipher.ENCRYPT_MODE, ks, gcm_spec);
decryptCipher = makeCipher(prov, "AES/GCM/NoPadding");
decryptCipher.init(Cipher.DECRYPT_MODE, ks,
encryptCipher.getParameters().
getParameterSpec(GCMParameterSpec.class));

// Setup input/output buffers
in = fillRandom(new byte[dataSize]);
encryptedData = new byte[encryptCipher.getOutputSize(in.length)];
out = new ByteArrayOutputStream(encryptedData.length);
encryptCipher.doFinal(in, 0, in.length, encryptedData, 0);
}

@Benchmark
public byte[] encrypt() throws Exception {
out.reset();
gcm_spec = new GCMParameterSpec(96, iv, next_iv_index(), 16);
encryptCipher.init(Cipher.ENCRYPT_MODE, ks, gcm_spec);

OutputStream cout = new CipherOutputStream(out, encryptCipher);
for (int i = 0; i < in.length; i += 512) {
cout.write(in, i, Math.min(512, in.length - i));
}
cout.close();
return out.toByteArray();
}

@Benchmark
public byte[] decrypt() throws Exception {
out.reset();
decryptCipher.init(Cipher.DECRYPT_MODE, ks,
encryptCipher.getParameters().
getParameterSpec(GCMParameterSpec.class));
OutputStream cout = new CipherOutputStream(out, decryptCipher);
for (int i = 0; i < encryptedData.length; i += 512) {
cout.write(encryptedData, i, Math.min(512, encryptedData.length - i));
}
cout.close();
return out.toByteArray();
}
}

1 comment on commit 36de61c

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.