Updated function tag parsing to use secure parser. Added xxe unit test. GT-2840

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/tags/FunctionTagLoader.java

@@ -0,0 +1,120 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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
+ * 
+ *      http://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 ghidra.app.plugin.core.function.tags;
+
+import java.io.*;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.xml.sax.*;
+
+import generic.jar.ResourceFile;
+import ghidra.framework.Application;
+import ghidra.program.model.listing.FunctionTag;
+import ghidra.util.Msg;
+import ghidra.xml.*;
+
+/**
+ * Reads function tags from  @see ghidra.framework.Application#getModuleDataFile(java.lang.String)
+ * or a File on the filesytem.
+ */
+public class FunctionTagLoader {
+
+	/**
+	 * Load function tags from filesystem. Useful for unit tests.
+	 * 
+	 * @param tagFile tag file
+	 * @return List list of function tags
+	 */
+	protected static List<FunctionTag> loadTags(File tagFile) {
+		return loadTags(new ResourceFile(tagFile));
+	}
+
+	/**
+	 * Load function tags from @see ghidra.framework.Application#getModuleDataFile(java.lang.String)
+	 * 
+	 * @param moduleDataFilePath data file loaded by Application
+	 * @return List list of function tags
+	 */
+	protected static List<FunctionTag> loadTags(String moduleDataFilePath) {
+		try {
+			return loadTags(Application.getModuleDataFile(moduleDataFilePath));
+		}
+		catch (FileNotFoundException e) {
+			Msg.error(null, "Error loading function tags file from " + moduleDataFilePath, e);
+		}
+		return new ArrayList<FunctionTag>();
+	}
+
+	protected static List<FunctionTag> loadTags(final ResourceFile tagDataFile) {
+		List<FunctionTag> tags = new ArrayList<>();
+
+		try {
+			ErrorHandler errHandler = new ErrorHandler() {
+				@Override
+				public void error(SAXParseException exception) throws SAXException {
+					throw new SAXException("Error: " + exception);
+				}
+
+				@Override
+				public void fatalError(SAXParseException exception) throws SAXException {
+					throw new SAXException("Fatal error: " + exception);
+				}
+
+				@Override
+				public void warning(SAXParseException exception) throws SAXException {
+					throw new SAXException("Warning: " + exception);
+				}
+			};
+
+			XmlPullParser parser;
+			try (InputStream inputStream = tagDataFile.getInputStream()) {
+				parser = new NonThreadedXmlPullParserImpl(inputStream, tagDataFile.getName(),
+					errHandler, false);
+			}
+
+			parser.start("tags");
+			while (parser.hasNext()) {
+				XmlElement el = parser.next();
+				// Parse value of name tag.
+				// Only the end XmlElement contains the inner text.
+				if (el.isEnd() && "name".equals(el.getName())) {
+					String name = el.getText();
+					String comment = "";
+					// If there's a name value, parse value of comment tag.
+					// Add name, comment to list of tags.
+					if (name != null && name.trim().length() != 0) {
+						if (parser.hasNext() && "comment".equals(parser.peek().getName())) {
+							el = parser.next();
+							comment = parser.end().getText();
+						}
+						FunctionTagTemp tag = new FunctionTagTemp(name, comment);
+						tags.add(tag);
+					}
+				}
+			}
+			parser.dispose();
+		}
+		catch (XmlException e) {
+			Msg.error(null, "Error parsing function tags from " + tagDataFile, e);
+		}
+		catch (SAXException | IOException e) {
+			Msg.error(null, "Error loading function tags from " + tagDataFile, e);
+		}
+
+		return tags;
+	}
+}

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/tags/SourceTagsPanel.java

@@ -18,16 +18,8 @@ package ghidra.app.plugin.core.function.tags;
 import java.io.IOException;
 import java.util.*;
 
-import javax.xml.parsers.*;
-
-import org.w3c.dom.Node;
-import org.w3c.dom.NodeList;
-import org.xml.sax.SAXException;
-
-import generic.jar.ResourceFile;
 import ghidra.app.cmd.function.AddFunctionTagCmd;
 import ghidra.app.cmd.function.CreateFunctionTagCmd;
-import ghidra.framework.Application;
 import ghidra.framework.cmd.Command;
 import ghidra.framework.plugintool.PluginTool;
 import ghidra.program.database.function.FunctionManagerDB;
