gnodet-bot commented on code in PR #26726:
URL: https://github.com/apache/camel/pull/26726#discussion_r4069341603
##########
components/camel-xmlsecurity/src/main/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2Message.java:
##########
@@ -314,6 +350,96 @@ 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) {
+ // Nothing to correlate against
+ return;
Review Comment:
⚠️ **Wrong comment — `null` URI means whole-document in XML DSig.**
Per JSR-105 (`javax.xml.crypto.dsig.Reference.getURI()`), `null` means the
URI attribute is **absent** from the `<Reference>` element. Per XML DSig 1.0
§4.4.3, an absent URI dereferences the same document (the whole octect stream),
equivalent to `URI=""`. Apache Santuario returns `null` for absent URIs in
exactly this sense.
So `null` here means _"the whole document is referenced"_ — it IS coverage,
not the absence of it. The comment "Nothing to correlate against" is factually
wrong and misleading to anyone maintaining this code. It should say the same
thing as the `uri.isEmpty()` branch does.
Also: there is no test for the `null`-URI case. Add one:
```suggestion
if (uri == null) {
// Absent URI (same as URI="") — the whole document is
covered
return;
}
```
And add to the test class:
```java
@Test
void aNullReferenceUriTreatedAsWholeDocumentCoverage() throws Exception {
check(WRAPPED, null);
}
```
where `check` passes `null` to `TestReference` (the `getURI()` stub already
returns the constructor argument, so this just works).
##########
components/camel-xmlsecurity/src/main/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2Message.java:
##########
@@ -314,6 +350,96 @@ 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) {
+ // Nothing to correlate against
+ return;
+ }
+ if (uri.isEmpty()) {
+ // The whole document is covered
+ return;
+ }
+ if (!uri.startsWith("#")) {
+ // External reference - it tells us nothing about the document
we are emitting
+ continue;
+ }
+ sameDocumentReferenceSeen = true;
+ String identifier = uri.substring(1);
+ if (identifier.startsWith("xpointer(/)")) {
+ // #xpointer(/) is the whole document
+ return;
+ }
+ if (coversElement(identifier, documentElement)) {
+ return;
+ }
+ }
+
+ if (sameDocumentReferenceSeen) {
+ throw new XmlSignatureException(
+ "Cannot extract the root node for the output document from
the XML signature document. "
+ + "None of the validated
References covers the document element, so the "
+ + "document contains content which
was not signed. Configure an output node "
+ + "search, or an
XmlSignatureChecker, which selects the signed content.");
+ }
+ }
+
+ private static boolean coversElement(String identifier, Element
documentElement) {
+ String xpointerId = getXPointerId(identifier);
+ String id = xpointerId != null ? xpointerId : identifier;
+
+ for (String attribute : ID_ATTRIBUTE_NAMES) {
+ if (id.equals(documentElement.getAttribute(attribute))) {
Review Comment:
⚠️ **`xml:id` (XML DSig 1.1 / RFC 3275) not handled — causes false
rejection.**
`ID_ATTRIBUTE_NAMES` covers `"Id"`, `"ID"`, `"id"` (XML DSig 1.0 convention)
and the `getElementById()` fallback covers schema-declared ID attributes, but
there is no handling for `xml:id` (defined in the [XML namespace
spec](https://www.w3.org/TR/xml-id/)).
XML DSig 1.1 explicitly recommends `xml:id` for element identification. A
document whose root carries `xml:id="myID"` and whose signature references
`#myID` would:
1. validate correctly (the digest matches),
2. **but throw `XmlSignatureException`** when
`enforceReferenceCoverage=true` because neither the attribute-name loop nor
`getElementById()` (which only looks at schema-declared ID types) finds the
match.
This is a false rejection that silently breaks valid signatures on XML DSig
1.1 documents. Add `xml:id` to the check:
```suggestion
for (String attribute : ID_ATTRIBUTE_NAMES) {
if (id.equals(documentElement.getAttribute(attribute))) {
return true;
}
}
// xml:id (XML DSig 1.1 / W3C xml:id spec)
if
(id.equals(documentElement.getAttributeNS("http://www.w3.org/XML/1998/namespace";,
"id"))) {
return true;
}
```
Also add a test case:
```java
@Test
void aXmlIdAttributeIsRecognised() throws Exception {
check("<signed xml:id=\"myID\"><b>bValue</b></signed>", "#myID");
}
```
--
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]
