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, 48 insertions, 3 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 20958a812c..c9a0b0b6e5 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
@@ -11,12 +11,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.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.Set;
 import java.util.stream.Collectors;
@@ -152,4 +156,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 6a2c3898a6..726c7b430e 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java
@@ -24,6 +24,9 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.parsers.SAXParserFactory;
+
 import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.gitrepo.RepoProject.CopyFile;
@@ -37,7 +40,6 @@ import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 import org.xml.sax.XMLReader;
 import org.xml.sax.helpers.DefaultHandler;
-import org.xml.sax.helpers.XMLReaderFactory;
 
 /**
  * Repo XML manifest parser.
@@ -137,8 +139,18 @@ public class ManifestParser extends DefaultHandler {
 		xmlInRead++;
 		final XMLReader xr;
 		try {
-			xr = XMLReaderFactory.createXMLReader();
-		} catch (SAXException e) {
+			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);
 		}
 		xr.setContentHandler(this);