From 794853ef4b045a236e85e36c7546ba8659f43f09 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 20 Jul 2026 19:05:16 +0300 Subject: [PATCH 1/2] Use AES/GCM instead of AES/CBC for json-crypto session encryption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL java/weak-cryptographic-algorithm flagged the hardcoded AES/CBC/PKCS5Padding session cipher in SimpleEncryptor.asymmetric() as vulnerable to padding-oracle attacks. Switch it to authenticated AES/GCM/NoPadding: a fresh session key is generated per message, so the random 96-bit nonce is never reused under the same key. Make SimpleDecryptor GCM-aware — it selects a GCMParameterSpec for GCM transformations and keeps IvParameterSpec for CBC and other modes. The self-describing "cipher" field means existing CBC/ECB values still decrypt unchanged. Add a GCM round-trip test alongside the existing CBC coverage. --- .../json/crypto/simple/SimpleDecryptor.java | 36 +++++++++++++++++-- .../json/crypto/simple/SimpleEncryptor.java | 14 +++++--- .../forgerock/json/crypto/JsonCryptoTest.java | 12 +++++++ 3 files changed, 55 insertions(+), 7 deletions(-) diff --git a/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleDecryptor.java b/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleDecryptor.java index f4d06c13ef..a5a8ef6833 100644 --- a/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleDecryptor.java +++ b/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleDecryptor.java @@ -12,16 +12,19 @@ * information: "Portions Copyrighted [year] [name of copyright owner]". * * Copyright 2011-2015 ForgeRock AS. + * Portions Copyrighted 2026 3A Systems, LLC */ package org.forgerock.json.crypto.simple; import javax.crypto.Cipher; +import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; import java.io.IOException; import java.security.GeneralSecurityException; import java.security.Key; +import java.security.spec.AlgorithmParameterSpec; import com.fasterxml.jackson.databind.ObjectMapper; import org.forgerock.json.crypto.JsonCryptoException; @@ -39,6 +42,9 @@ public class SimpleDecryptor implements JsonDecryptor { /** The type of cryptographic representation that this decryptor supports. */ public static final String TYPE = "x-simple-encryption"; + /** GCM authentication tag length, in bits, matching the encryptor default. */ + private static final int GCM_TAG_LENGTH_BITS = 128; + /** Converts between JSON constructs and Java objects. */ private final ObjectMapper mapper = new ObjectMapper(); @@ -84,8 +90,7 @@ public JsonValue decrypt(JsonValue value) throws JsonCryptoException { } Cipher symmetric = Cipher.getInstance(cipher); String iv = value.get("iv").asString(); - IvParameterSpec ivps = (iv == null ? null : new IvParameterSpec(Base64.decode(iv))); - symmetric.init(Cipher.DECRYPT_MODE, symmetricKey, ivps); + symmetric.init(Cipher.DECRYPT_MODE, symmetricKey, parameterSpec(cipher, iv)); byte[] plaintext = symmetric.doFinal(Base64.decode(value.get("data").required().asString())); return new JsonValue(mapper.readValue(plaintext, Object.class)); } catch (GeneralSecurityException | IOException | JsonValueException e) { @@ -93,4 +98,31 @@ public JsonValue decrypt(JsonValue value) throws JsonCryptoException { } } + /** + * Builds the algorithm parameters for the given cipher transformation and + * Base64-encoded IV/nonce. GCM requires a {@link GCMParameterSpec}; other + * modes (such as CBC) use a plain {@link IvParameterSpec}. Values written + * without an IV (for example legacy ECB) yield {@code null}, preserving the + * previous behaviour. Because the {@code cipher} field is self-describing, + * older CBC/ECB values continue to decrypt unchanged. + * + * @param cipher the cipher transformation stored with the encrypted value. + * @param iv the Base64-encoded IV/nonce, or {@code null} if none was stored. + * @return the algorithm parameter spec, or {@code null} when no IV is present. + */ + private static AlgorithmParameterSpec parameterSpec(String cipher, String iv) { + if (iv == null) { + return null; + } + byte[] ivBytes = Base64.decode(iv); + return isGcm(cipher) + ? new GCMParameterSpec(GCM_TAG_LENGTH_BITS, ivBytes) + : new IvParameterSpec(ivBytes); + } + + private static boolean isGcm(String cipher) { + String[] parts = cipher.split("/"); + return parts.length > 1 && parts[1].equalsIgnoreCase("GCM"); + } + } diff --git a/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleEncryptor.java b/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleEncryptor.java index 00874d5eca..5f4bfae124 100644 --- a/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleEncryptor.java +++ b/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleEncryptor.java @@ -101,11 +101,15 @@ private Object symmetric(Object object) throws GeneralSecurityException, IOExcep * @throws IOException if an I/O exception occurred. */ private Object asymmetric(Object object) throws GeneralSecurityException, IOException { - // Use CBC with a random IV rather than ECB; ECB leaks plaintext block - // patterns regardless of session-key freshness (CWE-327). The IV is stored - // alongside the data and the self-describing "cipher" field keeps this - // backward compatible with values previously written using ECB. - String symmetricCipher = "AES/CBC/PKCS5Padding"; + // Use GCM (authenticated encryption) with a random 96-bit nonce rather + // than CBC or ECB. GCM provides integrity and is not vulnerable to the + // padding-oracle attacks that affect CBC/PKCS#5, nor does it leak block + // patterns like ECB (CWE-327). A fresh session key is generated for every + // message, so the randomly generated nonce is never reused under the same + // key. The nonce is stored alongside the data and the self-describing + // "cipher" field keeps this backward compatible with values previously + // written using CBC or ECB. + String symmetricCipher = "AES/GCM/NoPadding"; KeyGenerator generator = KeyGenerator.getInstance("AES"); generator.init(128); SecretKey sessionKey = generator.generateKey(); diff --git a/commons/json-crypto/core/src/test/java/org/forgerock/json/crypto/JsonCryptoTest.java b/commons/json-crypto/core/src/test/java/org/forgerock/json/crypto/JsonCryptoTest.java index 58f6ecc9a7..567012e434 100644 --- a/commons/json-crypto/core/src/test/java/org/forgerock/json/crypto/JsonCryptoTest.java +++ b/commons/json-crypto/core/src/test/java/org/forgerock/json/crypto/JsonCryptoTest.java @@ -44,6 +44,8 @@ public class JsonCryptoTest { private static final String SYMMETRIC_CIPHER = "AES/CBC/PKCS5Padding"; + private static final String SYMMETRIC_GCM_CIPHER = "AES/GCM/NoPadding"; + private static final String ASYMMETRIC_CIPHER = "RSA/ECB/OAEPWithSHA1AndMGF1Padding"; private static final String PASSWORD = "P@55W0RD"; @@ -106,6 +108,16 @@ public void testAsymmetricEncryption() throws JsonCryptoException { assertThat(value.getObject()).isEqualTo(PLAINTEXT); } + @Test + public void testSymmetricGcmEncryption() throws JsonCryptoException { + JsonValue value = new JsonValue(PLAINTEXT); + value = new SimpleEncryptor(SYMMETRIC_GCM_CIPHER, secretKey, "secretKey").encrypt(value); + assertThat(value.getObject()).isNotEqualTo(PLAINTEXT); + assertThat(((Map) value.getObject()).get("cipher")).isEqualTo(SYMMETRIC_GCM_CIPHER); + value = new SimpleDecryptor(selector).decrypt(value); + assertThat(value.getObject()).isEqualTo(PLAINTEXT); + } + // @Test // public void testJsonCryptoTransformer() throws JsonCryptoException { // JsonValue value = new JsonValue(PLAINTEXT); From 3c163646f562d53ddc7dfa718d2acc01d5e87a9d Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 21 Jul 2026 13:40:30 +0300 Subject: [PATCH 2/2] Pin GCM parameters explicitly and enforce GCM/legacy coverage in tests - Pass GCMParameterSpec (96-bit nonce, 128-bit tag) explicitly instead of relying on provider-specific defaults, in both the session-key and the caller-supplied-key paths, matching the tag length SimpleDecryptor expects - Assert the asymmetric envelope uses AES/GCM/NoPadding so reverting the cipher switch fails the suite - Add checked-in AES/ECB and AES/CBC fixtures with a fixed key so legacy decryption is enforced by CI, not just same-build roundtrips - Document the NIST SP 800-38D 2^32-invocations bound for GCM under long-lived caller-supplied keys --- .../json/crypto/simple/SimpleDecryptor.java | 2 +- .../json/crypto/simple/SimpleEncryptor.java | 40 +++++++++++++-- .../forgerock/json/crypto/JsonCryptoTest.java | 50 +++++++++++++++++++ 3 files changed, 88 insertions(+), 4 deletions(-) diff --git a/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleDecryptor.java b/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleDecryptor.java index a5a8ef6833..22cae9bc38 100644 --- a/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleDecryptor.java +++ b/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleDecryptor.java @@ -42,7 +42,7 @@ public class SimpleDecryptor implements JsonDecryptor { /** The type of cryptographic representation that this decryptor supports. */ public static final String TYPE = "x-simple-encryption"; - /** GCM authentication tag length, in bits, matching the encryptor default. */ + /** GCM authentication tag length, in bits, matching {@link SimpleEncryptor}. */ private static final int GCM_TAG_LENGTH_BITS = 128; /** Converts between JSON constructs and Java objects. */ diff --git a/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleEncryptor.java b/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleEncryptor.java index 5f4bfae124..b499dbb1e4 100644 --- a/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleEncryptor.java +++ b/commons/json-crypto/core/src/main/java/org/forgerock/json/crypto/simple/SimpleEncryptor.java @@ -20,9 +20,11 @@ import javax.crypto.Cipher; import javax.crypto.KeyGenerator; import javax.crypto.SecretKey; +import javax.crypto.spec.GCMParameterSpec; import java.io.IOException; import java.security.GeneralSecurityException; import java.security.Key; +import java.security.SecureRandom; import java.util.HashMap; import com.fasterxml.jackson.databind.ObjectMapper; @@ -39,6 +41,15 @@ public class SimpleEncryptor implements JsonEncryptor { /** The type of cryptographic representation that this encryptor supports. */ public static final String TYPE = "x-simple-encryption"; + /** GCM authentication tag length, in bits. {@link SimpleDecryptor} assumes the same value. */ + private static final int GCM_TAG_LENGTH_BITS = 128; + + /** GCM nonce length, in bytes, per NIST SP 800-38D recommendation. */ + private static final int GCM_NONCE_LENGTH_BYTES = 12; + + /** Generates random GCM nonces. */ + private static final SecureRandom RANDOM = new SecureRandom(); + /** Converts between Java objects and JSON constructs. */ private final ObjectMapper mapper = new ObjectMapper(); @@ -79,7 +90,17 @@ public String getType() { */ private Object symmetric(Object object) throws GeneralSecurityException, IOException { Cipher symmetric = Cipher.getInstance(cipher); - symmetric.init(Cipher.ENCRYPT_MODE, key); + if (isGcm(cipher)) { + // Pin the 96-bit nonce and 128-bit tag instead of relying on the + // provider's GCM defaults, which the JCE leaves provider-specific; + // SimpleDecryptor assumes a 128-bit tag. The key supplied to this + // encryptor is caller-managed and typically long-lived, so with + // random nonces NIST SP 800-38D section 8.3 bounds GCM to at most + // 2^32 encryptions under the same key. + symmetric.init(Cipher.ENCRYPT_MODE, key, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, randomNonce())); + } else { + symmetric.init(Cipher.ENCRYPT_MODE, key); + } String data = Base64.encode(symmetric.doFinal(mapper.writeValueAsBytes(object))); byte[] iv = symmetric.getIV(); HashMap result = new HashMap<>(); @@ -106,7 +127,9 @@ private Object asymmetric(Object object) throws GeneralSecurityException, IOExce // padding-oracle attacks that affect CBC/PKCS#5, nor does it leak block // patterns like ECB (CWE-327). A fresh session key is generated for every // message, so the randomly generated nonce is never reused under the same - // key. The nonce is stored alongside the data and the self-describing + // key. The nonce and tag length are passed explicitly because the JCE + // leaves them provider-specific and SimpleDecryptor assumes a 128-bit + // tag. The nonce is stored alongside the data and the self-describing // "cipher" field keeps this backward compatible with values previously // written using CBC or ECB. String symmetricCipher = "AES/GCM/NoPadding"; @@ -114,7 +137,7 @@ private Object asymmetric(Object object) throws GeneralSecurityException, IOExce generator.init(128); SecretKey sessionKey = generator.generateKey(); Cipher symmetric = Cipher.getInstance(symmetricCipher); - symmetric.init(Cipher.ENCRYPT_MODE, sessionKey); + symmetric.init(Cipher.ENCRYPT_MODE, sessionKey, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, randomNonce())); String data = Base64.encode(symmetric.doFinal(mapper.writeValueAsBytes(object))); byte[] iv = symmetric.getIV(); Cipher asymmetric = Cipher.getInstance(cipher); @@ -133,6 +156,17 @@ private Object asymmetric(Object object) throws GeneralSecurityException, IOExce return result; } + private static byte[] randomNonce() { + byte[] nonce = new byte[GCM_NONCE_LENGTH_BYTES]; + RANDOM.nextBytes(nonce); + return nonce; + } + + private static boolean isGcm(String cipher) { + String[] parts = cipher.split("/"); + return parts.length > 1 && parts[1].equalsIgnoreCase("GCM"); + } + @Override public JsonValue encrypt(JsonValue value) throws JsonCryptoException { Object object = value.getObject(); diff --git a/commons/json-crypto/core/src/test/java/org/forgerock/json/crypto/JsonCryptoTest.java b/commons/json-crypto/core/src/test/java/org/forgerock/json/crypto/JsonCryptoTest.java index 567012e434..e40228a57e 100644 --- a/commons/json-crypto/core/src/test/java/org/forgerock/json/crypto/JsonCryptoTest.java +++ b/commons/json-crypto/core/src/test/java/org/forgerock/json/crypto/JsonCryptoTest.java @@ -21,6 +21,8 @@ import javax.crypto.KeyGenerator; import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; import java.security.Key; import java.security.KeyPair; @@ -52,6 +54,24 @@ public class JsonCryptoTest { private static final String PLAINTEXT = "Lorem ipsum dolor sit amet, consectetur adipiscing elit."; + /** + * Fixed AES key for the checked-in legacy values below; unlike the random + * per-run keys, it lets ciphertexts produced by pre-GCM builds be kept in + * the test as backward-compatibility fixtures. + */ + private static final SecretKey COMPAT_KEY = + new SecretKeySpec("0123456789abcdef".getBytes(StandardCharsets.US_ASCII), "AES"); + + /** {@link #PLAINTEXT} encrypted with {@link #COMPAT_KEY} using legacy AES/ECB/PKCS5Padding (no IV). */ + private static final String LEGACY_ECB_DATA = + "8sDxsiLqBe63ul0Pup+Bv3CREJeNI+bpMydH8hnajinHixZAmDwzoNGBDpyFApurpvwOZa6bF153jD2Rk0Jwfg=="; + + /** {@link #PLAINTEXT} encrypted with {@link #COMPAT_KEY} and {@link #LEGACY_CBC_IV} using AES/CBC/PKCS5Padding. */ + private static final String LEGACY_CBC_DATA = + "kgxfhkMPxKtmhfmxQt/p8bCj4bIZPH1s/AwAmVANAAuYLGRosaLsNrfclIosTJYKWAAGygDF/dkzs+2ye2+1tw=="; + + private static final String LEGACY_CBC_IV = "ZmVkY2JhOTg3NjU0MzIxMA=="; + private SecretKey secretKey; private PublicKey publicKey; @@ -64,6 +84,8 @@ public class JsonCryptoTest { return secretKey; } else if (key.equals("privateKey")) { return privateKey; + } else if (key.equals("compatKey")) { + return COMPAT_KEY; } else { return null; } @@ -104,6 +126,7 @@ public void testAsymmetricEncryption() throws JsonCryptoException { JsonValue value = new JsonValue(PLAINTEXT); value = new SimpleEncryptor(ASYMMETRIC_CIPHER, publicKey, "privateKey").encrypt(value); assertThat(value.getObject()).isNotEqualTo(PLAINTEXT); + assertThat(((Map) value.getObject()).get("cipher")).isEqualTo(SYMMETRIC_GCM_CIPHER); value = new SimpleDecryptor(selector).decrypt(value); assertThat(value.getObject()).isEqualTo(PLAINTEXT); } @@ -118,6 +141,33 @@ public void testSymmetricGcmEncryption() throws JsonCryptoException { assertThat(value.getObject()).isEqualTo(PLAINTEXT); } + // ----- backward compatibility ---------- + + @Test + public void testDecryptLegacyEcbValue() throws JsonCryptoException { + JsonValue value = new JsonValue(legacyValue("AES/ECB/PKCS5Padding", LEGACY_ECB_DATA, null)); + value = new SimpleDecryptor(selector).decrypt(value); + assertThat(value.getObject()).isEqualTo(PLAINTEXT); + } + + @Test + public void testDecryptLegacyCbcValue() throws JsonCryptoException { + JsonValue value = new JsonValue(legacyValue(SYMMETRIC_CIPHER, LEGACY_CBC_DATA, LEGACY_CBC_IV)); + value = new SimpleDecryptor(selector).decrypt(value); + assertThat(value.getObject()).isEqualTo(PLAINTEXT); + } + + private static Map legacyValue(String cipher, String data, String iv) { + HashMap result = new HashMap<>(); + result.put("cipher", cipher); + result.put("key", "compatKey"); + result.put("data", data); + if (iv != null) { + result.put("iv", iv); + } + return result; + } + // @Test // public void testJsonCryptoTransformer() throws JsonCryptoException { // JsonValue value = new JsonValue(PLAINTEXT);