[
https://issues.apache.org/jira/browse/KNOX-3484?focusedWorklogId=1043817&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1043817
]
ASF GitHub Bot logged work on KNOX-3484:
----------------------------------------
Author: ASF GitHub Bot
Created on: 24/Sep/26 21:51
Start Date: 24/Sep/26 21:51
Worklog Time Spent: 10m
Work Description: ppkarwasz commented on code in PR #1427:
URL: https://github.com/apache/knox/pull/1427#discussion_r4098848188
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/XmlUtils.java:
##########
@@ -22,41 +22,81 @@
import java.io.Writer;
import java.nio.file.Files;
-import javax.xml.XMLConstants;
+import jakarta.xml.bind.JAXBException;
+import jakarta.xml.bind.Unmarshaller;
import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.stream.XMLStreamException;
import javax.xml.transform.OutputKeys;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerException;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.stream.StreamResult;
+import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
+import org.apache.commons.xml.secure.SecureTransformerFactory;
+import org.apache.commons.xml.secure.SecureXMLInputFactory;
import org.w3c.dom.Document;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;
+/**
+ * XML parsing and serialization helpers.
+ *
+ * <p>Factories come from <a
href="https://commons.apache.org/proper/commons-secure-xml/";>Apache Commons
Secure XML</a>,
+ * which ignores external resources (DTDs, entities, XInclude,
stylesheets).</p>
+ */
public class XmlUtils {
public static Document readXml( File file ) throws
ParserConfigurationException, IOException, SAXException {
- return readXml(Files.newInputStream(file.toPath()));
+ try (InputStream input = Files.newInputStream(file.toPath())) {
+ return readXml(input);
+ }
}
public static Document readXml( InputStream input ) throws
ParserConfigurationException, IOException, SAXException {
- DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
- f.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
- f.setFeature("http://apache.org/xml/features/disallow-doctype-decl";, true);
- DocumentBuilder b = f.newDocumentBuilder();
- return b.parse( input );
+ return newDocumentBuilder().parse( input );
}
public static Document readXml( InputSource source ) throws
ParserConfigurationException, IOException, SAXException {
- DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
- f.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
- f.setFeature("http://apache.org/xml/features/disallow-doctype-decl";, true);
- DocumentBuilder b = f.newDocumentBuilder();
- return b.parse( source );
+ return newDocumentBuilder().parse( source );
+ }
+
+ /**
+ * Unmarshals the XML content of a file.
+ *
+ * @param unmarshaller the JAXB unmarshaller to use
+ * @param type the expected type of the root object
+ * @param file the file to read
+ * @param <T> the expected type of the root object
+ * @return the root object of the content tree
+ * @throws IOException if the file cannot be read
+ * @throws JAXBException if the content cannot be parsed or unmarshalled
+ */
Review Comment:
Thanks! Fixed in
https://github.com/apache/knox/pull/1427/commits/c9b22d33b99da02fdaa971705cbcbab2617056b9
Issue Time Tracking
-------------------
Worklog Id: (was: 1043817)
Time Spent: 40m (was: 0.5h)
> Use Apache Commons Secure XML for all JAXP factories
> ----------------------------------------------------
>
> Key: KNOX-3484
> URL: https://issues.apache.org/jira/browse/KNOX-3484
> Project: Apache Knox
> Issue Type: Improvement
> Reporter: Piotr Karwasz
> Priority: Minor
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Knox configures its XML parsers by hand. Every call site that creates a JAXP
> factory repeats its own subset of features and properties
> ({{{}disallow-doctype-decl{}}}, {{{}FEATURE_SECURE_PROCESSING{}}},
> {{{}SUPPORT_DTD{}}}, {{{}isSupportingExternalEntities{}}},
> {{{}ACCESS_EXTERNAL_*{}}}, ...), and the subsets differ from site to site.
> Some sites set nothing and rely on the defaults of whichever implementation
> (JDK, Woodstox, MOXy, Glassfish JAXB, Commons Digester) happens to serve
> them. Some ignore a failure to apply a setting.
> Keeping this correct is a recurring cost for the PMC: every new parser has to
> remember the full list, several of the feature names are
> implementation-specific, and whether a given setting takes effect depends on
> which implementation serves which call site.
> [Apache Commons Secure
> XML|https://commons.apache.org/proper/commons-secure-xml/] (1.0.0, Java 8+,
> no runtime dependencies, Apache-2.0) moves that work into one dependency. Its
> factories are hardened as delivered: external resources (DTDs, external
> entities, XInclude, schema imports, stylesheets) are ignored by non-removable
> resolvers, independently of the underlying JAXP implementation, and a factory
> that cannot be hardened fails loudly instead of silently. The guarantees are
> documented in its [threat
> model|https://commons.apache.org/proper/commons-secure-xml/threat_model.html]
> and tested against the JDK, Xerces, Woodstox and Saxon.
> This is a hardening and maintenance change: it replaces the ad hoc
> configuration with one consistent dependency, so that the PMC no longer has
> to configure XML parsers.
> h2. Current state
> ||Module||Class||API||Current configuration||
> |gateway-util-common|{{XmlUtils}} ({{{}readXml{}}}, {{{}createDocument{}}},
> {{{}getTransformer{}}})|DOM, TrAX|{{{}FEATURE_SECURE_PROCESSING{}}},
> {{disallow-doctype-decl}}|
> |gateway-service-admin|{{{}ServiceDefinitionUnmarshaller{}}},
> {{TopologyMarshaller}}|StAX + JAXB|{{{}SUPPORT_DTD{}}},
> {{{}isSupportingExternalEntities{}}}, {{isReplacingEntityReferences}}|
> |gateway-provider-rewrite|{{XmlFilterReader}}|StAX, XPath|StAX: same three
> properties; XPath: {{{}FEATURE_SECURE_PROCESSING{}}}, failure ignored|
> |gateway-discovery-ambari|{{ServiceURLPropertyConfig}}|XPath|none|
> |gateway-server|{{TopologyValidator}}|Validation|{{ACCESS_EXTERNAL_*}} on the
> {{Validator}} only|
> |gateway-spi|{{UrlRewriteRulesDescriptorAdapter}}|TrAX|none|
> |gateway-server|{{{}ServiceDefinitionsLoader{}}},
> {{{}ApplicationDeploymentContributor{}}}, {{TopologyToDescriptor}}|JAXB
> {{unmarshal(File/InputStream)}}|none (depends on the JAXB provider)|
> |gateway-service-remoteconfig|{{RemoteConfigurationRegistriesParser}}|JAXB
> {{unmarshal(File)}}|none (depends on the JAXB provider)|
> |gateway-topology-simple|{{ProviderConfigurationParser}}|JAXB
> {{unmarshal(InputStream)}}|none (depends on the JAXB provider)|
> |gateway-server, gateway-provider-rewrite-common|{{{}TopologyUtils{}}},
> {{{}XmlGatewayDescriptorImporter{}}}, {{XmlUrlRewriteRulesImporter}}|Commons
> Digester|none (depends on Digester's {{{}SAXParserFactory{}}})|
> h2. Proposed changes
> Add {{org.apache.commons:commons-secure-xml}} to the root
> {{dependencyManagement}} and to the modules above, then:
> * *DOM and TrAX:* {{XmlUtils}} and {{UrlRewriteRulesDescriptorAdapter}}
> obtain their factories from {{SecureDocumentBuilderFactory}} and
> {{{}SecureTransformerFactory{}}}. The security-motivated feature settings are
> removed; {{disallow-doctype-decl}} may be kept where Knox prefers to reject a
> DOCTYPE outright, as a behavioral choice rather than a security setting.
> * *StAX:* {{{}ServiceDefinitionUnmarshaller{}}}, {{TopologyMarshaller}} and
> {{XmlFilterReader}} obtain their {{XMLInputFactory}} from
> {{{}SecureXMLInputFactory.newFactory(){}}}. {{SUPPORT_DTD=false}} may
> likewise be kept as a behavioral choice: Commons Secure XML accepts settings
> that restrict further.
> * *XPath:* {{XmlFilterReader}} and {{ServiceURLPropertyConfig}} use
> {{{}SecureXPathFactory{}}}.
> * *Validation:* {{TopologyValidator}} uses {{{}SecureSchemaFactory{}}}.
> * *JAXB:* the {{unmarshal(File)}} and {{unmarshal(InputStream)}} calls are
> replaced with {{{}unmarshal(XMLEventReader){}}}, where the reader is created
> by {{{}SecureXMLInputFactory.newFactory().createXMLEventReader(...){}}}. The
> hardening then no longer depends on which JAXB provider (MOXy or the
> Glassfish runtime) serves a given context.
> * *Commons Digester:* the next Digester release depends on Commons Secure
> XML and hardens its parser by default, so upgrading Digester brings the same
> protection and the dependency comes at no extra cost. Until then, the three
> importers can pass a hardened reader explicitly:
> {{{}DigesterLoader.newDigester(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader(),
> rules){}}}.
> h2. Enforcement
> To keep raw factories from coming back, add a
> [forbidden-apis|https://github.com/policeman-tools/forbidden-apis] check with
> a signatures file that rejects the plain JAXP factory methods
> ({{{}DocumentBuilderFactory{}}}, {{{}SAXParserFactory{}}},
> {{{}XMLInputFactory{}}}, {{{}TransformerFactory{}}}, {{{}SchemaFactory{}}},
> {{{}XPathFactory{}}}) and the {{unmarshal(File)}} /
> {{unmarshal(InputStream)}} overloads of {{jakarta.xml.bind.Unmarshaller}} in
> main code.
> No new XXE tests are needed: the guarantees are the library's and are tested
> there. The existing KNOX-1308 and KNOX-3447 tests should keep passing
> unchanged.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