@@ -186,51 +178,6 @@ public class SourceTagsPanel extends TagListPanel {
 	 * @return the loaded tags
 	 */
 	private List<FunctionTag> loadTags() {
-		List<FunctionTag> tags = new ArrayList<>();
+		return FunctionTagLoader.loadTags(TAG_FILE);
-
-		try {
-			ResourceFile tagFile = Application.getModuleDataFile(TAG_FILE);
-			DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
-			DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
-			org.w3c.dom.Document doc = dBuilder.parse(tagFile.getInputStream());
-
-			if (doc == null) {
-				Msg.error(null, "Unable to parse input file: " + tagFile.getAbsolutePath());
-				return tags;
-			}
-
-			NodeList nList = doc.getElementsByTagName("tag");
-			if (nList == null || nList.getLength() == 0) {
-				Msg.error(null, "No tags defined in the input file: " + tagFile.getAbsolutePath());
-				return tags;
-			}
-
-			for (int i = 0; i < nList.getLength(); i++) {
-				String name = "";
-				String comment = "";
-				Node nNode = nList.item(i);
-				if (nNode == null) {
-					return tags; // shoudln't be null, but protect ourselves nonetheless
-				}
-				NodeList childNodes = nNode.getChildNodes();
-				for (int j = 0; j < childNodes.getLength(); j++) {
-					Node item = childNodes.item(j);
-					if (item.getNodeName().equals("name")) {
-						name = item.getTextContent();
-					}
-					if (item.getNodeName().equals("comment")) {
-						comment = item.getTextContent();
-					}
-				}
-
-				FunctionTagTemp tag = new FunctionTagTemp(name, comment);
-				tags.add(tag);
-			}
-		}
-		catch (ParserConfigurationException | SAXException | IOException e) {
-			Msg.error(this, "Error loading function tags from " + TAG_FILE, e);
-		}
-
-		return tags;
 	}
 }

Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/tags/FunctionTagLoaderTest.java

