From d6aabe3838731056e50193a4dec80aca720a4868 Mon Sep 17 00:00:00 2001 From: arpansharma Date: Sat, 22 Aug 2026 13:19:18 -0500 Subject: [PATCH] Reject malformed ML-KEM key-transport metadata with XMLEncryptionException 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. --- .../xml/security/encryption/XMLCipher.java | 26 +++++- .../security/encryption/XMLCipherUtil.java | 21 ++++- .../encryption/XMLEncryptionMLKEMTest.java | 93 ++++++++++++++++++- 3 files changed, 133 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/apache/xml/security/encryption/XMLCipher.java b/src/main/java/org/apache/xml/security/encryption/XMLCipher.java index ca91219e5..938af962b 100644 --- a/src/main/java/org/apache/xml/security/encryption/XMLCipher.java +++ b/src/main/java/org/apache/xml/security/encryption/XMLCipher.java @@ -2607,7 +2607,7 @@ KeyInfo newKeyInfo(Element element) throws XMLEncryptionException { * @param element * @return a new EncryptionMethod */ - EncryptionMethod newEncryptionMethod(Element element) { + EncryptionMethod newEncryptionMethod(Element element) throws XMLEncryptionException { String encAlgorithm = element.getAttributeNS(null, EncryptionConstants._ATT_ALGORITHM); EncryptionMethod result = newEncryptionMethod(encAlgorithm); @@ -2668,7 +2668,7 @@ EncryptionMethod newEncryptionMethod(Element element) { result.setKeyEncapsulationKeyDerivationMethod( new KeyDerivationMethodImpl(keyDerivationMethodElement, null)); } catch (XMLSecurityException xse) { - throw new RuntimeException(xse); + throw new XMLEncryptionException(xse); } } @@ -2678,7 +2678,7 @@ EncryptionMethod newEncryptionMethod(Element element) { EncryptionConstants._TAG_KEYLEN).item(0); if (keyLenElement != null) { result.setKeyEncapsulationKeyLength( - Integer.parseInt(keyLenElement.getFirstChild().getNodeValue())); + parseKeyEncapsulationKeyLength(keyLenElement)); } } @@ -2698,6 +2698,26 @@ EncryptionMethod newEncryptionMethod(Element element) { return result; } + /** + * Parses the {@code ghc:KeyLen} element content. The element is read from the + * (untrusted) message, so a missing, empty or non-numeric value is reported as an + * {@link XMLEncryptionException}, the exception type the decrypt API declares, rather + * than escaping as a {@code NullPointerException} or {@code NumberFormatException}. + */ + private int parseKeyEncapsulationKeyLength(Element keyLenElement) throws XMLEncryptionException { + Node child = keyLenElement.getFirstChild(); + String text = child == null ? null : child.getNodeValue(); + if (text == null || text.trim().isEmpty()) { + throw new XMLEncryptionException("KeyDerivation.InvalidParameter", EncryptionConstants._TAG_KEYLEN); + } + try { + return Integer.parseInt(text.trim()); + } catch (NumberFormatException e) { + throw new XMLEncryptionException(e, "KeyDerivation.InvalidParameter", + new Object[]{EncryptionConstants._TAG_KEYLEN}); + } + } + /** * @param element * @return a new EncryptionProperties diff --git a/src/main/java/org/apache/xml/security/encryption/XMLCipherUtil.java b/src/main/java/org/apache/xml/security/encryption/XMLCipherUtil.java index 31d95fb51..747911d64 100644 --- a/src/main/java/org/apache/xml/security/encryption/XMLCipherUtil.java +++ b/src/main/java/org/apache/xml/security/encryption/XMLCipherUtil.java @@ -28,6 +28,7 @@ import org.apache.xml.security.encryption.params.KeyAgreementParameters; import org.apache.xml.security.encryption.params.KeyDerivationParameters; import org.apache.xml.security.exceptions.XMLSecurityException; +import org.apache.xml.security.utils.Constants; import org.apache.xml.security.utils.EncryptionConstants; import org.apache.xml.security.utils.KeyUtils; import org.w3c.dom.Document; @@ -279,13 +280,29 @@ public static KeyDerivationParameters constructKeyDerivationParameter(KeyDerivat } HKDFParamsImpl hKDFParams = (HKDFParamsImpl) kdfParams; return HKDFParams.createBuilder(keyBitLength, hKDFParams.getPRFAlgorithm()) - .salt(hKDFParams.getSalt() != null ? Base64.getDecoder().decode(hKDFParams.getSalt()) : null) - .info(hKDFParams.getInfo() != null ? Base64.getDecoder().decode(hKDFParams.getInfo()) : null) + .salt(decodeBase64Parameter(hKDFParams.getSalt(), Constants._TAG_SALT)) + .info(decodeBase64Parameter(hKDFParams.getInfo(), EncryptionConstants._TAG_INFO)) .build(); } throw new XMLEncryptionException("unknownAlgorithm", keyDerivationAlgorithm); } + /** + * Base64-decodes an optional key derivation parameter read from the message. Malformed + * base64 is reported as an {@link XMLEncryptionException} rather than escaping as the + * {@link IllegalArgumentException} thrown by {@link Base64.Decoder#decode(String)}. + */ + private static byte[] decodeBase64Parameter(String value, String parameterName) throws XMLEncryptionException { + if (value == null) { + return null; + } + try { + return Base64.getDecoder().decode(value); + } catch (IllegalArgumentException e) { + throw new XMLEncryptionException(e, "KeyDerivation.InvalidParameter", new Object[]{parameterName}); + } + } + /** * Construct a {@code KeyDerivationMethod} DOM element from the given {@link KeyDerivationParameters}. * The inverse of {@link #constructKeyDerivationParameter(KeyDerivationMethod, int)}. Supports the same diff --git a/src/test/java/org/apache/xml/security/test/dom/encryption/XMLEncryptionMLKEMTest.java b/src/test/java/org/apache/xml/security/test/dom/encryption/XMLEncryptionMLKEMTest.java index 73dbc2d6c..3caf2ac3e 100644 --- a/src/test/java/org/apache/xml/security/test/dom/encryption/XMLEncryptionMLKEMTest.java +++ b/src/test/java/org/apache/xml/security/test/dom/encryption/XMLEncryptionMLKEMTest.java @@ -45,6 +45,7 @@ import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; import org.w3c.dom.Document; @@ -53,6 +54,7 @@ import org.w3c.dom.Text; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -79,6 +81,9 @@ class XMLEncryptionMLKEMTest { private static boolean mlKemAvailable; private static boolean bcAddedForTheTest; + /** Namespace of the HKDFParams / Salt / Info elements (xmldsig-more, 2021). */ + private static final String XMLDSIG_MORE_NS = "http://www.w3.org/2021/04/xmldsig-more#"; + private static java.util.Map keyPairs = new java.util.HashMap<>(); @BeforeAll @@ -311,6 +316,13 @@ void testMLKEMTruncatedEncapsulationRejected(String keyEncapsulationUri, String * for tests that want to corrupt or otherwise interfere with the decrypt half. */ private byte[] encryptToRecipient(PublicKey pubKey, String keyEncapsulationUri) throws Exception { + int wrapKeyBitLength = KeyUtils.getAESKeyBitSizeForWrapAlgorithm(EncryptionConstants.ALGO_ID_KEYWRAP_AES256); + HKDFParams kdfParams = HKDFParams.createBuilder(wrapKeyBitLength, XMLSignature.ALGO_ID_MAC_HMAC_SHA256).build(); + return encryptToRecipient(pubKey, keyEncapsulationUri, kdfParams); + } + + private byte[] encryptToRecipient(PublicKey pubKey, String keyEncapsulationUri, HKDFParams kdfParams) + throws Exception { DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); dbf.setNamespaceAware(true); Document doc = dbf.newDocumentBuilder().newDocument(); @@ -323,8 +335,6 @@ private byte[] encryptToRecipient(PublicKey pubKey, String keyEncapsulationUri) SecretKey cek = kg.generateKey(); String kwAlgorithm = EncryptionConstants.ALGO_ID_KEYWRAP_AES256; - int wrapKeyBitLength = KeyUtils.getAESKeyBitSizeForWrapAlgorithm(kwAlgorithm); - HKDFParams kdfParams = HKDFParams.createBuilder(wrapKeyBitLength, XMLSignature.ALGO_ID_MAC_HMAC_SHA256).build(); AlgorithmParameterSpec keyEncapsulationParameters = new KeyEncapsulationParameters(keyEncapsulationUri, kdfParams); @@ -350,6 +360,85 @@ private byte[] encryptToRecipient(PublicKey pubKey, String keyEncapsulationUri) return bos.toByteArray(); } + + /** + * Malformed key-transport metadata in the EncryptedKey must be rejected as + * {@link XMLEncryptionException}, the decrypt API's declared failure type, rather than + * escaping as a NumberFormatException (non-numeric or empty {@code ghc:KeyLen}) or an + * IllegalArgumentException (malformed base64 in the HKDF {@code Salt} or {@code Info}). + * All of these values are parsed from the untrusted message before any private-key operation. + */ + @ParameterizedTest + @CsvSource({ + EncryptionConstants.EncryptionSpecGHCNS + ",KeyLen,notanumber", + EncryptionConstants.EncryptionSpecGHCNS + ",KeyLen,''", + XMLDSIG_MORE_NS + ",Salt,!!!not-base64!!!", + XMLDSIG_MORE_NS + ",Info,@@@@" + }) + void testMLKEMMalformedKeyTransportMetadataRejected(String namespace, String localName, String badText) + throws Exception { + Assumptions.assumeTrue(mlKemAvailable, "ML-KEM requires BouncyCastle 1.84+ and Java 21+ (javax.crypto.KEM)"); + + KeyPair kp = keyPairs.get("ML-KEM-768"); + Document encDoc = parse(encryptToRecipientWithHkdfSaltAndInfo(kp.getPublic())); + Element target = (Element) encDoc.getElementsByTagNameNS(namespace, localName).item(0); + assertNotNull(target, "expected a <" + localName + "> element to mutate"); + replaceTextContent(encDoc, target, badText); + + assertThrows(XMLEncryptionException.class, () -> decryptDocument(encDoc, kp.getPrivate())); + } + + /** + * A childless {@code } has no text node at all; reading it must not surface as + * a NullPointerException from the decrypt path. + */ + @Test + void testMLKEMChildlessKeyLenRejected() throws Exception { + Assumptions.assumeTrue(mlKemAvailable, "ML-KEM requires BouncyCastle 1.84+ and Java 21+ (javax.crypto.KEM)"); + + KeyPair kp = keyPairs.get("ML-KEM-768"); + Document encDoc = parse(encryptToRecipient(kp.getPublic(), EncryptionConstants.ALGO_ID_KEYTRANSPORT_MLKEM_768)); + Element keyLen = (Element) encDoc.getElementsByTagNameNS( + EncryptionConstants.EncryptionSpecGHCNS, "KeyLen").item(0); + assertNotNull(keyLen, "expected a element to mutate"); + while (keyLen.hasChildNodes()) { + keyLen.removeChild(keyLen.getFirstChild()); + } + + assertThrows(XMLEncryptionException.class, () -> decryptDocument(encDoc, kp.getPrivate())); + } + + /** Encrypts with an HKDF that carries explicit Salt and Info elements, so they exist to mutate. */ + private byte[] encryptToRecipientWithHkdfSaltAndInfo(PublicKey pubKey) throws Exception { + int wrapKeyBitLength = KeyUtils.getAESKeyBitSizeForWrapAlgorithm(EncryptionConstants.ALGO_ID_KEYWRAP_AES256); + HKDFParams kdfParams = HKDFParams.createBuilder(wrapKeyBitLength, XMLSignature.ALGO_ID_MAC_HMAC_SHA256) + .salt(new byte[]{1, 2, 3, 4, 5, 6, 7, 8}) + .info(new byte[]{9, 10, 11, 12}) + .build(); + return encryptToRecipient(pubKey, EncryptionConstants.ALGO_ID_KEYTRANSPORT_MLKEM_768, kdfParams); + } + + private Document parse(byte[] xml) throws Exception { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + dbf.setNamespaceAware(true); + return dbf.newDocumentBuilder().parse(new java.io.ByteArrayInputStream(xml)); + } + + /** The full decrypt path: load the EncryptedData, unwrap the CEK with the ML-KEM private key, decrypt. */ + private void decryptDocument(Document encDoc, PrivateKey privKey) throws Exception { + Element encDataElem = (Element) encDoc.getElementsByTagNameNS( + EncryptionConstants.EncryptionSpecNS, "EncryptedData").item(0); + XMLCipher decryptCipher = XMLCipher.getInstance(); + decryptCipher.init(XMLCipher.DECRYPT_MODE, null); + EncryptedData encData = decryptCipher.loadEncryptedData(encDoc, encDataElem); + EncryptedKey ek = encData.getKeyInfo().itemEncryptedKey(0); + XMLCipher unwrapCipher = XMLCipher.getInstance(); + unwrapCipher.init(XMLCipher.UNWRAP_MODE, privKey); + Key cek = unwrapCipher.decryptKey(ek, encData.getEncryptionMethod().getAlgorithm()); + decryptCipher.init(XMLCipher.DECRYPT_MODE, cek); + decryptCipher.doFinal(encDoc, encDataElem); + } + private void replaceTextContent(Document doc, Element element, String newText) { NodeList children = element.getChildNodes(); for (int i = children.getLength() - 1; i >= 0; i--) {