diff --git a/api/src/main/java/com/google/apphosting/utils/servlet/DeferredTaskServlet.java b/api/src/main/java/com/google/apphosting/utils/servlet/DeferredTaskServlet.java index fefcbd8ce..affaf7c2d 100644 --- a/api/src/main/java/com/google/apphosting/utils/servlet/DeferredTaskServlet.java +++ b/api/src/main/java/com/google/apphosting/utils/servlet/DeferredTaskServlet.java @@ -20,6 +20,7 @@ import com.google.appengine.api.taskqueue.DeferredTaskContext; import com.google.apphosting.api.ApiProxy; import java.io.IOException; +import java.io.ObjectInputFilter; import java.io.ObjectInputStream; import java.io.ObjectStreamClass; import java.lang.reflect.Modifier; @@ -68,6 +69,38 @@ public class DeferredTaskServlet extends HttpServlet { DeferredTaskContext.class.getName() + ".doNotRetry"; static final String DEFERRED_MARK_RETRY_KEY = DeferredTaskContext.class.getName() + ".markRetry"; + /** + * System property that replaces {@link #DEFAULT_SERIAL_FILTER}, using the pattern syntax of + * {@link ObjectInputFilter.Config#createFilter}. An empty value disables the default filter. A + * JVM-wide {@code jdk.serialFilter} is always applied as well. + */ + static final String SERIAL_FILTER_PROPERTY = "appengine.deferredtask.serialFilter"; + + /** + * Classes rejected when deserializing a task payload: well-known deserialization gadgets that no + * {@link DeferredTask} needs. Everything else is left to the JVM-wide filter, if any. + */ + static final String DEFAULT_SERIAL_FILTER = + String.join( + ";", + "!org.apache.commons.collections.functors.*", + "!org.apache.commons.collections4.functors.*", + "!org.apache.commons.beanutils.BeanComparator", + "!org.codehaus.groovy.runtime.*", + "!org.springframework.beans.factory.*", + "!org.apache.xalan.xsltc.trax.*", + "!com.sun.org.apache.xalan.internal.xsltc.trax.*", + "!com.sun.rowset.*", + "!javax.management.*", + "!java.rmi.*", + "!sun.rmi.*", + "!javax.script.*", + "!org.mozilla.javascript.*", + "!bsh.*", + "!clojure.*", + "!org.python.*", + "!com.mchange.v2.c3p0.*"); + /** Thrown by readRequest when an error occurred during deserialization. */ protected static class DeferredTaskException extends Exception { public DeferredTaskException(Exception e) { @@ -227,6 +260,10 @@ protected Class resolveProxyClass(String[] interfaces) } } }; + ObjectInputFilter filter = serialFilter(); + if (filter != null) { + objectStream.setObjectInputFilter(filter); + } // Replacing DeferredTask to Runnable as we have DeferredTask in the 2 classloaders // (runtime and application), but we cannot cast one with another one. return (Runnable) objectStream.readObject(); @@ -234,4 +271,20 @@ protected Class resolveProxyClass(String[] interfaces) throw new DeferredTaskException(e); } } + + /** + * Returns the filter for task payloads: {@link #SERIAL_FILTER_PROPERTY} (or {@link + * #DEFAULT_SERIAL_FILTER}) merged with the JVM-wide filter, so that a class rejected by either is + * rejected. Returns {@code null} when neither is set. + */ + static ObjectInputFilter serialFilter() { + ObjectInputFilter filter = + ObjectInputFilter.Config.createFilter( + System.getProperty(SERIAL_FILTER_PROPERTY, DEFAULT_SERIAL_FILTER)); + ObjectInputFilter jvmWideFilter = ObjectInputFilter.Config.getSerialFilter(); + if (filter == null) { + return jvmWideFilter; + } + return jvmWideFilter == null ? filter : ObjectInputFilter.merge(filter, jvmWideFilter); + } } diff --git a/api/src/main/java/com/google/apphosting/utils/servlet/jakarta/DeferredTaskServlet.java b/api/src/main/java/com/google/apphosting/utils/servlet/jakarta/DeferredTaskServlet.java index 900d54a06..31a5af052 100644 --- a/api/src/main/java/com/google/apphosting/utils/servlet/jakarta/DeferredTaskServlet.java +++ b/api/src/main/java/com/google/apphosting/utils/servlet/jakarta/DeferredTaskServlet.java @@ -25,6 +25,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; +import java.io.ObjectInputFilter; import java.io.ObjectInputStream; import java.io.ObjectStreamClass; import java.lang.reflect.Modifier; @@ -68,6 +69,38 @@ public class DeferredTaskServlet extends HttpServlet { DeferredTaskContext.class.getName() + ".doNotRetry"; static final String DEFERRED_MARK_RETRY_KEY = DeferredTaskContext.class.getName() + ".markRetry"; + /** + * System property that replaces {@link #DEFAULT_SERIAL_FILTER}, using the pattern syntax of + * {@link ObjectInputFilter.Config#createFilter}. An empty value disables the default filter. A + * JVM-wide {@code jdk.serialFilter} is always applied as well. + */ + static final String SERIAL_FILTER_PROPERTY = "appengine.deferredtask.serialFilter"; + + /** + * Classes rejected when deserializing a task payload: well-known deserialization gadgets that no + * {@link DeferredTask} needs. Everything else is left to the JVM-wide filter, if any. + */ + static final String DEFAULT_SERIAL_FILTER = + String.join( + ";", + "!org.apache.commons.collections.functors.*", + "!org.apache.commons.collections4.functors.*", + "!org.apache.commons.beanutils.BeanComparator", + "!org.codehaus.groovy.runtime.*", + "!org.springframework.beans.factory.*", + "!org.apache.xalan.xsltc.trax.*", + "!com.sun.org.apache.xalan.internal.xsltc.trax.*", + "!com.sun.rowset.*", + "!javax.management.*", + "!java.rmi.*", + "!sun.rmi.*", + "!javax.script.*", + "!org.mozilla.javascript.*", + "!bsh.*", + "!clojure.*", + "!org.python.*", + "!com.mchange.v2.c3p0.*"); + /** Thrown by readRequest when an error occurred during deserialization. */ protected static class DeferredTaskException extends Exception { public DeferredTaskException(Exception e) { @@ -227,6 +260,10 @@ protected Class resolveProxyClass(String[] interfaces) } } }; + ObjectInputFilter filter = serialFilter(); + if (filter != null) { + objectStream.setObjectInputFilter(filter); + } // Replacing DeferredTask to Runnable as we have DeferredTask in the 2 classloaders // (runtime and application), but we cannot cast one with another one. return (Runnable) objectStream.readObject(); @@ -234,4 +271,20 @@ protected Class resolveProxyClass(String[] interfaces) throw new DeferredTaskException(e); } } + + /** + * Returns the filter for task payloads: {@link #SERIAL_FILTER_PROPERTY} (or {@link + * #DEFAULT_SERIAL_FILTER}) merged with the JVM-wide filter, so that a class rejected by either is + * rejected. Returns {@code null} when neither is set. + */ + static ObjectInputFilter serialFilter() { + ObjectInputFilter filter = + ObjectInputFilter.Config.createFilter( + System.getProperty(SERIAL_FILTER_PROPERTY, DEFAULT_SERIAL_FILTER)); + ObjectInputFilter jvmWideFilter = ObjectInputFilter.Config.getSerialFilter(); + if (filter == null) { + return jvmWideFilter; + } + return jvmWideFilter == null ? filter : ObjectInputFilter.merge(filter, jvmWideFilter); + } } diff --git a/api_dev/src/test/java/com/google/apphosting/utils/servlet/DeferredTaskServletTest.java b/api_dev/src/test/java/com/google/apphosting/utils/servlet/DeferredTaskServletTest.java index 15a3bb574..280e775f7 100644 --- a/api_dev/src/test/java/com/google/apphosting/utils/servlet/DeferredTaskServletTest.java +++ b/api_dev/src/test/java/com/google/apphosting/utils/servlet/DeferredTaskServletTest.java @@ -19,6 +19,8 @@ import static com.google.appengine.api.taskqueue.TaskOptions.Builder.withPayload; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; @@ -260,6 +262,48 @@ public void testReadBadContent() throws ServletException, IOException { assertEquals(0, getState().runCount); } + @Test + public void testSerialFilterRejectsBlockedClass() throws Exception { + // A known deserialization-gadget type (present in the JDK) that the default filter blocks. + java.io.ByteArrayOutputStream bos = new java.io.ByteArrayOutputStream(); + try (java.io.ObjectOutputStream oos = new java.io.ObjectOutputStream(bos)) { + oos.writeObject(new javax.management.BadAttributeValueExpException(null)); + } + initPostAndQueueMocks(); + initMocksWithOptions( + withPayload(bos.toByteArray(), DeferredTaskContext.RUNNABLE_TASK_CONTENT_TYPE)); + DeferredTaskServlet.DeferredTaskException thrown = + assertThrows( + DeferredTaskServlet.DeferredTaskException.class, + () -> getState().servlet.readRequest(getState().req, getState().resp)); + assertTrue( + "expected the payload to be rejected by the serialization filter, got: " + thrown.getCause(), + thrown.getCause() instanceof java.io.InvalidClassException); + } + + @Test + public void testSerialFilterEnvelope() { + // The default filter blocks the gadget packages and leaves ordinary classes to the JVM-wide + // filter (allowed here, since none is configured in this test). + java.io.ObjectInputFilter filter = DeferredTaskServlet.serialFilter(); + assertEquals( + java.io.ObjectInputFilter.Status.REJECTED, + filter.checkInput(filterInfo(javax.management.BadAttributeValueExpException.class))); + assertNotEquals( + java.io.ObjectInputFilter.Status.REJECTED, + filter.checkInput(filterInfo(TestDeferredTask.class))); + } + + private static java.io.ObjectInputFilter.FilterInfo filterInfo(Class clazz) { + return new java.io.ObjectInputFilter.FilterInfo() { + @Override public Class serialClass() { return clazz; } + @Override public long arrayLength() { return -1; } + @Override public long depth() { return 1; } + @Override public long references() { return 1; } + @Override public long streamBytes() { return 0; } + }; + } + @Test public void testWrongMethodTask() throws ServletException, IOException { when(getState().req.getMethod()).thenReturn("GET"); diff --git a/appengine_init/src/main/java/com/google/appengine/init/AppEngineWebXmlInitialParse.java b/appengine_init/src/main/java/com/google/appengine/init/AppEngineWebXmlInitialParse.java index 3b511e8f8..e5963870c 100644 --- a/appengine_init/src/main/java/com/google/appengine/init/AppEngineWebXmlInitialParse.java +++ b/appengine_init/src/main/java/com/google/appengine/init/AppEngineWebXmlInitialParse.java @@ -140,7 +140,12 @@ public final class AppEngineWebXmlInitialParse { public void handleRuntimeProperties() { Properties appEngineWebXmlProperties = new Properties(); try (final InputStream stream = new FileInputStream(appEngineWebXmlFile)) { - final XMLEventReader reader = XMLInputFactory.newInstance().createXMLEventReader(stream); + XMLInputFactory factory = XMLInputFactory.newInstance(); + // appengine-web.xml has no DTD; refuse DTDs and external entities so that parsing cannot read + // local files or make network requests. + factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); + factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + final XMLEventReader reader = factory.createXMLEventReader(stream); while (reader.hasNext()) { final XMLEvent event = reader.nextEvent(); if (event.isStartElement() diff --git a/utils/src/main/java/com/google/apphosting/utils/config/XmlUtils.java b/utils/src/main/java/com/google/apphosting/utils/config/XmlUtils.java index c165b6b81..dc817e4bc 100644 --- a/utils/src/main/java/com/google/apphosting/utils/config/XmlUtils.java +++ b/utils/src/main/java/com/google/apphosting/utils/config/XmlUtils.java @@ -33,6 +33,7 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.stream.StreamSource; import javax.xml.validation.SchemaFactory; +import javax.xml.validation.Validator; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.Node; @@ -59,9 +60,25 @@ static Document parseXml(InputStream inputStream) { return parseXml(inputStream, null); } + /** + * Returns a factory that does not resolve external entities, external DTDs or XIncludes, so that + * a configuration file cannot read local files or make network requests while it is parsed. A + * {@code DOCTYPE} is still accepted, because older {@code web.xml} files declare one. + */ + static DocumentBuilderFactory newDocumentBuilderFactory() throws ParserConfigurationException { + DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); + dbFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + dbFactory.setFeature("http://xml.org/sax/features/external-general-entities", false); + dbFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + dbFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + dbFactory.setXIncludeAware(false); + dbFactory.setExpandEntityReferences(false); + return dbFactory; + } + public static Document parseXml(InputStream inputStream, String filename) { try { - DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); + DocumentBuilderFactory dbFactory = newDocumentBuilderFactory(); DocumentBuilder dBuilder = dbFactory.newDocumentBuilder(); Document doc = dBuilder.parse(inputStream); doc.getDocumentElement().normalize(); @@ -126,11 +143,11 @@ static void validateXmlContent(String content, File schema) { try { SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); try { - factory - .newSchema(schema) - .newValidator() - .validate(new StreamSource(new ByteArrayInputStream( - content.getBytes(UTF_8)))); + Validator validator = factory.newSchema(schema).newValidator(); + // The document being validated must not pull in external DTDs or entities. + validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + validator.validate(new StreamSource(new ByteArrayInputStream(content.getBytes(UTF_8)))); } catch (SAXException ex) { throw new AppEngineConfigException( "XML error validating " + content + " against " + schema.getPath(), ex); diff --git a/utils/src/test/java/com/google/apphosting/utils/config/XmlUtilsXxeTest.java b/utils/src/test/java/com/google/apphosting/utils/config/XmlUtilsXxeTest.java new file mode 100644 index 000000000..63dfd9876 --- /dev/null +++ b/utils/src/test/java/com/google/apphosting/utils/config/XmlUtilsXxeTest.java @@ -0,0 +1,55 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed 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 + * + * https://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 com.google.apphosting.utils.config; + +import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; + +import java.io.ByteArrayInputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.w3c.dom.Document; + +/** Verifies that {@link XmlUtils} does not resolve external entities (XXE). */ +@RunWith(JUnit4.class) +public class XmlUtilsXxeTest { + + @Test + public void parseXml_doesNotResolveExternalEntity() throws Exception { + Path secret = Files.createTempFile("xxe-secret", ".txt"); + Files.writeString(secret, "TOP-SECRET-XXE-CANARY", UTF_8); + String xml = + "\n" + + "]>\n" + + "&x;"; + + Document doc; + try { + doc = XmlUtils.parseXml(new ByteArrayInputStream(xml.getBytes(UTF_8)), "queue.xml"); + } catch (AppEngineConfigException e) { + // Refusing the document outright is also an acceptable, safe outcome. + return; + } + String name = doc.getElementsByTagName("name").item(0).getTextContent(); + assertThat(name).doesNotContain("TOP-SECRET-XXE-CANARY"); + } +}