From 3b69bd1781e2997831b0243a7ff4ea83d519205f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csahvx655-wq=E2=80=9D?= <“sahvx655@gmail.com”> Date: Mon, 1 Jun 2026 11:32:11 +0530 Subject: [PATCH] Security: Harden XML parsers against XML External Entity (XXE) injection --- .../bundlerepository/impl/StaxParser.java | 2 + .../metadata/StreamMetadataProvider.java | 8 ++++ .../module/DefaultBindingModule.java | 6 +++ .../HandlerDeclarationVisitorTestCase.java | 10 +++++ .../scr/impl/BundleComponentActivator.java | 12 ++++++ .../scr/impl/metadata/ComponentBase.java | 12 ++++++ .../felix/scr/impl/xml/XmlHandlerTest.java | 41 +++++++++++++++++++ .../maven/osgicheck/impl/checks/SCRCheck.java | 9 ++++ .../ServicesMarkdownGeneratorMojo.java | 9 ++++ .../felix/utils/repository/StaxParser.java | 2 + 10 files changed, 111 insertions(+) diff --git a/bundlerepository/src/main/java/org/apache/felix/bundlerepository/impl/StaxParser.java b/bundlerepository/src/main/java/org/apache/felix/bundlerepository/impl/StaxParser.java index 7f4a5f912e..b0156d6c65 100644 --- a/bundlerepository/src/main/java/org/apache/felix/bundlerepository/impl/StaxParser.java +++ b/bundlerepository/src/main/java/org/apache/felix/bundlerepository/impl/StaxParser.java @@ -49,6 +49,8 @@ public static synchronized XMLInputFactory getFactory() setProperty(factory, XMLInputFactory.IS_NAMESPACE_AWARE, false); setProperty(factory, XMLInputFactory.IS_VALIDATING, false); setProperty(factory, XMLInputFactory.IS_COALESCING, false); + setProperty(factory, XMLInputFactory.SUPPORT_DTD, false); + setProperty(factory, XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); StaxParser.factory = factory; } return factory; diff --git a/ipojo/manipulator/manipulator/src/main/java/org/apache/felix/ipojo/manipulator/metadata/StreamMetadataProvider.java b/ipojo/manipulator/manipulator/src/main/java/org/apache/felix/ipojo/manipulator/metadata/StreamMetadataProvider.java index 034257eca8..8ff374f776 100644 --- a/ipojo/manipulator/manipulator/src/main/java/org/apache/felix/ipojo/manipulator/metadata/StreamMetadataProvider.java +++ b/ipojo/manipulator/manipulator/src/main/java/org/apache/felix/ipojo/manipulator/metadata/StreamMetadataProvider.java @@ -72,6 +72,14 @@ public List getMetadatas() throws IOException { XMLMetadataParser handler = new XMLMetadataParser(); XMLReader parser = XMLReaderFactory.createXMLReader(); + try { + parser.setFeature("http://xml.org/sax/features/external-general-entities", false); + parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + parser.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + } catch (Exception e) { + // Ignore + } parser.setContentHandler(handler); parser.setFeature("http://xml.org/sax/features/validation", true); parser.setFeature("http://apache.org/xml/features/validation/schema", true); diff --git a/ipojo/manipulator/manipulator/src/main/java/org/apache/felix/ipojo/manipulator/metadata/annotation/module/DefaultBindingModule.java b/ipojo/manipulator/manipulator/src/main/java/org/apache/felix/ipojo/manipulator/metadata/annotation/module/DefaultBindingModule.java index caa45f4fed..197ff70462 100644 --- a/ipojo/manipulator/manipulator/src/main/java/org/apache/felix/ipojo/manipulator/metadata/annotation/module/DefaultBindingModule.java +++ b/ipojo/manipulator/manipulator/src/main/java/org/apache/felix/ipojo/manipulator/metadata/annotation/module/DefaultBindingModule.java @@ -273,6 +273,12 @@ protected DocumentBuilder getFreshDocumentBuilder(Reporter reporter) { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); factory.setNamespaceAware(true); try { + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setFeature(javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setXIncludeAware(false); m_builder = factory.newDocumentBuilder(); } catch (ParserConfigurationException e) { // TODO GSA is this acceptable to throw a RuntimeException here ? diff --git a/ipojo/manipulator/manipulator/src/test/java/org/apache/felix/ipojo/manipulator/metadata/annotation/visitor/HandlerDeclarationVisitorTestCase.java b/ipojo/manipulator/manipulator/src/test/java/org/apache/felix/ipojo/manipulator/metadata/annotation/visitor/HandlerDeclarationVisitorTestCase.java index 1d5625828b..a35f0c518a 100644 --- a/ipojo/manipulator/manipulator/src/test/java/org/apache/felix/ipojo/manipulator/metadata/annotation/visitor/HandlerDeclarationVisitorTestCase.java +++ b/ipojo/manipulator/manipulator/src/test/java/org/apache/felix/ipojo/manipulator/metadata/annotation/visitor/HandlerDeclarationVisitorTestCase.java @@ -98,6 +98,16 @@ public void testAttributeConversionWithNoNamespace() throws Exception { private DocumentBuilder builder() throws ParserConfigurationException { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); factory.setNamespaceAware(true); + try { + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setFeature(javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setXIncludeAware(false); + } catch (Exception e) { + // Ignore + } return factory.newDocumentBuilder(); } } diff --git a/scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java b/scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java index e942d3bebc..242bab164f 100644 --- a/scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java +++ b/scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java @@ -427,6 +427,18 @@ private void loadDescriptor(final URL descriptorURL) getConfiguration().keepInstances(), m_trueCondition); final SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setNamespaceAware(true); + try + { + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setXIncludeAware(false); + } + catch (Exception e) + { + logger.log(Level.WARN, "Failed to configure SAXParserFactory to prevent XXE", e); + } final SAXParser parser = factory.newSAXParser(); parser.parse( stream, handler ); diff --git a/scr/src/test/java/org/apache/felix/scr/impl/metadata/ComponentBase.java b/scr/src/test/java/org/apache/felix/scr/impl/metadata/ComponentBase.java index d57b4b5adc..c29f494d89 100644 --- a/scr/src/test/java/org/apache/felix/scr/impl/metadata/ComponentBase.java +++ b/scr/src/test/java/org/apache/felix/scr/impl/metadata/ComponentBase.java @@ -52,6 +52,18 @@ private List readMetadata(InputStream in) { final SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setNamespaceAware(true); + try + { + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setXIncludeAware(false); + } + catch (Exception e) + { + // Ignore + } final SAXParser parser = factory.newSAXParser(); XmlHandler handler = new XmlHandler(new MockBundle(), logger, false, false, null); diff --git a/scr/src/test/java/org/apache/felix/scr/impl/xml/XmlHandlerTest.java b/scr/src/test/java/org/apache/felix/scr/impl/xml/XmlHandlerTest.java index e59e5dccc2..f39fca5d39 100755 --- a/scr/src/test/java/org/apache/felix/scr/impl/xml/XmlHandlerTest.java +++ b/scr/src/test/java/org/apache/felix/scr/impl/xml/XmlHandlerTest.java @@ -111,6 +111,38 @@ public void testSatisfyingConditionSpecified() throws Exception trueDependency.getTarget()); } + @Test(expected = Exception.class) + public void testParserRejectsXXE() throws Exception { + String xml = "\n" + + "\n" + + "]>\n" + + "\n" + + " \n" + + " \n" + + ""; + final Bundle bundle = Mockito.mock(Bundle.class); + Mockito.when(bundle.getLocation()).thenReturn("bundle"); + + try (java.io.ByteArrayInputStream stream = new java.io.ByteArrayInputStream(xml.getBytes("UTF-8"))) { + XmlHandler handler = new XmlHandler(bundle, new MockBundleLogger(), false, + false, null); + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + try { + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setXIncludeAware(false); + } catch (Exception e) { + // Ignore + } + final SAXParser parser = factory.newSAXParser(); + parser.parse(stream, handler); + } + } + private List parse(final URL descriptorURL, ServiceReference trueCondition) throws Exception { @@ -125,6 +157,15 @@ private List parse(final URL descriptorURL, false, trueCondition); final SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setNamespaceAware(true); + try { + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setXIncludeAware(false); + } catch (Exception e) { + // Ignore + } final SAXParser parser = factory.newSAXParser(); parser.parse(stream, handler); diff --git a/tools/osgicheck-maven-plugin/src/main/java/org/apache/felix/maven/osgicheck/impl/checks/SCRCheck.java b/tools/osgicheck-maven-plugin/src/main/java/org/apache/felix/maven/osgicheck/impl/checks/SCRCheck.java index 3b29ce55df..3f92c90e42 100644 --- a/tools/osgicheck-maven-plugin/src/main/java/org/apache/felix/maven/osgicheck/impl/checks/SCRCheck.java +++ b/tools/osgicheck-maven-plugin/src/main/java/org/apache/felix/maven/osgicheck/impl/checks/SCRCheck.java @@ -340,6 +340,15 @@ public ComponentLogger component(Bundle m_bundle, String implementationClassName try { final SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setNamespaceAware(true); + try { + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setXIncludeAware(false); + } catch (final Exception e) { + // Ignore or log + } final SAXParser parser = factory.newSAXParser(); parser.parse(file, handler); diff --git a/tools/osgicheck-maven-plugin/src/main/java/org/apache/felix/maven/osgicheck/impl/mddocgen/ServicesMarkdownGeneratorMojo.java b/tools/osgicheck-maven-plugin/src/main/java/org/apache/felix/maven/osgicheck/impl/mddocgen/ServicesMarkdownGeneratorMojo.java index 747708bed4..5cc098d2a8 100644 --- a/tools/osgicheck-maven-plugin/src/main/java/org/apache/felix/maven/osgicheck/impl/mddocgen/ServicesMarkdownGeneratorMojo.java +++ b/tools/osgicheck-maven-plugin/src/main/java/org/apache/felix/maven/osgicheck/impl/mddocgen/ServicesMarkdownGeneratorMojo.java @@ -147,6 +147,15 @@ private List readComponentMetadata(File serviceFile) { XmlHandler xmlHandler = new XmlHandler(new SyntheticBundle(servicesDirectory, serviceFile), mavenScrLogger, false, true, null); final SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setNamespaceAware(true); + try { + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setXIncludeAware(false); + } catch (Exception e) { + // Ignore or log + } final SAXParser parser = factory.newSAXParser(); parser.parse(serviceFile, xmlHandler); diff --git a/utils/src/main/java/org/apache/felix/utils/repository/StaxParser.java b/utils/src/main/java/org/apache/felix/utils/repository/StaxParser.java index 4798799404..6b25d89f8f 100644 --- a/utils/src/main/java/org/apache/felix/utils/repository/StaxParser.java +++ b/utils/src/main/java/org/apache/felix/utils/repository/StaxParser.java @@ -436,6 +436,8 @@ private static synchronized XMLInputFactory getInputFactory() { if (StaxParser.inputFactory == null) { XMLInputFactory factory = XMLInputFactory.newInstance(); factory.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, true); + factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); + factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); StaxParser.inputFactory = factory; } return StaxParser.inputFactory;