Use PKCS5Padding instead of ISO10126Padding for secrets encryption (#…

…14193)

* Use PKCS5 instead of ISO10126 padding for secrets encryption

If Graylog is run in an FIPS environment there is no
crypto provider available that supports "AES/CBC/ISO10126Padding".
This is likely because this padding standard was withdrawn from ISO in
2007.
It is considered bad practice to leave a subliminal channel in the
padding.

We tried to workaround this by explicitly using BouncyCastle
for the encryption/decryption.

This however creates problems with Oracle Java, because
we strip the signature off the bouncy castle jar while repackaging
it into our Uber-Jar.
In contrast to OpenJDK, Oracle Java does not allow the use
of unsigned Security Providers.

Solution:

Change our secret encryption to using PKCS5Padding instead.
There is a FIPS compatible provider "SunPKCS11-NSS-FIPS"
available which supports "AES/CBC/PKCS5Padding".

For backwards compatibility, we decrypt the ISO10126Padded keys
without stripping the padding, and do that manually.

Fixes #14153

Refs #13525

* use explicit type to simplify backport

* Also try legacy decoding when catching a ProviderException

This seems to happen with FIPS enabled OpenJDK 17

* Try decrypting legacy keys on any Exception

* improve wording on comments

* improve changelog

changelog/unreleased/issue-14153.toml

@@ -0,0 +1,8 @@
+type = "fixed"
+message = "Improve JVM security provider compatibility by switching to a widely supported cipher transformation"
+
+issues = ["14153"]
+pulls = ["14193"]
+
+contributors = [""]
+

graylog2-server/src/main/java/org/graylog2/security/AESTools.java

@@ -17,7 +17,8 @@
 package org.graylog2.security;
 
 import org.apache.shiro.codec.Hex;
-import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.bouncycastle.crypto.InvalidCipherTextException;
+import org.bouncycastle.crypto.paddings.ISO10126d2Padding;
 import org.cryptomator.siv.SivMode;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -26,6 +27,7 @@
 import javax.crypto.Cipher;
 import javax.crypto.spec.IvParameterSpec;
 import javax.crypto.spec.SecretKeySpec;
+import java.security.GeneralSecurityException;
 import java.security.SecureRandom;
 import java.util.Arrays;
 
