From 70a731f73a79abcc5bcba5c14243599978ae4998 Mon Sep 17 00:00:00 2001 From: Ludovic Champenois Date: Sat, 24 Oct 2026 06:53:28 -0700 Subject: [PATCH] Harden deferred-task deserialization and XML config parsing DeferredTaskServlet (both javax and jakarta variants) deserialized the task payload through a plain ObjectInputStream with no class filter. The endpoint is registered in every app and its only guard is the X-AppEngine-QueueName header, which the infrastructure attaches to every delivered Task Queue task. Anyone who can create a task in the project (cloudtasks.tasks.create / taskqueue) can therefore have an arbitrary serialized object graph of their choosing deserialized in the app, which is remote code execution when an exploitable gadget class is on the app classpath. Add an ObjectInputFilter to readRequest(): - a default blocklist of well-known deserialization-gadget packages (DEFAULT_SERIAL_FILTER), overridable via the appengine.deferredtask.serialFilter system property; - merged with any JVM-wide jdk.serialFilter so a class rejected by either is rejected. Legitimate DeferredTask payloads are unaffected. Also harden the XML config parsers against XXE: XmlUtils (DocumentBuilderFactory and the schema Validator) and AppEngineWebXmlInitialParse (StAX) now disable DTDs / external entities / external DTD and schema access. These parse appengine-web.xml, queue.xml, cron.xml, dos.xml, etc. Tested: new unit tests for the filter (blocked class rejected, normal class and real DeferredTask allowed) and for XXE (external entity not resolved); existing DeferredTaskServletTest (11) and AppEngineWebXmlInitialParseTest (37) still pass. Verified live on App Engine (see PR): on a stock runtime the gadget payload is fully deserialized (ClassCastException); on this patched runtime it is refused (InvalidClassException: filter status: REJECTED) while normal deferred tasks and push tasks keep working. Co-Authored-By: Claude Opus 4.8 --- .../utils/servlet/DeferredTaskServlet.java | 53 ++++++++++++++++++ .../servlet/jakarta/DeferredTaskServlet.java | 53 ++++++++++++++++++ .../servlet/DeferredTaskServletTest.java | 44 +++++++++++++++ .../init/AppEngineWebXmlInitialParse.java | 7 ++- .../apphosting/utils/config/XmlUtils.java | 29 ++++++++-- .../utils/config/XmlUtilsXxeTest.java | 55 +++++++++++++++++++ 6 files changed, 234 insertions(+), 7 deletions(-) create mode 100644 utils/src/test/java/com/google/apphosting/utils/config/XmlUtilsXxeTest.java 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"); + } +}