diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/tags/FunctionTagLoader.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/tags/FunctionTagLoader.java new file mode 100644 index 0000000000..ca117c013b --- /dev/null +++ b/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 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 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(); + } + + protected static List loadTags(final ResourceFile tagDataFile) { + List 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; + } +} diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/tags/SourceTagsPanel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/tags/SourceTagsPanel.java index 13002f9a87..ff4cd5d294 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/tags/SourceTagsPanel.java +++ b/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 loadTags() { - List tags = new ArrayList<>(); - - 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; + return FunctionTagLoader.loadTags(TAG_FILE); } } diff --git a/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/tags/FunctionTagLoaderTest.java b/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/tags/FunctionTagLoaderTest.java new file mode 100644 index 0000000000..0afa12fc2e --- /dev/null +++ b/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 = "\n" + + "\n" + + " COMPRESSION \n" + + " CONSTRUCTOR \n" + + " CRYPTO \n" + + " DESTRUCTOR \n" + + " IO \n" + + " LIBRARY \n" + + " NETWORK \n" + + " UNPACKER \n" + + "\n"; + + private String FUNCTION_TAGS_EMPTY_TAGS = "\n" + + "\n" + ""; + + private String FUNCTION_TAGS_HAS_BLANK_NAME_VALUE = "\n" + + "\n" + + " COMPRESSION \n" + + " CONSTRUCTOR \n" + + " CRYPTO \n" + // a name tag has a blank value + + " " + + "IM A COMMENT \n" + + + " IO \n" + + " LIBRARY \n" + + " NETWORK \n" + + " UNPACKER \n" + + "\n"; + + private String FUNCTION_TAGS_HAS_COMMENT_VALUE = "\n" + + "\n" + + " COMPRESSION \n" + + " CONSTRUCTOR \n" + + " CRYPTO \n" + // a name tag has a comment value as well + + " DESTRUCTOR " + + "IM A COMMENT \n" + + + " IO \n" + + " LIBRARY \n" + + " NETWORK \n" + + " UNPACKER \n" + + "\n"; + + private String FUNCTION_TAGS_HAS_NO_NAME_VALUE = "\n" + + "\n" + + " COMPRESSION \n" + + " CONSTRUCTOR \n" + + " CRYPTO \n" + // Create a comment tag with no name tag. + + " IM A COMMENT WITH NO NAME \n" + + " IO \n" + + " LIBRARY \n" + + " NETWORK \n" + + " UNPACKER \n" + + "\n"; + + private String FUNCTION_TAGS_MALFORMED_XML = "\n" + + "\n" + // end "tag" in start position + + " COMPRESSION \n" + + "\n"; + + private String FUNCTION_TAGS_NO_COMMENT_TAG = "\n" + + "\n" + + " COMPRESSION \n" + + " CONSTRUCTOR \n" + + " CRYPTO \n" + + " DESTRUCTOR \n" + + " IO \n" + + " LIBRARY \n" + + " NETWORK \n" + + " UNPACKER \n" + + "\n"; + + + // @formatter:on + + @Test + public void testLoadTags_EmptyFile() throws Exception { + // Create file without contents + File xxeFile = createTempFileForTest(); + List tags = FunctionTagLoader.loadTags(xxeFile); + + List 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 tags = FunctionTagLoader.loadTags(xxeFile); + + List 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 tags = FunctionTagLoader.loadTags(xxeFile); + + List 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 tags = FunctionTagLoader.loadTags(xxeFile); + + List 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 tags = FunctionTagLoader.loadTags(xxeFile); + + List 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 tags = FunctionTagLoader.loadTags(xxeFile); + + List 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 tags = FunctionTagLoader.loadTags(xxeFile); + + List 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 tags = FunctionTagLoader.loadTags(xxeFile); + + List 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 tags = FunctionTagLoader.loadTags(xxeFile); + + List 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); + } +} diff --git a/Ghidra/Framework/Generic/src/test/java/ghidra/xml/ThreadedXmlParserTest.java b/Ghidra/Framework/Generic/src/test/java/ghidra/xml/ThreadedXmlParserTest.java index ecc28e8ac6..2073d856bb 100644 --- a/Ghidra/Framework/Generic/src/test/java/ghidra/xml/ThreadedXmlParserTest.java +++ b/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 { "" + "" + "" + "" + ""; + + private static final String XXE_XML = "\n" + + "\n" + + "]>&xxe; fizzbizz"; + public ThreadedXmlParserTest() { super(); } - @Before - public void setUp() throws Exception { + /** + *

+ * 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. + *

+ * 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. + *

+ * 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 - public void tearDown() throws Exception { + parser.start("foo"); + XmlElement x1 = parser.next(); + assertFalse("File contents (external entity) should not be in xml.", + x1.getText().contains(fileContents)); } @Test