gnodet-bot commented on code in PR #26726:
URL: https://github.com/apache/camel/pull/26726#discussion_r4080616337
##########
components/camel-xmlsecurity/src/main/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2Message.java:
##########
@@ -155,7 +196,11 @@ public void mapToMessage(Input input, Message output)
throws Exception {
node = getNodeForMessageBodyInEnvelopingCase(input);
} else {
// enveloped or detached XML signature --> remove signature
element
- node = input.getMessageBodyDocument().getDocumentElement();
+ Element documentElement =
input.getMessageBodyDocument().getDocumentElement();
+ if (enforceReferenceCoverage) {
+ checkDocumentElementIsCoveredByAReference(input,
documentElement);
Review Comment:
๐ **Integration path through `mapToMessage` is still not covered by any
test.**
`DefaultXmlSignature2MessageReferenceCoverageTest` calls
`checkDocumentElementIsCoveredByAReference` directly with a stub
`Input`/`Reference`, so the `if (enforceReferenceCoverage)` guard at this line
and the full `mapToMessage` โ verify endpoint path are not exercised at all. A
single test in `XmlSignatureTest` that:
1. Signs a fragment with an id-based reference
2. Wraps the signed fragment inside a larger document
3. Validates the wrapped document with `enforceReferenceCoverage=true` set
on a `DefaultXmlSignature2Message` bean
4. Asserts a `XmlSignatureException` is thrown
would be the proof that the feature works end-to-end. Without it, a
regression that breaks the wiring (e.g. the `enforceReferenceCoverage` field is
checked but the exception propagation path fails) would go undetected. This is
the gap `davsclaus` raised and it was not addressed by the unit test additions.
##########
components/camel-xmlsecurity/src/main/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2Message.java:
##########
@@ -314,6 +359,110 @@ protected Node
getNodeForMessageBodyInEnvelopingCase(Input input) throws Excepti
return node;
}
+ /**
+ * Checks that a validated Reference actually covered the document element
the default search is about to emit.
+ * <p>
+ * Core signature validation only proves that each Reference's digest
matches the content that Reference resolves
+ * to. It says nothing about the rest of the document. So an attacker can
take a legitimately signed fragment, embed
+ * it unchanged inside a larger document of their own, and validation
still passes - the same-document URI resolves
+ * to that fragment exactly as before - while this method would hand the
whole attacker document downstream as
+ * verified content. That is XML signature wrapping.
+ * <p>
+ * The check is deliberately narrow, so that it rejects that shape and
nothing else. It only complains when the
+ * signature carries same-document references and none of them covers the
document element. A Reference with an
+ * empty URI covers the whole document, and a signature whose References
are all external says nothing about this
+ * document either way, so both are left alone.
+ *
+ * @param input the verification input, carrying the validated
References
+ * @param documentElement the element the default search would emit
+ */
+ protected void checkDocumentElementIsCoveredByAReference(Input input,
Element documentElement) throws Exception {
+ List<Reference> references = getReferencesForMessageMapping(input);
+ if (references == null || references.isEmpty()) {
+ return;
+ }
+
+ boolean sameDocumentReferenceSeen = false;
+ for (Reference reference : references) {
+ String uri = reference.getURI();
+ if (uri == null) {
+ // An absent URI tells us nothing about this document. Like an
external reference below it must not
+ // short-circuit the check for the references that follow it;
a lone absent-URI reference still leaves
+ // sameDocumentReferenceSeen false, so the document is
correctly rejected.
+ continue;
Review Comment:
๐ก **Design choice is valid but the comment misrepresents the spec โ worth a
one-line fix.**
Per the `javax.xml.crypto.URIReference.getURI()` contract, `null` means the
`URI` attribute is **absent** from the `<Reference>` element. Per the W3C XML
Signature spec (ยง4.3.3.2), an absent URI identifies the whole enclosing
document โ identical in effect to `URI=""`. So a `null` URI that survived
validation **does** cover the document element; treating it as `continue` is
conservative (safe, never fail-open) but it will produce a false rejection for
any conforming signature that omits the `URI` attribute entirely.
oscerd's reasoning โ *"treating absent URI as 'accept' would accept the
wrapping case via a crafted absent-URI reference"* โ is not quite right: by the
time this check runs, the signature processor has already **validated** the
reference, so the absent-URI reference's digest was verified against the whole
document content. An attacker cannot attach a null-URI reference to a signature
that was originally signed over a fragment, because the digest would not match.
The current behaviour (false-positive for null-URI signatures, no
false-negatives) is the safer tradeoff, but the comment should say so:
```suggestion
if (uri == null) {
// Absent URI (getURI() == null per JSR-105) identifies the
whole document per the XML Signature
// spec, the same as URI="". However, treating it as
whole-document coverage here would let an
// attacker bypass the check by attaching a null-URI
reference, so we skip it conservatively:
// a lone absent-URI reference leaves
sameDocumentReferenceSeen false and the document is rejected.
continue;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
