You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "oscerd (via GitHub)" <gi...@apache.org> on 2026/09/22 07:21:11 UTC

[PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

oscerd opened a new pull request, #26726:
URL: https://github.com/apache/camel/pull/26726

   This fixes [CAMEL-24444](https://issues.apache.org/jira/browse/CAMEL-24444).
   
   Core XML-signature validation proves only that each `Reference`'s digest matches the content that Reference resolves to β€” not that the node the route ultimately consumes is covered by any signature. For an enveloped or detached signature handled by the default output-node search, an attacker can therefore present a document where a valid `Reference` covers a sub-element while the whole (unsigned) document element is what gets emitted onto the route: XML Signature Wrapping.
   
   A new opt-in `enforceReferenceCoverage` on `DefaultXmlSignature2Message` (default off) requires that one of the validated References cover the document element being emitted, otherwise verification fails. It is off by default because the legitimate detached-signature flow β€” a Reference deliberately covering a sub-element with the whole document emitted on output β€” would otherwise break; a deployment that wants the guarantee enables it, or supplies a custom output-node search / `XmlSignatureChecker` when the default search does not fit.
   
   **Test:** `DefaultXmlSignature2MessageReferenceCoverageTest` covers the covered and uncovered cases with the option both on and off.
   
   πŸ€– Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "oscerd (via GitHub)" <gi...@apache.org>.
oscerd commented on code in PR #26726:
URL: https://github.com/apache/camel/pull/26726#discussion_r4122571666


##########
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:
   Covered since 11a736c and fd17dc0. `XmlSignatureTest` now drives the `enforceReferenceCoverage` guard in `mapToMessage` end to end, through `xmlsecurity-verify`, in both directions. `enforceReferenceCoverageRejectsASignatureCoveringOnlyASubElement` verifies a document whose only reference covers a sub-element and expects `XmlSignatureException`. `enforceReferenceCoverageAcceptsAWholeDocumentReference` verifies an enveloped `URI=""` signature on its own route and expects it to pass. The acceptance test fails if the check is made to reject unconditionally, so it exercises the wiring rather than only the helper.
   
   _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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "oscerd (via GitHub)" <gi...@apache.org>.
oscerd commented on code in PR #26726:
URL: https://github.com/apache/camel/pull/26726#discussion_r4104282708


##########
components/camel-xmlsecurity/src/test/java/org/apache/camel/component/xmlsecurity/XmlSignatureTest.java:
##########
@@ -382,6 +387,41 @@ public void configure() {
         };
     }
 
+    RouteBuilder createEnforceReferenceCoverageRoute() {
+        return new RouteBuilder() {
+            public void configure() {
+                onException(XmlSignatureException.class).handled(true).to("mock:enforceCoverageException");
+                from("direct:enforceCoverage")
+                        .to("xmlsecurity-sign:enforceCoverage?keyAccessor=#keyAccessorDefault"
+                            + "&xpathsToIdAttributes=#xpathsToIdAttributes"
+                            + "&schemaResourceUri=org/apache/camel/component/xmlsecurity/Test.xsd&signatureId=&clearHeaders=false")
+                        .to("xmlsecurity-verify:enforceCoverage?keySelector=#keySelectorDefault"
+                            + "&schemaResourceUri=org/apache/camel/component/xmlsecurity/Test.xsd"
+                            + "&xmlSignature2Message=#enforceCoverageMapper")
+                        .to("mock:enforceCoverageResult");
+            }
+        };
+    }
+
+    @Test
+    void enforceReferenceCoverageRejectsASignatureCoveringOnlyASubElement() throws Exception {
+        // a detached signature legitimately covers a sub-element while the whole document is emitted - the same shape
+        // as an XML signature wrapping attack. With enforceReferenceCoverage on, the default output-node search must
+        // refuse to emit the uncovered document element. This drives the check through mapToMessage, not in isolation.
+        String detachedPayload = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                                 + "<ns:root xmlns:ns=\"http://test\"><a ID=\"myID\"><b>bValue</b></a></ns:root>";
+
+        MockEndpoint exceptionMock = getMockEndpoint("mock:enforceCoverageException");
+        exceptionMock.expectedMessageCount(1);
+        MockEndpoint resultMock = getMockEndpoint("mock:enforceCoverageResult");
+        resultMock.expectedMessageCount(0);
+
+        TestSupport.sendBody(this.template, "direct:enforceCoverage", detachedPayload,
+                Collections.singletonMap(XmlSignatureConstants.HEADER_CONTENT_REFERENCE_URI, (Object) "#myID"));
+
+        MockEndpoint.assertIsSatisfied(context);
+    }

Review Comment:
   Added in fd17dc0: `enforceReferenceCoverageAcceptsAWholeDocumentReference`, the acceptance counterpart to the rejection test. It signs an enveloped signature whose reference covers the whole document (URI="" with the enveloped-signature transform) and runs it through the full sign β†’ verify β†’ mapToMessage pipeline with `enforceReferenceCoverage=true`, asserting the exchange reaches `mock:enforceCoverageResult`.
   
   I used a dedicated enveloped route (`direct:enforceCoverageAccept`) rather than the existing `direct:enforceCoverage`, because the latter is a detached signature keyed on `xpathsToIdAttributes` and structurally signs a sub-element by id, so it can't produce a clean whole-document reference.
   
   To confirm the test actually exercises the wiring (not just the predicate), I temporarily made the coverage check reject unconditionally: the new acceptance test then fails (`mock:enforceCoverageResult` receives 0) while the rejection test still passes β€” so the two tests together pin both directions of the `if (enforceReferenceCoverage)` guard end-to-end. Full-reactor `mvn clean install -DskipTests` is green.



-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "oscerd (via GitHub)" <gi...@apache.org>.
oscerd commented on PR #26726:
URL: https://github.com/apache/camel/pull/26726#issuecomment-5812349533

   Thanks for the thorough re-review β€” and for verifying the `xml:id` branch is live (`XmlVerifierProcessor:288` β†’ `setNamespaceAware(true)`) and confirming the `wsu:Id` exclusion reasoning. Agreed on both.
   
   Two things done:
   
   1. **Port label removed.** You're right that the public `isEnforceReferenceCoverage`/`setEnforceReferenceCoverage` pair is API growth for a patch line. `enforceReferenceCoverage` is opt-in and off by default, so it's main/4.23 material rather than a maintenance backport. Dropped `port/camel-4.22.x`; if 4.22.x wants the protection later, that can be a separate, deliberate call.
   
   2. **End-to-end wiring test added** β€” `XmlSignatureTest#enforceReferenceCoverageRejectsASignatureCoveringOnlyASubElement`. Instead of DOM-wrapping, it uses the detached-signature path: a real signature that covers a sub-element by id while the whole document is emitted β€” the same shape as the wrapping attack. With `enforceReferenceCoverage` on, `xmlsecurity-verify` rejects it. I verified it fails when the `mapToMessage` call site is deleted (`mock:enforceCoverageResult` receives the message instead of `mock:exception`), so it covers the wiring, not just the predicate.
   
   One heads-up: the "apply suggestion" commit that improved the null-URI comment left an unmatched `}` that broke compilation β€” fixed in f512ed9. The improved comment itself is kept; it reads better than my original.
   
   _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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #26726:
URL: https://github.com/apache/camel/pull/26726#issuecomment-5772710039

   <!-- camel-backport-bot -->
   
   ### πŸ”„ Backport Bot
   
   This bugfix targets `main` and may need porting to:
   - `camel-4.22.x`
   
   Labels `port/<branch>` have been added. Remove a label to opt out of porting to that branch.
   
   Port PRs will be created automatically when this PR is merged. Comment `/port` to create them now.
   
   ℹ️ If you push additional commits after `/port`, use `/port` again to update the port PRs.


-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26726:
URL: https://github.com/apache/camel/pull/26726#issuecomment-5772991405

   <!-- ci-tested-modules -->
   :test_tube: **CI tested the following changed modules:**
   
   - `catalog/camel-catalog`
   - `components/camel-xmlsecurity`
   
   ---
   
   <details><summary>:microscope: Scalpel shadow comparison β€” Scalpel: 9 of 692 tested, 26 compile-only β€” current: 9 all tested</summary>
   
   [Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **36 affected modules** (current approach: 9).
   
   **Skip-tests mode would test 9 modules** (2 direct + 8 downstream), **skip tests for 26** (generated code, meta-modules)
   
   <details><summary>:warning: Modules only in Scalpel (27)</summary>
   
   - `apache-camel`
   - `camel-allcomponents`
   - `camel-catalog`
   - `camel-catalog-console`
   - `camel-catalog-maven`
   - `camel-catalog-suggest`
   - `camel-componentdsl`
   - `camel-endpointdsl`
   - `camel-endpointdsl-support`
   - `camel-itest`
   - `camel-jbang-core`
   - `camel-jbang-it`
   - `camel-jbang-main`
   - `camel-jbang-plugin-edit`
   - `camel-jbang-plugin-generate`
   - `camel-jbang-plugin-kubernetes`
   - `camel-jbang-plugin-test`
   - `camel-kamelet-main`
   - `camel-launcher`
   - `camel-report-maven-plugin`
   - `camel-route-parser`
   - `camel-yaml-dsl`
   - `camel-yaml-dsl-deserializers`
   - `camel-yaml-dsl-maven-plugin`
   - `coverage`
   - `docs`
   - `dummy-component`
   
   </details>
   
   
   <details><summary>Modules Scalpel would test (9)</summary>
   
   - `camel-catalog` ← catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/xmlsecurity-verify-component.adoc
   - `camel-jbang-mcp` ← downstream of org.apache.camel:camel-catalog
   - `camel-jbang-plugin-mcp` ← downstream of org.apache.camel:camel-jbang-core
   - `camel-jbang-plugin-route-parser` ← downstream of org.apache.camel:camel-route-parser
   - `camel-jbang-plugin-tui` ← downstream of org.apache.camel:camel-catalog
   - `camel-jbang-plugin-validate` ← downstream of org.apache.camel:camel-yaml-dsl-validator
   - `camel-launcher-container` ← downstream of org.apache.camel:camel-launcher
   - `camel-xmlsecurity` ← components/camel-xmlsecurity/src/main/docs/xmlsecurity-verify-component.adoc, components/camel-xmlsecurity/src/main/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2Message.java, components/camel-xmlsecurity/src/test/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2MessageReferenceCoverageTest.java
   - `camel-yaml-dsl-validator` ← downstream of org.apache.camel:camel-catalog
   - `camel-yaml-dsl-validator-maven-plugin` ← downstream of org.apache.camel:camel-yaml-dsl-validator
   
   </details>
   
   <details><summary>Modules with tests skipped (26)</summary>
   
   - `apache-camel`
   - `camel-allcomponents`
   - `camel-catalog-console`
   - `camel-catalog-maven`
   - `camel-catalog-suggest`
   - `camel-componentdsl`
   - `camel-endpointdsl`
   - `camel-endpointdsl-support`
   - `camel-itest`
   - `camel-jbang-core`
   - `camel-jbang-it`
   - `camel-jbang-main`
   - `camel-jbang-plugin-edit`
   - `camel-jbang-plugin-generate`
   - `camel-jbang-plugin-kubernetes`
   - `camel-jbang-plugin-test`
   - `camel-kamelet-main`
   - `camel-launcher`
   - `camel-report-maven-plugin`
   - `camel-route-parser`
   - `camel-yaml-dsl`
   - `camel-yaml-dsl-deserializers`
   - `camel-yaml-dsl-maven-plugin`
   - `coverage`
   - `docs`
   - `dummy-component`
   
   </details>
   
   > :information_source: Shadow mode β€” Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
   
   </details>
   
   <details><summary>All tested modules (36 modules, 5m 17s total)</summary>
   
   **Total reactor time:** 5m 17s
   
   | Module | Duration | Status |
   | --- | --- | --- |
   | Camel :: Launcher | 52.8s | SUCCESS |
   | Camel :: JBang :: MCP | 42.5s | SUCCESS |
   | Camel :: JBang :: Plugin :: TUI | 34.9s | SUCCESS |
   | Camel :: XML Security | 26.4s | SUCCESS |
   | Camel :: Catalog :: Camel Catalog | 24.8s | SUCCESS |
   | Camel :: Component DSL | 21.7s | SUCCESS |
   | Camel :: JBang :: Plugin :: Kubernetes | 16.4s | SUCCESS |
   | Camel :: Docs | 15.8s | SUCCESS |
   | Camel :: Catalog :: Camel Report Maven Plugin | 8.4s | SUCCESS |
   | Camel :: Kamelet Main | 7.9s | SUCCESS |
   | Camel :: YAML DSL :: Validator | 7.8s | SUCCESS |
   | Camel :: JBang :: Plugin :: Testing | 7.7s | SUCCESS |
   | Camel :: Catalog :: Camel Route Parser | 7.7s | SUCCESS |
   | Camel :: YAML DSL :: Deserializers | 5.9s | SUCCESS |
   | Camel :: JBang :: Plugin :: Validate | 5.8s | SUCCESS |
   | Camel :: All Components Sync point | 5.4s | SUCCESS |
   | Camel :: YAML DSL :: Validator Maven Plugin | 4.0s | SUCCESS |
   | Camel :: YAML DSL :: Maven Plugins | 3.3s | SUCCESS |
   | Camel :: Catalog :: Maven | 2.9s | SUCCESS |
   | Camel :: Catalog :: Suggest (deprecated) | 2.6s | SUCCESS |
   | Camel :: JBang :: Plugin :: Edit | 2.0s | SUCCESS |
   | Camel :: Assembly | 1.7s | SUCCESS |
   | Camel :: Coverage | 1.4s | SUCCESS |
   | Camel :: JBang :: Integration tests | 1.1s | SUCCESS |
   | Camel :: Catalog :: Dummy Component | 1.0s | SUCCESS |
   | Camel :: JBang :: Main | 1.0s | SUCCESS |
   | Camel :: JBang :: Plugin :: Generate | 0.9s | SUCCESS |
   | Camel :: Catalog :: Console | 0.8s | SUCCESS |
   | Camel :: Endpoint DSL :: Support | 0.7s | SUCCESS |
   | Camel :: Launcher :: Container | 0.7s | SUCCESS |
   | Camel :: JBang :: Plugin :: MCP | 0.7s | SUCCESS |
   | Camel :: JBang :: Plugin :: Route Parser | 0.5s | SUCCESS |
   | Camel :: Endpoint DSL | n/a |  |
   | Camel :: Integration Tests | n/a |  |
   | Camel :: JBang :: Core | n/a |  |
   | Camel :: YAML DSL | n/a |  |
   
   **Top 20 slowest modules:**
   - `Camel :: Launcher` (52.8s)
   - `Camel :: JBang :: MCP` (42.5s)
   - `Camel :: JBang :: Plugin :: TUI` (34.9s)
   - `Camel :: XML Security` (26.4s)
   - `Camel :: Catalog :: Camel Catalog` (24.8s)
   - `Camel :: Component DSL` (21.7s)
   - `Camel :: JBang :: Plugin :: Kubernetes` (16.4s)
   - `Camel :: Docs` (15.8s)
   - `Camel :: Catalog :: Camel Report Maven Plugin` (8.4s)
   - `Camel :: Kamelet Main` (7.9s)
   - `Camel :: YAML DSL :: Validator` (7.8s)
   - `Camel :: JBang :: Plugin :: Testing` (7.7s)
   - `Camel :: Catalog :: Camel Route Parser` (7.7s)
   - `Camel :: YAML DSL :: Deserializers` (5.9s)
   - `Camel :: JBang :: Plugin :: Validate` (5.8s)
   - `Camel :: All Components Sync point` (5.4s)
   - `Camel :: YAML DSL :: Validator Maven Plugin` (4.0s)
   - `Camel :: YAML DSL :: Maven Plugins` (3.3s)
   - `Camel :: Catalog :: Maven` (2.9s)
   - `Camel :: Catalog :: Suggest (deprecated)` (2.6s)
   
   </details>
   
   ---
   :gear: [View full build and test results](https://github.com/apache/camel/actions/runs/35699172045)


-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #26726:
URL: https://github.com/apache/camel/pull/26726#discussion_r4112510335


##########
components/camel-xmlsecurity/src/main/java/org/apache/camel/component/xmlsecurity/api/DefaultXmlSignature2Message.java:
##########
@@ -314,6 +359,111 @@ 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) {
+                // 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.

Review Comment:
   Two inaccuracies here. (1) XMLDSig does not give an omitted URI a referent β€” it is the "application knows the object" case; only `URI=""` is the whole document. (2) A lone absent-URI reference leaves `sameDocumentReferenceSeen` false, so the method returns without throwing β€” the document is left alone (like the all-external case), not rejected.
   
   ```suggestion
                   // An absent URI tells us nothing about this document: XMLDSig leaves its referent to the application,
                   // unlike URI="", which is the whole document. Like an external reference below, it must not
                   // short-circuit the check for the references that follow it. A signature whose references are all
                   // absent or external leaves sameDocumentReferenceSeen false and is left alone, as described above.
   ```



-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus merged PR #26726:
URL: https://github.com/apache/camel/pull/26726


-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26726:
URL: https://github.com/apache/camel/pull/26726#issuecomment-5772690011

   :star2: Thank you for your contribution to the Apache Camel project! :star2:
     :robot: CI automation will test this PR automatically.
   
     :camel: Apache Camel Committers, please review the following items:
   
     * First-time contributors **require MANUAL approval** for the GitHub Actions to run
     * You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
     * You can label PRs using `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
     * Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
     
     :warning: Be careful when sharing logs. Review their contents before sharing them publicly.


-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "oscerd (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CAMEL-24444: camel-xmlsecurity - option to require the verified output to be covered by a Reference [camel]

Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #26726:
URL: https://github.com/apache/camel/pull/26726#discussion_r4092565846


##########
components/camel-xmlsecurity/src/test/java/org/apache/camel/component/xmlsecurity/XmlSignatureTest.java:
##########
@@ -382,6 +387,41 @@ public void configure() {
         };
     }
 
+    RouteBuilder createEnforceReferenceCoverageRoute() {
+        return new RouteBuilder() {
+            public void configure() {
+                onException(XmlSignatureException.class).handled(true).to("mock:enforceCoverageException");
+                from("direct:enforceCoverage")
+                        .to("xmlsecurity-sign:enforceCoverage?keyAccessor=#keyAccessorDefault"
+                            + "&xpathsToIdAttributes=#xpathsToIdAttributes"
+                            + "&schemaResourceUri=org/apache/camel/component/xmlsecurity/Test.xsd&signatureId=&clearHeaders=false")
+                        .to("xmlsecurity-verify:enforceCoverage?keySelector=#keySelectorDefault"
+                            + "&schemaResourceUri=org/apache/camel/component/xmlsecurity/Test.xsd"
+                            + "&xmlSignature2Message=#enforceCoverageMapper")
+                        .to("mock:enforceCoverageResult");
+            }
+        };
+    }
+
+    @Test
+    void enforceReferenceCoverageRejectsASignatureCoveringOnlyASubElement() throws Exception {
+        // a detached signature legitimately covers a sub-element while the whole document is emitted - the same shape
+        // as an XML signature wrapping attack. With enforceReferenceCoverage on, the default output-node search must
+        // refuse to emit the uncovered document element. This drives the check through mapToMessage, not in isolation.
+        String detachedPayload = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                                 + "<ns:root xmlns:ns=\"http://test\"><a ID=\"myID\"><b>bValue</b></a></ns:root>";
+
+        MockEndpoint exceptionMock = getMockEndpoint("mock:enforceCoverageException");
+        exceptionMock.expectedMessageCount(1);
+        MockEndpoint resultMock = getMockEndpoint("mock:enforceCoverageResult");
+        resultMock.expectedMessageCount(0);
+
+        TestSupport.sendBody(this.template, "direct:enforceCoverage", detachedPayload,
+                Collections.singletonMap(XmlSignatureConstants.HEADER_CONTENT_REFERENCE_URI, (Object) "#myID"));
+
+        MockEndpoint.assertIsSatisfied(context);
+    }

Review Comment:
   🟠 **Integration test only covers the rejection path β€” the happy-path acceptance is untested end-to-end.**
   
   `enforceReferenceCoverageRejectsASignatureCoveringOnlyASubElement` proves that a sub-element reference causes rejection. It does not prove that a signature whose reference covers the document element is accepted through the full sign→verify→mapToMessage pipeline.
   
   The unit tests in `DefaultXmlSignature2MessageReferenceCoverageTest` cover the acceptance logic (e.g. `anEmptyReferenceUriCoversTheWholeDocument`, `aReferenceToTheDocumentElementsOwnIdIsAccepted`) but those use stub `Input`/`Reference` objects. A regression that e.g. inverted the `if (enforceReferenceCoverage)` guard but kept the call wired would still pass all current tests.
   
   Add one test that sends an enveloped-style payload where `URI=""` covers the whole document, with `enforceReferenceCoverage=true`, and asserts that `mock:enforceCoverageResult` receives a message (i.e. the route completes normally). The existing route already handles this β€” it just needs a test sending a payload with a whole-document reference.



-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org