Skip to content

Reject malformed ML-KEM key-transport metadata with XMLEncryptionException - #6

Merged
ffang merged 1 commit into
ffang:PQC-ENCRYPTIONfrom
Arpan0995:pqc-encryption-robustness
Aug 26, 2026
Merged

Reject malformed ML-KEM key-transport metadata with XMLEncryptionException#6
ffang merged 1 commit into
ffang:PQC-ENCRYPTIONfrom
Arpan0995:pqc-encryption-robustness

Conversation

@Arpan0995

Copy link
Copy Markdown

Following the inbound-path note on apache#652, I ran a small robustness pass over the DOM decrypt path: one valid ML-KEM-768 document, then a batch of malformed variants through XMLCipher.loadEncryptedData / decryptKey, checking what type of exception escapes. The KEM ciphertext handling itself is solid (every truncation and byte-flip of the EncryptedKey CipherValue came back as XMLSecurityException). Three of the parsed metadata fields, though, escaped the declared XMLEncryptionException with an unchecked exception, all triggered before any private-key operation:

  • ghc:KeyLen was parsed with Integer.parseInt(keyLenElement.getFirstChild().getNodeValue()): a childless <ghc:KeyLen/> threw NullPointerException, and empty, non-numeric or overflowing text threw NumberFormatException.
  • Malformed base64 in the HKDF Salt or Info threw IllegalArgumentException from Base64.getDecoder().decode(...) in XMLCipherUtil#constructKeyDerivationParameter. That method is shared with the existing ECDH-ES path, so the decode is not new here, but it is newly reachable through ML-KEM key transport.
  • A KeyDerivationMethod that failed to parse was rethrown as throw new RuntimeException(xse), wrapping a declared exception as unchecked. (Noted by inspection; my inputs did not trigger it.)

None of these are memory-safety or authentication issues, and a caller gets a clean rejection for a corrupted ciphertext already. The concern is narrower: a caller catching XMLEncryptionException, the documented contract, will not catch an NPE, NumberFormatException or IllegalArgumentException, so on a service decrypting untrusted XML these surface as uncaught errors rather than clean rejections.

The change. Guard the KeyLen parse (null child, empty text, NumberFormatException) and wrap the base64 decode, reporting both through XMLEncryptionException with the existing KeyDerivation.InvalidParameter message; and let newEncryptionMethod(Element) declare the checked exception instead of wrapping it, which is safe because the factory is package-internal and both of its callers (newEncryptedData, newEncryptedKey) already declare XMLEncryptionException.

Tests. Adds two DOM negative tests to XMLEncryptionMLKEMTest: a parameterized one over the malformed-metadata cases (non-numeric and empty KeyLen, malformed Salt, malformed Info) and one for a childless KeyLen, each asserting XMLEncryptionException from the full decrypt path. I confirmed they are not vacuous by running them against the previous code: all five fail there and pass with this change.

Verification. mvn test -P bouncycastle over XMLEncryptionMLKEMTest (14 executions: the nine existing plus the five new, 0 failures), StaxMLKEMEncryptionTest, XMLCipherTest, XMLEncryption11Test, BaltimoreEncTest, EncryptionFormattingTest and the two KeyWrapEncryption* classes: 148 executions with no regressions. (One pre-existing error, XMLCipherTest#testEncryptForDataExceeding8192bytes, fails identically on the untouched branch on my machine and is unrelated.) Without the profile the new tests compile and skip via the existing assumeTrue guard.

Two things deliberately left alone: the pre-existing KeySize parse a few lines above KeyLen has the same unguarded shape but predates this PR and would need its own message key, so I did not fold it in; and no behavior changes for well-formed input, since the same strict Base64.getDecoder() is used.

…ption

Parsing the EncryptedKey's key-transport metadata could escape the declared
XMLEncryptionException with an unchecked exception before any private-key
operation: a childless or non-numeric ghc:KeyLen threw NullPointerException
or NumberFormatException from an unguarded Integer.parseInt, malformed base64
in the HKDF Salt or Info threw IllegalArgumentException from Base64.Decoder,
and a KeyDerivationMethod that failed to parse was rethrown wrapped in a
RuntimeException.

Guard the KeyLen parse and the base64 decode, report all three through
XMLEncryptionException (reusing the KeyDerivation.InvalidParameter message),
and let newEncryptionMethod(Element) declare the checked exception instead
of wrapping it; both of its callers already declare XMLEncryptionException.
Adds DOM negative tests for the reproduced cases; each fails against the
previous code.
@Arpan0995

Copy link
Copy Markdown
Author

Rebased-status note: this branch still applies cleanly on the current PQC-ENCRYPTION tip after #5 merged (test-merges with no conflicts), so no rebase is needed. It stays green with the earlier verification (14/14 in XMLEncryptionMLKEMTest plus the encryption regression set). Happy to rebase or split it further if that helps review.

@Arpan0995

Copy link
Copy Markdown
Author

Hi @ffang small nudge on this one, no rush. If the metadata hardening here looks right, I'm glad to rebase it against the current apache#652 branch or split it however is easiest to review. And if you'd rather fold these checks into your own commit, I'm happy to close this in favor of that. Thanks again for merging the earlier test PRs.

@ffang
ffang merged commit 6017d2b into ffang:PQC-ENCRYPTION Aug 26, 2026
@ffang

ffang commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Hi @ffang small nudge on this one, no rush. If the metadata hardening here looks right, I'm glad to rebase it against the current apache#652 branch or split it however is easiest to review. And if you'd rather fold these checks into your own commit, I'm happy to close this in favor of that. Thanks again for merging the earlier test PRs.

Merged, thanks @Arpan0995 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants