diff --git a/processing/src/main/java/org/apache/druid/crypto/CryptoService.java b/processing/src/main/java/org/apache/druid/crypto/CryptoService.java index 9e63c0adfa94..c7c91c781dc6 100644 --- a/processing/src/main/java/org/apache/druid/crypto/CryptoService.java +++ b/processing/src/main/java/org/apache/druid/crypto/CryptoService.java @@ -28,6 +28,7 @@ import javax.crypto.Cipher; import javax.crypto.SecretKey; import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.PBEKeySpec; import javax.crypto.spec.SecretKeySpec; @@ -36,6 +37,7 @@ import java.security.SecureRandom; import java.security.spec.InvalidKeySpecException; import java.security.spec.KeySpec; +import java.util.Arrays; /** * Utility class for symmetric key encryption (i.e. same secret is used for encryption and decryption) of byte[] @@ -51,6 +53,20 @@ public class CryptoService // Based on Javadocs on SecureRandom, It is threadsafe as well. private static final SecureRandom SECURE_RANDOM_INSTANCE = new SecureRandom(); + private static final String AUTHENTICATED_CIPHER_ALGORITHM = "AES"; + private static final String AUTHENTICATED_CIPHER_TRANSFORMATION = "AES/GCM/NoPadding"; + private static final int GCM_IV_SIZE = 12; + private static final int GCM_TAG_LENGTH_BITS = 128; + private static final int AUTHENTICATED_FORMAT_MAGIC = 0xD2525549; + + /** + * Negative magic followed by a format version. A valid legacy payload starts with its nonnegative salt length, so + * this header unambiguously distinguishes authenticated ciphertext from the legacy format. + */ + private static final byte[] AUTHENTICATED_FORMAT_HEADER = { + (byte) 0xD2, 0x52, 0x55, 0x49, 0x01 + }; + // User provided secret phrase used for encrypting data private final char[] passPhrase; @@ -60,7 +76,7 @@ public class CryptoService private final int iterationCount; private final int keyLength; - // Cipher algorithm information + // Cipher algorithm information retained for decrypting ciphertext written by earlier versions. private final String cipherAlgName; private final String cipherAlgMode; private final String cipherAlgPadding; @@ -95,8 +111,8 @@ public CryptoService( this.iterationCount = iterationCount == null ? 65536 : iterationCount; this.keyLength = keyLength == null ? 128 : keyLength; - // encrypt/decrypt a test string to ensure all params are valid - String testString = "duh! !! !!!"; + // Validate authenticated parameters eagerly; the legacy transformation is decrypt-only and validated on first use. + final String testString = "duh! !! !!!"; Preconditions.checkState( testString.equals(StringUtils.fromUtf8(decrypt(encrypt(StringUtils.toUtf8(testString))))), "decrypt(encrypt(testString)) failed" @@ -106,22 +122,28 @@ public CryptoService( public byte[] encrypt(byte[] plain) { try { - byte[] salt = new byte[saltSize]; + final byte[] salt = new byte[saltSize]; SECURE_RANDOM_INSTANCE.nextBytes(salt); - SecretKey tmp = getKeyFromPassword(passPhrase, salt); - SecretKey secret = new SecretKeySpec(tmp.getEncoded(), cipherAlgName); + final SecretKey tmp = getKeyFromPassword(passPhrase, salt); + final SecretKey secret = new SecretKeySpec(tmp.getEncoded(), AUTHENTICATED_CIPHER_ALGORITHM); - // error-prone warns if the transformation is not a compile-time constant - // since it cannot check it for insecure combinations. - @SuppressWarnings("InsecureCryptoUsage") - Cipher ecipher = Cipher.getInstance(transformation); - ecipher.init(Cipher.ENCRYPT_MODE, secret); - return new EncryptedData( + final byte[] iv = new byte[GCM_IV_SIZE]; + SECURE_RANDOM_INSTANCE.nextBytes(iv); + + final Cipher ecipher = Cipher.getInstance(AUTHENTICATED_CIPHER_TRANSFORMATION); + ecipher.init(Cipher.ENCRYPT_MODE, secret, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, iv)); + ecipher.updateAAD(AUTHENTICATED_FORMAT_HEADER); + + final byte[] encryptedData = new EncryptedData( salt, - ecipher.getParameters().getParameterSpec(IvParameterSpec.class).getIV(), + iv, ecipher.doFinal(plain) ).toByteAray(); + return ByteBuffer.allocate(Math.addExact(AUTHENTICATED_FORMAT_HEADER.length, encryptedData.length)) + .put(AUTHENTICATED_FORMAT_HEADER) + .put(encryptedData) + .array(); } catch (Exception ex) { log.noStackTrace().warn(ex, "Encryption failed"); @@ -129,18 +151,23 @@ public byte[] encrypt(byte[] plain) } } + // Legacy ciphertext may use a weaker configured transformation; new ciphertext is always written using GCM. public byte[] decrypt(byte[] data) { try { - EncryptedData encryptedData = EncryptedData.fromByteArray(data); + if (hasAuthenticatedFormatMagic(data)) { + return decryptAuthenticated(data); + } + + final EncryptedData encryptedData = EncryptedData.fromByteArray(data); - SecretKey tmp = getKeyFromPassword(passPhrase, encryptedData.getSalt()); - SecretKey secret = new SecretKeySpec(tmp.getEncoded(), cipherAlgName); + final SecretKey tmp = getKeyFromPassword(passPhrase, encryptedData.getSalt()); + final SecretKey secret = new SecretKeySpec(tmp.getEncoded(), cipherAlgName); // error-prone warns if the transformation is not a compile-time constant // since it cannot check it for insecure combinations. @SuppressWarnings("InsecureCryptoUsage") - Cipher dcipher = Cipher.getInstance(transformation); + final Cipher dcipher = Cipher.getInstance(transformation); dcipher.init(Cipher.DECRYPT_MODE, secret, new IvParameterSpec(encryptedData.getIv())); return dcipher.doFinal(encryptedData.getCipher()); } @@ -150,11 +177,49 @@ public byte[] decrypt(byte[] data) } } - private SecretKey getKeyFromPassword(char[] passPhrase, byte[] salt) + private byte[] decryptAuthenticated(final byte[] data) throws Exception + { + Preconditions.checkArgument( + data.length >= AUTHENTICATED_FORMAT_HEADER.length + && Arrays.equals( + data, + 0, + AUTHENTICATED_FORMAT_HEADER.length, + AUTHENTICATED_FORMAT_HEADER, + 0, + AUTHENTICATED_FORMAT_HEADER.length + ), + "Unsupported encrypted data version" + ); + + final EncryptedData encryptedData = EncryptedData.fromByteArray( + Arrays.copyOfRange(data, AUTHENTICATED_FORMAT_HEADER.length, data.length) + ); + Preconditions.checkArgument(encryptedData.getIv().length == GCM_IV_SIZE, "Invalid GCM IV size"); + + final SecretKey tmp = getKeyFromPassword(passPhrase, encryptedData.getSalt()); + final SecretKey secret = new SecretKeySpec(tmp.getEncoded(), AUTHENTICATED_CIPHER_ALGORITHM); + final Cipher dcipher = Cipher.getInstance(AUTHENTICATED_CIPHER_TRANSFORMATION); + dcipher.init( + Cipher.DECRYPT_MODE, + secret, + new GCMParameterSpec(GCM_TAG_LENGTH_BITS, encryptedData.getIv()) + ); + dcipher.updateAAD(AUTHENTICATED_FORMAT_HEADER); + return dcipher.doFinal(encryptedData.getCipher()); + } + + private static boolean hasAuthenticatedFormatMagic(final byte[] data) + { + return data.length >= Integer.BYTES + && ByteBuffer.wrap(data).getInt() == AUTHENTICATED_FORMAT_MAGIC; + } + + private SecretKey getKeyFromPassword(final char[] passPhrase, final byte[] salt) throws NoSuchAlgorithmException, InvalidKeySpecException { - SecretKeyFactory factory = SecretKeyFactory.getInstance(secretKeyFactoryAlg); - KeySpec spec = new PBEKeySpec(passPhrase, salt, iterationCount, keyLength); + final SecretKeyFactory factory = SecretKeyFactory.getInstance(secretKeyFactoryAlg); + final KeySpec spec = new PBEKeySpec(passPhrase, salt, iterationCount, keyLength); return factory.generateSecret(spec); } @@ -188,8 +253,10 @@ public byte[] getCipher() public byte[] toByteAray() { - int headerLength = 12; - ByteBuffer bb = ByteBuffer.allocate(salt.length + iv.length + cipher.length + headerLength); + final int headerLength = 12; + final int encryptedDataLength = + Math.addExact(Math.addExact(Math.addExact(salt.length, iv.length), cipher.length), headerLength); + final ByteBuffer bb = ByteBuffer.allocate(encryptedDataLength); bb.putInt(salt.length) .putInt(iv.length) .putInt(cipher.length) @@ -203,19 +270,25 @@ public byte[] toByteAray() public static EncryptedData fromByteArray(byte[] array) { - ByteBuffer bb = ByteBuffer.wrap(array); - - int saltSize = bb.getInt(); - int ivSize = bb.getInt(); - int cipherSize = bb.getInt(); - - byte[] salt = new byte[saltSize]; + Preconditions.checkArgument(array.length >= 12, "Invalid encrypted data"); + final ByteBuffer bb = ByteBuffer.wrap(array); + + final int saltSize = bb.getInt(); + final int ivSize = bb.getInt(); + final int cipherSize = bb.getInt(); + final long payloadSize = (long) saltSize + ivSize + cipherSize; + Preconditions.checkArgument( + saltSize >= 0 && ivSize >= 0 && cipherSize >= 0 && payloadSize == bb.remaining(), + "Invalid encrypted data" + ); + + final byte[] salt = new byte[saltSize]; bb.get(salt); - byte[] iv = new byte[ivSize]; + final byte[] iv = new byte[ivSize]; bb.get(iv); - byte[] cipher = new byte[cipherSize]; + final byte[] cipher = new byte[cipherSize]; bb.get(cipher); return new EncryptedData(salt, iv, cipher); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java index 55119a9e4ccc..d2197d0e254c 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -232,18 +232,18 @@ public List requiredFields() public byte[] getCacheKey() { try { - MessageDigest md = MessageDigest.getInstance("SHA-1"); - byte[] fieldNameBytes = StringUtils.toUtf8(Joiner.on(",").join(fieldNames)); - byte[] sha1 = md.digest(StringUtils.toUtf8(fnAggregate + fnReset + fnCombine)); + final MessageDigest md = MessageDigest.getInstance("SHA-256"); + final byte[] fieldNameBytes = StringUtils.toUtf8(Joiner.on(",").join(fieldNames)); + final byte[] scriptDigest = md.digest(StringUtils.toUtf8(fnAggregate + fnReset + fnCombine)); - return ByteBuffer.allocate(1 + fieldNameBytes.length + sha1.length) + return ByteBuffer.allocate(1 + fieldNameBytes.length + scriptDigest.length) .put(AggregatorUtil.JS_CACHE_TYPE_ID) .put(fieldNameBytes) - .put(sha1) + .put(scriptDigest) .array(); } catch (NoSuchAlgorithmException e) { - throw new RuntimeException("Unable to get SHA1 digest instance", e); + throw new RuntimeException("Unable to get SHA-256 digest instance", e); } } diff --git a/processing/src/test/java/org/apache/druid/crypto/CryptoServiceTest.java b/processing/src/test/java/org/apache/druid/crypto/CryptoServiceTest.java index a0090db8555b..302a02c68607 100644 --- a/processing/src/test/java/org/apache/druid/crypto/CryptoServiceTest.java +++ b/processing/src/test/java/org/apache/druid/crypto/CryptoServiceTest.java @@ -19,32 +19,97 @@ package org.apache.druid.crypto; +import org.apache.druid.error.DruidException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Base64; public class CryptoServiceTest { + private static final String PASSPHRASE = "random-passphrase"; + private static final CryptoService CRYPTO_SERVICE = createCryptoService(PASSPHRASE); + + /** + * Fixture written by the legacy AES/CBC/PKCS5Padding implementation with an eight-byte salt and a 16-byte IV. + */ + private static final byte[] LEGACY_CIPHERTEXT = Base64.getDecoder().decode( + "AAAACAAAABAAAAAgAAECAwQFBgcQERITFBUWFxgZGhscHR4fv7hZzZdQZX4Q18kH0DXtzmraN52ciCGxrcutYbQ3qp8=" + ); + @Test public void testEncryptDecrypt() { - CryptoService cryptoService = new CryptoService( - "random-passphrase", - "AES", - "CBC", - "PKCS5Padding", - "PBKDF2WithHmacSHA256", - 8, - 65536, - 128 + final byte[] original = "i am a test string".getBytes(StandardCharsets.UTF_8); + + final byte[] encrypted = CRYPTO_SERVICE.encrypt(original); + final byte[] decrypted = CRYPTO_SERVICE.decrypt(encrypted); + + Assertions.assertArrayEquals(original, decrypted); + } + + @Test + public void testEncryptUsesFreshAuthenticatedCiphertext() + { + final byte[] original = "i am a test string".getBytes(StandardCharsets.UTF_8); + final byte[] first = CRYPTO_SERVICE.encrypt(original); + final byte[] second = CRYPTO_SERVICE.encrypt(original); + + Assertions.assertFalse(Arrays.equals(first, second)); + Assertions.assertArrayEquals(original, CRYPTO_SERVICE.decrypt(first)); + Assertions.assertArrayEquals(original, CRYPTO_SERVICE.decrypt(second)); + } + + @Test + public void testDecryptLegacyCiphertext() + { + Assertions.assertEquals( + "legacy ciphertext", + new String(CRYPTO_SERVICE.decrypt(LEGACY_CIPHERTEXT), StandardCharsets.UTF_8) ); + } + + @Test + public void testAuthenticatedCiphertextRejectsTampering() + { + final byte[] encrypted = CRYPTO_SERVICE.encrypt("authenticated".getBytes(StandardCharsets.UTF_8)); - byte[] original = "i am a test string".getBytes(StandardCharsets.UTF_8); + // Format version, salt, IV, and cipher text are all protected or safely rejected during parsing. + assertDecryptionFails(withFlippedByte(encrypted, 4)); + assertDecryptionFails(withFlippedByte(encrypted, 17)); + assertDecryptionFails(withFlippedByte(encrypted, 25)); + assertDecryptionFails(withFlippedByte(encrypted, encrypted.length - 1)); + assertDecryptionFails(Arrays.copyOf(encrypted, encrypted.length - 1)); + } - byte[] decrypted = cryptoService.decrypt(cryptoService.encrypt(original)); + @Test + public void testAuthenticatedCiphertextCannotBeDowngradedToLegacyFormat() + { + final byte[] encrypted = CRYPTO_SERVICE.encrypt("authenticated".getBytes(StandardCharsets.UTF_8)); + assertDecryptionFails(withFlippedByte(encrypted, 0)); + } - Assertions.assertArrayEquals(original, decrypted); + @Test + public void testAuthenticatedCiphertextRejectsWrongPassphrase() + { + final byte[] encrypted = CRYPTO_SERVICE.encrypt("authenticated".getBytes(StandardCharsets.UTF_8)); + final CryptoService otherCryptoService = createCryptoService("different-passphrase"); + + Assertions.assertThrows(DruidException.class, () -> otherCryptoService.decrypt(encrypted)); + } + + @Test + public void testMalformedLegacyLengthsAreRejected() + { + final byte[] malformed = ByteBuffer.allocate(12) + .putInt(Integer.MAX_VALUE) + .putInt(Integer.MAX_VALUE) + .putInt(Integer.MAX_VALUE) + .array(); + assertDecryptionFails(malformed); } @Test @@ -53,7 +118,7 @@ public void testInvalidParamsConstructorFailure() Assertions.assertThrows( RuntimeException.class, () -> new CryptoService( - "random-passphrase", + PASSPHRASE, "ABCD", "EFGH", "PAXXDDING", @@ -64,4 +129,47 @@ public void testInvalidParamsConstructorFailure() ) ); } + + @Test + public void testInvalidLegacyCipherParametersFailWhenDecryptingLegacyCiphertext() + { + final CryptoService cryptoService = new CryptoService( + PASSPHRASE, + "ABCD", + "EFGH", + "PAXXDDING", + "PBKDF2WithHmacSHA256", + 8, + 65536, + 128 + ); + + Assertions.assertThrows(DruidException.class, () -> cryptoService.decrypt(LEGACY_CIPHERTEXT)); + } + + private static CryptoService createCryptoService(final String passphrase) + { + return new CryptoService( + passphrase, + "AES", + "CBC", + "PKCS5Padding", + "PBKDF2WithHmacSHA256", + 8, + 65536, + 128 + ); + } + + private static byte[] withFlippedByte(final byte[] original, final int index) + { + final byte[] tampered = original.clone(); + tampered[index] ^= 0x01; + return tampered; + } + + private static void assertDecryptionFails(final byte[] encrypted) + { + Assertions.assertThrows(DruidException.class, () -> CRYPTO_SERVICE.decrypt(encrypted)); + } } diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/JavaScriptAggregatorTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/JavaScriptAggregatorTest.java index 2275b915f526..5d423fe4a563 100644 --- a/processing/src/test/java/org/apache/druid/query/aggregation/JavaScriptAggregatorTest.java +++ b/processing/src/test/java/org/apache/druid/query/aggregation/JavaScriptAggregatorTest.java @@ -286,6 +286,21 @@ public void testJavaScriptDisabledFactorizeBuffered() Assert.assertTrue(false); } + @Test + public void testCacheKeyUsesSha256Digest() + { + final JavaScriptAggregatorFactory factory = new JavaScriptAggregatorFactory( + "foo", + ImmutableList.of("foo"), + SCRIPT_DOUBLE_SUM.get("fnAggregate"), + SCRIPT_DOUBLE_SUM.get("fnReset"), + SCRIPT_DOUBLE_SUM.get("fnCombine"), + new JavaScriptConfig(false) + ); + + Assert.assertEquals(1 + StringUtils.toUtf8("foo").length + 32, factory.getCacheKey().length); + } + public static void main(String... args) { final JavaScriptAggregatorBenchmark.LoopingDoubleColumnSelector selector = new JavaScriptAggregatorBenchmark.LoopingDoubleColumnSelector( diff --git a/server/src/test/java/org/apache/druid/client/cache/MemcachedCacheBenchmark.java b/server/src/test/java/org/apache/druid/client/cache/MemcachedCacheBenchmark.java index fcb42b101cb1..2e34139f5c7b 100644 --- a/server/src/test/java/org/apache/druid/client/cache/MemcachedCacheBenchmark.java +++ b/server/src/test/java/org/apache/druid/client/cache/MemcachedCacheBenchmark.java @@ -108,6 +108,8 @@ public int getExpiration() ); randBytes = new byte[objectSize * 1024]; + // A fresh fixed-seed generator keeps the representative payload identical across benchmark setup invocations. + // codeql[java/random-used-once] new Random(0).nextBytes(randBytes); }