You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "garydgregory (via GitHub)" <gi...@apache.org> on 2026/10/04 13:35:16 UTC
[PR] Main commons secure xml [activemq]
garydgregory opened a new pull request, #2635:
URL: https://github.com/apache/activemq/pull/2635
Harden XML calls with Apache Commons Secure XML
CC @ppkarwasz
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Main commons secure xml [activemq]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2635:
URL: https://github.com/apache/activemq/pull/2635#discussion_r4178610463
##########
activemq-broker/src/main/java/org/apache/activemq/util/XmlFactories.java:
##########
@@ -35,42 +35,11 @@ public final class XmlFactories {
private XmlFactories() { /* Do not instantiate */ }
public static DocumentBuilderFactory getSafeDocumentBuilderFactory() {
- DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();
-
- // See https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.md#java
- trySetFeature(builderFactory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
- trySetFeature(builderFactory,"http://apache.org/xml/features/disallow-doctype-decl", true);
- trySetFeature(builderFactory,"http://xml.org/sax/features/external-general-entities", false);
- trySetFeature(builderFactory,"http://xml.org/sax/features/external-parameter-entities", false);
- trySetFeature(builderFactory,"http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
-
- return builderFactory;
+ return SecureDocumentBuilderFactory.newInstance();
Review Comment:
What about inlinining this method and deprecating this class?
This method has only **two** callers and both could just use `newNSDocumentBuilder` that we are about to be introduced in version `1.1.0`.
A small difference in the two callers would need to be settled:
- `RuntimeConfigurationBroker#loadConfiguration` uses a namepace-aware parser and retrieves the broker element via:
```java
doc.getElementsByTagNameNS("*","broker").item(0);
```
- `CreateCommand#copyActivemqConf` uses a namespace-unaware parser and retrieves the same broker element via `XPath`.
I think both could use a namespace-aware parser
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Main commons secure xml [activemq]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #2635:
URL: https://github.com/apache/activemq/pull/2635#issuecomment-5982250784
Hi @mattrpav CC @ppkarwasz
Commons Secure XML is a tiny library that:
- Prevents XXE Attacks: It disables external entity resolution, blocking XML External Entity (XXE) vulnerabilities that lead to data exposure.
- Prevents Billion Laughs Attacks: It restricts recursive entity expansion, preventing denial-of-service (DoS) attempts.
- Configures standard XML parsers with safe, restrictive settings out of the box.
- Eliminates manual, error-prone security configuration of DocumentBuilderFactory, SAXParserFactory, TransformerFactory, and so on
HTH!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Harden XML with Commons Secure XML [activemq]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #2635:
URL: https://github.com/apache/activemq/pull/2635#discussion_r4179103765
##########
activemq-broker/src/main/java/org/apache/activemq/util/XmlFactories.java:
##########
@@ -35,42 +35,11 @@ public final class XmlFactories {
private XmlFactories() { /* Do not instantiate */ }
public static DocumentBuilderFactory getSafeDocumentBuilderFactory() {
- DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();
-
- // See https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.md#java
- trySetFeature(builderFactory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
- trySetFeature(builderFactory,"http://apache.org/xml/features/disallow-doctype-decl", true);
- trySetFeature(builderFactory,"http://xml.org/sax/features/external-general-entities", false);
- trySetFeature(builderFactory,"http://xml.org/sax/features/external-parameter-entities", false);
- trySetFeature(builderFactory,"http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
-
- return builderFactory;
+ return SecureDocumentBuilderFactory.newInstance();
Review Comment:
I implemented @ppkarwasz 's suggestion in commit 6264883fc77692a3ae289cc257c008313d294e01.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Main commons secure xml [activemq]
Posted by "mattrpav (via GitHub)" <gi...@apache.org>.
mattrpav commented on PR #2635:
URL: https://github.com/apache/activemq/pull/2635#issuecomment-5982119771
Hi @garydgregory
thank you for the PR. Can you share a bit what the advantages are of using this library?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Main commons secure xml [activemq]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #2635:
URL: https://github.com/apache/activemq/pull/2635#issuecomment-5982397776
Oh, another use case is (like me) when you offer an option to embedded AMQ, AMQ is not in full control of the classpath. When the JRE resolves the JAXP implementation, you may get implementation dependent issues. We talk about this in the introduction in https://commons.apache.org/proper/commons-secure-xml/apidocs/index.html#tldr if you care for the details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Harden XML with Commons Secure XML [activemq]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #2635:
URL: https://github.com/apache/activemq/pull/2635#discussion_r4179176556
##########
activemq-runtime-config/src/main/java/org/apache/activemq/plugin/RuntimeConfigurationBroker.java:
##########
Review Comment:
Version 1.1.0 will be available tomorrow evening after 11 PM EDT. I will update the PR, or create a new one.
##########
activemq-runtime-config/src/main/java/org/apache/activemq/plugin/RuntimeConfigurationBroker.java:
##########
Review Comment:
Version 1.1.0 will be available tomorrow evening after 11 PM EDT. I will update the PR then, or create a new one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Harden XML with Commons Secure XML [activemq]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2635:
URL: https://github.com/apache/activemq/pull/2635#discussion_r4179144150
##########
activemq-runtime-config/src/main/java/org/apache/activemq/plugin/RuntimeConfigurationBroker.java:
##########
Review Comment:
What about upgrading to `1.1.0`? All this could collapse to:
```
Document doc = SecureDocumentBuilderFactory.newNSDocumentBuilder().parse(configToMonitor.getInputStream());
```
##########
activemq-runtime-config/src/main/java/org/apache/activemq/plugin/RuntimeConfigurationBroker.java:
##########
Review Comment:
What about upgrading to `1.1.0`? All this could collapse to:
```java
Document doc = SecureDocumentBuilderFactory.newNSDocumentBuilder().parse(configToMonitor.getInputStream());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Main commons secure xml [activemq]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #2635:
URL: https://github.com/apache/activemq/pull/2635#issuecomment-5982954814
> * Prevents XXE Attacks: It disables external entity resolution, blocking XML External Entity (XXE) vulnerabilities that lead to data exposure.
And most importantly, this prevents **reports** of XXE vulnerabilities. As far as I can tell, you only use XML to parse **trusted** configuration file, which makes all those reports moot, yet you had to harden your parsers for some reason…
Using Commons Secure XML, you can keep your XML code as simple as possible and at the same time reject all XXE reports.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Harden XML with Commons Secure XML [activemq]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2635:
URL: https://github.com/apache/activemq/pull/2635#discussion_r4179188400
##########
activemq-console/src/main/java/org/apache/activemq/console/command/CreateCommand.java:
##########
@@ -158,10 +160,10 @@ protected void copyActivemqConf(File srcBase, File targetBase, String activemqCo
File dest = new File(targetBase, DEFAULT_TARGET_ACTIVEMQ_CONF);
context.print("Copying from: " + src.getCanonicalPath() + "\n to: " + dest.getCanonicalPath());
- DocumentBuilder builder = XmlFactories.getSafeDocumentBuilderFactory().newDocumentBuilder();
+ DocumentBuilder builder = SecureDocumentBuilderFactory.newNSInstance().newDocumentBuilder();
Element docElem = builder.parse(src).getDocumentElement();
- XPath xpath = XPathFactory.newInstance().newXPath();
+ XPath xpath = SecureXPathFactory.newInstance().newXPath();
Attr brokerNameAttr = (Attr) xpath.evaluate(DEFAULT_BROKERNAME_XPATH, docElem, XPathConstants.NODE);
Review Comment:
This will create a problem with namespaces: the XPath used is `/beans/broker/@brokerName`, but both `beans` and `broker` have a namespace, so the XPath will not find anything with a namespace sensitive parser.
I see a couple of solutions:
1. Use `newInstance()` instead of `newNSInstance()`, but this means this path can not upgrade to `newNSDocumentBuilder()`.
2. Change the XPath expression to `/*[local-name()='beans'/*[local-name()='broker']/@brokerName`.
3. Or use DOM to find the attribute:
```java
Element brokerElement = (Element) docElem.getElementsByTagNameNS("*", "broker").item(0);
Attr brokerNameAttr = brokerElement.getAttributeNodeNS(null, "brokerName");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Harden XML with Commons Secure XML [activemq]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2635:
URL: https://github.com/apache/activemq/pull/2635#discussion_r4179830040
##########
activemq-console/src/main/java/org/apache/activemq/console/command/CreateCommand.java:
##########
@@ -158,10 +160,10 @@ protected void copyActivemqConf(File srcBase, File targetBase, String activemqCo
File dest = new File(targetBase, DEFAULT_TARGET_ACTIVEMQ_CONF);
context.print("Copying from: " + src.getCanonicalPath() + "\n to: " + dest.getCanonicalPath());
- DocumentBuilder builder = XmlFactories.getSafeDocumentBuilderFactory().newDocumentBuilder();
+ DocumentBuilder builder = SecureDocumentBuilderFactory.newNSInstance().newDocumentBuilder();
Element docElem = builder.parse(src).getDocumentElement();
- XPath xpath = XPathFactory.newInstance().newXPath();
+ XPath xpath = SecureXPathFactory.newInstance().newXPath();
Attr brokerNameAttr = (Attr) xpath.evaluate(DEFAULT_BROKERNAME_XPATH, docElem, XPathConstants.NODE);
Review Comment:
Unless I am mistaken, all ActiveMQ configurations need to use namespaces, otherwise Spring will not accept them. Therefore you can also set a `NamespaceContext` on the `XPath` (there is a `SimpleNamespaceContext` in `spring-core` if you don't want to create your own), bind the namespaces and use something like `/b:beans/a:broker/@brokerName`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Harden XML with Commons Secure XML [activemq]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #2635:
URL: https://github.com/apache/activemq/pull/2635#discussion_r4179683338
##########
activemq-console/src/main/java/org/apache/activemq/console/command/CreateCommand.java:
##########
@@ -158,10 +160,10 @@ protected void copyActivemqConf(File srcBase, File targetBase, String activemqCo
File dest = new File(targetBase, DEFAULT_TARGET_ACTIVEMQ_CONF);
context.print("Copying from: " + src.getCanonicalPath() + "\n to: " + dest.getCanonicalPath());
- DocumentBuilder builder = XmlFactories.getSafeDocumentBuilderFactory().newDocumentBuilder();
+ DocumentBuilder builder = SecureDocumentBuilderFactory.newNSInstance().newDocumentBuilder();
Element docElem = builder.parse(src).getDocumentElement();
- XPath xpath = XPathFactory.newInstance().newXPath();
+ XPath xpath = SecureXPathFactory.newInstance().newXPath();
Attr brokerNameAttr = (Attr) xpath.evaluate(DEFAULT_BROKERNAME_XPATH, docElem, XPathConstants.NODE);
Review Comment:
@ppkarwasz I implemented your suggestion 2 in commit d64bcfd05d025d9d2657749d539a7f861c981617.
I like the XPath approach with namespaces because it is more precise IMO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Harden XML with Commons Secure XML [activemq]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #2635:
URL: https://github.com/apache/activemq/pull/2635#discussion_r4179844266
##########
activemq-console/src/main/java/org/apache/activemq/console/command/CreateCommand.java:
##########
@@ -158,10 +160,10 @@ protected void copyActivemqConf(File srcBase, File targetBase, String activemqCo
File dest = new File(targetBase, DEFAULT_TARGET_ACTIVEMQ_CONF);
context.print("Copying from: " + src.getCanonicalPath() + "\n to: " + dest.getCanonicalPath());
- DocumentBuilder builder = XmlFactories.getSafeDocumentBuilderFactory().newDocumentBuilder();
+ DocumentBuilder builder = SecureDocumentBuilderFactory.newNSInstance().newDocumentBuilder();
Element docElem = builder.parse(src).getDocumentElement();
- XPath xpath = XPathFactory.newInstance().newXPath();
+ XPath xpath = SecureXPathFactory.newInstance().newXPath();
Attr brokerNameAttr = (Attr) xpath.evaluate(DEFAULT_BROKERNAME_XPATH, docElem, XPathConstants.NODE);
Review Comment:
Hm, that may be the case but it feels like we are entering clean up territory instead of migrating to Commons Secure XML. I'd rather leave that one for the AMQ team and not make this PR more complicated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact