This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch 2_4_x-fixes in repository https://gitbox.apache.org/repos/asf/ws-wss4j.git
commit c2bf8b40c841f257c909181896f3f66722b2facf Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Mon Sep 21 08:02:31 2026 +0100 Stricter policy enforcement (#715) --- .../wss4j/policy/stax/enforcer/PolicyEnforcer.java | 48 +++++++- .../stax/enforcer/PolicyEnforcerFactory.java | 82 ++++++++++++- .../policy/stax/test/VulnerabliltyVectorsTest.java | 130 +++++++++++++++++++++ .../test/resources/testdata/wsdl/rpcOperation.wsdl | 64 ++++++++++ 4 files changed, 319 insertions(+), 5 deletions(-) diff --git a/ws-security-policy-stax/src/main/java/org/apache/wss4j/policy/stax/enforcer/PolicyEnforcer.java b/ws-security-policy-stax/src/main/java/org/apache/wss4j/policy/stax/enforcer/PolicyEnforcer.java index c95fde087..5e48f7281 100644 --- a/ws-security-policy-stax/src/main/java/org/apache/wss4j/policy/stax/enforcer/PolicyEnforcer.java +++ b/ws-security-policy-stax/src/main/java/org/apache/wss4j/policy/stax/enforcer/PolicyEnforcer.java @@ -204,8 +204,7 @@ public class PolicyEnforcer implements SecurityEventListener { if (operationName != null) { if (soapOperationName.equals(operationName)) { return operationPolicy; - } else if ((operationName.getNamespaceURI() == null || operationName.getNamespaceURI().length() == 0) - && soapOperationName.getLocalPart().equals(operationName.getLocalPart())) { + } else if (matchesOnLocalPartAlone(operationName, soapOperationName)) { noNamespaceOperation = operationPolicy; } } @@ -221,8 +220,49 @@ public class PolicyEnforcer implements SecurityEventListener { if (policyOperationName.equals(soapOperationName)) { return true; } - return (policyOperationName.getNamespaceURI() == null || policyOperationName.getNamespaceURI().length() == 0) - && soapOperationName.getLocalPart().equals(policyOperationName.getLocalPart()); + return matchesOnLocalPartAlone(policyOperationName, soapOperationName); + } + + /** + * A policy operation name that carries no namespace can only be compared against the local + * part of the Body element - the namespace it should have is simply not known. That + * comparison is a last resort for operations whose QName could not be determined, and it + * accepts a Body element in ANY namespace. + * + * It is only defensible while that local part still identifies one operation. As soon as a + * second operation shares it, matching on the local part alone no longer says which of the + * two the SOAP stack will dispatch to, and an attacker can pick whichever of the two + * policies is the weaker by varying nothing but the namespace of the Body element - or, on + * the SOAPAction path, by naming the weaker operation in the SOAPAction header while the + * Body dispatches to the stronger one. Refuse the loose match in that case, so that the + * caller fails closed instead of enforcing a policy that belongs to another operation. + */ + private boolean matchesOnLocalPartAlone(QName policyOperationName, QName soapOperationName) { + String policyNamespace = policyOperationName.getNamespaceURI(); + if (policyNamespace != null && policyNamespace.length() > 0) { + return false; + } + String localPart = policyOperationName.getLocalPart(); + if (!soapOperationName.getLocalPart().equals(localPart)) { + return false; + } + if (countOperationsNamed(localPart) > 1) { + LOG.warn("More than one operation is named {}; refusing to match the policy for it " + + "against {} on the local name alone", localPart, soapOperationName); + return false; + } + return true; + } + + private int countOperationsNamed(String localPart) { + int count = 0; + for (OperationPolicy operationPolicy : operationPolicies) { + QName operationName = operationPolicy.getOperationName(); + if (operationName != null && localPart.equals(operationName.getLocalPart())) { + count++; + } + } + return count; } /** diff --git a/ws-security-policy-stax/src/main/java/org/apache/wss4j/policy/stax/enforcer/PolicyEnforcerFactory.java b/ws-security-policy-stax/src/main/java/org/apache/wss4j/policy/stax/enforcer/PolicyEnforcerFactory.java index d25bbfde3..7f32cfcae 100644 --- a/ws-security-policy-stax/src/main/java/org/apache/wss4j/policy/stax/enforcer/PolicyEnforcerFactory.java +++ b/ws-security-policy-stax/src/main/java/org/apache/wss4j/policy/stax/enforcer/PolicyEnforcerFactory.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.Map; import javax.wsdl.Binding; +import javax.wsdl.BindingInput; import javax.wsdl.BindingOperation; import javax.wsdl.Definition; import javax.wsdl.Message; @@ -38,7 +39,11 @@ import javax.wsdl.WSDLElement; import javax.wsdl.WSDLException; import javax.wsdl.extensions.ExtensibilityElement; import javax.wsdl.extensions.UnknownExtensibilityElement; +import javax.wsdl.extensions.soap.SOAPBinding; +import javax.wsdl.extensions.soap.SOAPBody; import javax.wsdl.extensions.soap.SOAPOperation; +import javax.wsdl.extensions.soap12.SOAP12Binding; +import javax.wsdl.extensions.soap12.SOAP12Body; import javax.wsdl.extensions.soap12.SOAP12Operation; import javax.wsdl.factory.WSDLFactory; import javax.wsdl.xml.WSDLReader; @@ -258,7 +263,15 @@ public class PolicyEnforcerFactory { } } if (operationName == null) { - operationName = new QName(null, operation.getName()); + //For rpc style the Body child element is the operation name, qualified + //with the namespace given on the soap:body of the binding input, so the + //QName that will be reported in the OperationSecurityEvent is knowable + //here too. Registering it keeps the operation out of the local-name-only + //fallback in PolicyEnforcer, which matches a Body element in ANY + //namespace. Where the namespace is not given we are no worse off than + //before and still register the bare operation name. + operationName = new QName(rpcStyleBodyNamespace(binding, bindingOperation), + operation.getName()); } OperationPolicy operationPolicy = new OperationPolicy(operationName); operationPolicyList.add(operationPolicy); @@ -288,6 +301,73 @@ public class PolicyEnforcerFactory { return operationPolicyList; } + /** + * Returns the namespace that the SOAP Body child element carries for an rpc style + * operation - the namespace attribute of its soap:body - or null when the operation is + * not rpc style or when no namespace is given. + */ + private String rpcStyleBodyNamespace(Binding binding, BindingOperation bindingOperation) { + if (!isRpcStyle(binding, bindingOperation)) { + return null; + } + BindingInput bindingInput = bindingOperation.getBindingInput(); + if (bindingInput == null) { + return null; + } + List<?> extensibilityElements = bindingInput.getExtensibilityElements(); + if (extensibilityElements == null) { + return null; + } + for (int i = 0; i < extensibilityElements.size(); i++) { + Object extensibilityElement = extensibilityElements.get(i); + String namespace = null; + if (extensibilityElement instanceof SOAPBody) { + namespace = ((SOAPBody) extensibilityElement).getNamespaceURI(); + } else if (extensibilityElement instanceof SOAP12Body) { + namespace = ((SOAP12Body) extensibilityElement).getNamespaceURI(); + } + if (namespace != null && namespace.length() > 0) { + return namespace; + } + } + return null; + } + + /** + * The style is given either on the binding operation itself or, failing that, on the + * binding. The default in WSDL 1.1 is "document". + */ + private boolean isRpcStyle(Binding binding, BindingOperation bindingOperation) { + String style = findStyle(bindingOperation.getExtensibilityElements()); + if (style == null) { + style = findStyle(binding.getExtensibilityElements()); + } + return "rpc".equals(style); + } + + private String findStyle(List<?> extensibilityElements) { + if (extensibilityElements == null) { + return null; + } + for (int i = 0; i < extensibilityElements.size(); i++) { + Object extensibilityElement = extensibilityElements.get(i); + String style = null; + if (extensibilityElement instanceof SOAPOperation) { + style = ((SOAPOperation) extensibilityElement).getStyle(); + } else if (extensibilityElement instanceof SOAP12Operation) { + style = ((SOAP12Operation) extensibilityElement).getStyle(); + } else if (extensibilityElement instanceof SOAPBinding) { + style = ((SOAPBinding) extensibilityElement).getStyle(); + } else if (extensibilityElement instanceof SOAP12Binding) { + style = ((SOAP12Binding) extensibilityElement).getStyle(); + } + if (style != null && style.length() > 0) { + return style; + } + } + return null; + } + private Policy getPolicy(Service service, Port port, Binding binding, BindingOperation bindingOperation, Operation operation) throws WSSPolicyException { List<Policy> policies = new ArrayList<>(); diff --git a/ws-security-policy-stax/src/test/java/org/apache/wss4j/policy/stax/test/VulnerabliltyVectorsTest.java b/ws-security-policy-stax/src/test/java/org/apache/wss4j/policy/stax/test/VulnerabliltyVectorsTest.java index 8f83f24d6..834df9397 100644 --- a/ws-security-policy-stax/src/test/java/org/apache/wss4j/policy/stax/test/VulnerabliltyVectorsTest.java +++ b/ws-security-policy-stax/src/test/java/org/apache/wss4j/policy/stax/test/VulnerabliltyVectorsTest.java @@ -22,6 +22,7 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.InputStream; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Properties; @@ -32,8 +33,10 @@ import javax.xml.transform.stream.StreamResult; import javax.xml.xpath.XPathConstants; import javax.xml.xpath.XPathExpression; +import org.apache.neethi.Policy; import org.apache.wss4j.common.ext.WSSecurityException; import org.apache.wss4j.dom.handler.WSHandlerConstants; +import org.apache.wss4j.policy.stax.OperationPolicy; import org.apache.wss4j.policy.stax.enforcer.PolicyEnforcer; import org.apache.wss4j.policy.stax.enforcer.PolicyEnforcerFactory; import org.apache.wss4j.policy.stax.enforcer.PolicyInputProcessor; @@ -124,6 +127,133 @@ public class VulnerabliltyVectorsTest extends AbstractTestBase { assertEquals(WSSecurityException.INVALID_SECURITY, ex.getFaultCode()); } + /** + * An operation whose policy is registered without a namespace is matched on its local name + * alone, which accepts a Body element in any namespace. That is only tenable while the + * local name identifies one operation: once a second operation shares it, the local name no + * longer says which of the two the SOAP stack will dispatch to, and the policy of the one + * could be enforced for a message that invokes the other. Fail closed instead. + */ + @Test + public void testAmbiguousNoNamespaceOperationNameDoesNotMatchAnyNamespace() throws Exception { + List<OperationPolicy> operationPolicies = + Arrays.asList(operationPolicy(new QName(null, "getBalance"), null), + operationPolicy(new QName("http://www.example.net/secure";, "getBalance"), null)); + + PolicyEnforcer policyEnforcer = + new PolicyEnforcer(operationPolicies, "", false, null, 0, null, false); + + OperationSecurityEvent operationSecurityEvent = new OperationSecurityEvent(); + operationSecurityEvent.setOperation(new QName("http://www.example.net/other";, "getBalance")); + + WSSecurityException ex = assertThrows(WSSecurityException.class, + () -> policyEnforcer.registerSecurityEvent(operationSecurityEvent)); + + assertEquals(WSSecurityException.INVALID_SECURITY, ex.getFaultCode()); + } + + /** + * The same ambiguity on the SOAPAction path: the SOAPAction header selects the policy of the + * operation registered without a namespace, while the Body element dispatches to the other + * operation of that name. The cross-check added for SOAPAction spoofing must reject this. + */ + @Test + public void testAmbiguousNoNamespaceOperationNameDoesNotSatisfyTheSOAPActionCheck() throws Exception { + List<OperationPolicy> operationPolicies = + Arrays.asList(operationPolicy(new QName(null, "getBalance"), "urn:getBalanceUnsecured"), + operationPolicy(new QName("http://www.example.net/secure";, "getBalance"), "urn:getBalance")); + + PolicyEnforcer policyEnforcer = + new PolicyEnforcer(operationPolicies, "urn:getBalanceUnsecured", false, null, 0, null, false); + + OperationSecurityEvent operationSecurityEvent = new OperationSecurityEvent(); + operationSecurityEvent.setOperation(new QName("http://www.example.net/secure";, "getBalance")); + + WSSecurityException ex = assertThrows(WSSecurityException.class, + () -> policyEnforcer.registerSecurityEvent(operationSecurityEvent)); + + assertEquals("SOAPAction (urn:getBalanceUnsecured) does not match with the current Operation: " + + "{http://www.example.net/secure}getBalance";, + ex.getCause().getMessage()); + assertEquals(WSSecurityException.INVALID_SECURITY, ex.getFaultCode()); + } + + /** + * An operation name that carries a namespace is matched exactly, so the policy of the + * namespace-less operation of the same name is not selected for it. + */ + @Test + public void testNamespacedOperationNameSelectsItsOwnPolicy() throws Exception { + List<OperationPolicy> operationPolicies = + Arrays.asList(operationPolicy(new QName(null, "getBalance"), null), + operationPolicy(new QName("http://www.example.net/secure";, "getBalance"), null)); + + PolicyEnforcer policyEnforcer = + new PolicyEnforcer(operationPolicies, "", false, null, 0, null, false); + + OperationSecurityEvent operationSecurityEvent = new OperationSecurityEvent(); + operationSecurityEvent.setOperation(new QName("http://www.example.net/secure";, "getBalance")); + + policyEnforcer.registerSecurityEvent(operationSecurityEvent); + } + + /** + * Where the local name is unambiguous the fallback is kept, so an integrator that registers + * its operations without a namespace keeps working. + */ + @Test + public void testUnambiguousNoNamespaceOperationNameStillMatches() throws Exception { + List<OperationPolicy> operationPolicies = + Arrays.asList(operationPolicy(new QName(null, "getBalance"), null)); + + PolicyEnforcer policyEnforcer = + new PolicyEnforcer(operationPolicies, "", false, null, 0, null, false); + + OperationSecurityEvent operationSecurityEvent = new OperationSecurityEvent(); + operationSecurityEvent.setOperation(new QName("http://www.example.net/other";, "getBalance")); + + policyEnforcer.registerSecurityEvent(operationSecurityEvent); + } + + /** + * An rpc style operation is registered under the QName that will actually appear as the Body + * child element - the operation name qualified with the namespace from soap:body - and so is + * matched exactly rather than on its local name alone. + */ + @Test + public void testRpcOperationIsBoundToItsSOAPBodyNamespace() throws Exception { + PolicyEnforcerFactory policyEnforcerFactory = PolicyEnforcerFactory.newInstance( + this.getClass().getClassLoader().getResource("testdata/wsdl/rpcOperation.wsdl")); + PolicyEnforcer policyEnforcer = policyEnforcerFactory.newPolicyEnforcer("", false, null, 0, false); + + OperationSecurityEvent operationSecurityEvent = new OperationSecurityEvent(); + operationSecurityEvent.setOperation(new QName("http://www.example.net/rpc";, "getBalance")); + + policyEnforcer.registerSecurityEvent(operationSecurityEvent); + } + + @Test + public void testRpcOperationInAnotherNamespaceIsRejected() throws Exception { + PolicyEnforcerFactory policyEnforcerFactory = PolicyEnforcerFactory.newInstance( + this.getClass().getClassLoader().getResource("testdata/wsdl/rpcOperation.wsdl")); + PolicyEnforcer policyEnforcer = policyEnforcerFactory.newPolicyEnforcer("", false, null, 0, false); + + OperationSecurityEvent operationSecurityEvent = new OperationSecurityEvent(); + operationSecurityEvent.setOperation(new QName("http://example.com/evil";, "getBalance")); + + WSSecurityException ex = assertThrows(WSSecurityException.class, + () -> policyEnforcer.registerSecurityEvent(operationSecurityEvent)); + + assertEquals(WSSecurityException.INVALID_SECURITY, ex.getFaultCode()); + } + + private static OperationPolicy operationPolicy(QName operationName, String soapAction) { + OperationPolicy operationPolicy = new OperationPolicy(operationName); + operationPolicy.setPolicy(new Policy().normalize(true)); + operationPolicy.setOperationAction(soapAction); + return operationPolicy; + } + @Test public void testSignedBodyRelocationToHeader() throws Exception { InputStream sourceDocument = this.getClass().getClassLoader().getResourceAsStream("testdata/plain-soap-1.1.xml"); diff --git a/ws-security-policy-stax/src/test/resources/testdata/wsdl/rpcOperation.wsdl b/ws-security-policy-stax/src/test/resources/testdata/wsdl/rpcOperation.wsdl new file mode 100644 index 000000000..d8f178a52 --- /dev/null +++ b/ws-security-policy-stax/src/test/resources/testdata/wsdl/rpcOperation.wsdl @@ -0,0 +1,64 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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. +--> +<wsdl:definitions + name="RpcService" + targetNamespace="http://www.example.net/RpcService"; + xmlns:tns="http://www.example.net/RpcService"; + xmlns:xs="http://www.w3.org/2001/XMLSchema"; + xmlns:soap="http://schemas.xmlsoap.org/wsdl/soap/"; + xmlns:wsdl="http://schemas.xmlsoap.org/wsdl/"; + > + + <!-- An rpc style operation: the Body child element is the operation name qualified + with the namespace given below on soap:body, not a schema element. --> + + <wsdl:message name="getBalanceRequest"> + <wsdl:part name="account" type="xs:string"/> + </wsdl:message> + <wsdl:message name="getBalanceResponse"> + <wsdl:part name="balance" type="xs:string"/> + </wsdl:message> + + <wsdl:portType name="RpcPort"> + <wsdl:operation name="getBalance"> + <wsdl:input message="tns:getBalanceRequest"/> + <wsdl:output message="tns:getBalanceResponse"/> + </wsdl:operation> + </wsdl:portType> + + <wsdl:binding name="RpcSOAPBinding" type="tns:RpcPort"> + <soap:binding transport="http://schemas.xmlsoap.org/soap/http"; style="rpc"/> + <wsdl:operation name="getBalance"> + <soap:operation soapAction="getBalance" style="rpc"/> + <wsdl:input> + <soap:body use="literal" namespace="http://www.example.net/rpc"/> + </wsdl:input> + <wsdl:output> + <soap:body use="literal" namespace="http://www.example.net/rpc"/> + </wsdl:output> + </wsdl:operation> + </wsdl:binding> + + <wsdl:service name="RpcService"> + <wsdl:port name="Rpc" binding="tns:RpcSOAPBinding"> + <soap:address location="http://localhost:8080/RpcService"/> + </wsdl:port> + </wsdl:service> +</wsdl:definitions>