@@ -38,10 +40,8 @@ public class AESTools {
 
     private static final SivMode SIV_MODE = new SivMode();
 
-    // We use the Bouncy Castle provider to ensure the availability of the ISO10126 padding on FIPS enabled JVM
-    // environments. See: https://github.com/Graylog2/graylog2-server/issues/13525
-    private static final BouncyCastleProvider CIPHER_PROVIDER = new BouncyCastleProvider();
-    private static final String CIPHER_TRANSFORMATION = "AES/CBC/ISO10126Padding";
+    private static final String CIPHER_TRANSFORMATION = "AES/CBC/PKCS5Padding";
+    private static final String CIPHER_NO_PADDING_TRANSFORMATION = "AES/CBC/NoPadding";
 
     /**
      * Encrypt the given plain text value with the given encryption key and salt using AES CBC.
@@ -60,10 +60,10 @@ public static String encrypt(String plainText, String encryptionKey, String salt
         checkNotNull(salt, "Salt must not be null.");
         try {
             @SuppressWarnings("CIPHER_INTEGRITY")
-            Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION, CIPHER_PROVIDER);
+            Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION);
             SecretKeySpec key = new SecretKeySpec(adjustToIdealKeyLength(encryptionKey), "AES");
-            cipher.init(Cipher.ENCRYPT_MODE, key, new IvParameterSpec(salt.getBytes("UTF-8")));
-            return Hex.encodeToString(cipher.doFinal(plainText.getBytes("UTF-8")));
+            cipher.init(Cipher.ENCRYPT_MODE, key, new IvParameterSpec(salt.getBytes(UTF_8)));
+            return Hex.encodeToString(cipher.doFinal(plainText.getBytes(UTF_8)));
         } catch (Exception e) {
             LOG.error("Could not encrypt value.", e);
         }
@@ -88,14 +88,32 @@ public static String decrypt(String cipherText, String encryptionKey, String sal
         checkNotNull(salt, "Salt must not be null.");
         try {
             @SuppressWarnings("CIPHER_INTEGRITY")
-            Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION, CIPHER_PROVIDER);
+            Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION);
             SecretKeySpec key = new SecretKeySpec(adjustToIdealKeyLength(encryptionKey), "AES");
-            cipher.init(Cipher.DECRYPT_MODE, key, new IvParameterSpec(salt.getBytes("UTF-8")));
-            return new String(cipher.doFinal(Hex.decode(cipherText)), "UTF-8");
-        } catch (Exception e) {
-            LOG.error("Could not decrypt value.", e);
+            cipher.init(Cipher.DECRYPT_MODE, key, new IvParameterSpec(salt.getBytes(UTF_8)));
+            return new String(cipher.doFinal(Hex.decode(cipherText)), UTF_8);
+        } catch (Exception ignored) {
+            // This is likely a BadPaddingException, but try to decrypt legacy secrets in any case
+            try {
+                return decryptLegacy(cipherText, encryptionKey, salt);
+            } catch (Exception ex) {
+                LOG.error("Could not decrypt (legacy) value.", ex);
+                return null;
+            }
         }
-        return null;
+    }
+
+    // Decrypt secrets that were created with ISO10126Padding
+    // First decrypt it with a "no padding" transform and then strip the padding manually.
+    // We do this because the ISO10126Padding has been deprecated and is not present
+    // when running in FIPS mode.
+    private static String decryptLegacy(String cipherText, String encryptionKey, String salt) throws GeneralSecurityException, InvalidCipherTextException {
+        Cipher cipher = Cipher.getInstance(CIPHER_NO_PADDING_TRANSFORMATION);
+        SecretKeySpec key = new SecretKeySpec(adjustToIdealKeyLength(encryptionKey), "AES");
+        cipher.init(Cipher.DECRYPT_MODE, key, new IvParameterSpec(salt.getBytes(UTF_8)));
+        byte[] decrypted = cipher.doFinal(Hex.decode(cipherText));
+        final int padCount = new ISO10126d2Padding().padCount(decrypted);
+        return new String(Arrays.copyOf(decrypted, decrypted.length - padCount), UTF_8);
     }
 
     /**

graylog2-server/src/test/java/org/graylog2/security/AESToolsTest.java

@@ -39,8 +39,8 @@ public void testEncryptDecrypt() {
     }
 
     @Test
-    public void testDecryptStaticCipherText() {
-        // The cipherText was encrypted using an AES/CBC/ISO10126Padding transformation.
+    public void testDecryptStaticISO10126PaddedCipherText() {
+        // The cipherText was encrypted using the legacy AES/CBC/ISO10126Padding transformation.
         // If this test fails, we changed the transformation. If the change was intentional, this test must
         // be updated, and we need to create a migration to re-encrypt all existing secrets in the database.
         // Otherwise, existing secrets cannot be decrypted anymore!
@@ -50,6 +50,31 @@ public void testDecryptStaticCipherText() {
         final String decrypt = AESTools.decrypt(cipherText, "1234567890123456", salt);
         Assert.assertEquals("I am secret", decrypt);
     }
+    @Test
+    public void testDecryptStaticISO10126PaddedLongCipherText() {
+        // The cipherText was encrypted using the legacy AES/CBC/ISO10126Padding transformation.
+        // If this test fails, we changed the transformation. If the change was intentional, this test must
+        // be updated, and we need to create a migration to re-encrypt all existing secrets in the database.
+        // Otherwise, existing secrets cannot be decrypted anymore!
+        final String cipherText = "d8d9ade5456543950e7ff7441b7157ed71564f5b7d656098a8ec87c074ed2d333a797711e06817135ef6d7cfce0a2eb6";
+        final String salt = "17c4bd3761b530f7";
+
+        final String decrypt = AESTools.decrypt(cipherText, "1234567890123456", salt);
+        Assert.assertEquals("I am a very very very long secret", decrypt);
+    }
+
+    @Test
+    public void testDecryptStaticPKCS5PaddedCipherText() {
+        // The cipherText was encrypted using an AES/CBC/PKCS5Padding transformation.
+        // If this test fails, we changed the transformation. If the change was intentional, this test must
+        // be updated, and we need to create a migration to re-encrypt all existing secrets in the database.
+        // Otherwise, existing secrets cannot be decrypted anymore!
+        final String cipherText = "f0b3e951a4b4537e1466a9cd9621eabb";
+        final String salt = "612ac41505dc0120";
+
+        final String decrypt = AESTools.decrypt(cipherText, "1234567890123456", salt);
+        Assert.assertEquals("I am secret", decrypt);
+    }
 
     @Test
     public void testEncryptDecryptWithKeyBeingLargerThan32Bytes() {