@@ -0,0 +1,273 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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
+ * 
+ *      http://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 ghidra.app.plugin.core.function.tags;
+
+import static org.junit.Assert.assertEquals;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.Test;
+
+import ghidra.program.model.listing.FunctionTag;
+import ghidra.test.AbstractGhidraHeadedIntegrationTest;
+
+public class FunctionTagLoaderTest extends AbstractGhidraHeadedIntegrationTest {
+
+	// @formatter:off
+
+	// this xml is the same as what is loaded in Ghidra by default, 
+	// located in Base/data/functionTags.xml
+	private String FUNCTION_TAGS_DEFAULT = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+									+	"<tags>\n"
+									+	"<tag> <name>COMPRESSION</name> <comment/> </tag>\n"
+									+	"<tag> <name>CONSTRUCTOR</name> <comment/> </tag>\n"
+									+	"<tag> <name>CRYPTO</name> <comment/> </tag>\n"
+									+	"<tag> <name>DESTRUCTOR</name> <comment/> </tag>\n"
+									+	"<tag> <name>IO</name> <comment/> </tag>\n"
+									+	"<tag> <name>LIBRARY</name> <comment/> </tag>\n"
+									+	"<tag> <name>NETWORK</name> <comment/> </tag>\n"
+									+	"<tag> <name>UNPACKER</name> <comment/> </tag>\n"
+									+	"</tags>\n";
+
+	private String FUNCTION_TAGS_EMPTY_TAGS = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" 
+											+ "<tags>\n" + "</tags>";
+
+	private String FUNCTION_TAGS_HAS_BLANK_NAME_VALUE = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+			+	"<tags>\n"
+			+	"<tag> <name>COMPRESSION</name> </tag>\n"
+			+	"<tag> <name>CONSTRUCTOR</name> </tag>\n"
+			+	"<tag> <name>CRYPTO</name> </tag>\n"
+			// a name tag has a blank value
+			+	"<tag> <name>  </name> " 
+			+	"<comment>IM A COMMENT</comment> </tag>\n"
+
+			+	"<tag> <name>IO</name> <comment/> </tag>\n"
+			+	"<tag> <name>LIBRARY</name> <comment/> </tag>\n"
+			+	"<tag> <name>NETWORK</name> <comment/> </tag>\n"
+			+	"<tag> <name>UNPACKER</name> <comment/> </tag>\n"
+			+	"</tags>\n";
+
+	private String FUNCTION_TAGS_HAS_COMMENT_VALUE = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+											+	"<tags>\n"
+											+	"<tag> <name>COMPRESSION</name> </tag>\n"
+											+	"<tag> <name>CONSTRUCTOR</name> </tag>\n"
+											+	"<tag> <name>CRYPTO</name> </tag>\n"
+											// a name tag has a comment value as well
+											+	"<tag> <name>DESTRUCTOR</name> " 
+											+	"<comment>IM A COMMENT</comment> </tag>\n"
+
+											+	"<tag> <name>IO</name> <comment/> </tag>\n"
+											+	"<tag> <name>LIBRARY</name> <comment/> </tag>\n"
+											+	"<tag> <name>NETWORK</name> <comment/> </tag>\n"
+											+	"<tag> <name>UNPACKER</name> <comment/> </tag>\n"
+											+	"</tags>\n";
+	
+	private String FUNCTION_TAGS_HAS_NO_NAME_VALUE = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+			+	"<tags>\n"
+			+	"<tag> <name>COMPRESSION</name> </tag>\n"
+			+	"<tag> <name>CONSTRUCTOR</name> </tag>\n"
+			+	"<tag> <name>CRYPTO</name> </tag>\n"
+			// Create a comment tag with no name tag.
+			+	"<tag> <comment>IM A COMMENT WITH NO NAME</comment> </tag>\n"
+			+	"<tag> <name>IO</name> <comment/> </tag>\n"
+			+	"<tag> <name>LIBRARY</name> <comment/> </tag>\n"
+			+	"<tag> <name>NETWORK</name> <comment/> </tag>\n"
+			+	"<tag> <name>UNPACKER</name> <comment/> </tag>\n"
+			+	"</tags>\n";
+
+	private String FUNCTION_TAGS_MALFORMED_XML = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+											+	"<tags>\n"
+											// end "tag" in start position
+											+	"</tag> <name>COMPRESSION</name> </tag>\n" 
+											+	"</tags>\n";
+
+	private String FUNCTION_TAGS_NO_COMMENT_TAG = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+										+	"<tags>\n"
+										+	"<tag> <name>COMPRESSION</name> </tag>\n"
+										+	"<tag> <name>CONSTRUCTOR</name> </tag>\n"
+										+	"<tag> <name>CRYPTO</name> </tag>\n"
+										+	"<tag> <name>DESTRUCTOR</name> </tag>\n"
+										+	"<tag> <name>IO</name> <comment/> </tag>\n"
+										+	"<tag> <name>LIBRARY</name> </tag>\n"
+										+	"<tag> <name>NETWORK</name> </tag>\n"
+										+	"<tag> <name>UNPACKER</name> </tag>\n"
+										+	"</tags>\n";
+
+
+	// @formatter:on
+
+	@Test
+	public void testLoadTags_EmptyFile() throws Exception {
+		// Create file without contents 
+		File xxeFile = createTempFileForTest();
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		assertEquals(tags, expectedTags);
+	}
+
+	@Test
+	public void testLoadTags_EmptyTags() throws Exception {
+		// Create file with contents
+		File xxeFile = createTempFileForTest();
+		Files.write(xxeFile.toPath(), FUNCTION_TAGS_EMPTY_TAGS.getBytes());
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		assertEquals(tags, expectedTags);
+	}
+
+	@Test
+	public void testLoadTags_FileDoesNotExist() throws Exception {
+		// Create temp file, then delete it.
+		File xxeFile = createTempFileForTest();
+		xxeFile.delete();
+
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		assertEquals(tags, expectedTags);
+	}
+
+	@Test
+	public void testLoadTags_MalformedXml() throws Exception {
+		// Create file with contents
+		File xxeFile = createTempFileForTest();
+		Files.write(xxeFile.toPath(), FUNCTION_TAGS_MALFORMED_XML.getBytes());
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		assertEquals(tags, expectedTags);
+	}
+
+	@Test
+	/**
+	 * Test parsing xml that is the same as what is loaded in Ghidra by default, 
+	 * located in Base/data/functionTags.xml
+	 * @throws IOException
+	 */
+	public void testLoadTags_XmlDefault() throws IOException {
+
+		// Create file with contents
+		File xxeFile = createTempFileForTest();
+		Files.write(xxeFile.toPath(), FUNCTION_TAGS_DEFAULT.getBytes());
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		expectedTags.add(new FunctionTagTemp("COMPRESSION", ""));
+		expectedTags.add(new FunctionTagTemp("CONSTRUCTOR", ""));
+		expectedTags.add(new FunctionTagTemp("CRYPTO", ""));
+		expectedTags.add(new FunctionTagTemp("DESTRUCTOR", ""));
+		expectedTags.add(new FunctionTagTemp("IO", ""));
+		expectedTags.add(new FunctionTagTemp("LIBRARY", ""));
+		expectedTags.add(new FunctionTagTemp("NETWORK", ""));
+		expectedTags.add(new FunctionTagTemp("UNPACKER", ""));
+
+		assertEquals(tags, expectedTags);
+	}
+
+	@Test
+	public void testLoadTags_XmlHasBlankNameValue() throws IOException {
+
+		// Create file with contents
+		File xxeFile = createTempFileForTest();
+		Files.write(xxeFile.toPath(), FUNCTION_TAGS_HAS_BLANK_NAME_VALUE.getBytes());
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		expectedTags.add(new FunctionTagTemp("COMPRESSION", ""));
+		expectedTags.add(new FunctionTagTemp("CONSTRUCTOR", ""));
+		expectedTags.add(new FunctionTagTemp("CRYPTO", ""));
+		expectedTags.add(new FunctionTagTemp("IO", ""));
+		expectedTags.add(new FunctionTagTemp("LIBRARY", ""));
+		expectedTags.add(new FunctionTagTemp("NETWORK", ""));
+		expectedTags.add(new FunctionTagTemp("UNPACKER", ""));
+
+		assertEquals(tags, expectedTags);
+	}
+
+	@Test
+	public void testLoadTags_XmlHasCommentValue() throws IOException {
+
+		// Create file with contents
+		File xxeFile = createTempFileForTest();
+		Files.write(xxeFile.toPath(), FUNCTION_TAGS_HAS_COMMENT_VALUE.getBytes());
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		expectedTags.add(new FunctionTagTemp("COMPRESSION", ""));
+		expectedTags.add(new FunctionTagTemp("CONSTRUCTOR", ""));
+		expectedTags.add(new FunctionTagTemp("CRYPTO", ""));
+		expectedTags.add(new FunctionTagTemp("DESTRUCTOR", "IM A COMMENT"));
+		expectedTags.add(new FunctionTagTemp("IO", ""));
+		expectedTags.add(new FunctionTagTemp("LIBRARY", ""));
+		expectedTags.add(new FunctionTagTemp("NETWORK", ""));
+		expectedTags.add(new FunctionTagTemp("UNPACKER", ""));
+
+		assertEquals(tags, expectedTags);
+	}
+
+	@Test
+	public void testLoadTags_XmlNoCommentTag() throws IOException {
+
+		// Create file with contents
+		File xxeFile = createTempFileForTest();
+		Files.write(xxeFile.toPath(), FUNCTION_TAGS_NO_COMMENT_TAG.getBytes());
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		expectedTags.add(new FunctionTagTemp("COMPRESSION", ""));
+		expectedTags.add(new FunctionTagTemp("CONSTRUCTOR", ""));
+		expectedTags.add(new FunctionTagTemp("CRYPTO", ""));
+		expectedTags.add(new FunctionTagTemp("DESTRUCTOR", ""));
+		expectedTags.add(new FunctionTagTemp("IO", ""));
+		expectedTags.add(new FunctionTagTemp("LIBRARY", ""));
+		expectedTags.add(new FunctionTagTemp("NETWORK", ""));
+		expectedTags.add(new FunctionTagTemp("UNPACKER", ""));
+
+		assertEquals(tags, expectedTags);
+	}
+
+	@Test
+	/**
+	 * Test parsing xml with a comment tag but without a name tag. Skip creation of FunctionTag 
+	 * (don't want one without a name).
+	 *   
+	 * @throws IOException
+	 */
+	public void testLoadTags_XmlNoNameTag() throws IOException {
+
+		// Create file with contents
+		File xxeFile = createTempFileForTest();
+		Files.write(xxeFile.toPath(), FUNCTION_TAGS_HAS_NO_NAME_VALUE.getBytes());
+		List<FunctionTag> tags = FunctionTagLoader.loadTags(xxeFile);
+
+		List<FunctionTag> expectedTags = new ArrayList<>();
+		expectedTags.add(new FunctionTagTemp("COMPRESSION", ""));
+		expectedTags.add(new FunctionTagTemp("CONSTRUCTOR", ""));
+		expectedTags.add(new FunctionTagTemp("CRYPTO", ""));
+		expectedTags.add(new FunctionTagTemp("IO", ""));
+		expectedTags.add(new FunctionTagTemp("LIBRARY", ""));
+		expectedTags.add(new FunctionTagTemp("NETWORK", ""));
+		expectedTags.add(new FunctionTagTemp("UNPACKER", ""));
+
+		assertEquals(tags, expectedTags);
+	}
+}

