Resolve dsig11:DEREncodedKeyValue on the StAX inbound path

[Arpan0995]

Follow-up to #4 (now merged into this branch), completing the KeyValue round trip discussed on apache#651.

What #4 left open. With #4 the outbound side emits a schema-valid dsig11:DEREncodedKeyValue for ML-DSA, but the StAX inbound processor resolved a KeyValue only through its RSA, DSA and EC forms, so the document got past schema validation and then failed at key resolution with "No or unsupported key in KeyValue". Full StAX sign to StAX verify still did not work.

The change.

• New DEREncodedKeyValueSecurityToken (mirrors ECKeyValueSecurityToken): rebuilds the public key from the DER SubjectPublicKeyInfo by trying each supported key type's KeyFactory, using the same key-type list as the DOM DEREncodedKeyValue.
• SecurityTokenFactoryImpl resolves it from both placements: nested inside ds:KeyValue (what this library emits) and as a direct ds:KeyInfo child, which is where XML Signature 1.1 defines it, so documents from other implementations resolve too.

With this, a StAX-signed ML-DSA document verifies through InboundXMLSec#processInMessage with no out-of-band verification key.

Tests. StaxMLDSAKeyValueInboundTest, parameterized across ML-DSA-44/65/87, with no setSignatureVerificationKey so the key can only come from the document:

• nested placement: verifies, and the key the processor resolved (captured via KeyValueTokenSecurityEvent) is byte-identical to the signer's public key;
• KeyInfo-child placement: the same, after re-parenting the element to the canonical position;
• tampered SignatureValue: rejected, asserting the failure is signature validation ("INVALID signature") rather than key resolution.

I confirmed the tests are not vacuous by running them without the factory changes: all nine fail there and pass with this change.

Verification. mvn test -P bouncycastle over the new class plus StaxMLDSAKeyValueTest, StaxMLDSASignatureTest and SignatureVerificationTest: no regressions, so the existing RSA/DSA/EC KeyValue resolution is unaffected. Without the profile the new tests compile and skip via the existing assumeTrue guard.

One observation, not changed here: the DOM DEREncodedKeyValue#getPublicKey reports failures with the message key DEREncodedKeyValue.UnsupportedEncodedKey, which is not present in xmlsecurity_en.properties, so that path would surface the raw key. The StAX token uses the existing stax.unsupportedKeyValue instead.

[ffang]

Thanks Arpan — nice follow-up, and the KeyInfo-direct-child placement is a good addition, XML Signature 1.1 does define it that way.

Looks like we each wrote our own DEREncodedKeyValueSecurityToken independently and landed on almost the same design, but there's a bug in buildPublicKey()'s exception handling here: it only catches NoSuchAlgorithmException | InvalidKeySpecException, and that turns out not to be enough. With BC 1.85 (what we depend on), KeyFactory.generatePublic() for XDH/X25519/X448/EdDSA/Ed25519/Ed448 throws an unchecked ArrayIndexOutOfBoundsException for malformed/short input instead of InvalidKeySpecException, so it isn't caught here and propagates out.

I checked out this branch and reproduced it end-to-end: signed a real ML-DSA document, corrupted the DEREncodedKeyValue content to 8 garbage bytes, and ran it through InboundXMLSec#processInMessage. It throws ArrayIndexOutOfBoundsException: Index 9 out of bounds for length 8 uncaught, instead of the intended stax.unsupportedKeyValue / XMLSecurityException. Since KeyInfo content in an inbound document is attacker-controlled, a malformed DEREncodedKeyValue can crash the verification pipeline with an uncontrolled runtime exception rather than being rejected cleanly.

StaxMLDSAKeyValueInboundTest doesn't have a garbage-content case that would catch this (the old StaxMLDSAKeyValueTest had one before this PR folded its other tests into the new file).

Could you:

1. Add a RuntimeException catch alongside the two checked exceptions in buildPublicKey(), with a comment noting some BC KeyFactorySpis throw unchecked exceptions for malformed input, which must not propagate since this is untrusted, attacker-controlled input.
2. Add a "garbage DER content is rejected cleanly" case to StaxMLDSAKeyValueInboundTest.

For reference, here's what my own (unpublished) version of buildPublicKey() does for 1:

} catch (NoSuchAlgorithmException | InvalidKeySpecException | RuntimeException e) { //NOPMD
    // Do nothing, try the next type. Some providers (e.g. BC's XDH/EdDSA KeyFactorySpi)
    // throw an unchecked exception like ArrayIndexOutOfBoundsException instead of
    // InvalidKeySpecException for malformed/short input, which must not propagate
    // since encodedKey here is untrusted, attacker-controlled input.
}

And for the test, something like

@ParameterizedTest
@CsvSource({
    "http://www.w3.org/tbd#ml-dsa-44,ML-DSA-44",
    "http://www.w3.org/tbd#ml-dsa-65,ML-DSA-65",
    "http://www.w3.org/tbd#ml-dsa-87,ML-DSA-87"
})
void testInboundGarbageDerContentRejectedCleanly(String sigAlgorithm, String jcaAlgorithm) throws Exception {
    Assumptions.assumeTrue(isBcInstalled() && keyPairs.containsKey(jcaAlgorithm),
        "ML-DSA requires BouncyCastle 1.81+");

    KeyPair kp = keyPairs.get(jcaAlgorithm);
    byte[] corrupted = corruptDerEncodedKeyValue(signWithKeyValue(sigAlgorithm, kp));

    // Garbage bytes decode to no known SubjectPublicKeyInfo, so this must fail cleanly at
    // key resolution with a well-defined error, not an uncaught RuntimeException.
    XMLStreamException ex = Assertions.assertThrows(XMLStreamException.class,
        () -> verifyInbound(corrupted, new ArrayList<>()));
    String chain = messageChain(ex);
    Assertions.assertFalse(chain.contains("ArrayIndexOutOfBoundsException"),
        "Malformed DEREncodedKeyValue content must not surface as an uncaught RuntimeException: " + chain);
}

