Use AES/GCM instead of AES/CBC for json-crypto session encryption

[vharseko]

Summary

Fixes CodeQL high alert java/weak-cryptographic-algorithm in commons/json-crypto. SimpleEncryptor.asymmetric() hardcoded AES/CBC/PKCS5Padding for the per-message session key, which CodeQL flags as vulnerable to padding-oracle attacks.

Changes

• SimpleEncryptor — the session cipher is now AES/GCM/NoPadding (authenticated encryption). A fresh session key is generated per message, so the randomly generated 96-bit nonce is never reused under the same key.
• SimpleDecryptor — made GCM-aware. GCM needs a GCMParameterSpec, whereas the old code always built an IvParameterSpec (which throws for GCM). The mode is read from the self-describing cipher field, so GCM uses GCMParameterSpec(128, nonce) and CBC/other modes keep IvParameterSpec.
• Backward compatibility — existing values written with CBC (or legacy ECB) still decrypt unchanged, since the algorithm is stored alongside each value.
• Tests — added a GCM round-trip test; the existing CBC round-trip test continues to cover backward compatibility.

Verification

mvn clean test on the json-crypto module: core 5/5, cli 1/1, all green.

[maximthomas]

Right direction — GCM is the correct cipher, and driving the decryptor's parameter spec off the self-describing cipher field is the right mechanism. Verified backward compat holds for both legacy ECB and CBC values. Three asks, none blocking.

1. Encryptor trusts a provider default the decryptor hardcodes

SimpleDecryptor.java:46 pins GCM_TAG_LENGTH_BITS = 128, but SimpleEncryptor.java:117 calls init(ENCRYPT_MODE, sessionKey) with no GCMParameterSpec — the 12-byte nonce / 128-bit tag is a SunJCE convention, not a JCE guarantee, and the tag length isn't persisted. Under a provider with a different default (BouncyCastle preferred, PKCS#11/HSM, FIPS), every value fails as AEADBadTagException: Tag mismatch! — indistinguishable from real tampering. Pass the spec explicitly:

byte[] nonce = new byte[GCM_NONCE_LENGTH_BYTES];   // 12
random.nextBytes(nonce);
symmetric.init(Cipher.ENCRYPT_MODE, sessionKey, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, nonce));

2. No test covers the changed line

The only behavioural change is symmetricCipher at SimpleEncryptor.java:112, inside asymmetric(). The new test passes a SecretKey, so encrypt() dispatches to symmetric() — unchanged code. Reverting line 112 to CBC leaves the suite green. Add to testAsymmetricEncryption:

assertThat(((Map<?, ?>) value.getObject()).get("cipher")).isEqualTo(SYMMETRIC_GCM_CIPHER);

A checked-in legacy ECB payload would also turn the backward-compat claim into something CI enforces — currently every test encrypts and decrypts with the same build.

3. Comment overstates its scope

SimpleEncryptor.java:107-109 ("fresh session key per message, so the nonce is never reused") holds for asymmetric() only. The new test also blesses GCM for symmetric(), where the key is caller-supplied and long-lived — random 96-bit nonces there are bounded by NIST SP 800-38D §8.3 (2^32 invocations/key). Scope the comment to asymmetric().

[vharseko]

@maximthomas Thanks for the thorough review — all three points addressed in 3c16364.

1. Explicit GCM parameters. SimpleEncryptor now always passes GCMParameterSpec(128, nonce) with a 96-bit SecureRandom nonce instead of relying on provider defaults. Note this applied to both paths, not just asymmetric(): the symmetric() path had the same gap when a caller supplies a GCM cipher string (which is exactly what the new GCM test exercises), so both inits are pinned now. One nuance on the failure scenario: BouncyCastle's default tag is also 128 bits (and its 16-byte default IV would round-trip fine since the IV is stored verbatim), so BC-as-preferred likely wouldn't break — the real exposure is PKCS#11/HSM and FIPS providers, some of which use a different tag length or refuse to init GCM without parameters at all. Either way, pinning explicitly is strictly better.

2. Coverage of the changed line. testAsymmetricEncryption now asserts the envelope's outer cipher is AES/GCM/NoPadding, so reverting the switch fails the suite. Also took your fixture suggestion: added checked-in legacy AES/ECB (no iv) and AES/CBC payloads encrypted under a fixed AES key, with testDecryptLegacyEcbValue / testDecryptLegacyCbcValue decrypting them — backward compat is now enforced by CI rather than by same-build round-trips. The fixtures cover all three parameterSpec() branches in the decryptor (null / IvParameterSpec / GCMParameterSpec, the last via the round-trip tests).

3. Comment scope. The "fresh session key per message" comment lives inside asymmetric() and is accurate there, so I left its claim scoped as-is — but you're right that the test now blesses GCM for symmetric() with a long-lived caller-supplied key, and that constraint was undocumented. symmetric() now carries a comment stating the NIST SP 800-38D §8.3 bound: with random 96-bit nonces, at most 2^32 encryptions under the same key.

Module suite: 7 tests, 0 failures.