oscerd commented on code in PR #26726:
URL: https://github.com/apache/camel/pull/26726#discussion_r4080562504
##########
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:
Fixed in 1eadf1f — changed the `uri == null` branch from `return` to
`continue`, matching the external-reference branch below it. A lone absent-URI
reference now leaves `sameDocumentReferenceSeen` false, so it is still
correctly rejected, while a following `#myID` reference is no longer skipped.
Covered by `aNullReferenceUriDoesNotDisableTheCheckForLaterReferences`, which
fails against the un-fixed code (verified).
_Claude Code on behalf of oscerd_
##########
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:
Fixed in 1eadf1f — `coversElement` now returns false immediately for an
empty identifier, before the `getAttribute` comparison where `""` for a missing
attribute would `"".equals("")` and match anything. Covered by
`aBareHashReferenceDoesNotCoverAnything` (`URI="#"`) and
`anXPointerWithAnEmptyIdDoesNotCoverAnything` (`#xpointer(id(''))`), both of
which fail against the un-fixed code.
_Claude Code on behalf of oscerd_
##########
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:
The comment is gone — the `null` branch now `continue`s (1eadf1f). On the
"null means whole-document coverage" reading: I went the other way
deliberately, because treating an absent URI as "accept the whole document"
reintroduces a bypass @davsclaus probed in the adjacent thread — `[null,
"#evil"]` would accept even when no reference covers the document element. A
same-document reference with `URI=""` is the reliable "whole document" signal
and still returns; an absent URI is treated as "tells us nothing", so a lone
one fails closed.
_Claude Code on behalf of oscerd_
##########
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:
Fixed in 1eadf1f — `coversElement` now also checks
`getAttributeNS("http://www.w3.org/XML/1998/namespace";, "id")` (via
`XMLConstants.XML_NS_URI`), so an `xml:id` on the document element is
recognised and a reference to it is no longer falsely rejected. Covered by
`aReferenceToTheDocumentElementsXmlIdIsAccepted` (the coverage test now parses
namespace-aware, matching the signature processor).
_Claude Code on behalf of oscerd_
##########
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:
I went with documenting the limitation rather than broadening the match. A
namespace-agnostic local-name scan would reintroduce a wrapping bypass: the
check runs *after* validation, so it must agree with how the signature
processor resolved the reference. `xml:id` and DTD/schema-declared IDs are
mechanisms the processor itself resolves through (added `xml:id` in 1eadf1f).
But an arbitrary `*:Id` matched by local name is not — an attacker can place
`wsu:Id="myID"` on their wrapper root while the validated `#myID` actually
resolved to the original fragment via a real ID, and the scan would wrongly
accept.
So the setter javadoc and `xmlsecurity-verify-component.adoc` now state that
`enforceReferenceCoverage` matches `Id`/`ID`/`id`, `xml:id`, and
schema/DTD-declared IDs, and that a namespaced `wsu:Id` is not matched by name
— use an `outputNodeSearch` for those. Happy to revisit if you'd prefer a
WS-Security-namespace-specific allowance, but a blanket local-name scan I think
is unsafe here.
_Claude Code on behalf of oscerd_
##########
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:
The check itself is now comprehensively unit-tested, including both bypasses
you found — `aNullReferenceUriDoesNotDisableTheCheckForLaterReferences`,
`aBareHashReferenceDoesNotCoverAnything`,
`anXPointerWithAnEmptyIdDoesNotCoverAnything` — each verified to fail against
the un-fixed code, plus `xml:id` acceptance.
You're right that the wiring in `mapToMessage` (the flag check + the call)
is not yet exercised end to end. I held off on the full sign-a-sub-element /
wrap / verify test in this round because building a correct XSW fixture (a real
enveloped signature plus the DOM surgery to wrap it) is easy to get subtly
wrong, and a fragile test on a security feature is worse than none. I'd like to
add it as a focused follow-up so it demonstrates the attack is actually
stopped, rather than rush it here. Let me know if you'd rather it block this PR.
_Claude Code on behalf of oscerd_
--
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]
