This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/xop in repository https://gitbox.apache.org/repos/asf/ws-wss4j.git
commit 5515a63622a2a49551add7c98ff996b4759ed2aa Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Tue Sep 22 11:29:22 2026 +0100 Xop hardening fix --- .../apache/wss4j/common/util/AttachmentUtils.java | 10 ++- .../wss4j/common/util/AttachmentUtilsTest.java | 80 ++++++++++++++++++++++ .../org/apache/wss4j/dom/util/WSSecurityUtil.java | 6 +- .../apache/wss4j/dom/util/WSSecurityUtilTest.java | 58 ++++++++++++++++ 4 files changed, 152 insertions(+), 2 deletions(-) diff --git a/ws-security-common/src/main/java/org/apache/wss4j/common/util/AttachmentUtils.java b/ws-security-common/src/main/java/org/apache/wss4j/common/util/AttachmentUtils.java index 50febaca5..0432678fe 100644 --- a/ws-security-common/src/main/java/org/apache/wss4j/common/util/AttachmentUtils.java +++ b/ws-security-common/src/main/java/org/apache/wss4j/common/util/AttachmentUtils.java @@ -645,9 +645,17 @@ public final class AttachmentUtils { } public static String getAttachmentId(String xopUri) throws WSSecurityException { + if (xopUri == null || !xopUri.startsWith("cid:")) { + throw new WSSecurityException( + WSSecurityException.ErrorCode.INVALID_SECURITY, + "empty", new Object[] {"Not an attachment reference: " + xopUri} + ); + } try { return URLDecoder.decode(xopUri.substring("cid:".length()), StandardCharsets.UTF_8.name()); - } catch (UnsupportedEncodingException e) { + } catch (UnsupportedEncodingException | IllegalArgumentException e) { + // URLDecoder raises IllegalArgumentException for a malformed escape - "cid:%" say - + // which is unchecked, and so would otherwise leave WSS4J as one. throw new WSSecurityException( WSSecurityException.ErrorCode.INVALID_SECURITY, "empty", new Object[] {"Attachment ID cannot be decoded: " + xopUri} diff --git a/ws-security-common/src/test/java/org/apache/wss4j/common/util/AttachmentUtilsTest.java b/ws-security-common/src/test/java/org/apache/wss4j/common/util/AttachmentUtilsTest.java new file mode 100644 index 000000000..7d07bfc6d --- /dev/null +++ b/ws-security-common/src/test/java/org/apache/wss4j/common/util/AttachmentUtilsTest.java @@ -0,0 +1,80 @@ +/** + * 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.wss4j.common.util; + +import org.apache.wss4j.common.ext.WSSecurityException; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * The attachment URI reaches getAttachmentId from the wire at most call sites, so every shape it + * can take has to come back as a WSSecurityException rather than an unchecked one. + */ +public class AttachmentUtilsTest { + + @Test + public void testAttachmentIdIsDecoded() throws Exception { + assertEquals("attachment", AttachmentUtils.getAttachmentId("cid:attachment")); + assertEquals("a b", AttachmentUtils.getAttachmentId("cid:a+b")); + assertEquals("a b", AttachmentUtils.getAttachmentId("cid:a%20b")); + assertEquals("", AttachmentUtils.getAttachmentId("cid:")); + } + + @Test + public void testUriShorterThanTheCidPrefixIsRejected() { + // "".substring(4) is a StringIndexOutOfBoundsException, not a security fault + for (String uri : new String[] {"", "c", "ci", "cid"}) { + WSSecurityException ex = + assertThrows(WSSecurityException.class, () -> AttachmentUtils.getAttachmentId(uri), + "Expected a rejection of " + "\"" + uri + "\""); + assertEquals(WSSecurityException.ErrorCode.INVALID_SECURITY, ex.getErrorCode()); + } + } + + @Test + public void testUriThatIsNotAnAttachmentReferenceIsRejected() { + for (String uri : new String[] {"#foo", "http://example.com/a";, "CID:attachment", " cid:a"}) { + WSSecurityException ex = + assertThrows(WSSecurityException.class, () -> AttachmentUtils.getAttachmentId(uri), + "Expected a rejection of " + uri); + assertEquals(WSSecurityException.ErrorCode.INVALID_SECURITY, ex.getErrorCode()); + } + } + + @Test + public void testNullUriIsRejected() { + WSSecurityException ex = + assertThrows(WSSecurityException.class, () -> AttachmentUtils.getAttachmentId(null)); + assertEquals(WSSecurityException.ErrorCode.INVALID_SECURITY, ex.getErrorCode()); + } + + @Test + public void testMalformedEscapeIsRejected() { + // URLDecoder raises IllegalArgumentException for these, which is unchecked + for (String uri : new String[] {"cid:%", "cid:%zz", "cid:a%2"}) { + WSSecurityException ex = + assertThrows(WSSecurityException.class, () -> AttachmentUtils.getAttachmentId(uri), + "Expected a rejection of " + uri); + assertEquals(WSSecurityException.ErrorCode.INVALID_SECURITY, ex.getErrorCode()); + } + } +} diff --git a/ws-security-dom/src/main/java/org/apache/wss4j/dom/util/WSSecurityUtil.java b/ws-security-dom/src/main/java/org/apache/wss4j/dom/util/WSSecurityUtil.java index 1496a740e..264720ea5 100644 --- a/ws-security-dom/src/main/java/org/apache/wss4j/dom/util/WSSecurityUtil.java +++ b/ws-security-dom/src/main/java/org/apache/wss4j/dom/util/WSSecurityUtil.java @@ -630,7 +630,11 @@ public final class WSSecurityUtil { boolean removeAttachments) throws WSSecurityException { for (Element includeElement : includeElements) { String xopURI = includeElement.getAttributeNS(null, "href"); - if (xopURI != null) { + // getAttributeNS answers an absent attribute with "", never null, so the old null + // check admitted an xop:Include carrying no href at all. Test for an attachment + // reference the way the other xop:Include call sites do, and leave anything else + // alone: an Include that names no attachment has no attachment bytes to inline. + if (xopURI.startsWith("cid:")) { // Retrieve the attachment bytes byte[] attachmentBytes = WSSecurityUtil.getBytesFromAttachment(xopURI, attachmentCallbackHandler, removeAttachments); diff --git a/ws-security-dom/src/test/java/org/apache/wss4j/dom/util/WSSecurityUtilTest.java b/ws-security-dom/src/test/java/org/apache/wss4j/dom/util/WSSecurityUtilTest.java index 5005dec8f..80e748693 100644 --- a/ws-security-dom/src/test/java/org/apache/wss4j/dom/util/WSSecurityUtilTest.java +++ b/ws-security-dom/src/test/java/org/apache/wss4j/dom/util/WSSecurityUtilTest.java @@ -19,15 +19,25 @@ package org.apache.wss4j.dom.util; +import java.util.Collections; import java.util.List; +import javax.security.auth.callback.CallbackHandler; +import javax.security.auth.callback.UnsupportedCallbackException; + import org.apache.wss4j.common.ConfigurationConstants; +import org.apache.wss4j.common.ext.WSSecurityException; +import org.apache.wss4j.common.util.SOAPUtil; import org.apache.wss4j.dom.WSConstants; import org.apache.wss4j.dom.engine.WSSConfig; import org.apache.wss4j.dom.handler.HandlerAction; import org.junit.jupiter.api.Test; +import org.w3c.dom.Document; +import org.w3c.dom.Element; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; public class WSSecurityUtilTest { @@ -103,4 +113,52 @@ public class WSSecurityUtilTest { assertEquals(WSConstants.SIGN, decodeActions.get(0).getAction()); assertEquals(WSConstants.DKT_ENCR, decodeActions.get(1).getAction()); } + + /** + * An xop:Include that names no attachment has no attachment bytes to inline, so it is left + * as it is. It used to be read as an attachment reference regardless - getAttributeNS + * answers an absent attribute with "", never null - and "" parsed as an attachment id threw + * StringIndexOutOfBoundsException out of the engine. + */ + @Test + public void testInlineAttachmentsIgnoresIncludeThatNamesNoAttachment() throws Exception { + for (String href : new String[] {null, "", "x", "#foo", "http://example.com/a"}) { + Document doc = SOAPUtil.toSOAPPart(SOAPUtil.SAMPLE_SOAP_MSG); + Element include = doc.createElementNS(WSConstants.XOP_NS, "xop:Include"); + if (href != null) { + include.setAttributeNS(null, "href", href); + } + doc.getDocumentElement().appendChild(include); + + WSSecurityUtil.inlineAttachments( + Collections.singletonList(include), rejectingAttachmentCallbackHandler(), true); + + assertSame(include, doc.getDocumentElement().getLastChild(), + "The Include should have been left alone for href " + href); + } + } + + /** + * An Include that does name an attachment is still resolved, and a failure to resolve it is + * reported as a WSSecurityException. + */ + @Test + public void testInlineAttachmentsResolvesCidHref() throws Exception { + Document doc = SOAPUtil.toSOAPPart(SOAPUtil.SAMPLE_SOAP_MSG); + Element include = doc.createElementNS(WSConstants.XOP_NS, "xop:Include"); + include.setAttributeNS(null, "href", "cid:missing"); + doc.getDocumentElement().appendChild(include); + + assertThrows(WSSecurityException.class, + () -> WSSecurityUtil.inlineAttachments( + Collections.singletonList(include), rejectingAttachmentCallbackHandler(), true)); + } + + private static CallbackHandler rejectingAttachmentCallbackHandler() { + // Non-null, so that getBytesFromAttachment gets as far as parsing the URI + return callbacks -> { + throw new UnsupportedCallbackException(callbacks[0]); + }; + } + } \ No newline at end of file
