ManifestParser: Do not accept DOCTYPE and entities

These open the door for XXE attacks [1] and manifest do not need them.

[1] https://en.wikipedia.org/wiki/XML_external_entity_attack

Change-Id: Ia79971e1c34afaf287584ae4a7f71baebcb48b6a

2 files changed, 44 insertions, 1 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java
index fca27d32aa..0949d040e9 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java
@@ -12,12 +12,16 @@ package org.eclipse.jgit.gitrepo;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.io.ByteArrayInputStream;
+import java.io.File;
 import java.io.IOException;
 import java.net.URI;
+import java.nio.file.Files;
+import java.nio.file.StandardOpenOption;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
@@ -221,4 +225,33 @@ public class ManifestParserTest {
 		testNormalize("", "");
 		testNormalize("a/b", "a/b");
 	}
+
+	@Test
+	public void testXXE() throws Exception {
+		File externalEntity = File.createTempFile("injected", "xml");
+		externalEntity.deleteOnExit();
+		Files.write(externalEntity.toPath(),
+				"<evil>injected xml</evil>"
+						.getBytes(UTF_8),
+				StandardOpenOption.WRITE);
+		String baseUrl = "https://git.google.com/";
+		StringBuilder xmlContent = new StringBuilder();
+		xmlContent.append("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n")
+				.append("<!DOCTYPE booo [ <!ENTITY foobar SYSTEM \"")
+				.append(externalEntity.getPath()).append("\"> ]>\n")
+				.append("<manifest>")
+				.append("<remote name=\"remote1\" fetch=\".\" />")
+				.append("<default revision=\"master\" remote=\"remote1\" />")
+				.append("&foobar;")
+				.append("<project path=\"foo\" name=\"foo\" groups=\"a,test\" />")
+				.append("</manifest>");
+
+		IOException e = assertThrows(IOException.class,
+				() -> new ManifestParser(null, null, "master", baseUrl, null,
+						null)
+						.read(new ByteArrayInputStream(
+								xmlContent.toString().getBytes(UTF_8))));
+		assertTrue(e.getCause().getMessage().contains("DOCTYPE"));
+	}
+
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java
index b033177e05..58b4d3dc56 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java
@@ -142,7 +142,17 @@ public class ManifestParser extends DefaultHandler {
 		xmlInRead++;
 		final XMLReader xr;
 		try {
-			xr = SAXParserFactory.newInstance().newSAXParser().getXMLReader();
+			SAXParserFactory spf = SAXParserFactory.newInstance();
+			spf.setFeature(
+					"http://xml.org/sax/features/external-general-entities", //$NON-NLS-1$
+					false);
+			spf.setFeature(
+					"http://xml.org/sax/features/external-parameter-entities", //$NON-NLS-1$
+					false);
+			spf.setFeature(
+					"http://apache.org/xml/features/disallow-doctype-decl", //$NON-NLS-1$
+					true);
+			xr = spf.newSAXParser().getXMLReader();
 		} catch (SAXException | ParserConfigurationException e) {
 			throw new IOException(JGitText.get().noXMLParserAvailable, e);
 		}