diff options
5 files changed, 96 insertions, 33 deletions
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java index f4cd35bc5bd..4a8353d92ec 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java @@ -29,7 +29,6 @@ import com.yahoo.vespa.hosted.node.admin.task.util.file.MakeDirectory; import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath; import com.yahoo.vespa.hosted.node.admin.task.util.fs.ContainerPath; -import javax.crypto.CipherOutputStream; import java.io.IOException; import java.io.OutputStream; import java.io.UncheckedIOException; @@ -49,7 +48,6 @@ import java.util.function.Supplier; import java.util.logging.Logger; import java.util.regex.Pattern; import java.util.stream.IntStream; -import java.util.stream.Stream; import static com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder.nameEndsWith; import static com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder.nameMatches; @@ -275,7 +273,7 @@ public class CoredumpHandler { static OutputStream maybeWrapWithEncryption(OutputStream wrappedStream, Optional<SecretSharedKey> sharedCoreKey) { return sharedCoreKey - .map(key -> (OutputStream)new CipherOutputStream(wrappedStream, SharedKeyGenerator.makeAesGcmEncryptionCipher(key))) + .map(key -> SharedKeyGenerator.makeAesGcmEncryptionCipher(key).wrapOutputStream(wrappedStream)) .orElse(wrappedStream); } diff --git a/security-utils/src/main/java/com/yahoo/security/AeadCipher.java b/security-utils/src/main/java/com/yahoo/security/AeadCipher.java new file mode 100644 index 00000000000..598f5d01db7 --- /dev/null +++ b/security-utils/src/main/java/com/yahoo/security/AeadCipher.java @@ -0,0 +1,44 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.security; + +import org.bouncycastle.crypto.io.CipherInputStream; +import org.bouncycastle.crypto.io.CipherOutputStream; +import org.bouncycastle.crypto.modes.AEADBlockCipher; + +import java.io.InputStream; +import java.io.OutputStream; + +/** + * AEAD cipher wrapper to hide the underlying crypto provider used. + * + * @author vekterli + */ +public class AeadCipher { + + private final AEADBlockCipher cipher; + + private AeadCipher(AEADBlockCipher cipher) { + this.cipher = cipher; + } + + static AeadCipher of(AEADBlockCipher cipher) { + return new AeadCipher(cipher); + } + + /** + * Returns a wrapping <code>OutputStream</code> that, depending on the cipher mode, either + * encrypts or decrypts all data that is written to it before passing it on to <code>out</code>. + */ + public OutputStream wrapOutputStream(OutputStream out) { + return new CipherOutputStream(out, cipher); + } + + /** + * Returns a wrapping <code>InputStream</code> that, depending on the cipher mode, either + * encrypts or decrypts all data that is read from the underlying input stream. + */ + public InputStream wrapInputStream(InputStream in) { + return new CipherInputStream(in, cipher); + } + +} diff --git a/security-utils/src/main/java/com/yahoo/security/SharedKeyGenerator.java b/security-utils/src/main/java/com/yahoo/security/SharedKeyGenerator.java index 66a87a94707..16bd82e2af3 100644 --- a/security-utils/src/main/java/com/yahoo/security/SharedKeyGenerator.java +++ b/security-utils/src/main/java/com/yahoo/security/SharedKeyGenerator.java @@ -6,15 +6,14 @@ import com.yahoo.security.hpke.Ciphersuite; import com.yahoo.security.hpke.Hpke; import com.yahoo.security.hpke.Kdf; import com.yahoo.security.hpke.Kem; +import org.bouncycastle.crypto.engines.AESEngine; +import org.bouncycastle.crypto.modes.GCMBlockCipher; +import org.bouncycastle.crypto.params.AEADParameters; +import org.bouncycastle.crypto.params.KeyParameter; -import javax.crypto.Cipher; import javax.crypto.KeyGenerator; -import javax.crypto.NoSuchPaddingException; import javax.crypto.SecretKey; -import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.SecretKeySpec; -import java.security.InvalidAlgorithmParameterException; -import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; import java.security.PrivateKey; import java.security.PublicKey; @@ -93,33 +92,29 @@ public class SharedKeyGenerator { 'h','e','r','e','B','d','r','a','g','o','n','s' // Nothing up my sleeve! }; - private static Cipher makeAesGcmCipher(SecretSharedKey secretSharedKey, int cipherMode) { - try { - var cipher = Cipher.getInstance(AES_GCM_ALGO_SPEC); - var gcmSpec = new GCMParameterSpec(AES_GCM_AUTH_TAG_BITS, FIXED_96BIT_IV_FOR_SINGLE_USE_KEY); - cipher.init(cipherMode, secretSharedKey.secretKey(), gcmSpec); - return cipher; - } catch (NoSuchAlgorithmException | NoSuchPaddingException - | InvalidKeyException | InvalidAlgorithmParameterException e) { - throw new RuntimeException(e); - } + private static AeadCipher makeAesGcmCipher(SecretSharedKey secretSharedKey, boolean forEncryption) { + var aeadParams = new AEADParameters(new KeyParameter(secretSharedKey.secretKey().getEncoded()), + AES_GCM_AUTH_TAG_BITS, FIXED_96BIT_IV_FOR_SINGLE_USE_KEY); + var cipher = new GCMBlockCipher(new AESEngine()); + cipher.init(forEncryption, aeadParams); + return AeadCipher.of(cipher); } /** - * Creates an AES-GCM Cipher that can be used to encrypt arbitrary plaintext. + * Creates an AES-GCM cipher that can be used to encrypt arbitrary plaintext. * * The given secret key MUST NOT be used to encrypt more than one plaintext. */ - public static Cipher makeAesGcmEncryptionCipher(SecretSharedKey secretSharedKey) { - return makeAesGcmCipher(secretSharedKey, Cipher.ENCRYPT_MODE); + public static AeadCipher makeAesGcmEncryptionCipher(SecretSharedKey secretSharedKey) { + return makeAesGcmCipher(secretSharedKey, true); } /** - * Creates an AES-GCM Cipher that can be used to decrypt ciphertext that was previously + * Creates an AES-GCM cipher that can be used to decrypt ciphertext that was previously * encrypted with the given secret key. */ - public static Cipher makeAesGcmDecryptionCipher(SecretSharedKey secretSharedKey) { - return makeAesGcmCipher(secretSharedKey, Cipher.DECRYPT_MODE); + public static AeadCipher makeAesGcmDecryptionCipher(SecretSharedKey secretSharedKey) { + return makeAesGcmCipher(secretSharedKey, false); } } diff --git a/security-utils/src/test/java/com/yahoo/security/SharedKeyTest.java b/security-utils/src/test/java/com/yahoo/security/SharedKeyTest.java index 23e22345cc6..aede100574d 100644 --- a/security-utils/src/test/java/com/yahoo/security/SharedKeyTest.java +++ b/security-utils/src/test/java/com/yahoo/security/SharedKeyTest.java @@ -3,8 +3,6 @@ package com.yahoo.security; import org.junit.jupiter.api.Test; -import javax.crypto.CipherInputStream; -import javax.crypto.CipherOutputStream; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -142,7 +140,7 @@ public class SharedKeyTest { static byte[] streamEncryptString(String data, SecretSharedKey secretSharedKey) throws IOException { var cipher = SharedKeyGenerator.makeAesGcmEncryptionCipher(secretSharedKey); var outStream = new ByteArrayOutputStream(); - try (var cipherStream = new CipherOutputStream(outStream, cipher)) { + try (var cipherStream = cipher.wrapOutputStream(outStream)) { cipherStream.write(data.getBytes(StandardCharsets.UTF_8)); cipherStream.flush(); } @@ -154,7 +152,7 @@ public class SharedKeyTest { var inStream = new ByteArrayInputStream(encrypted); var total = ByteBuffer.allocate(encrypted.length); // Assume decrypted form can't be _longer_ byte[] tmp = new byte[8]; // short buf to test chunking - try (var cipherStream = new CipherInputStream(inStream, cipher)) { + try (var cipherStream = cipher.wrapInputStream(inStream)) { while (true) { int read = cipherStream.read(tmp); if (read == -1) { @@ -180,4 +178,32 @@ public class SharedKeyTest { assertEquals(terrifyingSecret, decrypted); } + // javax.crypto.CipherOutputStream swallows exceptions caused by MAC failures in cipher + // decryption mode (!) and must therefore _not_ be used for this purpose. This is documented, + // but still very surprising behavior. + @Test + void cipher_output_stream_tag_mismatch_is_not_swallowed() throws Exception { + var receiverKeyPair = KeyUtils.generateX25519KeyPair(); + var myShared = SharedKeyGenerator.generateForReceiverPublicKey(receiverKeyPair.getPublic(), KEY_ID_1); + String plaintext = "...hello world?"; + byte[] encrypted = streamEncryptString(plaintext, myShared); + // Corrupt MAC tag in ciphertext + encrypted[encrypted.length - 1] ^= 0x80; + // We don't necessarily know _which_ exception is thrown, but one _should_ be thrown! + assertThrows(Exception.class, () -> doOutputStreamCipherDecrypt(myShared, encrypted)); + // Also try with corrupted ciphertext (pre MAC tag) + encrypted[encrypted.length - 1] ^= 0x80; // Flip MAC bit back to correct state + encrypted[encrypted.length - 17] ^= 0x80; // Pre 128-bit MAC tag + assertThrows(Exception.class, () -> doOutputStreamCipherDecrypt(myShared, encrypted)); + } + + private static void doOutputStreamCipherDecrypt(SecretSharedKey myShared, byte[] encrypted) throws Exception { + var cipher = SharedKeyGenerator.makeAesGcmDecryptionCipher(myShared); + var outStream = new ByteArrayOutputStream(); + try (var cipherStream = cipher.wrapOutputStream(outStream)) { + cipherStream.write(encrypted); + cipherStream.flush(); + } + } + } diff --git a/vespaclient-java/src/main/java/com/yahoo/vespa/security/tool/crypto/CipherUtils.java b/vespaclient-java/src/main/java/com/yahoo/vespa/security/tool/crypto/CipherUtils.java index 051189c20b6..87d3cb4d9f0 100644 --- a/vespaclient-java/src/main/java/com/yahoo/vespa/security/tool/crypto/CipherUtils.java +++ b/vespaclient-java/src/main/java/com/yahoo/vespa/security/tool/crypto/CipherUtils.java @@ -1,8 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.security.tool.crypto; -import javax.crypto.Cipher; -import javax.crypto.CipherOutputStream; +import com.yahoo.security.AeadCipher; + import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -19,11 +19,11 @@ public class CipherUtils { * * @param input source stream to read from * @param output destination stream to write to - * @param cipher a Cipher in either ENCRYPT or DECRYPT mode + * @param cipher an {@link AeadCipher} created with for either encryption or decryption * @throws IOException if any file operation fails */ - public static void streamEncipher(InputStream input, OutputStream output, Cipher cipher) throws IOException { - try (var cipherStream = new CipherOutputStream(output, cipher)) { + public static void streamEncipher(InputStream input, OutputStream output, AeadCipher cipher) throws IOException { + try (var cipherStream = cipher.wrapOutputStream(output)) { input.transferTo(cipherStream); cipherStream.flush(); } |