Details
-
Improvement
-
Status: Resolved
-
Minor
-
Resolution: Fixed
-
1.3.0
-
None
Description
Since data flows can generally deal with non-trusted data, the processors should handle it in a secure manner.
In case of XML there are various known vulnerabilities - OWASP. Some can be mitigated via XML parser/XSLT Processor features.
The TransformXml processor should have a setting enabling these secure settings.
Attachments
Issue Links
- relates to
-
NIFI-9399 Apply Secure Processing to TransformXml XSLT Sources
- Resolved
- links to
Activity
GitHub user yuri1969 reopened a pull request:
https://github.com/apache/nifi/pull/1946
NIFI-4125 - Add basic security settings to TransformXml
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-
-
- For all changes:
-
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [x] Is your initial contribution a single, squashed commit?
-
-
- For code changes:
-
- [x] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
-
-
- For documentation related changes:
-
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
-
-
- Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
- Note:
-
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yuri1969/nifi XSLT-securing
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1946.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1946
commit 9469c21f3f3dc3ba137e258e3be711cb2ea5a14d
Author: yuri1969 <1969yuri1969@gmail.com>
Date: 2017-06-25T20:10:31Z
Initial propose
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124109148
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -98,6 +99,16 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SECURE_PROCESSING = new PropertyDescriptor.Builder()
+ .name("secure-processing")
+ .displayName("Secure processing")
+ .description("Whether or not to mitigate various XML-related attacks like XXE (XML External Entity) attacks.")
+ .required(true)
+ .defaultValue("true")
— End diff –
Although having this default to true is probably a better idea from a security standpoint, to maintain backwards-compatibility (and not possibly disrupt existing flows), maybe this should default to false?
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124109266
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -166,8 +178,17 @@ protected PropertyDescriptor getSupportedDynamicPropertyDescriptor(final String
.build();
}
- private Templates newTemplates(String path) throws TransformerConfigurationException {
+ private Templates newTemplates(ProcessContext context, String path) throws TransformerConfigurationException {
+ final Boolean secureProcessing = context.getProperty(SECURE_PROCESSING).asBoolean();
TransformerFactory factory = TransformerFactory.newInstance();
+
+ if (secureProcessing) {
+ factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ // don't be overly DTD-unfriendly forcing http://apache.org/xml/features/disallow-doctype-decl
+ factory.setFeature("http://saxon.sf.net/feature/parserFeature?uri=http://xml.org/sax/features/external-parameter-entities", false);-
- End diff –
-
Does this make a call out to that URL, or is that just the name of the feature?
Github user yuri1969 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124115889
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -166,8 +178,17 @@ protected PropertyDescriptor getSupportedDynamicPropertyDescriptor(final String
.build();
}
- private Templates newTemplates(String path) throws TransformerConfigurationException {
+ private Templates newTemplates(ProcessContext context, String path) throws TransformerConfigurationException {
+ final Boolean secureProcessing = context.getProperty(SECURE_PROCESSING).asBoolean();
TransformerFactory factory = TransformerFactory.newInstance();
+
+ if (secureProcessing) {
+ factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ // don't be overly DTD-unfriendly forcing http://apache.org/xml/features/disallow-doctype-decl
+ factory.setFeature("http://saxon.sf.net/feature/parserFeature?uri=http://xml.org/sax/features/external-parameter-entities", false);-
- End diff –
-
The URL is Saxon's (horrible) feature name.
Github user yuri1969 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124116697
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -98,6 +99,16 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SECURE_PROCESSING = new PropertyDescriptor.Builder()
+ .name("secure-processing")
+ .displayName("Secure processing")
+ .description("Whether or not to mitigate various XML-related attacks like XXE (XML External Entity) attacks.")
+ .required(true)
+ .defaultValue("true")
— End diff –
You are right, it alters `TransformXml` output in specific cases. TBH I don't know what is the NiFi's backwards-compatibility policy.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124117425
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -166,8 +178,17 @@ protected PropertyDescriptor getSupportedDynamicPropertyDescriptor(final String
.build();
}
- private Templates newTemplates(String path) throws TransformerConfigurationException {
+ private Templates newTemplates(ProcessContext context, String path) throws TransformerConfigurationException {
+ final Boolean secureProcessing = context.getProperty(SECURE_PROCESSING).asBoolean();
TransformerFactory factory = TransformerFactory.newInstance();
+
+ if (secureProcessing) {
+ factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ // don't be overly DTD-unfriendly forcing http://apache.org/xml/features/disallow-doctype-decl
+ factory.setFeature("http://saxon.sf.net/feature/parserFeature?uri=http://xml.org/sax/features/external-parameter-entities", false);-
- End diff –
-
Ouch! Oh well, if it works it works
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124117608
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -98,6 +99,16 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SECURE_PROCESSING = new PropertyDescriptor.Builder()
+ .name("secure-processing")
+ .displayName("Secure processing")
+ .description("Whether or not to mitigate various XML-related attacks like XXE (XML External Entity) attacks.")
+ .required(true)
+ .defaultValue("true")
— End diff –
I'm not sure if there's an official backwards-compatibility policy, but I think to be safe, when adding new (optional) functionality via a processor property, the default value should match the existing behavior (in this case false).
Github user yuri1969 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124119598
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -98,6 +99,16 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SECURE_PROCESSING = new PropertyDescriptor.Builder()
+ .name("secure-processing")
+ .displayName("Secure processing")
+ .description("Whether or not to mitigate various XML-related attacks like XXE (XML External Entity) attacks.")
+ .required(true)
+ .defaultValue("true")
— End diff –
Fair enough. I'll set the default to false.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124423881
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -98,6 +99,16 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SECURE_PROCESSING = new PropertyDescriptor.Builder()
+ .name("secure-processing")
+ .displayName("Secure processing")
+ .description("Whether or not to mitigate various XML-related attacks like XXE (XML External Entity) attacks.")
+ .required(true)
+ .defaultValue("false")
— End diff –
Is there a reason you decided to default this to `false`? Do the secure processing features add substantial time or drastically reduce the feature set of the processor?
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124428785
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -98,6 +99,16 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SECURE_PROCESSING = new PropertyDescriptor.Builder()
+ .name("secure-processing")
+ .displayName("Secure processing")
+ .description("Whether or not to mitigate various XML-related attacks like XXE (XML External Entity) attacks.")
+ .required(true)
+ .defaultValue("false")
— End diff –
That's my bad, I suggested he change it to keep the default pre-existing behavior. If true is better, I'm fine with that
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1946#discussion_r124429931
— Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java —
@@ -98,6 +99,16 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SECURE_PROCESSING = new PropertyDescriptor.Builder()
+ .name("secure-processing")
+ .displayName("Secure processing")
+ .description("Whether or not to mitigate various XML-related attacks like XXE (XML External Entity) attacks.")
+ .required(true)
+ .defaultValue("false")
— End diff –
No worries, I found those comments in the email thread but for some reason they did not show for me in the GitHub UI. I understand the desire for backward compatibility but this is a security fix, so as long as it is well-documented in the processor and the release notes/migration notes for the release that contains this, I think `true` is a safe choice.
Github user yuri1969 commented on the issue:
https://github.com/apache/nifi/pull/1946
Reverted back to turned on by default.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1946
Upon testing this feature, I'm not sure it's necessary. Java SE 5+ has a [restriction on entity expansion to 64,000 elements enabled by default](https://docs.oracle.com/javase/1.5.0/docs/guide/xml/jaxp/JAXP-Compatibility_150.html#JAXP_security) ([more information from Blaise Doughan here](http://blog.bdoughan.com/2011/03/preventing-entity-expansion-attacks-in.html)), so if I try to ingest the following XXE file, I get an appropriate error response:
```
<!DOCTYPE foo [
<!ENTITY a "1234567890" >
<!ENTITY b "&a;&a;&a;&a;&a;&a;&a;&a;&a;&a;" >
<!ENTITY c "&b;&b;&b;&b;&b;&b;&b;&b;&b;&b;" >
<!ENTITY d "&c;&c;&c;&c;&c;&c;&c;&c;&c;&c;" >
<!ENTITY e "&d;&d;&d;&d;&d;&d;&d;&d;&d;&d;" >
<!ENTITY f "&e;&e;&e;&e;&e;&e;&e;&e;&e;&e;" >
<!ENTITY g "&f;&f;&f;&f;&f;&f;&f;&f;&f;&f;" >
<!ENTITY h "&g;&g;&g;&g;&g;&g;&g;&g;&g;&g;" >
<!ENTITY i "&h;&h;&h;&h;&h;&h;&h;&h;&h;&h;" >
<!ENTITY j "&i;&i;&i;&i;&i;&i;&i;&i;&i;&i;" >
]>
<foo>&j;</foo>
```
Error when trying to view flowfile content in "formatted" view (current master):
`2017-06-28 15:02:40,226 ERROR [NiFi Web Server-18] o.a.nifi.web.ContentViewerController Unable to generate view of data: Unable to transform content as XML: net.sf.saxon.trans.XPathException: org.xml.sax.SAXParseException; lineNumber: 1; columnNumber: 1; JAXP00010001: The parser has encountered more than "64000" entity expansions in this document; this is the limit imposed by the JDK.`
When routed to a `TransformXML` processor, I get an identical stacktrace in all three of the following scenarios:
1. Current master
2. This patch applied with "Secure processing" `false`
3. This patch applied with "Secure processing" `true`
Stacktrace:
```
2017-06-28 15:04:49,456 ERROR [Timer-Driven Process Thread-9] o.a.n.processors.standard.TransformXml TransformXml[id=efe47d1c-015c-1000-7bae-719994808e8a] Unable to transform StandardFlowFileRecord[uuid=9f91546e-f7b4-4113-97fa-6abb1006b8c9,claim=StandardContentClaim [resourceClaim=StandardResourceClaim[id=1498687343214-1, container=default, section=1], offset=485, length=485],offset=0,name=xxe.xml,size=485] due to org.apache.nifi.processor.exception.ProcessException: IOException thrown from TransformXml[id=efe47d1c-015c-1000-7bae-719994808e8a]: java.io.IOException: net.sf.saxon.trans.XPathException: org.xml.sax.SAXParseException; lineNumber: 1; columnNumber: 1; JAXP00010001: The parser has encountered more than "64000" entity expansions in this document; this is the limit imposed by the JDK.: {}
org.apache.nifi.processor.exception.ProcessException: IOException thrown from TransformXml[id=efe47d1c-015c-1000-7bae-719994808e8a]: java.io.IOException: net.sf.saxon.trans.XPathException: org.xml.sax.SAXParseException; lineNumber: 1; columnNumber: 1; JAXP00010001: The parser has encountered more than "64000" entity expansions in this document; this is the limit imposed by the JDK.
at org.apache.nifi.controller.repository.StandardProcessSession.write(StandardProcessSession.java:2806)
at org.apache.nifi.processors.standard.TransformXml.onTrigger(TransformXml.java:234)
at org.apache.nifi.processor.AbstractProcessor.onTrigger(AbstractProcessor.java:27)
at org.apache.nifi.controller.StandardProcessorNode.onTrigger(StandardProcessorNode.java:1120)
at org.apache.nifi.controller.tasks.ContinuallyRunProcessorTask.call(ContinuallyRunProcessorTask.java:147)
at org.apache.nifi.controller.tasks.ContinuallyRunProcessorTask.call(ContinuallyRunProcessorTask.java:47)
at org.apache.nifi.controller.scheduling.TimerDrivenSchedulingAgent$1.run(TimerDrivenSchedulingAgent.java:132)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.io.IOException: net.sf.saxon.trans.XPathException: org.xml.sax.SAXParseException; lineNumber: 1; columnNumber: 1; JAXP00010001: The parser has encountered more than "64000" entity expansions in this document; this is the limit imposed by the JDK.
at org.apache.nifi.processors.standard.TransformXml$2.process(TransformXml.java:261)
at org.apache.nifi.controller.repository.StandardProcessSession.write(StandardProcessSession.java:2785)
... 13 common frames omitted
Caused by: net.sf.saxon.trans.XPathException: org.xml.sax.SAXParseException; lineNumber: 1; columnNumber: 1; JAXP00010001: The parser has encountered more than "64000" entity expansions in this document; this is the limit imposed by the JDK.
at net.sf.saxon.event.Sender.sendSAXSource(Sender.java:460)
at net.sf.saxon.event.Sender.send(Sender.java:171)
at net.sf.saxon.Controller.transform(Controller.java:1692)
at net.sf.saxon.s9api.XsltTransformer.transform(XsltTransformer.java:547)
at net.sf.saxon.jaxp.TransformerImpl.transform(TransformerImpl.java:179)
at org.apache.nifi.processors.standard.TransformXml$2.process(TransformXml.java:259)
... 14 common frames omitted
Caused by: org.xml.sax.SAXParseException: JAXP00010001: The parser has encountered more than "64000" entity expansions in this document; this is the limit imposed by the JDK.
at com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException(ErrorHandlerWrapper.java:203)
at com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.fatalError(ErrorHandlerWrapper.java:177)
at com.sun.org.apache.xerces.internal.impl.XMLErrorReporter.reportError(XMLErrorReporter.java:400)
at com.sun.org.apache.xerces.internal.impl.XMLErrorReporter.reportError(XMLErrorReporter.java:327)
at com.sun.org.apache.xerces.internal.impl.XMLErrorReporter.reportError(XMLErrorReporter.java:284)
at com.sun.org.apache.xerces.internal.impl.XMLEntityManager.startEntity(XMLEntityManager.java:1317)
at com.sun.org.apache.xerces.internal.impl.XMLEntityManager.startEntity(XMLEntityManager.java:1241)
at com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanEntityReference(XMLDocumentFragmentScannerImpl.java:1902)
at com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl$FragmentContentDriver.next(XMLDocumentFragmentScannerImpl.java:3058)
at com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl.next(XMLDocumentScannerImpl.java:606)
at com.sun.org.apache.xerces.internal.impl.XMLNSDocumentScannerImpl.next(XMLNSDocumentScannerImpl.java:118)
at com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanDocument(XMLDocumentFragmentScannerImpl.java:504)
at com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:848)
at com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:777)
at com.sun.org.apache.xerces.internal.parsers.XMLParser.parse(XMLParser.java:141)
at com.sun.org.apache.xerces.internal.parsers.AbstractSAXParser.parse(AbstractSAXParser.java:1213)
at com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl$JAXPSAXParser.parse(SAXParserImpl.java:643)
at net.sf.saxon.event.Sender.sendSAXSource(Sender.java:440)
... 19 common frames omitted
```
If this protection is enabled by the JRE, and disabling "Secure processing" doesn't affect it, what does this feature provide?
Github user yuri1969 commented on the issue:
https://github.com/apache/nifi/pull/1946
The `transformXml` uses Saxon HE implementation of a XSLT Processor. So Saxon implements the features.
- `FEATURE_SECURE_PROCESSING` is XSLT Processor feature that restricts XSLT functionality like `system-property()` to access Java system properties, using relative URIs in `xsl:result-document`, etc. So it should mitigate some threats coming from using non-trusted XSLTs.
- both `http://xml.org/sax/features/external` are XML Parser features that restrict using `<!ENTITY` of `<!DOCTYPE` in a XML file. An attacker can use entities to obtain access to your FS.
For example a malicious non-trusted input XML:
```
<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE foo [
<!ELEMENT foo ANY >
<!ENTITY xxe SYSTEM "file:///etc/passwd" >]>
<foo>&xxe;</foo>
```
Then a XSLT output containing element `foo` leaks content of your /etc/passwd file.
- Billion laughs/LOL bomb/Entity expansion DoS is trully secured by JRE default as you showed. So no need for a feature here.
Commit 3bf1d127062a2d52d7be32e5ef29e19242219f48 in nifi's branch refs/heads/master from yuri1969
[ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=3bf1d12 ]
NIFI-4125 Added secure transform feature and configuration to TransformXML processor to mitigate XXE file system leaks.
This closes #1946.
Signed-off-by: Andy LoPresto <alopresto@apache.org>
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1946
Thanks for that detailed explanation. I updated the commit message to fit our format.
Ran `contrib-check` and all tests pass. +1, merging.
GitHub user yuri1969 opened a pull request:
https://github.com/apache/nifi/pull/1946
NIFI-4125- Add basic security settings to TransformXmlThank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
in the commit message?
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yuri1969/nifi XSLT-securing
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1946.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1946
commit 9469c21f3f3dc3ba137e258e3be711cb2ea5a14d
Author: yuri1969 <1969yuri1969@gmail.com>
Date: 2017-06-25T20:10:31Z
Initial propose