Skip to content

Commit 9037d61

Browse files
authored
fixes AES decryption threading issue (#5384)
A single Cipher object was shared between multiple inputstreams when decrypting AES data. This caused decryption to fail when mutliple threads created input streams for decryption. This could potentially cause failure for a single thread also if it interleaves reading from multiple streams that use the same Cipher. Modified the code to create a Cipher per input stream.
1 parent 763367f commit 9037d61

File tree

2 files changed

+77
-10
lines changed

2 files changed

+77
-10
lines changed

core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java

+12-10
Original file line numberDiff line numberDiff line change
@@ -426,20 +426,21 @@ public byte[] getDecryptionParameters() {
426426
}
427427

428428
public class AESGCMFileDecrypter implements FileDecrypter {
429-
private final Cipher cipher;
430429
private final Key fek;
431430

432431
AESGCMFileDecrypter(Key fek) {
432+
this.fek = fek;
433+
}
434+
435+
@Override
436+
public InputStream decryptStream(InputStream inputStream) throws CryptoException {
437+
Cipher cipher;
433438
try {
434439
cipher = Cipher.getInstance(transformation);
435440
} catch (NoSuchAlgorithmException | NoSuchPaddingException e) {
436441
throw new CryptoException("Error obtaining cipher for transform " + transformation, e);
437442
}
438-
this.fek = fek;
439-
}
440443

441-
@Override
442-
public InputStream decryptStream(InputStream inputStream) throws CryptoException {
443444
byte[] initVector = new byte[GCM_IV_LENGTH_IN_BYTES];
444445
try {
445446
IOUtils.readFully(inputStream, initVector);
@@ -531,20 +532,21 @@ public byte[] getDecryptionParameters() {
531532

532533
@SuppressFBWarnings(value = "CIPHER_INTEGRITY", justification = "CBC is provided for WALs")
533534
public class AESCBCFileDecrypter implements FileDecrypter {
534-
private final Cipher cipher;
535535
private final Key fek;
536536

537537
AESCBCFileDecrypter(Key fek) {
538+
this.fek = fek;
539+
}
540+
541+
@Override
542+
public InputStream decryptStream(InputStream inputStream) throws CryptoException {
543+
Cipher cipher;
538544
try {
539545
cipher = Cipher.getInstance(transformation);
540546
} catch (NoSuchAlgorithmException | NoSuchPaddingException e) {
541547
throw new CryptoException("Error obtaining cipher for transform " + transformation, e);
542548
}
543-
this.fek = fek;
544-
}
545549

546-
@Override
547-
public InputStream decryptStream(InputStream inputStream) throws CryptoException {
548550
byte[] initVector = new byte[IV_LENGTH_IN_BYTES];
549551
try {
550552
IOUtils.readFully(inputStream, initVector);

core/src/test/java/org/apache/accumulo/core/crypto/CryptoTest.java

+65
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import static org.junit.jupiter.api.Assertions.assertNotEquals;
3030
import static org.junit.jupiter.api.Assertions.assertNotNull;
3131
import static org.junit.jupiter.api.Assertions.assertThrows;
32+
import static org.junit.jupiter.api.Assertions.assertTrue;
3233

3334
import java.io.ByteArrayInputStream;
3435
import java.io.ByteArrayOutputStream;
@@ -45,7 +46,10 @@
4546
import java.util.Arrays;
4647
import java.util.Collection;
4748
import java.util.HashMap;
49+
import java.util.List;
4850
import java.util.Map;
51+
import java.util.concurrent.Executors;
52+
import java.util.concurrent.Future;
4953

5054
import javax.crypto.Cipher;
5155
import javax.crypto.NoSuchPaddingException;
@@ -497,6 +501,67 @@ public void testPerTableFactory() {
497501
assertEquals(2, factory.getCount());
498502
}
499503

504+
@Test
505+
public void testMultipleThreads() throws Exception {
506+
testMultipleThreads(WAL);
507+
testMultipleThreads(TABLE);
508+
}
509+
510+
private void testMultipleThreads(Scope scope) throws Exception {
511+
512+
byte[] plainText = new byte[1024 * 1024];
513+
for (int i = 0; i < plainText.length; i++) {
514+
plainText[i] = (byte) (i % 128);
515+
}
516+
517+
AESCryptoService cs = new AESCryptoService();
518+
cs.init(getAllCryptoProperties(ConfigMode.CRYPTO_TABLE_ON));
519+
CryptoEnvironment encEnv = new CryptoEnvironmentImpl(scope, null, null);
520+
FileEncrypter encrypter = cs.getFileEncrypter(encEnv);
521+
byte[] params = encrypter.getDecryptionParameters();
522+
523+
assertNotNull(params);
524+
525+
ByteArrayOutputStream out = new ByteArrayOutputStream();
526+
DataOutputStream dataOut = new DataOutputStream(out);
527+
OutputStream encrypted = encrypter.encryptStream(dataOut);
528+
529+
assertNotNull(encrypted);
530+
DataOutputStream cipherOut = new DataOutputStream(encrypted);
531+
532+
cipherOut.write(plainText);
533+
534+
cipherOut.close();
535+
dataOut.close();
536+
encrypted.close();
537+
out.close();
538+
byte[] cipherText = out.toByteArray();
539+
540+
var executor = Executors.newCachedThreadPool();
541+
542+
List<Future<Boolean>> verifyFutures = new ArrayList<>();
543+
544+
FileDecrypter decrypter = cs.getFileDecrypter(new CryptoEnvironmentImpl(scope, null, params));
545+
546+
// verify that each input stream returned by decrypter.decryptStream() is independent when used
547+
// by multiple threads
548+
for (int i = 0; i < 32; i++) {
549+
var future = executor.submit(() -> {
550+
try (ByteArrayInputStream in = new ByteArrayInputStream(cipherText);
551+
DataInputStream decrypted = new DataInputStream(decrypter.decryptStream(in))) {
552+
byte[] dataRead = new byte[plainText.length];
553+
decrypted.readFully(dataRead);
554+
return Arrays.equals(plainText, dataRead);
555+
}
556+
});
557+
verifyFutures.add(future);
558+
}
559+
560+
for (var future : verifyFutures) {
561+
assertTrue(future.get());
562+
}
563+
}
564+
500565
private ArrayList<Key> testData() {
501566
ArrayList<Key> keys = new ArrayList<>();
502567
keys.add(new Key("a", "cf", "cq"));

0 commit comments

Comments
 (0)