This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/dangling-npe in repository https://gitbox.apache.org/repos/asf/ws-xmlschema.git
commit a1aba119a9aef2ce7647f66ecc02585f6d19dd72 Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Wed Sep 16 14:58:31 2026 +0100 Fix some potential NPEs --- README.txt | 10 ++ THREAT-MODEL.md | 18 ++-- .../schema/docpath/XmlSchemaPathFinder.java | 5 +- .../ws/commons/schema/walker/XmlSchemaScope.java | 9 ++ .../ws/commons/schema/walker/XmlSchemaWalker.java | 14 ++- .../schema/walker/DanglingReferenceWalkerTest.java | 119 +++++++++++++++++++++ 6 files changed, 165 insertions(+), 10 deletions(-) diff --git a/README.txt b/README.txt index 9c1b8cf5..0cb6324c 100644 --- a/README.txt +++ b/README.txt @@ -68,6 +68,14 @@ adjust the per-document limits: as an entity in one - and FEATURE_SECURE_PROCESSING bounds entity expansion by both count and accumulated size. + The collections returned by the "read-only" accessors on the schema model + are, by default, the live internal collections rather than unmodifiable + views. To wrap them so that modification throws instead, set: + + org.apache.ws.commons.schema.protectReadOnlyCollections + Set to true to return unmodifiable views from the read-only + accessors. The default is false. + For example, set a limit with: -Dorg.apache.ws.commons.schema.walker.maxDecisionPoints=20000 @@ -76,6 +84,8 @@ For example, set a limit with: -Dorg.apache.ws.commons.schema.maxNestingDepth=256 + -Dorg.apache.ws.commons.schema.protectReadOnlyCollections=true + =================== Security =================== diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index 24ac6f76..a5fa6dc4 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -243,10 +243,11 @@ A finding is in-model only if it reaches a row marked **yes**. - Opens **no** listening sockets *(inferred — §14 Q10)*. - Spawns **no** child processes *(inferred — §14 Q10)*. - Installs **no** signal handlers *(inferred — §14 Q10)*. -- Reads only the documented system property - `org.apache.ws.commons.schema.extension_registry` for - security-relevant decisions; does **not** consume `LD_*`-style - envvars *(inferred — §14 Q10)*. +- Reads only documented system properties for security-relevant + decisions — `org.apache.ws.commons.schema.extension_registry`, the + resource limits of §5a, and + `org.apache.ws.commons.schema.protectReadOnlyCollections`; does + **not** consume `LD_*`-style envvars *(inferred — §14 Q10)*. - Writes **nothing** to the filesystem of its own initiative; the `XmlSchemaSerializer` writes to the `OutputStream` the caller hands in *(inferred — §14 Q10)*. @@ -269,6 +270,7 @@ points*: | `org.apache.ws.commons.schema.maxImportDepth` system property | `64` *(documented: `README.txt`)* | operator-tunable per-process limit | maximum import/include resolution depth for one schema read | | `org.apache.ws.commons.schema.maxSchemaResolutions` system property | `1000` *(documented: `README.txt`)* | operator-tunable per-process limit | maximum schema documents resolved during one top-level read | | `org.apache.ws.commons.schema.maxNestingDepth` system property | `512` *(documented: `README.txt`)* | operator-tunable per-process limit | maximum structural nesting depth while building the schema model, including nested include/import/redefine document resolutions | +| `org.apache.ws.commons.schema.protectReadOnlyCollections` system property | `false` *(documented: `README.txt`, `CollectionFactory.java` lines 37-48)* | in-process convenience, not a trust boundary | when false, the "read-only" model accessors return the **live internal collections**, not unmodifiable views; §7 places the in-process caller outside the attacker model, so this is a correctness guard rather than a security control | | `DocumentBuilderFactory` provider | JDK default (typically Xerces fork) *(inferred — §14 Q6)* | depends on the JDK | shape of XML parsing for `read(InputSource)` / stream-shaped `read(Source)` paths | ### The insecure-default case @@ -802,8 +804,11 @@ untrusted-actor-set value is `OUT-OF-MODEL: trusted-input` (proposed). **Q10.** Negative-side inventory in §5: XMLSchema opens **no** sockets *other than what the JDK URL handler does when following an import*; spawns **no** processes; installs **no** signal handlers; -reads **only** the documented system property; writes **nothing** of -its own initiative. Confirm? *(maps to §5)* +reads **only** the documented system properties (the §5a table, which +includes `protectReadOnlyCollections` — note that its `false` default +means the "read-only" accessors hand out the live internal +collections); writes **nothing** of its own initiative. Confirm? +*(maps to §5)* **Q11.** Build-time variants: confirm there are no compile-time feature toggles; the security envelope is shaped only by runtime extension @@ -915,6 +920,7 @@ the JavaDoc / source comments. The project website is | `XmlSchemaCollection.java` | internal parser sets `FEATURE_SECURE_PROCESSING` and disables external DTD/entity resolution unconditionally; DOCTYPE declarations are accepted | §5a, §8 P2 | | `XmlSchemaCollection.java` line 745 | `AccessController.doPrivileged` wrapper for the SAX parse | §5 | | `XmlSchema.java` | serializer `TransformerFactory` sets `FEATURE_SECURE_PROCESSING` and disables external DTD/stylesheet access where supported | §5a, §8 P2 | +| `XmlSchemaSerializer.java` lines 1566-1567 | `DocumentBuilderFactory` sets `FEATURE_SECURE_PROCESSING`; used only via `newDocument()`, so it never parses input and carries no XXE surface | §5a, §8 P2 | | `xmlschema-core/src/main/java/.../resolver/DefaultURIResolver.java` | URL composed from `baseUri` + `schemaLocation`; scheme allowlist plus base-scheme / authority checks, but no host filtering | §3 item 7, §9 SSRF disclaim, §10 item 1, §11 first bullet, §14 Q12 | | `xmlschema-core/src/main/java/.../resolver/URIResolver.java` | Resolver interface — caller-pluggable | §2 caller-roles, §10 item 1 | | `xmlschema-walker/src/main/java/.../docpath/DomBuilderFromSax.java` line 81 | `factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, TRUE)` | §5a, §8 P2 | diff --git a/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/docpath/XmlSchemaPathFinder.java b/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/docpath/XmlSchemaPathFinder.java index 8045c13c..e92184e2 100644 --- a/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/docpath/XmlSchemaPathFinder.java +++ b/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/docpath/XmlSchemaPathFinder.java @@ -28,6 +28,7 @@ import javax.xml.namespace.QName; import org.apache.ws.commons.schema.XmlSchemaAny; import org.apache.ws.commons.schema.XmlSchemaElement; +import org.apache.ws.commons.schema.XmlSchemaException; import org.apache.ws.commons.schema.walker.XmlSchemaTypeInfo; import org.xml.sax.Attributes; import org.xml.sax.SAXException; @@ -449,7 +450,7 @@ public final class XmlSchemaPathFinder<U, V> extends DefaultHandler { private void recordDecisionPoint() { ++decisionPointCount; if (decisionPointCount > MAX_DECISION_POINTS) { - throw new IllegalStateException("More than " + MAX_DECISION_POINTS + throw new XmlSchemaException("More than " + MAX_DECISION_POINTS + " decision points were created while matching this document; the schema" + " likely contains ambiguous (Unique Particle Attribution violating)" + " content models. The limit may be changed with the" @@ -460,7 +461,7 @@ public final class XmlSchemaPathFinder<U, V> extends DefaultHandler { private void recordReplayedEvent() { ++replayedEventCount; if (replayedEventCount > MAX_REPLAYED_EVENTS) { - throw new IllegalStateException("More than " + MAX_REPLAYED_EVENTS + throw new XmlSchemaException("More than " + MAX_REPLAYED_EVENTS + " traversed elements were replayed while backtracking through this" + " document; the schema likely contains ambiguous (Unique Particle" + " Attribution violating) content models. The limit may be changed with the" diff --git a/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/walker/XmlSchemaScope.java b/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/walker/XmlSchemaScope.java index fcd7b674..c05349a5 100644 --- a/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/walker/XmlSchemaScope.java +++ b/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/walker/XmlSchemaScope.java @@ -516,6 +516,10 @@ final class XmlSchemaScope { } final QName groupName = groupRef.getTargetQName(); + if (attrGroup == null) { + throw new XmlSchemaException("The attribute group reference " + groupName + + " does not resolve to an attribute group in this collection."); + } if ((groupName != null) && !attributeGroupsInProgress.add(groupName)) { throw new XmlSchemaException("Cyclic attribute group reference detected involving " + groupName + '.'); @@ -586,6 +590,11 @@ final class XmlSchemaScope { } } + if (globalAttr == null) { + throw new XmlSchemaException("The attribute reference " + attrQName + + " does not resolve to an attribute in this collection."); + } + XmlSchemaSimpleType schemaType = globalAttr.getSchemaType(); if (schemaType == null) { final QName typeQName = globalAttr.getSchemaTypeName(); diff --git a/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/walker/XmlSchemaWalker.java b/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/walker/XmlSchemaWalker.java index 4a906677..b040bbd1 100644 --- a/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/walker/XmlSchemaWalker.java +++ b/xmlschema-walker/src/main/java/org/apache/ws/commons/schema/walker/XmlSchemaWalker.java @@ -328,8 +328,9 @@ public final class XmlSchemaWalker { } } else if (!element.isAbstract()) { - throw new IllegalStateException("Element " + element.getQName() - + " is not abstract and has no type."); + throw new XmlSchemaException("Element " + element.getQName() + + " is not abstract and has no type; its type reference does" + + " not resolve to a type in this collection."); } // 8. Now handle substitute elements, if any. @@ -369,6 +370,10 @@ public final class XmlSchemaWalker { group = g.getParticle(); } } + if (group == null) { + throw new XmlSchemaException("The group reference " + groupName + + " does not resolve to a group in this collection."); + } walk(group, groupRef.getMinOccurs(), groupRef.getMaxOccurs()); } finally { if (groupName != null) { @@ -567,6 +572,11 @@ public final class XmlSchemaWalker { globalElem = schemasByNamespace.getElementByName(elemQName); } + if (globalElem == null) { + throw new XmlSchemaException("The element reference " + elemQName + + " does not resolve to an element in this collection."); + } + /* * An XML Schema element reference defines the id, minOccurs, and * maxOccurs attributes, while the global element definition defines id diff --git a/xmlschema-walker/src/test/java/org/apache/ws/commons/schema/walker/DanglingReferenceWalkerTest.java b/xmlschema-walker/src/test/java/org/apache/ws/commons/schema/walker/DanglingReferenceWalkerTest.java new file mode 100644 index 00000000..dfcf4ea6 --- /dev/null +++ b/xmlschema-walker/src/test/java/org/apache/ws/commons/schema/walker/DanglingReferenceWalkerTest.java @@ -0,0 +1,119 @@ +/** + * 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.ws.commons.schema.walker; + +import java.io.StringReader; + +import javax.xml.namespace.QName; + +import org.apache.ws.commons.schema.XmlSchemaCollection; +import org.apache.ws.commons.schema.XmlSchemaElement; +import org.apache.ws.commons.schema.XmlSchemaException; + +import org.junit.Assert; +import org.junit.Test; + +/** + * xmlschema-core does not check that references resolve, so a schema carrying a + * dangling ref parses cleanly and only fails when the walker dereferences it. Those + * failures must stay inside the documented XmlSchemaException surface rather than + * escaping as NullPointerException or IllegalStateException. + */ +public class DanglingReferenceWalkerTest extends Assert { + + private static void walkRoot(String body) { + XmlSchemaCollection collection = new XmlSchemaCollection(); + collection.read(new StringReader( + "<xs:schema xmlns:xs=\"http://www.w3.org/2001/XMLSchema\"; xmlns:tns=\"urn:dangling\"" + + " targetNamespace=\"urn:dangling\" elementFormDefault=\"qualified\">" + + body + + "</xs:schema>")); + XmlSchemaElement root = + collection.getElementByQName(new QName("urn:dangling", "root")); + assertNotNull("the schema under test must declare a 'root' element", root); + new XmlSchemaWalker(collection).walk(root); + } + + private static void assertRejected(String body, String expectedFragment) { + try { + walkRoot(body); + fail("expected the dangling reference to be rejected"); + } catch (XmlSchemaException expected) { + assertTrue(expected.getMessage(), expected.getMessage().contains(expectedFragment)); + } + } + + @Test + public void testDanglingAttributeGroupReferenceIsRejected() { + assertRejected("<xs:complexType name=\"t\">" + + "<xs:attributeGroup ref=\"tns:missing\"/>" + + "</xs:complexType>" + + "<xs:element name=\"root\" type=\"tns:t\"/>", + "attribute group reference"); + } + + @Test + public void testDanglingGroupReferenceIsRejected() { + assertRejected("<xs:complexType name=\"t\"><xs:sequence>" + + "<xs:group ref=\"tns:missing\"/>" + + "</xs:sequence></xs:complexType>" + + "<xs:element name=\"root\" type=\"tns:t\"/>", + "group reference"); + } + + @Test + public void testDanglingElementReferenceIsRejected() { + assertRejected("<xs:complexType name=\"t\"><xs:sequence>" + + "<xs:element ref=\"tns:missing\"/>" + + "</xs:sequence></xs:complexType>" + + "<xs:element name=\"root\" type=\"tns:t\"/>", + "element reference"); + } + + @Test + public void testDanglingAttributeReferenceIsRejected() { + assertRejected("<xs:complexType name=\"t\">" + + "<xs:attribute ref=\"tns:missing\"/>" + + "</xs:complexType>" + + "<xs:element name=\"root\" type=\"tns:t\"/>", + "attribute reference"); + } + + @Test + public void testDanglingTypeReferenceIsRejected() { + assertRejected("<xs:element name=\"root\" type=\"tns:missing\"/>", + "has no type"); + } + + @Test + public void testResolvableReferencesStillWalk() { + walkRoot("<xs:attributeGroup name=\"ag\">" + + "<xs:attribute name=\"a\" type=\"xs:string\"/>" + + "</xs:attributeGroup>" + + "<xs:group name=\"g\"><xs:sequence>" + + "<xs:element name=\"inner\" type=\"xs:string\"/>" + + "</xs:sequence></xs:group>" + + "<xs:element name=\"global\" type=\"xs:string\"/>" + + "<xs:complexType name=\"t\"><xs:sequence>" + + "<xs:group ref=\"tns:g\"/><xs:element ref=\"tns:global\"/>" + + "</xs:sequence><xs:attributeGroup ref=\"tns:ag\"/></xs:complexType>" + + "<xs:element name=\"root\" type=\"tns:t\"/>"); + } +}
