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

This resolves two issues:
 * `javax.crypto.OutputCipherStream` swallows MAC tag mismatch exceptions
   when the stream is closed, which means that corruptions (intentional
   or not) are not caught. This is documented behavior, but still very
   surprising and a rather questionable default. BC's interchangeable
   `CipherOutputStream` throws as expected. To avoid regressions, add an
   explicit test that both ciphertext and MAC tag corruptions are propagated.
 * The default-provided `AES/GCM/NoPadding` `Cipher` instance will not emit
   decrypted plaintext per `update()` chunk, but buffer everything until
   `doFinal()` is invoked when the stream is closed. This means that decrypting
   very large ciphertexts can blow up memory usage since internal output
   buffers are reallocated and increased per iteration...! Instead use an
   explicit BC `GCMBlockCipher` which has the expected behavior (and actually
   lets cipher streams, well, _stream_). Add an `AeadCipher` abstraction to
   avoid leaking BC APIs outside the security module.

3 files changed, 90 insertions, 25 deletions

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