davsclaus commented on code in PR #26726:
URL: https://github.com/apache/camel/pull/26726#discussion_r4071655376
##########
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:
🔴 **An empty identifier matches any element.**
`Element.getAttribute` returns `""` for a missing attribute, so when `id` is
empty this is `"".equals("")` on the very first iteration and the document is
accepted regardless of what it contains. Two reachable inputs produce an empty
`id`:
- `URI="#"` → `identifier = uri.substring(1)` = `""`, and
`getXPointerId("")` returns null, so `id = ""`.
- `URI="#xpointer(id(''))"` → `getXPointerId` strips the quotes and returns
`""`.
Both confirmed on this branch:
```
PROBE-A: URI="#" ACCEPTED (check bypassed)
PROBE-B: #xpointer(id('')) ACCEPTED (check bypassed)
```
An empty identifier cannot name the document element, so it should simply
not match:
```suggestion
if (id.isEmpty()) {
// an empty identifier names nothing - it must not fall through
to the getAttribute comparison
// below, where Element.getAttribute returns "" for a missing
attribute and would match anything
return false;
}
for (String attribute : ID_ATTRIBUTE_NAMES) {
if (id.equals(documentElement.getAttribute(attribute))) {
```
##########
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) {
Review Comment:
🔴 **This `return` exits the whole method, not just this iteration — so one
null-URI Reference disables the check for every Reference after it.**
Probed on this branch with references `[null, "#myID"]` against the wrapped
document:
```
PROBE-C: null-URI ref first -> ACCEPTED (check bypassed)
```
The `#myID` reference never gets examined and `sameDocumentReferenceSeen` is
never set, so the throw below is unreachable. The external-reference branch a
few lines down already handles the "tells us nothing" case correctly with
`continue` — this should match it. With `continue`, a lone null-URI reference
still ends up accepted (nothing sets `sameDocumentReferenceSeen`), so the
intended behaviour is preserved while the bypass closes.
```suggestion
if (uri == null) {
// Nothing to correlate against - but keep looking at the
remaining references
continue;
}
```
##########
components/camel-xmlsecurity/src/main/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2Message.java:
##########
@@ -142,8 +142,40 @@ public class DefaultXmlSignature2Message implements
XmlSignature2Message {
*/
public static final String OUTPUT_NODE_SEARCH_TYPE_XPATH = "XPath";
+ private static final String[] ID_ATTRIBUTE_NAMES = { "Id", "ID", "id" };
Review Comment:
🟠**Namespaced id attributes are not found, so legitimate documents are
rejected.**
These names are matched with the namespace-unaware `Element.getAttribute`,
which compares qualified names. A document element carrying `wsu:Id="myID"` —
the WS-Security convention, and a very plausible user of this option — is
therefore rejected even though the Reference does cover it:
```
PROBE-D: <signed xmlns:wsu="http://ns"; wsu:Id="myID"/> with URI="#myID"
-> REJECTED: None of the validated References covers the document
element
```
The `getElementById` fallback below doesn't rescue it: that only works when
a DTD or schema declared the attribute as type ID, which a plainly-parsed
instance document has not.
Either scan the element's attributes for a local name in this set regardless
of namespace, or — if that is deliberately out of scope — say so in the
`setEnforceReferenceCoverage` javadoc and the component docs, so operators know
the option is for unprefixed `Id`/`ID`/`id` only.
##########
components/camel-xmlsecurity/src/main/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2Message.java:
##########
@@ -155,7 +187,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:
🟠This branch — the actual wiring of the feature — is not covered by any
test. `DefaultXmlSignature2MessageReferenceCoverageTest` calls
`checkDocumentElementIsCoveredByAReference` directly with a stub `Input` and a
stub `Reference`, so a regression that stopped calling it from here, or flipped
the flag check, would not be caught.
An end-to-end test through `xmlsecurity-verify` with a real signature over a
sub-element, wrapped in an attacker document, would both cover this line and
demonstrate the feature actually stops the attack it describes.
--
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]
