Reject malformed HKDF Salt and Info with XMLEncryptionException - #653
Open
Arpan0995 wants to merge 1 commit into
Open
Reject malformed HKDF Salt and Info with XMLEncryptionException#653Arpan0995 wants to merge 1 commit into
Arpan0995 wants to merge 1 commit into
Conversation
On the decrypt path of an ECDH-ES (or X25519/X448) key agreement, the HKDF Salt and Info elements of the KeyDerivationMethod are read from the message and base64-decoded in XMLCipherUtil.constructKeyDerivationParameter. Malformed base64 there threw the IllegalArgumentException from Base64.Decoder, escaping the XMLEncryptionException the decrypt API declares, so a caller handling the declared type would not catch it. Wrap the decode and report it as XMLEncryptionException using the existing KeyDerivation.InvalidParameter message. The same strict decoder is used, so well-formed input is unaffected. Adds a test with a positive control and the two malformed cases; both fail without this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On the decrypt path of an ECDH-ES (or X25519/X448) key agreement, the HKDF
SaltandInfoelements of theKeyDerivationMethodare read from the message and base64-decoded inXMLCipherUtil#constructKeyDerivationParameter. Malformed base64 in either element throws theIllegalArgumentExceptionfromBase64.Decoder, which escapes theXMLEncryptionExceptionthatXMLCipher#decryptKeydeclares. A caller handling the declared exception type therefore does not catch it, and on a service decrypting untrusted XML the malformed input surfaces as an uncaught error rather than a clean rejection. Reproduced onmainwith a secp256r1 ECDH-ES round trip and a mutatedSaltorInfo; the only pre-condition is a message the recipient is willing to decrypt, and the failure happens before the key agreement is attempted.The change. Wrap the two decodes in a small helper that reports the failure as
XMLEncryptionExceptionusing the existingKeyDerivation.InvalidParametermessage (Key derivation parameter {0} is illegal), naming the offending element. The same strictBase64.getDecoder()is used, so well-formed input is unaffected.Test.
XMLCipherKeyAgreementMalformedHKDFParamsTest: a positive control (the round trip decrypts with well-formed parameters) and a parameterized case for a malformedSaltand a malformedInfo, each assertingXMLEncryptionExceptionfrom the full decrypt path. It runs on the default profile with the JDK's EC provider, no BouncyCastle needed. I confirmed the test is not vacuous by running it against the unchanged code: the positive control passes and both malformed cases fail with the rawIllegalArgumentException: Illegal base64 character.Verification.
mvn testover the new class andXMLCipherTest(which covers the ECDH-ES, X25519 and X448 key-agreement round trips): 40 executions, no regressions.For context, I found this while looking at the pending ML-KEM key-transport work (#652), which reaches the same code through a new path; the ECDH-ES path is affected on
maintoday, so it seemed right to fix it here independently. The same guard is included in that branch via ffang#6 and can be dropped there on rebase once this lands.