Skip to content

Encrypt in-memory secrets with AES/GCM and a fresh IV per value - #128

Open
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:guarded-string-aes-gcm
Open

vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:guarded-string-aes-gcm

Conversation

@vharseko

Copy link
Copy Markdown
Member

Follow-up to the high-severity CodeQL alerts on EncryptorImpl (java/weak-cryptographic-algorithm #1 #2, java/static-initialization-vector #11 #12).

Problem

EncryptorImpl served two purposes with one piece of code. With defaultKey=true it is the wire format of the legacy connector server protocol (AES/CBC, key and IV built into the framework). With defaultKey=false — what EncryptorFactory.newRandomEncryptor() returned, i.e. what GuardedString and GuardedByteArray use for the secrets they hold in memory — it generated a random key but kept the framework's fixed IV. So two equal secrets encrypted to identical bytes (spot the reused password in a heap dump), and a modified ciphertext decrypted to garbage without complaint.

Change

  • AesGcmEncryptor (new): AES-256/GCM, key per instance, random 12-byte IV for every encryption written in front of the ciphertext, 128-bit tag. newRandomEncryptor() returns it. These secrets never leave the process, so nothing depends on their format.
  • EncryptorImpl keeps only the legacy wire format, with a javadoc saying why it is what it is: it must stay byte-for-byte compatible with the .NET connector server, and it is obfuscation rather than protection — the transport is what protects it (Verify the connector server certificate against the host over the legacy SSL connection #122). The boolean constructor goes (its only caller was the factory).

Tests

EncryptorImplTests:

  • defaultEncryptorKeepsLegacyWireFormat pins encrypt("secret") to 66df076267e3a421575ce6fa2ec1d5c7, computed independently with openssl enc -aes-128-cbc and the framework's key/IV — the .NET compatibility guard;
  • round trip; two random encryptors cannot read each other;
  • same input twice → different ciphertext (failed on the old code: fixed IV);
  • a flipped byte in the first block of a three-block plaintext → exception (failed on the old code: CBC returned garbage, padding intact).

connector-framework-internal suite: 474 tests, green (serializer round trips of GuardedString/GuardedByteArray, remote tests).

Alerts

The four EncryptorImpl alerts remain on the legacy path by design and are being dismissed as "won't fix" with this PR as the reason. Already dismissed with recorded reasons: DESede in PasswordDecryptor (#5 #6, DSEE retro-changelog format), LDAP {SHA}/{MD5} schemes (#27 #28, the directory's format), SHA-1 connector key hash (#25 #26, stored connectorserver.key format; SHA-256 needs dual support — separate task).

GuardedString and GuardedByteArray protected their contents with the same
AES/CBC code as the legacy wire format: a random key, but the framework's
fixed IV, so equal secrets encrypted to equal bytes and a modified
ciphertext decrypted without complaint. Those secrets never leave the
process, so nothing depends on their format: newRandomEncryptor() now
returns an AES-256/GCM encryptor with a random IV for every encryption.

EncryptorImpl keeps only the legacy wire format, which must stay
byte-compatible with the .NET connector server; a test pins its output.
@vharseko vharseko added security Security fix / CVE remediation java Pull requests that update java code tests Test additions or fixes framework OpenICF-java-framework labels Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework OpenICF-java-framework java Pull requests that update java code security Security fix / CVE remediation tests Test additions or fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants