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 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 void filter(ContainerRequestContext context) { } Document doc = null; + Element signedBody = null; InputStream is = message.getContent(InputStream.class); if (is != null) { try { @@ -67,7 +69,15 @@ public void filter(ContainerRequestContext context) { } 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 void filter(ContainerRequestContext context) { 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.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 @@ public class AbstractXmlSigInHandler extends AbstractXmlSecInHandler { + private static final Set 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 @@ protected void checkSignature(Message message) { 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 @@ protected String getUserName(Crypto crypto, Message message) { } - 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 @@ protected Element validateReference(Element root, Reference ref) { 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 @@ protected Element validateReference(Element root, Reference ref) { 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 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); + } +}