/** Replaces the DEREncodedKeyValue's base64 content with bytes that decode to no known SubjectPublicKeyInfo. */
private byte[] corruptDerEncodedKeyValue(byte[] signed) throws Exception {
    Document document;
    try (InputStream is = new ByteArrayInputStream(signed)) {
        document = XMLUtils.read(is, false);
    }
    Element der = (Element) document.getElementsByTagNameNS(
        "http://www.w3.org/2009/xmldsig11#", "DEREncodedKeyValue").item(0);
    byte[] garbage = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07};
    NodeList children = der.getChildNodes();
    for (int i = children.getLength() - 1; i >= 0; i--) {
        der.removeChild(children.item(i));
    }
    der.appendChild(document.createTextNode(Base64.getEncoder().encodeToString(garbage)));

    ByteArrayOutputStream bos = new ByteArrayOutputStream();
    javax.xml.transform.TransformerFactory.newInstance().newTransformer().transform(
        new javax.xml.transform.dom.DOMSource(document),
        new javax.xml.transform.stream.StreamResult(bos));
    return bos.toByteArray();
}

(Dropped both into your branch to check: 12/12 pass with the fix, and the new test fails 3/3 with the exact ArrayIndexOutOfBoundsException above when I revert just the catch clause, so it's not a vacuous test.)

Everything else here looks good to me.

Best Regards
Freeman

[Arpan0995]

Good catch, and thanks for reproducing it end to end. You are right that the untrusted DEREncodedKeyValue content can drive an unchecked exception out of a KeyFactorySpi (BC's XDH/EdDSA throw ArrayIndexOutOfBoundsException on short input), which is exactly the kind of thing the key-resolution loop must contain.

Both changes are in:

1. buildPublicKey() now also catches RuntimeException in the per-key-type loop, with a comment noting that some providers throw unchecked exceptions for malformed input and that must not propagate since the encoding is attacker-controlled.
2. Added testInboundGarbageDerContentRejectedCleanly to StaxMLDSAKeyValueInboundTest, parameterized across ML-DSA-44/65/87: it replaces the DEREncodedKeyValue content with garbage bytes and asserts the pipeline rejects it cleanly rather than surfacing an uncaught ArrayIndexOutOfBoundsException.

I verified the test is not vacuous the same way you did: with only the catch reverted it fails 3/3 with ArrayIndexOutOfBoundsException: Index 9 out of bounds for length 8, and passes with the fix. Full class is green (12/12), the broader signature suite has no regressions, and the project's PMD and errorprone gates pass.

One related note, only for your awareness and not part of this PR: the DOM DEREncodedKeyValue#getPublicKey has the same narrow catch (NoSuchAlgorithmException | InvalidKeySpecException), so it would surface the same unchecked exception on the DOM decrypt path for a malformed DER key. Happy to send a separate change for that if it is useful.

[ffang]

Good catch, and thanks for reproducing it end to end. You are right that the untrusted DEREncodedKeyValue content can drive an unchecked exception out of a KeyFactorySpi (BC's XDH/EdDSA throw ArrayIndexOutOfBoundsException on short input), which is exactly the kind of thing the key-resolution loop must contain.

Both changes are in:

1. `buildPublicKey()` now also catches `RuntimeException` in the per-key-type loop, with a comment noting that some providers throw unchecked exceptions for malformed input and that must not propagate since the encoding is attacker-controlled.

2. Added `testInboundGarbageDerContentRejectedCleanly` to `StaxMLDSAKeyValueInboundTest`, parameterized across ML-DSA-44/65/87: it replaces the DEREncodedKeyValue content with garbage bytes and asserts the pipeline rejects it cleanly rather than surfacing an uncaught `ArrayIndexOutOfBoundsException`.

I verified the test is not vacuous the same way you did: with only the catch reverted it fails 3/3 with ArrayIndexOutOfBoundsException: Index 9 out of bounds for length 8, and passes with the fix. Full class is green (12/12), the broader signature suite has no regressions, and the project's PMD and errorprone gates pass.

One related note, only for your awareness and not part of this PR: the DOM DEREncodedKeyValue#getPublicKey has the same narrow catch (NoSuchAlgorithmException | InvalidKeySpecException), so it would surface the same unchecked exception on the DOM decrypt path for a malformed DER key. Happy to send a separate change for that if it is useful.

Sure, please send another commit in this PR as well to address the same issue in the DOM path, thanks!

[Arpan0995]

Added as a third commit. DEREncodedKeyValue#getPublicKey() now also catches RuntimeException in the key-type loop, mirroring the StAX fix.

It's reachable on the DOM path via DEREncodedKeyValueResolver (a default KeyResolver) through KeyInfo#getPublicKey(). Reproduced on BC 1.85: without the catch it throws an uncaught ArrayIndexOutOfBoundsException: Index 9 out of bounds for length 8; with it, resolution fails cleanly.

DEREncodedKeyValueMalformedContentTest inserts BouncyCastle at first provider position (as XMLCipherTest does) so its KeyFactory is exercised, skipped otherwise; it fails without the catch and passes with it, PMD/errorprone clean, existing tests still green.

[ffang]

Thanks @Arpan0995 , merged!