Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-6301 - Added a SafeXMLConfiguration which disables XML DTDs whic… #3507

Closed
wants to merge 7 commits into from

Conversation

@thenatog
Copy link
Contributor

thenatog commented May 30, 2019

…h may call external entities.

NIFI-6301 - Fixed unit test. Added comments.
NIFI-6301 - Removed unused rule from test.
NIFI-6301 - Changed read() methods to use a boolean instead. Updated comments.
NIFI-6301 - Fixing checkstyle errors.

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Enables X functionality; fixes bug NIFI-YYYY.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • 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.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

  • 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?
  • 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.

@alopresto

This comment has been minimized.

Copy link
Contributor

alopresto commented May 30, 2019

Reviewing...

private static final String XXE_ERROR_MESSAGE = "XML configuration file contained an external entity. To eliminate XXE vulnerabilities, NiFi has external entity processing disabled.";

@Override
public void initFileLocator(FileLocator loc) {

This comment has been minimized.

Copy link
@alopresto

alopresto May 30, 2019

Contributor

Moving public method below constructors.


@Override
public void read(Reader in) throws ConfigurationException, IOException {
try {

This comment has been minimized.

Copy link
@alopresto

alopresto May 30, 2019

Contributor

I refactored this duplicate code into a private method (see d67d6e4).


try {
// Act
// Service will fail to enable because test-xxe.xml contains a DTD

This comment has been minimized.

Copy link
@alopresto

alopresto May 30, 2019

Contributor

When I run the actual NiFi instance, the service is enabled. There is a bulletin shown, and a stacktrace in the error log, but the service is enabled.

Screen Shot 2019-05-30 at 2 57 56 PM

2019-05-30 14:59:20,528 ERROR [Timer-Driven Process Thread-9] o.a.n.c.s.StandardControllerServiceNode StandardControllerServiceNode{controllerServiceHolder=org.apache.nifi:nifi-lookup-services-nar:1.10.0-SNAPSHOT, versionedComponentId=null, comment='', processGroup=StandardProcessGroup[identifier=0abd5fbf-016b-1000-00a7-2df537bcba2a], active=true} Failed to invoke @OnEnabled method due to org.apache.nifi.reporting.InitializationException: org.apache.commons.configuration2.ex.ConfigurationException: XML configuration file contained an external entity. To prevent XXE vulnerabilities, NiFi has external entity processing disabled.: {}
org.apache.nifi.reporting.InitializationException: org.apache.commons.configuration2.ex.ConfigurationException: XML configuration file contained an external entity. To prevent XXE vulnerabilities, NiFi has external entity processing disabled.
	at org.apache.nifi.lookup.configuration2.CommonsConfigurationLookupService.onEnabled(CommonsConfigurationLookupService.java:121)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.apache.nifi.util.ReflectionUtils.invokeMethodsWithAnnotations(ReflectionUtils.java:142)
	at org.apache.nifi.util.ReflectionUtils.invokeMethodsWithAnnotations(ReflectionUtils.java:130)
	at org.apache.nifi.util.ReflectionUtils.invokeMethodsWithAnnotations(ReflectionUtils.java:75)
	at org.apache.nifi.util.ReflectionUtils.invokeMethodsWithAnnotation(ReflectionUtils.java:52)
	at org.apache.nifi.controller.service.StandardControllerServiceNode$2.run(StandardControllerServiceNode.java:435)
	at org.apache.nifi.engine.FlowEngine$2.run(FlowEngine.java:110)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: org.apache.commons.configuration2.ex.ConfigurationException: XML configuration file contained an external entity. To prevent XXE vulnerabilities, NiFi has external entity processing disabled.
	at org.apache.nifi.security.xml.SafeXMLConfiguration.delegateRead(SafeXMLConfiguration.java:139)
	at org.apache.nifi.security.xml.SafeXMLConfiguration.read(SafeXMLConfiguration.java:128)
	at org.apache.commons.configuration2.io.FileHandler.loadFromStreamDirectly(FileHandler.java:1080)
	at org.apache.commons.configuration2.io.FileHandler.loadFromStream(FileHandler.java:1055)
	at org.apache.commons.configuration2.io.FileHandler.load(FileHandler.java:990)
	at org.apache.commons.configuration2.io.FileHandler.load(FileHandler.java:973)
	at org.apache.commons.configuration2.io.FileHandler.load(FileHandler.java:702)
	at org.apache.commons.configuration2.builder.FileBasedConfigurationBuilder.initFileHandler(FileBasedConfigurationBuilder.java:312)
	at org.apache.commons.configuration2.builder.ReloadingFileBasedConfigurationBuilder.initFileHandler(ReloadingFileBasedConfigurationBuilder.java:185)
	at org.apache.commons.configuration2.builder.FileBasedConfigurationBuilder.initResultInstance(FileBasedConfigurationBuilder.java:291)
	at org.apache.commons.configuration2.builder.FileBasedConfigurationBuilder.initResultInstance(FileBasedConfigurationBuilder.java:60)
	at org.apache.commons.configuration2.builder.BasicConfigurationBuilder.createResult(BasicConfigurationBuilder.java:421)
	at org.apache.commons.configuration2.builder.BasicConfigurationBuilder.getConfiguration(BasicConfigurationBuilder.java:285)
	at org.apache.nifi.lookup.configuration2.CommonsConfigurationLookupService.onEnabled(CommonsConfigurationLookupService.java:119)
	... 17 common frames omitted
Caused by: org.apache.commons.configuration2.ex.ConfigurationException: Error parsing file:/Users/alopresto/Workspace/nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/resources/xxe_from_report.xml
	at org.apache.commons.configuration2.XMLConfiguration.load(XMLConfiguration.java:1030)
	at org.apache.commons.configuration2.XMLConfiguration.read(XMLConfiguration.java:996)
	at org.apache.nifi.security.xml.SafeXMLConfiguration.lambda$read$1(SafeXMLConfiguration.java:129)
	at org.apache.nifi.security.xml.SafeXMLConfiguration.delegateRead(SafeXMLConfiguration.java:135)
	... 30 common frames omitted
Caused by: org.xml.sax.SAXParseException: DOCTYPE is disallowed when the feature "http://apache.org/xml/features/disallow-doctype-decl" set to true.
	at com.sun.org.apache.xerces.internal.parsers.DOMParser.parse(DOMParser.java:257)
	at com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderImpl.parse(DocumentBuilderImpl.java:339)
	at org.apache.commons.configuration2.XMLConfiguration.load(XMLConfiguration.java:1023)
	... 33 common frames omitted
2019-05-30 14:59:20,529 ERROR [Timer-Driven Process Thread-9] o.a.n.c.s.StandardControllerServiceNode Failed to invoke @OnEnabled method of XMLFileLookupService[id=0abec592-016b-1000-192b-309d4b845a7b] due to org.apache.nifi.reporting.InitializationException: org.apache.commons.configuration2.ex.ConfigurationException: XML configuration file contained an external entity. To prevent XXE vulnerabilities, NiFi has external entity processing disabled.

This comment has been minimized.

Copy link
@alopresto

alopresto May 30, 2019

Contributor

After re-examination, the service stays in Enabling state.

This comment has been minimized.

Copy link
@thenatog

thenatog May 31, 2019

Author Contributor

It may be better to implement this check as a validator instead. I'll add an XMLValidator which checks for XXEs.

@thenatog

This comment has been minimized.

Copy link
Contributor Author

thenatog commented Jun 6, 2019

I've added an XXE validator to the CommonsConfigurationLookupService to do a simple check if the file contains an !ENTITY declaration. This in combination with the SafeXMLConfiguration will restrict XXE usage in XML configuration files in NiFi.

@alopresto

This comment has been minimized.

Copy link
Contributor

alopresto commented Jun 11, 2019

I built the PR successfully and ran a local instance.

  • Using local_xxe_file.xml the controller service validation correctly returned "Invalid" and explained the reason with the XXE error message
  • Using whitespace_xxe_file.xml the controller service validation correctly returned "Invalid" and explained the reason with the XXE error message
  • Using multiline_xxe_file.xml the controller service validated and shows "Disabled". When the "Enable" action is taken, the controller service stays in "Enabling" mode. The dialog processes as it would in a successful operation. Disabling the controller service takes ~15-30 seconds. This is not ideal for user experience

I'm going to see if I can enforce the same expected behavior from the regular and whitespace XXE file on the multiline XXE file. I am also going to suppress the stacktrace unless DEBUG is enabled, as the stacktrace doesn't add valuable information to the provided error message.

@alopresto

This comment has been minimized.

Copy link
Contributor

alopresto commented Jun 11, 2019

I was able to modify some code to allow multiline XXE attacks to be caught during validation. I updated the unit tests to accurately reflect this.

I discovered another issue -- the XXEValidator seems to continue running on a looped thread even when the XMLFileLookupService is Invalid, and even after it's been deleted (see screenshot). I'll continue investigating.

Screen Shot 2019-06-10 at 7 22 55 PM

@alopresto

This comment has been minimized.

Copy link
Contributor

alopresto commented Jun 11, 2019

Continuing to investigate. Not sure which timer/thread is triggering the validation, but you can see from the log it continues with cached data even after the controller service is deleted.

2019-06-10 19:47:23,833 INFO [main] org.apache.nifi.NiFi Controller initialization took 16163440198 nanoseconds (16 seconds).
2019-06-10 19:47:57,672 INFO [NiFi Web Server-22] o.a.nifi.controller.ExtensionBuilder Created Controller Service of type org.apache.nifi.lookup.XMLFileLookupService with identifier 446f5dd4-016b-1000-32cc-80a04680722c
2019-06-10 19:47:57,985 INFO [Flow Service Tasks Thread-2] o.a.nifi.controller.StandardFlowService Saved flow controller org.apache.nifi.controller.FlowController@53e44c8 // Another save pending = false
2019-06-10 19:48:18,618 INFO [Validate Components Thread-4] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:18,618 WARN [Validate Components Thread-4] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:48:18,630 INFO [Flow Service Tasks Thread-1] o.a.nifi.controller.StandardFlowService Saved flow controller org.apache.nifi.controller.FlowController@53e44c8 // Another save pending = false
2019-06-10 19:48:21,426 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:21,427 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:48:26,432 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:26,432 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:48:31,435 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:31,435 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:48:36,439 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:36,439 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:48:41,444 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:41,444 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:48:46,450 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:46,450 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:48:51,454 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:51,454 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:48:52,528 INFO [NiFi Web Server-19] o.a.n.c.flow.StandardFlowManager StandardControllerServiceNode{controllerServiceHolder=org.apache.nifi:nifi-lookup-services-nar:1.10.0-SNAPSHOT, versionedComponentId=null, comment='', processGroup=null, active=false} removed from Flow Controller
2019-06-10 19:48:52,843 INFO [Flow Service Tasks Thread-2] o.a.nifi.controller.StandardFlowService Saved flow controller org.apache.nifi.controller.FlowController@53e44c8 // Another save pending = false
2019-06-10 19:48:56,460 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:48:56,460 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
2019-06-10 19:49:01,463 INFO [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Validating /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml for XXE attack
2019-06-10 19:49:01,464 WARN [Validate Components Thread-2] o.apache.nifi.security.xml.XXEValidator Detected XXE attack in /Users/alopresto/Workspace/nifi/nifi-commons/nifi-security-utils/src/test/resources/local_xxe_file.xml
@alopresto

This comment has been minimized.

Copy link
Contributor

alopresto commented Jun 11, 2019

Leaving my additional commits here for now: alopresto/nifi/pr3507

@andrewmlim

This comment has been minimized.

Copy link
Contributor

andrewmlim commented Jun 11, 2019

Was testing and had no problem verifying the new behavior in the XMLFileLookupService as a Reporting Task Controller Service:

XXE

But when I tried to create the controller service on the Process Group level, the UI would hang.

Investigated by @mcgilman and this looks to be a result of https://issues.apache.org/jira/browse/NIFI-6341 which was fixed recently.

thenatog and others added 5 commits May 29, 2019
…h may call external entities.

NIFI-6301 - Fixed unit test. Added comments.
NIFI-6301 - Removed unused rule from test.
NIFI-6301 - Changed read() methods to use a boolean instead. Updated comments.
NIFI-6301 - Fixing checkstyle errors.
…s that contain XXE declarations. Added unit tests and related XML test files.
@thenatog thenatog force-pushed the thenatog:NIFI-6301-rebased branch from 3ea0668 to bf4bf26 Jun 11, 2019
@thenatog

This comment has been minimized.

Copy link
Contributor Author

thenatog commented Jun 12, 2019

@alopresto I've verified the issue you had with the validator still running even after the controller service is deleted. It occurs for 'controller level' controller services, and not for process-group scoped controller services. It appears to occur for all validators, as I have tested it with another validator other than the XXE one I have added. I have opened a new Jira to track this issue: https://issues.apache.org/jira/browse/NIFI-6371
Good find.

@asfgit asfgit closed this in 75fb34c Jun 12, 2019
@alopresto

This comment has been minimized.

Copy link
Contributor

alopresto commented Jun 12, 2019

Ran contrib-check and all tests pass. +1, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.