This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/max-hops in repository https://gitbox.apache.org/repos/asf/ws-xmlschema.git
commit 56d73471b788fe072792c49af9eb4f6acc5ed275 Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Thu Sep 24 07:02:53 2026 +0100 Add a bound on the number of redirects the DefaultURIResolver can do --- 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 {