Ghidra/Framework/Generic/src/test/java/ghidra/xml/ThreadedXmlParserTest.java

@@ -18,6 +18,8 @@ package ghidra.xml;
 import static org.junit.Assert.*;
 
 import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.CyclicBarrier;
@@ -25,7 +27,8 @@ import java.util.concurrent.LinkedBlockingQueue;
 
 import javax.swing.SwingUtilities;
 
-import org.junit.*;
+import org.junit.Assert;
+import org.junit.Test;
 import org.xml.sax.*;
 
 import generic.test.AbstractGenericTest;
@@ -41,18 +44,54 @@ public class ThreadedXmlParserTest extends AbstractGenericTest {
 		"<project name=\"foo\"/>" + "<project name=\"foo\"/>" + "<project name=\"foo\"/>" +
 		"<project name=\"foo\"/>" + "</doc>";
 
+	
+	private static final String XXE_XML = "<?xml version=\"1.0\" encoding=\"ISO-8859-1\"?>\n" +
+		"<!DOCTYPE foo [\n" + "    <!ELEMENT foo ANY >\n" +
+		"<!ENTITY xxe SYSTEM \"file://@TEMP_FILE@\">]><foo>&xxe; fizzbizz</foo>";
+	
 	public ThreadedXmlParserTest() {
 		super();
 	}
 
-	@Before
+	/**
-	public void setUp() throws Exception {
+	 * <p>
+	 * XML External Entities attacks benefit from an XML feature to build documents dynamically at 
+	 * the time of processing.  An XML entity allows inclusion of data dynamically from a given 
+	 * resource.  External entities allow an XML document to include data from an external URI. 
+	 * Unless configured to do otherwise, external entities force the XML parser to access the 
+	 * resource specified by the URI, e.g., a file on the local machine or remote system. 
+	 * This behavior exposes the application to XML External Entity (XXE) attacks.
+	 * <p>
+	 * Normally, a custom Entity Resolver implementing org.xml.sax.EntityResolver should not return null 
+	 * as it will then default to the SAX Entity Resolver that allows and resolves external 
+	 * entities. 
+	 * <p>
+	 * This test ensures external entities are ignored whether or not a default SAX Entity Resolver 
+	 * is used. Using the ThreadedXmlPullParserImpl constructor which takes an InputStream (rather 
+	 * than ResourceFile) will use a default Entity Resolver. The XmlUtilities.createSecureSAXParserFactory 
+	 * factory configurations will disable external entities regardless of which Entity Resolver is used.
+	 * 
+	 * @throws Exception
+	 */
+	@Test
+	public void testXXEXml() throws Exception {
+		// Create file with contents
+		String fileContents = "foobar";
+		File xxeFile = createTempFileForTest();
+		Files.write(xxeFile.toPath(), fileContents.getBytes());
+		// Place file path in XML ENTITY
+		String xxeXml = XXE_XML.replace("@TEMP_FILE@", xxeFile.getPath());
 
-	}
+		// This constructor will use a default EntityResolver
+		ThreadedXmlPullParserImpl parser =
+			new ThreadedXmlPullParserImpl(new ByteArrayInputStream(xxeXml.getBytes()),
+				testName.getMethodName(), new TestErrorHandler(), false, 3);
 
-	@After
+		parser.start("foo");
-	public void tearDown() throws Exception {
+		XmlElement x1 = parser.next();
 
+		assertFalse("File contents (external entity) should not be in xml.",
+			x1.getText().contains(fileContents));
 	}
 
 	@Test