diff --git a/apitools/org.eclipse.pde.api.tools.ui/src/org/eclipse/pde/api/tools/ui/internal/actions/ExportSessionAction.java b/apitools/org.eclipse.pde.api.tools.ui/src/org/eclipse/pde/api/tools/ui/internal/actions/ExportSessionAction.java index 4f3736c169..8106ce358b 100644 --- a/apitools/org.eclipse.pde.api.tools.ui/src/org/eclipse/pde/api/tools/ui/internal/actions/ExportSessionAction.java +++ b/apitools/org.eclipse.pde.api.tools.ui/src/org/eclipse/pde/api/tools/ui/internal/actions/ExportSessionAction.java @@ -45,10 +45,12 @@ import org.eclipse.pde.api.tools.ui.internal.ApiUIPlugin; import org.eclipse.pde.api.tools.ui.internal.IApiToolsConstants; import org.eclipse.pde.api.tools.ui.internal.views.APIToolingView; +import org.eclipse.pde.internal.core.util.XmlTransformerFactory; /** * Drop-down action to select the active session. */ +@SuppressWarnings("restriction") public class ExportSessionAction extends Action { private static final String DELTAS_XSLT_TRANSFORM_PATH = "/compare.xsl"; //$NON-NLS-1$ private static final String XML_FILE_EXTENSION = ".xml"; //$NON-NLS-1$ @@ -153,9 +155,8 @@ protected IStatus run(IProgressMonitor monitor) { } writer = new BufferedWriter(new FileWriter(reportFile)); Result result = new StreamResult(writer); - // create an instance of TransformerFactory - TransformerFactory transFact = TransformerFactory.newInstance(); - Transformer trans = transFact.newTransformer(xsltSource); + TransformerFactory f = XmlTransformerFactory.createTransformerFactoryWithErrorOnDOCTYPE(); + Transformer trans = f.newTransformer(xsltSource); trans.transform(xmlSource, result); } catch (TransformerException | IOException e) { ApiUIPlugin.log(e); diff --git a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseReportConverter.java b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseReportConverter.java index 45494501ec..0cae1b47c0 100644 --- a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseReportConverter.java +++ b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseReportConverter.java @@ -66,6 +66,7 @@ import org.eclipse.pde.api.tools.internal.provisional.search.IMetadata; import org.eclipse.pde.api.tools.internal.util.Signatures; import org.eclipse.pde.api.tools.internal.util.Util; +import org.eclipse.pde.internal.core.util.XmlTransformerFactory; import org.osgi.framework.Version; import org.w3c.dom.Element; import org.w3c.dom.NamedNodeMap; @@ -820,7 +821,7 @@ protected void applyXSLT(File xsltFile, File xmlfile, File htmloutput) throws Tr protected void applyXSLT(Source xslt, File xmlfile, File htmlfile) throws TransformerException { Source xml = new StreamSource(xmlfile); Result html = new StreamResult(htmlfile); - TransformerFactory factory = TransformerFactory.newInstance(); + TransformerFactory factory = XmlTransformerFactory.createTransformerFactoryWithErrorOnDOCTYPE(); Transformer former = factory.newTransformer(xslt); former.transform(xml, html); } diff --git a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/util/Util.java b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/util/Util.java index 0f96a48ec4..bce51216d4 100644 --- a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/util/Util.java +++ b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/util/Util.java @@ -141,6 +141,7 @@ import org.eclipse.pde.api.tools.internal.provisional.problems.IApiProblem; import org.eclipse.pde.api.tools.internal.provisional.problems.IApiProblemTypes; import org.eclipse.pde.api.tools.internal.search.SkippedComponent; +import org.eclipse.pde.internal.core.util.XmlTransformerFactory; import org.objectweb.asm.Opcodes; import org.osgi.framework.Version; import org.w3c.dom.Document; @@ -153,6 +154,7 @@ * * @since 1.0.0 */ +@SuppressWarnings("restriction") public final class Util { public static final String DOT_TGZ = ".tgz"; //$NON-NLS-1$ @@ -1900,7 +1902,7 @@ public static InputStream getInputStreamFromString(String string) { public static String serializeDocument(Document document) throws CoreException { try { ByteArrayOutputStream s = new ByteArrayOutputStream(); - TransformerFactory factory = TransformerFactory.newInstance(); + TransformerFactory factory = XmlTransformerFactory.createTransformerFactoryWithErrorOnDOCTYPE(); Transformer transformer = factory.newTransformer(); transformer.setOutputProperty(OutputKeys.METHOD, "xml"); //$NON-NLS-1$ transformer.setOutputProperty(OutputKeys.INDENT, "yes"); //$NON-NLS-1$ diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java index 03e984df31..a855e49847 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java @@ -69,6 +69,7 @@ import org.eclipse.pde.core.target.TargetBundle; import org.eclipse.pde.core.target.TargetFeature; import org.eclipse.pde.internal.core.PDECore; +import org.eclipse.pde.internal.core.util.XmlTransformerFactory; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -733,7 +734,8 @@ public String serialize() { try { document.appendChild(containerElement); StreamResult result = new StreamResult(new StringWriter()); - Transformer transformer = TransformerFactory.newInstance().newTransformer(); + TransformerFactory f = XmlTransformerFactory.createTransformerFactoryWithErrorOnDOCTYPE(); + Transformer transformer = f.newTransformer(); transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes"); //$NON-NLS-1$ transformer.transform(new DOMSource(document), result); return result.getWriter().toString(); diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinitionPersistenceHelper.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinitionPersistenceHelper.java index 3e30a34d01..674506106b 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinitionPersistenceHelper.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinitionPersistenceHelper.java @@ -38,6 +38,7 @@ import org.eclipse.pde.core.target.ITargetPlatformService; import org.eclipse.pde.internal.core.ICoreConstants; import org.eclipse.pde.internal.core.PDECore; +import org.eclipse.pde.internal.core.util.XmlTransformerFactory; import org.w3c.dom.DOMException; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -123,7 +124,7 @@ public static void persistXML(ITargetDefinition definition, OutputStream output) return; } StreamResult outputTarget = new StreamResult(output); - TransformerFactory factory = TransformerFactory.newInstance(); + TransformerFactory factory = XmlTransformerFactory.createTransformerFactoryWithErrorOnDOCTYPE(); Transformer transformer = factory.newTransformer(); transformer.setOutputProperty(OutputKeys.METHOD, "xml"); //$NON-NLS-1$ transformer.setOutputProperty(OutputKeys.INDENT, "yes"); //$NON-NLS-1$ diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPersistence38Helper.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPersistence38Helper.java index a5a4aa07f3..a16c753c8c 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPersistence38Helper.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPersistence38Helper.java @@ -35,6 +35,7 @@ import org.eclipse.pde.core.target.ITargetLocationFactory; import org.eclipse.pde.core.target.NameVersionDescriptor; import org.eclipse.pde.internal.core.PDECore; +import org.eclipse.pde.internal.core.util.XmlTransformerFactory; import org.w3c.dom.Element; import org.w3c.dom.Node; import org.w3c.dom.NodeList; @@ -250,7 +251,8 @@ private static ITargetLocation deserializeBundleContainer(Element location) thro throw new CoreException(Status.error(NLS.bind(Messages.TargetPersistence38Helper_NoTargetLocationExtension, type))); } StreamResult result = new StreamResult(new StringWriter()); - Transformer transformer = TransformerFactory.newInstance().newTransformer(); + TransformerFactory factory = XmlTransformerFactory.createTransformerFactoryWithErrorOnDOCTYPE(); + Transformer transformer = factory.newTransformer(); transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes"); //$NON-NLS-1$ transformer.transform(new DOMSource(location), result); container = locFactory.getTargetLocation(type, result.getWriter().toString()); diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/XmlTransformerFactory.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/XmlTransformerFactory.java new file mode 100644 index 0000000000..f9cfba064a --- /dev/null +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/XmlTransformerFactory.java @@ -0,0 +1,30 @@ +/******************************************************************************* + * Copyright (c) 2023 Joerg Kubitz and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.pde.internal.core.util; + +import javax.xml.XMLConstants; +import javax.xml.transform.TransformerFactory; + +public class XmlTransformerFactory { + private XmlTransformerFactory() { + // static Utility only + } + + public static TransformerFactory createTransformerFactoryWithErrorOnDOCTYPE() { + TransformerFactory factory = TransformerFactory.newInstance(); + // prohibit the use of all protocols by external entities: + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); //$NON-NLS-1$ + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); //$NON-NLS-1$ + return factory; + } + + +} diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java index 75b2508e9c..49e31ecdde 100644 --- a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java @@ -7,6 +7,7 @@ @RunWith(Suite.class) @SuiteClasses({ // DependencyManagerTest.class, // + XmlTransformerTest.class, // WorkspaceModelManagerTest.class, // WorkspaceProductModelManagerTest.class, // }) diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/XmlTransformerTest.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/XmlTransformerTest.java new file mode 100644 index 0000000000..1d5d78cb27 --- /dev/null +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/XmlTransformerTest.java @@ -0,0 +1,159 @@ +/******************************************************************************* + * Copyright (c) 2023 Joerg Kubitz and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.pde.core.tests.internal; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.lang.reflect.InvocationTargetException; +import java.net.ServerSocket; +import java.net.Socket; +import java.net.SocketException; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.function.IntFunction; + +import javax.xml.transform.Result; +import javax.xml.transform.Source; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerException; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.stream.StreamResult; +import javax.xml.transform.stream.StreamSource; + +import org.eclipse.pde.internal.core.util.XmlTransformerFactory; +import org.junit.Test; +public class XmlTransformerTest { + + List tmpFiles = new ArrayList<>(); + + @Test + public void testTransformXmlWithExternalEntity() throws Exception { + TransformerFactory transformerFactory = XmlTransformerFactory.createTransformerFactoryWithErrorOnDOCTYPE(); + try { + testParseXmlWithExternalEntity(transformerFactory, this::createMalciousXml); + assertTrue("TransformerException expected", false); + } catch (TransformerException e) { + String message = e.getMessage(); + assertTrue(message, message.contains("DTD")); + } + } + + @Test + public void testTransformXmlWithoutExternalEntity() throws Exception { + TransformerFactory transformerFactory = XmlTransformerFactory.createTransformerFactoryWithErrorOnDOCTYPE(); + testParseXmlWithExternalEntity(transformerFactory, this::createNormalXml); + } + + + InputStream createMalciousXml(int localPort) { + try { + Path tempSecret = Files.createTempFile("test", ".txt"); + tmpFiles.add(tempSecret); + Files.writeString(tempSecret, "secret"); + Path tempDtd = Files.createTempFile("test", ".dtd"); + tmpFiles.add(tempDtd); + String dtdContent = "\n" // + + "\n" // + + "\">\n" // + + "%var2;\n"; + Files.writeString(tempDtd, dtdContent); + StringBuilder sb = new StringBuilder(); + sb.append("\n"); + URI dtdURi = tempDtd.toUri(); + String dtdUrl = dtdURi.toURL().toString(); + sb.append("\n"); + sb.append("&var3;&var4;"); + String xml = sb.toString(); + return new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + InputStream createNormalXml(int localPort) { + StringBuilder sb = new StringBuilder(); + sb.append("\n"); + sb.append("hello"); + String xml = sb.toString(); + return new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)); + } + + private void cleanup() throws IOException { + for (Path path : tmpFiles) { + Files.delete(path); + } + } + + public void testParseXmlWithExternalEntity(TransformerFactory transformerFactory, + IntFunction xmlSupplier) + throws Exception { + Collection exceptionsInOtherThreads = new ConcurrentLinkedQueue<>(); + Thread httpServerThread = null; + try (ServerSocket serverSocket = new ServerSocket(0)) { + int localPort = serverSocket.getLocalPort(); + httpServerThread = new Thread("httpServerThread") { + @Override + public void run() { + try (Socket socket = serverSocket.accept()) { + try (BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream()))) { + String firstLine = in.readLine(); + try (OutputStream outputStream = socket.getOutputStream()) { + outputStream.write("HTTP/1.1 200 OK\r\n".getBytes(StandardCharsets.UTF_8)); + } + assertTrue(firstLine, firstLine.startsWith("GET")); + assertFalse("Server received secret: " + firstLine, firstLine.contains("secret")); // var3 + assertFalse("Server was contacted", true); + } + } catch (SocketException closed) { + // expected + } catch (Throwable e) { + exceptionsInOtherThreads.add(e); + } + } + }; + httpServerThread.start(); + String formatted; + + try (InputStream xmlStream = xmlSupplier.apply(localPort)) { + Transformer xformer = transformerFactory.newTransformer(); + Source source = new StreamSource(xmlStream); + try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) { + Result result = new StreamResult(outputStream); + xformer.transform(source, result); + formatted = outputStream.toString(StandardCharsets.UTF_8); + } + } + assertTrue(formatted, formatted.contains("")); + assertFalse("Formatter injected secret: " + formatted, formatted.contains("secret")); + } finally { + cleanup(); + } + httpServerThread.join(5000); + assertFalse(httpServerThread.isAlive()); + for (Throwable e : exceptionsInOtherThreads) { + throw new InvocationTargetException(e, e.getMessage()); + } + } +}