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 e13dd509 Add a bound on the number of redirects the DefaultURIResolver
can do (#164)
e13dd509 is described below
commit e13dd5094df227a4f8310da67e6f40bbdc30c49d
Author: Colm O hEigeartaigh <[email protected]>
AuthorDate: Thu Sep 24 07:10:20 2026 +0100
Add a bound on the number of redirects the DefaultURIResolver can do (#164)
---
README.txt | 9 ++
THREAT-MODEL.md | 20 +++-
.../schema/resolver/DefaultURIResolver.java | 101 ++++++++++++++++++-
.../src/test/java/tests/RemoteFetchBoundsTest.java | 108 +++++++++++++++++++++
4 files changed, 230 insertions(+), 8 deletions(-)
diff --git a/README.txt b/README.txt
index c5153d9b..e7a2834a 100644
--- a/README.txt
+++ b/README.txt
@@ -112,6 +112,15 @@ stack. The following JVM system property adjusts the limit:
are unaffected either way, so this is not on its own a defence against
an untrusted schema document - see the Security section below.
+ org.apache.ws.commons.schema.remote.maxRedirects
+ How many HTTP redirects one remote schema fetch may follow. The default
+ is 5. Redirects are followed by the resolver rather than by the JDK, so
+ the chain is bounded, every hop goes through the same checks as the
+ location the schema named, and the whole chain counts against the one
+ fetch deadline above. A redirect that changes scheme is refused either
+ way, so an http location cannot become a file read. Set it to 0 to
+ refuse a redirected schema location outright.
+
org.apache.ws.commons.schema.local.allowFileSystem
Whether a schema location may be read from the filesystem at all. The
default is true. Set it to false where schema documents are expected
diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md
index 9bda3e77..d94559c8 100644
--- a/THREAT-MODEL.md
+++ b/THREAT-MODEL.md
@@ -291,6 +291,7 @@ points*:
| `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.remote.allowNetwork` system property | `true`
*(documented: `README.txt`)* | operator opt-out for deployments with no remote
schema sets | when `false`, `DefaultURIResolver` refuses a location whose
effective scheme is `http` or `https`; local `file:` / `jar:` reads are
unaffected, so it closes the remote-fetch half of §9's SSRF disclaimer but not
the local-read half |
+| `org.apache.ws.commons.schema.remote.maxRedirects` system property | `5`
*(documented: `README.txt`)* | operator-tunable bound on one fetch's redirect
chain | `DefaultURIResolver` follows redirects itself rather than leaving them
to the JDK, so the chain is bounded, each hop is re-checked against the scheme
and authority rules, and the chain shares one fetch deadline; `0` refuses a
redirected location. A hop that changes scheme is refused |
| `org.apache.ws.commons.schema.local.allowFileSystem` system property |
`true` *(documented: `README.txt`)* | operator opt-out for deployments whose
schema documents stand alone | when `false`, `DefaultURIResolver` refuses a
`file:` location, a `jar:file:` one, and a relative location with no base URI;
with `remote.allowNetwork=false` it leaves the resolver with nothing to fetch,
which is the nearest the shipped resolver comes to the catalog-only default §14
Q12(b) declined to make the [...]
| `org.apache.ws.commons.schema.remote.connectTimeoutMillis` /
`.readTimeoutMillis` / `.maxFetchMillis` / `.maxBytes` system properties |
`5000` / `10000` / `30000` / `67108864` *(documented: `README.txt`)* |
operator-tunable per-fetch bounds | bound one remote `DefaultURIResolver` fetch
in wall-clock time and bytes; without them the JDK opens a `schemaLocation`
with no timeout and no size limit, and a single import can hold a thread or its
heap indefinitely |
| `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 |
@@ -489,9 +490,10 @@ matching disclaimer.
`https` targets it does allow, it applies **no host or address
filtering of any kind**: any
`http(s)` host is fetched on request, including loopback, link-local
- (`169.254.169.254`) and RFC1918 addresses, and the JDK follows HTTP
- redirects without consulting the resolver again — so a host allowlist
- is not enforceable at the `resolveEntity` boundary. The caller is
+ (`169.254.169.254`) and RFC1918 addresses. Redirects are now followed by
+ the resolver rather than the JDK and each hop is re-checked, so a
+ destination rule *could* be enforced across a chain — but none is
+ applied, by host or by address, so the reach is unchanged. The caller is
responsible for installing a restricting `URIResolver` if the input
schema is attacker-controlled *(documented: `DefaultURIResolver.java`;
ratified — §14 Q12)*. An operator with no remote schema sets can set
@@ -759,6 +761,18 @@ Revise this document when any of the following lands:
rule as first written: it tested only the URI authority, so
`file:////host/share/x.xsd`, which parses with no authority and
carries the host in its path instead, was not caught.
+- **2026-09-17** — `DefaultURIResolver` now follows HTTP redirects itself
+ instead of leaving them to the JDK, bounded by a new
+ `org.apache.ws.commons.schema.remote.maxRedirects` property (default
+ `5`, `0` to refuse a redirected location). A revision trigger under the
+ first bullet above; recorded in §5a. Each hop is re-checked against the
+ scheme and authority rules and a scheme-changing hop is refused, so the
+ location that is fetched is one these checks have passed — which the
+ JDK's own following did not give. The whole chain shares one fetch
+ deadline, so a redirect chain cannot buy time. §9 is corrected: it said a
+ destination rule was unenforceable at this boundary because redirects
+ escaped it, which is no longer the reason — none is applied, but one now
+ could be.
- **2026-09-17** — a companion
`org.apache.ws.commons.schema.local.allowFileSystem` system property
refuses `file:` and `jar:file:` locations, and a relative location with
diff --git
a/xmlschema-core/src/main/java/org/apache/ws/commons/schema/resolver/DefaultURIResolver.java
b/xmlschema-core/src/main/java/org/apache/ws/commons/schema/resolver/DefaultURIResolver.java
index 079235c0..2272cebc 100644
---
a/xmlschema-core/src/main/java/org/apache/ws/commons/schema/resolver/DefaultURIResolver.java
+++
b/xmlschema-core/src/main/java/org/apache/ws/commons/schema/resolver/DefaultURIResolver.java
@@ -21,6 +21,7 @@ package org.apache.ws.commons.schema.resolver;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
+import java.net.HttpURLConnection;
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
@@ -81,6 +82,15 @@ public class DefaultURIResolver implements
CollectionURIResolver {
public static final String MAX_BYTES_PROPERTY =
"org.apache.ws.commons.schema.remote.maxBytes";
+ /**
+ * How many HTTP redirects one fetch may follow. Redirects are followed by
this resolver
+ * rather than by the JDK, so that the chain is bounded, every hop is
checked the way the
+ * location the schema named was checked, and the whole chain counts
against one fetch
+ * deadline. Set it to <code>0</code> to refuse a redirected schema
location outright.
+ */
+ public static final String MAX_REDIRECTS_PROPERTY =
+ "org.apache.ws.commons.schema.remote.maxRedirects";
+
/**
* Whether a schema location may be fetched over the network at all. Set
it to
* <code>false</code> in a deployment whose schema sets are entirely
local: an
@@ -111,6 +121,7 @@ public class DefaultURIResolver implements
CollectionURIResolver {
private static final long DEFAULT_READ_TIMEOUT_MILLIS = 10L * 1000L;
private static final long DEFAULT_MAX_FETCH_MILLIS = 30L * 1000L;
private static final long DEFAULT_MAX_BYTES = 64L * 1024L * 1024L;
+ private static final long DEFAULT_MAX_REDIRECTS = 5L;
private final long connectTimeoutMillis =
getLongProperty(CONNECT_TIMEOUT_PROPERTY,
DEFAULT_CONNECT_TIMEOUT_MILLIS);
@@ -119,6 +130,9 @@ public class DefaultURIResolver implements
CollectionURIResolver {
private final long maxFetchMillis =
getLongProperty(MAX_FETCH_MILLIS_PROPERTY, DEFAULT_MAX_FETCH_MILLIS);
private final long maxBytes = getLongProperty(MAX_BYTES_PROPERTY,
DEFAULT_MAX_BYTES);
+ // Zero is meaningful here -- it refuses a redirect outright -- so the
minimum is 0, not 1.
+ private final long maxRedirects =
+ getLongProperty(MAX_REDIRECTS_PROPERTY, DEFAULT_MAX_REDIRECTS, 0L);
private final boolean allowNetwork =
getBooleanProperty(ALLOW_NETWORK_PROPERTY, true);
private final boolean allowFileSystem =
getBooleanProperty(ALLOW_FILE_SYSTEM_PROPERTY, true);
@@ -247,11 +261,42 @@ public class DefaultURIResolver implements
CollectionURIResolver {
if (closed) {
throw new IOException("The schema location \"" + systemId +
"\" is closed.");
}
- URLConnection connection = url.openConnection();
- connection.setDoInput(true);
- connection.setConnectTimeout(toIntMillis(connectTimeoutMillis));
- connection.setReadTimeout(toIntMillis(readTimeoutMillis));
+ // One deadline for the whole fetch, so a redirect chain cannot
buy more time.
deadlineNanos = System.nanoTime() + maxFetchMillis * 1000000L;
+ URL target = url;
+ long hops = 0;
+ URLConnection connection = null;
+ while (connection == null) {
+ checkDeadline();
+ URLConnection candidate = target.openConnection();
+ candidate.setDoInput(true);
+ candidate.setConnectTimeout(toIntMillis(connectTimeoutMillis));
+ candidate.setReadTimeout(toIntMillis(readTimeoutMillis));
+ String redirectedTo = null;
+ if (candidate instanceof HttpURLConnection) {
+ HttpURLConnection http = (HttpURLConnection)candidate;
+ // Followed here rather than by the JDK: that bounds the
chain, puts every hop
+ // through the same checks as the location the schema
named, and keeps the
+ // whole chain inside one deadline.
+ http.setInstanceFollowRedirects(false);
+ if (isRedirect(http.getResponseCode())) {
+ redirectedTo = http.getHeaderField("Location");
+ http.disconnect();
+ if (hops >= maxRedirects) {
+ throw new IOException("The schema location \"" +
systemId
+ + "\" redirected more than "
+ maxRedirects
+ + " times, the maximum set
by "
+ + MAX_REDIRECTS_PROPERTY +
".");
+ }
+ hops++;
+ }
+ }
+ if (redirectedTo == null) {
+ connection = candidate;
+ } else {
+ target = nextHop(target, redirectedTo);
+ }
+ }
// A declared length is a courtesy: it is absent for a chunked
response and is in any
// case whatever the host chose to claim. The running count below
is the real limit.
if (connection.getContentLengthLong() > maxBytes) {
@@ -262,6 +307,48 @@ public class DefaultURIResolver implements
CollectionURIResolver {
delegate = connection.getInputStream();
}
+ private boolean isRedirect(int code) {
+ return code == HttpURLConnection.HTTP_MOVED_PERM
+ || code == HttpURLConnection.HTTP_MOVED_TEMP
+ || code == HttpURLConnection.HTTP_SEE_OTHER
+ || code == 307
+ || code == 308;
+ }
+
+ /**
+ * The next URL in a redirect chain, or an exception if it is one this
resolver will not
+ * fetch. A redirect that changes scheme is refused, which is what the
JDK does when it
+ * follows redirects itself, so an http location cannot become a file
read or an https one
+ * be downgraded.
+ */
+ private URL nextHop(URL from, String location) throws IOException {
+ if (location == null || location.trim().length() == 0) {
+ throw new IOException("The schema location \"" + systemId
+ + "\" redirected without saying where
to.");
+ }
+ URL next;
+ try {
+ next = new URL(from, location.trim());
+ } catch (MalformedURLException e) {
+ throw new IOException("The schema location \"" + systemId
+ + "\" redirected to \"" + location.trim()
+ + "\", which is not a usable URL.", e);
+ }
+ if (!next.getProtocol().equalsIgnoreCase(from.getProtocol())) {
+ throw new IOException("The schema location \"" + systemId
+ + "\" redirected from the scheme \"" +
from.getProtocol()
+ + "\" to \"" + next.getProtocol() +
"\".");
+ }
+ try {
+ verifyPermittedLocation(next.toString(), systemId);
+ } catch (XmlSchemaException e) {
+ // Surface it as an IOException: this runs inside a read(),
where the parser
+ // expects I/O failures.
+ throw new IOException(e.getMessage(), e);
+ }
+ return next;
+ }
+
private void checkDeadline() throws IOException {
if (System.nanoTime() - deadlineNanos >= 0) {
throw new IOException("Fetching the schema location \"" +
systemId
@@ -337,6 +424,10 @@ public class DefaultURIResolver implements
CollectionURIResolver {
}
private static long getLongProperty(final String name, long defaultValue) {
+ return getLongProperty(name, defaultValue, 1L);
+ }
+
+ private static long getLongProperty(final String name, long defaultValue,
long minimum) {
try {
String value = AccessController.doPrivileged(new
PrivilegedAction<String>() {
public String run() {
@@ -345,7 +436,7 @@ public class DefaultURIResolver implements
CollectionURIResolver {
});
if (value != null && value.trim().length() > 0) {
long parsed = Long.parseLong(value.trim());
- if (parsed > 0) {
+ if (parsed >= minimum) {
return parsed;
}
}
diff --git a/xmlschema-core/src/test/java/tests/RemoteFetchBoundsTest.java
b/xmlschema-core/src/test/java/tests/RemoteFetchBoundsTest.java
index ae38b252..41bc91d8 100644
--- a/xmlschema-core/src/test/java/tests/RemoteFetchBoundsTest.java
+++ b/xmlschema-core/src/test/java/tests/RemoteFetchBoundsTest.java
@@ -54,6 +54,17 @@ public class RemoteFetchBoundsTest extends Assert {
private static final int MODE_TRICKLE = 1;
/** Sends a well-formed but endless body. */
private static final int MODE_FLOOD = 2;
+ /** Redirects to a sibling path on the same server, once per connection. */
+ private static final int MODE_REDIRECT = 3;
+ /** Serves a small valid schema for namespace urn:b. */
+ private static final int MODE_SCHEMA = 4;
+ /** Redirects for ever, so the hop cap is what stops it. */
+ private static final int MODE_REDIRECT_LOOP = 5;
+ /** Redirects to a file: URL, changing scheme. */
+ private static final int MODE_REDIRECT_TO_FILE = 6;
+
+ private static final String SCHEMA_BODY =
+ "<xs:schema xmlns:xs=\"http://www.w3.org/2001/XMLSchema\";
targetNamespace=\"urn:b\"/>";
private void startServer(final int mode) throws IOException {
server = new ServerSocket(0, 50, InetAddress.getByName("127.0.0.1"));
@@ -79,6 +90,13 @@ public class RemoteFetchBoundsTest extends Assert {
return;
}
OutputStream out = socket.getOutputStream();
+ if (mode == MODE_REDIRECT || mode == MODE_SCHEMA || mode ==
MODE_REDIRECT_LOOP
+ || mode == MODE_REDIRECT_TO_FILE) {
+ out.write(oneShotResponse(mode).getBytes(StandardCharsets.UTF_8));
+ out.flush();
+ socket.close();
+ return;
+ }
out.write("HTTP/1.1 200 OK\r\nContent-Type:
text/xml\r\n\r\n".getBytes(StandardCharsets.UTF_8));
out.flush();
byte[] chunk = mode == MODE_TRICKLE
@@ -97,6 +115,41 @@ public class RemoteFetchBoundsTest extends Assert {
}
}
+ private String oneShotResponse(int mode) {
+ if (mode == MODE_SCHEMA) {
+ return "HTTP/1.1 200 OK\r\nContent-Type:
text/xml\r\nContent-Length: "
+ + SCHEMA_BODY.getBytes(StandardCharsets.UTF_8).length
+ + "\r\nConnection: close\r\n\r\n" + SCHEMA_BODY;
+ }
+ String location = mode == MODE_REDIRECT_TO_FILE
+ ? "file:///etc/passwd"
+ : "http://127.0.0.1:"; + server.getLocalPort() + "/next.xsd";
+ return "HTTP/1.1 302 Found\r\nLocation: " + location
+ + "\r\nContent-Length: 0\r\nConnection: close\r\n\r\n";
+ }
+
+ /** First connection answers with {@code first}, every later one with
{@code rest}. */
+ private void startServer(final int first, final int rest) throws
IOException {
+ server = new ServerSocket(0, 50, InetAddress.getByName("127.0.0.1"));
+ running = true;
+ Thread thread = new Thread(new Runnable() {
+ public void run() {
+ boolean isFirst = true;
+ while (running) {
+ try {
+ Socket socket = server.accept();
+ serve(socket, isFirst ? first : rest);
+ isFirst = false;
+ } catch (IOException e) {
+ return;
+ }
+ }
+ }
+ });
+ thread.setDaemon(true);
+ thread.start();
+ }
+
private String importing() {
return "<xs:schema xmlns:xs=\"http://www.w3.org/2001/XMLSchema\";
targetNamespace=\"urn:a\">"
+ "<xs:import namespace=\"urn:b\"
schemaLocation=\"http://127.0.0.1:";
@@ -129,6 +182,14 @@ public class RemoteFetchBoundsTest extends Assert {
}
private void assertRefusedWithin(long millis) throws IOException {
+ assertRefusedWithin(millis, null);
+ }
+
+ /**
+ * @param messageFragment when given, the reason the fetch was refused
must mention it, so a
+ * test for one bound cannot pass because a different bound happened
to fire first.
+ */
+ private void assertRefusedWithin(long millis, String messageFragment)
throws IOException {
long start = System.currentTimeMillis();
try {
new XmlSchemaCollection().read(new StringReader(importing()));
@@ -136,6 +197,10 @@ public class RemoteFetchBoundsTest extends Assert {
} catch (XmlSchemaException expected) {
long elapsed = System.currentTimeMillis() - start;
assertTrue("refused, but only after " + elapsed + "ms", elapsed <
millis);
+ if (messageFragment != null) {
+ assertTrue("refused for the wrong reason: " +
expected.getMessage(),
+ expected.getMessage().contains(messageFragment));
+ }
}
}
@@ -158,6 +223,49 @@ public class RemoteFetchBoundsTest extends Assert {
assertRefusedWithin(30000);
}
+ /**
+ * A schema that has simply moved must still resolve. Bounding a fetch is
not a reason to stop
+ * following a redirect, and schemas do get reorganised behind one.
+ */
+ @Test(timeout = 60000)
+ public void testMovedSchemaIsStillFollowed() throws Exception {
+ startServer(MODE_REDIRECT, MODE_SCHEMA);
+ XmlSchemaCollection collection = new XmlSchemaCollection();
+ collection.read(new StringReader(importing()));
+ assertNotNull("a schema behind a redirect must still resolve",
+ collection.schemaForNamespace("urn:b"));
+ }
+
+ @Test(timeout = 60000)
+ public void testEndlessRedirectChainIsCutOffAtTheHopCap() throws Exception
{
+ System.setProperty(DefaultURIResolver.MAX_REDIRECTS_PROPERTY, "3");
+ try {
+ startServer(MODE_REDIRECT_LOOP, MODE_REDIRECT_LOOP);
+ assertRefusedWithin(30000, "redirected more than 3 times");
+ } finally {
+ System.clearProperty(DefaultURIResolver.MAX_REDIRECTS_PROPERTY);
+ }
+ }
+
+ /** Zero hops is meaningful: it refuses a redirected location outright. */
+ @Test(timeout = 60000)
+ public void testZeroHopsRefusesARedirect() throws Exception {
+ System.setProperty(DefaultURIResolver.MAX_REDIRECTS_PROPERTY, "0");
+ try {
+ startServer(MODE_REDIRECT, MODE_SCHEMA);
+ assertRefusedWithin(30000, "redirected more than 0 times");
+ } finally {
+ System.clearProperty(DefaultURIResolver.MAX_REDIRECTS_PROPERTY);
+ }
+ }
+
+ /** A redirect may not turn a network fetch into a local read. */
+ @Test(timeout = 60000)
+ public void testRedirectChangingSchemeIsRefused() throws Exception {
+ startServer(MODE_REDIRECT_TO_FILE, MODE_SCHEMA);
+ assertRefusedWithin(30000, "redirected from the scheme");
+ }
+
/** Local schemas keep the system-id-only path: no buffering, no behaviour
change. */
@Test
public void testLocalImportStillResolves() throws Exception {
