feat: adds safe XML parsing (#928)

* chore: add xxe prevention logic to parser * chore: test unsave vs safe parsing of doctype * chore: use unsafe constructor in test * chore: cleanup of IDE files * chore: fmt:format
GoogleCloudPlatform · May 2, 2023 · 6b85be3 · 6b85be3
1 parent 9c4e38c
commit 6b85be3
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 1 deletion.
diff --git a/appengine-plugins-core/.gitignore b/appengine-plugins-core/.gitignore
@@ -8,6 +8,9 @@ target/
 .idea/encodings.xml
 .idea/libraries/
 .idea/workspace.xml
+.idea/jarRepositories.xml
+.idea/codeStyles/Project.xml
+.idea/codeStyles/codeStyleConfig.xml
 .classpath
 .project
 .settings/*

diff --git a/...gine-plugins-core/src/main/java/com/google/cloud/tools/appengine/AppEngineDescriptor.java b/...gine-plugins-core/src/main/java/com/google/cloud/tools/appengine/AppEngineDescriptor.java
@@ -22,6 +22,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import javax.annotation.Nullable;
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import org.w3c.dom.DOMException;
@@ -35,10 +36,12 @@
 public class AppEngineDescriptor {
 
   private static final String APP_ENGINE_NAMESPACE = "http://appengine.google.com/ns/1.0";
+  private static final String DISALLOW_DOCTYPE_DECLARATIONS =
+      "http://apache.org/xml/features/disallow-doctype-decl";
   private final Document document;
 
   // private to force use of parse method
-  private AppEngineDescriptor(Document document) {
+  protected AppEngineDescriptor(Document document) {
     this.document = document;
   }
 
@@ -55,6 +58,8 @@ public static AppEngineDescriptor parse(InputStream in) throws IOException, SAXE
     try {
       DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
       documentBuilderFactory.setNamespaceAware(true);
+      documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+      documentBuilderFactory.setFeature(DISALLOW_DOCTYPE_DECLARATIONS, true);
       return new AppEngineDescriptor(documentBuilderFactory.newDocumentBuilder().parse(in));
     } catch (ParserConfigurationException exception) {
       throw new SAXException("Cannot parse appengine-web.xml", exception);

diff --git a/...-plugins-core/src/test/java/com/google/cloud/tools/appengine/AppEngineDescriptorTest.java b/...-plugins-core/src/test/java/com/google/cloud/tools/appengine/AppEngineDescriptorTest.java
@@ -18,17 +18,24 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
 import org.junit.Assert;
 import org.junit.Test;
+import org.w3c.dom.Document;
 import org.xml.sax.SAXException;
+import org.xml.sax.SAXParseException;
 
 public class AppEngineDescriptorTest {
 
@@ -53,6 +60,13 @@ public class AppEngineDescriptorTest {
   private static final String RUNTIME = "<runtime>" + RUNTIME_ID + "</runtime>";
   private static final String ENVIRONMENT =
       "<env-variables><env-var name='keya' value='vala' /><env-var name='key2' value='val2' /><env-var name='keyc' value='valc' /></env-variables>";
+  private static final String INJECTED_MALICIOUS_ENTITY = "-bar";
+  private static final String DOCTYPE_TAG_WITH_ENTITIES =
+      "<!DOCTYPE test [<!ELEMENT appengine-web-app ANY> <!ENTITY suffix \""
+          + INJECTED_MALICIOUS_ENTITY
+          + "\" > ]>";
+  private static final String PROJECT_ID_WITH_ENTITY =
+      "<application>" + TEST_ID + "&suffix;" + "</application>";
 
   private static final String XML_WITHOUT_PROJECT_ID = ROOT_START_TAG + ROOT_END_TAG;
   private static final String XML_WITHOUT_VERSION = ROOT_START_TAG + PROJECT_ID + ROOT_END_TAG;
@@ -65,6 +79,9 @@ public class AppEngineDescriptorTest {
   private static final String XML_WITH_VERSION_AND_PROJECT_ID_WRONG_NS =
       ROOT_START_TAG_WITH_INVALID_NS + PROJECT_ID + VERSION + ROOT_END_TAG;
 
+  private static final String XML_WITH_UNSAFE_DOCTYPE =
+      DOCTYPE_TAG_WITH_ENTITIES + ROOT_START_TAG + PROJECT_ID_WITH_ENTITY + ROOT_END_TAG;
+
   @Test
   public void testParse_noProjectId() throws IOException, SAXException, AppEngineException {
     AppEngineDescriptor descriptor = parse(XML_WITHOUT_PROJECT_ID);
@@ -219,8 +236,49 @@ public void testParseAttributeMapValues() throws AppEngineException, IOException
     assertEquals(expectedEnvironment, environment);
   }
 
+  @Test
+  public void testParse_documentWithEntities()
+      throws IOException, SAXException, AppEngineException {
+    AppEngineDescriptor unsafelyParsedXmlDescriptor = parseUnsafe(XML_WITH_UNSAFE_DOCTYPE);
+    assertEquals(TEST_ID + INJECTED_MALICIOUS_ENTITY, unsafelyParsedXmlDescriptor.getProjectId());
+    Exception thrownWhenParsingDoctype = null;
+    AppEngineDescriptor safelyParsedXmlDescriptor = null;
+    try {
+      safelyParsedXmlDescriptor = parse(XML_WITH_UNSAFE_DOCTYPE);
+    } catch (Exception ex) {
+      thrownWhenParsingDoctype = ex;
+    }
+    assertNull(safelyParsedXmlDescriptor);
+    assertNotNull(thrownWhenParsingDoctype);
+    assertTrue(thrownWhenParsingDoctype instanceof SAXParseException);
+    assertTrue(thrownWhenParsingDoctype.getMessage().contains("DOCTYPE is disallowed"));
+  }
+
   private static AppEngineDescriptor parse(String xmlString) throws IOException, SAXException {
     return AppEngineDescriptor.parse(
         new ByteArrayInputStream(xmlString.getBytes(StandardCharsets.UTF_8)));
   }
+
+  private static AppEngineDescriptor parseUnsafe(String xmlString)
+      throws IOException, SAXException {
+    return UnsafeAppEngineDescriptor.parse(
+        new ByteArrayInputStream(xmlString.getBytes(StandardCharsets.UTF_8)));
+  }
+
+  private static class UnsafeAppEngineDescriptor extends AppEngineDescriptor {
+    private UnsafeAppEngineDescriptor(Document document) {
+      super(document);
+    }
+
+    public static AppEngineDescriptor parse(InputStream in) throws IOException, SAXException {
+      Preconditions.checkNotNull(in, "Null input");
+      try {
+        DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
+        documentBuilderFactory.setNamespaceAware(true);
+        return new UnsafeAppEngineDescriptor(documentBuilderFactory.newDocumentBuilder().parse(in));
+      } catch (ParserConfigurationException exception) {
+        throw new SAXException("Cannot parse appengine-web.xml", exception);
+      }
+    }
+  }
 }