Merge pull request #24891 from vespa-engine/vekterli/use-bouncycastle-for-cipher-and-streaming

Use BouncyCastle AES GCM cipher and I/O streams instead of JCA

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();
         }