This is an automated email from the ASF dual-hosted git repository.
coheigea pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ws-xmlschema.git
The following commit(s) were added to refs/heads/master by this push:
new dff5741e Fix some potential NPEs (#151)
dff5741e is described below
commit dff5741e175c838eb993d65ada0c5c7161161d2f
Author: Colm O hEigeartaigh <[email protected]>
AuthorDate: Wed Sep 16 15:11:04 2026 +0100
Fix some potential NPEs (#151)
---
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\"/>");
+ }
+}
