This is an automated email from the ASF dual-hosted git repository.
coheigea pushed a commit to branch 4.1.x-fixes
in repository https://gitbox.apache.org/repos/asf/cxf.git
The following commit(s) were added to refs/heads/4.1.x-fixes by this push:
new 426ab88e9b6 Prevent XML Signature wrapping in AbstractXmlSigInHandler
(#3504)
426ab88e9b6 is described below
commit 426ab88e9b6776c128594b160d62831543841f7b
Author: Colm O hEigeartaigh <[email protected]>
AuthorDate: Fri Sep 25 16:19:37 2026 +0100
Prevent XML Signature wrapping in AbstractXmlSigInHandler (#3504)
* Prevent XML Signature wrapping in AbstractXmlSigInHandler
* Apply suggestion from @reta
Co-authored-by: Andriy Redko <[email protected]>
* Fixup
---------
Co-authored-by: Andriy Redko <[email protected]>
(cherry picked from commit e43776be4dca84494478e45977f78b44546b7dd6)
---
.../rs/security/saml/SamlEnvelopedInHandler.java | 18 +-
.../rs/security/xml/AbstractXmlSigInHandler.java | 73 ++++--
.../cxf/rs/security/xml/XmlSigInHandlerTest.java | 266 +++++++++++++++++++++
3 files changed, 332 insertions(+), 25 deletions(-)
diff --git
a/rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/saml/SamlEnvelopedInHandler.java
b/rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/saml/SamlEnvelopedInHandler.java
index e22474e2ef8..79adb9bf29e 100644
---
a/rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/saml/SamlEnvelopedInHandler.java
+++
b/rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/saml/SamlEnvelopedInHandler.java
@@ -27,6 +27,7 @@ import javax.xml.stream.XMLStreamReader;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
+import org.w3c.dom.Node;
import org.w3c.dom.NodeList;
import jakarta.ws.rs.HttpMethod;
@@ -57,6 +58,7 @@ public class SamlEnvelopedInHandler extends
AbstractSamlInHandler {
}
Document doc = null;
+ Element signedBody = null;
InputStream is = message.getContent(InputStream.class);
if (is != null) {
try {
@@ -67,7 +69,15 @@ public class SamlEnvelopedInHandler extends
AbstractSamlInHandler {
} else {
XMLStreamReader reader = message.getContent(XMLStreamReader.class);
if (reader instanceof W3CDOMStreamReader) {
- doc = ((W3CDOMStreamReader)reader).getDocument();
+ W3CDOMStreamReader w3cReader = (W3CDOMStreamReader)reader;
+ doc = w3cReader.getDocument();
+
+ // A detached XML Signature has already been validated, and
the reader
+ // is restricted to the signed element, which is then the body
+ Node node = w3cReader.getCurrentNode();
+ if (node instanceof Element && node !=
doc.getDocumentElement()) {
+ signedBody = (Element)node;
+ }
}
}
if (doc == null) {
@@ -85,7 +95,11 @@ public class SamlEnvelopedInHandler extends
AbstractSamlInHandler {
validateToken(message, samlElement);
doc.getDocumentElement().removeChild(samlElement);
- if (bodyIsRoot) {
+ if (signedBody != null) {
+ message.setContent(XMLStreamReader.class,
+ new W3CDOMStreamReader(signedBody));
+ message.setContent(InputStream.class, null);
+ } else if (bodyIsRoot) {
message.setContent(XMLStreamReader.class,
new W3CDOMStreamReader(doc));
message.setContent(InputStream.class, null);
diff --git
a/rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/xml/AbstractXmlSigInHandler.java
b/rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/xml/AbstractXmlSigInHandler.java
index daa1c2a9d2e..585b3e099a0 100644
---
a/rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/xml/AbstractXmlSigInHandler.java
+++
b/rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/xml/AbstractXmlSigInHandler.java
@@ -25,6 +25,7 @@ import java.security.cert.X509Certificate;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
+import java.util.Set;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@@ -55,6 +56,15 @@ import org.apache.xml.security.utils.Constants;
public class AbstractXmlSigInHandler extends AbstractXmlSecInHandler {
+ private static final Set<String> ALLOWED_TRANSFORMS = Set.of(
+ Transforms.TRANSFORM_ENVELOPED_SIGNATURE,
+ Transforms.TRANSFORM_C14N_OMIT_COMMENTS,
+ Transforms.TRANSFORM_C14N_WITH_COMMENTS,
+ Transforms.TRANSFORM_C14N11_OMIT_COMMENTS,
+ Transforms.TRANSFORM_C14N11_WITH_COMMENTS,
+ Transforms.TRANSFORM_C14N_EXCL_OMIT_COMMENTS,
+ Transforms.TRANSFORM_C14N_EXCL_WITH_COMMENTS);
+
private boolean removeSignature = true;
private boolean persistSignature = true;
private boolean keyInfoMustBeAvailable = true;
@@ -169,20 +179,23 @@ public class AbstractXmlSigInHandler extends
AbstractXmlSecInHandler {
if (!valid) {
throwFault("Signature validation failed", null);
}
+ Element signedEl = getSignedElement(root, ref);
+ // Only pass on the signed element. This is the root for an enveloped
signature, and
+ // a child of the (unsigned) root for a detached signature. The
reader's document
+ // is still the full document, e.g. for SamlEnvelopedInHandler to get
the assertion
+ Element body = isEnveloping(root) ? root : signedEl;
if (removeSignature) {
if (!isEnveloping(root)) {
- Element signedEl = getSignedElement(root, ref);
signedEl.removeAttribute("ID");
root.removeChild(signatureElement);
} else {
- Element actualBody = getActualBody(root);
Document newDoc = DOMUtils.createDocument();
- newDoc.adoptNode(actualBody);
- root = actualBody;
+ newDoc.adoptNode(signedEl);
+ body = signedEl;
}
}
message.setContent(XMLStreamReader.class,
- new W3CDOMStreamReader(root));
+ new W3CDOMStreamReader(body));
message.setContent(InputStream.class, null);
}
@@ -196,19 +209,6 @@ public class AbstractXmlSigInHandler extends
AbstractXmlSecInHandler {
}
- private Element getActualBody(Element envelopingSigElement) {
- Element objectNode = getNode(envelopingSigElement,
Constants.SignatureSpecNS, "Object", 0);
- if (objectNode == null) {
- throwFault("Object envelope is not available", null);
- }
- Element node = DOMUtils.getFirstElement(objectNode);
- if (node == null) {
- throwFault("No signed data is found", null);
- }
- return node;
-
- }
-
private Element getSignatureElement(Element sigParentElement) {
if (isEnveloping(sigParentElement)) {
return sigParentElement;
@@ -262,20 +262,36 @@ public class AbstractXmlSigInHandler extends
AbstractXmlSecInHandler {
String c14TransformExpected = sigProps != null ?
sigProps.getSignatureC14nTransform() : null;
boolean envelopedConfirmed = false;
for (int i = 0; i < transforms.getLength(); i++) {
+ String transformURI = null;
try {
Transform tr = transforms.item(i);
- if
(Transforms.TRANSFORM_ENVELOPED_SIGNATURE.equals(tr.getURI())) {
- envelopedConfirmed = true;
- } else if (c14TransformExpected != null &&
c14TransformExpected.equals(tr.getURI())) {
- c14TransformConfirmed = true;
- }
+ transformURI = tr.getURI();
} catch (Exception ex) {
throwFault("Problem accessing Transform instance", ex);
}
+ // Only allow transforms which cover the whole of the signed
element, so that
+ // no unsigned content (e.g. excluded via XPath) is passed on to
the application
+ if (!ALLOWED_TRANSFORMS.contains(transformURI)) {
+ throwFault("Signature Transform is not supported", null);
+ }
+ if (Transforms.TRANSFORM_ENVELOPED_SIGNATURE.equals(transformURI))
{
+ envelopedConfirmed = true;
+ } else if (c14TransformExpected != null &&
c14TransformExpected.equals(transformURI)) {
+ c14TransformConfirmed = true;
+ }
}
if (enveloped && !envelopedConfirmed) {
throwFault("Only enveloped signatures are currently supported",
null);
}
+ // The Signature is a child of the document root, so an enveloped
signature
+ // must reference the root. Otherwise the signed element could be
wrapped in
+ // unsigned content which would then be passed on to the application.
+ if (envelopedConfirmed && !enveloped) {
+ throwFault("Enveloped signature must reference the document root",
null);
+ }
+ if (isEnveloping(root)) {
+ validateEnvelopingReference(root, signedEl);
+ }
if (c14TransformExpected != null && !c14TransformConfirmed) {
throwFault("Transform Canonicalization is not supported", null);
}
@@ -291,6 +307,17 @@ public class AbstractXmlSigInHandler extends
AbstractXmlSecInHandler {
return signedEl;
}
+ // The signed element must be the only element in the only Object of the
enveloping
+ // Signature, so that no unsigned content can be passed on to the
application
+ private void validateEnvelopingReference(Element root, Element signedEl) {
+ List<Element> objects = DOMUtils.getChildrenWithName(root,
Constants.SignatureSpecNS, "Object");
+ if (objects.size() != 1 || signedEl.getParentNode() != objects.get(0)
+ || DOMUtils.getFirstElement(objects.get(0)) != signedEl
+ || DOMUtils.getNextElement(signedEl) != null) {
+ throwFault("Enveloping signature must reference the signed Object
content", null);
+ }
+ }
+
private Element getSignedElement(Element root, Reference ref) {
String rootId = root.getAttribute("ID");
String expectedID = ref.getURI().substring(1);
diff --git
a/rt/rs/security/xml/src/test/java/org/apache/cxf/rs/security/xml/XmlSigInHandlerTest.java
b/rt/rs/security/xml/src/test/java/org/apache/cxf/rs/security/xml/XmlSigInHandlerTest.java
new file mode 100644
index 00000000000..d5ca578c25a
--- /dev/null
+++
b/rt/rs/security/xml/src/test/java/org/apache/cxf/rs/security/xml/XmlSigInHandlerTest.java
@@ -0,0 +1,266 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cxf.rs.security.xml;
+
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.Properties;
+
+import javax.xml.stream.XMLStreamReader;
+
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+
+import jakarta.ws.rs.WebApplicationException;
+import org.apache.cxf.helpers.DOMUtils;
+import org.apache.cxf.message.ExchangeImpl;
+import org.apache.cxf.message.Message;
+import org.apache.cxf.message.MessageImpl;
+import org.apache.cxf.rt.security.SecurityConstants;
+import org.apache.cxf.staxutils.StaxUtils;
+import org.apache.cxf.staxutils.W3CDOMStreamReader;
+import org.apache.wss4j.common.crypto.Crypto;
+import org.apache.wss4j.common.crypto.CryptoFactory;
+import org.apache.wss4j.common.crypto.CryptoType;
+import org.apache.xml.security.algorithms.MessageDigestAlgorithm;
+import org.apache.xml.security.signature.XMLSignature;
+import org.apache.xml.security.transforms.Transforms;
+import org.apache.xml.security.utils.Constants;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+public class XmlSigInHandlerTest {
+
+ private static Crypto crypto;
+
+ @BeforeClass
+ public static void setUpCrypto() throws Exception {
+ org.apache.xml.security.Init.init();
+ Properties props = new Properties();
+ props.put("org.apache.wss4j.crypto.provider",
"org.apache.wss4j.common.crypto.Merlin");
+ props.put("org.apache.wss4j.crypto.merlin.keystore.type", "jks");
+ props.put("org.apache.wss4j.crypto.merlin.keystore.password",
"password");
+ props.put("org.apache.wss4j.crypto.merlin.keystore.alias", "alice");
+ props.put("org.apache.wss4j.crypto.merlin.keystore.file", "alice.jks");
+ crypto = CryptoFactory.getInstance(props);
+ }
+
+ @Test
+ public void testEnvelopedSignature() throws Exception {
+ Document doc = createSignedDocument();
+
+ assertBody(doc, "CXF");
+ }
+
+ @Test
+ public void testWrappedEnvelopedSignatureIsRejected() throws Exception {
+ Document signedDoc = createSignedDocument();
+ Element signedRoot = signedDoc.getDocumentElement();
+ Element signature =
+ DOMUtils.getFirstChildWithName(signedRoot,
Constants.SignatureSpecNS, "Signature");
+
+ // Wrap the signed element in a new unsigned root and move the
Signature
+ // to be a direct child of that root
+ Document doc = DOMUtils.createDocument();
+ Element root = createBook(doc, "evil");
+ doc.appendChild(root);
+ signedRoot.removeChild(signature);
+ root.appendChild(doc.importNode(signedRoot, true));
+ root.appendChild(doc.importNode(signature, true));
+
+ assertRejected(doc);
+ }
+
+ @Test
+ public void testEnvelopingSignature() throws Exception {
+ Document doc = createEnvelopingSignedDocument();
+
+ assertBody(doc, "CXF");
+ }
+
+ @Test
+ public void testEnvelopingSignatureWithUnsignedObjectContentIsRejected()
throws Exception {
+ Document doc = createEnvelopingSignedDocument();
+ Element object =
+ DOMUtils.getFirstChildWithName(doc.getDocumentElement(),
Constants.SignatureSpecNS, "Object");
+
+ // Add an unsigned element before the signed element in the same Object
+ object.insertBefore(createBook(doc, "evil"), object.getFirstChild());
+
+ assertRejected(doc);
+ }
+
+ @Test
+ public void testEnvelopingSignatureWithUnsignedObjectIsRejected() throws
Exception {
+ Document doc = createEnvelopingSignedDocument();
+ Element root = doc.getDocumentElement();
+ Element object = DOMUtils.getFirstChildWithName(root,
Constants.SignatureSpecNS, "Object");
+
+ // Add an unsigned Object before the signed Object
+ Element evilObject = doc.createElementNS(Constants.SignatureSpecNS,
"ds:Object");
+ evilObject.appendChild(createBook(doc, "evil"));
+ root.insertBefore(evilObject, object);
+
+ assertRejected(doc);
+ }
+
+ @Test
+ public void testEnvelopingSignatureWithObjectInKeyInfo() throws Exception {
+ Document doc = createEnvelopingSignedDocument();
+ Element keyInfo =
+ DOMUtils.getFirstChildWithName(doc.getDocumentElement(),
Constants.SignatureSpecNS, "KeyInfo");
+
+ // An unsigned Object nested in the KeyInfo must not be passed on as
the body
+ Element evilObject = doc.createElementNS(Constants.SignatureSpecNS,
"ds:Object");
+ evilObject.appendChild(createBook(doc, "evil"));
+ keyInfo.appendChild(evilObject);
+
+ assertBody(doc, "CXF");
+ }
+
+ @Test
+ public void testDetachedSignature() throws Exception {
+ Document doc = createDetachedSignedDocument();
+
+ // Only the signed element is passed on, not the unsigned wrapper
+ assertBody(doc, "CXF");
+ }
+
+ @Test
+ public void testDetachedSignatureWithUnsignedContent() throws Exception {
+ Document doc = createDetachedSignedDocument();
+
+ // Add unsigned content to the unsigned root, before the signed element
+ Element root = doc.getDocumentElement();
+ root.insertBefore(createBook(doc, "evil"), root.getFirstChild());
+
+ assertBody(doc, "CXF");
+ }
+
+ private static void assertBody(Document doc, String expectedName) throws
Exception {
+ Message message = createMessage(doc);
+ new AbstractXmlSigInHandler() { }.checkSignature(message);
+
+ XMLStreamReader reader = message.getContent(XMLStreamReader.class);
+ assertNotNull(reader);
+ Element root = StaxUtils.read(reader).getDocumentElement();
+ assertEquals("Book", root.getLocalName());
+ assertEquals(expectedName,
DOMUtils.getFirstElement(root).getTextContent());
+ }
+
+ private static void assertRejected(Document doc) {
+ Message message = createMessage(doc);
+ try {
+ new AbstractXmlSigInHandler() { }.checkSignature(message);
+ fail("Failure expected on a wrapped signature");
+ } catch (WebApplicationException ex) {
+ assertEquals(400, ex.getResponse().getStatus());
+ }
+ }
+
+ private static Message createMessage(Document doc) {
+ Message message = new MessageImpl();
+ message.setExchange(new ExchangeImpl());
+ message.put(Message.HTTP_REQUEST_METHOD, "POST");
+ message.put(SecurityConstants.SIGNATURE_CRYPTO, crypto);
+ message.setContent(XMLStreamReader.class, new
W3CDOMStreamReader(doc.getDocumentElement()));
+ return message;
+ }
+
+ private static Document createSignedDocument() throws Exception {
+ Document doc = DOMUtils.createDocument();
+ Element root = createBook(doc, "CXF");
+ doc.appendChild(root);
+
+ String id = "_book";
+ root.setAttributeNS(null, "Id", id);
+ root.setIdAttributeNS(null, "Id", true);
+
+ XMLSignature sig = new XMLSignature(doc, "",
XMLSignature.ALGO_ID_SIGNATURE_RSA_SHA256);
+ root.appendChild(sig.getElement());
+ Transforms transforms = new Transforms(doc);
+ transforms.addTransform(Transforms.TRANSFORM_ENVELOPED_SIGNATURE);
+ transforms.addTransform(Transforms.TRANSFORM_C14N_EXCL_OMIT_COMMENTS);
+ sig.addDocument("#" + id, transforms,
MessageDigestAlgorithm.ALGO_ID_DIGEST_SHA256);
+ sign(sig);
+ return doc;
+ }
+
+ private static Document createEnvelopingSignedDocument() throws Exception {
+ Document doc = DOMUtils.createDocument();
+ Element book = createBook(doc, "CXF");
+
+ String id = "_book";
+ book.setAttributeNS(null, "Id", id);
+ book.setIdAttributeNS(null, "Id", true);
+
+ XMLSignature sig = new XMLSignature(doc, "",
XMLSignature.ALGO_ID_SIGNATURE_RSA_SHA256);
+ doc.appendChild(sig.getElement());
+ Element object = doc.createElementNS(Constants.SignatureSpecNS,
"ds:Object");
+ object.appendChild(book);
+ sig.getElement().appendChild(object);
+ Transforms transforms = new Transforms(doc);
+ transforms.addTransform(Transforms.TRANSFORM_C14N_EXCL_OMIT_COMMENTS);
+ sig.addDocument("#" + id, transforms,
MessageDigestAlgorithm.ALGO_ID_DIGEST_SHA256);
+ sign(sig);
+ return doc;
+ }
+
+ private static Document createDetachedSignedDocument() throws Exception {
+ Document doc = DOMUtils.createDocument();
+ Element root = doc.createElementNS("http://org.apache.cxf/rs/env";,
"env:Envelope");
+ doc.appendChild(root);
+ Element book = createBook(doc, "CXF");
+ root.appendChild(book);
+
+ String id = "_book";
+ book.setAttributeNS(null, "Id", id);
+ book.setIdAttributeNS(null, "Id", true);
+
+ XMLSignature sig = new XMLSignature(doc, "",
XMLSignature.ALGO_ID_SIGNATURE_RSA_SHA256);
+ root.appendChild(sig.getElement());
+ Transforms transforms = new Transforms(doc);
+ transforms.addTransform(Transforms.TRANSFORM_C14N_EXCL_OMIT_COMMENTS);
+ sig.addDocument("#" + id, transforms,
MessageDigestAlgorithm.ALGO_ID_DIGEST_SHA256);
+ sign(sig);
+ return doc;
+ }
+
+ private static Element createBook(Document doc, String bookName) {
+ Element book = doc.createElementNS(null, "Book");
+ Element name = doc.createElementNS(null, "name");
+ name.setTextContent(bookName);
+ book.appendChild(name);
+ return book;
+ }
+
+ private static void sign(XMLSignature sig) throws Exception {
+ CryptoType cryptoType = new CryptoType(CryptoType.TYPE.ALIAS);
+ cryptoType.setAlias("alice");
+ X509Certificate cert = crypto.getX509Certificates(cryptoType)[0];
+ PrivateKey key = crypto.getPrivateKey("alice", "password");
+ sig.addKeyInfo(cert);
+ sig.sign(key);
+ }
+}
