Skip to content

Commit

Permalink
NIFI-7680 Added convenience methods for creating XML DocumentBuilder …
Browse files Browse the repository at this point in the history 
…instances.

Added unit tests.

NIFI-7680 Duplicated DocumentBuilder creation method in NotificationServiceManager to avoid nifi-bootstrap dependency on nifi-security-utils.
Explicitly added commons-lang3 to lib/bootstrap/ directory in nifi-assembly.

NIFI-7680 Reverted unnecessary dependency changes.
Added explicit dependencies where necessary.

Signed-off-by: Matthew Burgess <mattyb149@apache.org>

This closes #4436
  • Loading branch information
@alopresto @mattyb149
alopresto authored and mattyb149 committed Jul 28, 2020
1 parent cee6cfd commit 7f0416e
Show file tree
Hide file tree
Showing 27 changed files with 447 additions and 343 deletions.
66 changes: 42 additions & 24 deletions nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/NotificationServiceManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,25 @@
*/
package org.apache.nifi.bootstrap;

import org.apache.nifi.parameter.ParameterLookup;
import java.io.BufferedInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import org.apache.nifi.attribute.expression.language.StandardPropertyValue;
import org.apache.nifi.bootstrap.notification.NotificationContext;
import org.apache.nifi.bootstrap.notification.NotificationInitializationContext;
Expand All @@ -27,6 +45,7 @@
import org.apache.nifi.components.PropertyValue;
import org.apache.nifi.components.ValidationContext;
import org.apache.nifi.components.ValidationResult;
import org.apache.nifi.parameter.ParameterLookup;
import org.apache.nifi.registry.VariableRegistry;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -37,26 +56,6 @@
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import java.io.BufferedInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

public class NotificationServiceManager {
private static final Logger logger = LoggerFactory.getLogger(NotificationServiceManager.class);
private final Map<String, ConfiguredNotificationService> servicesById = new HashMap<>();
Expand Down Expand Up @@ -88,6 +87,27 @@ public void setMaxNotificationAttempts(final int maxAttempts) {
this.maxAttempts = maxAttempts;
}

private static DocumentBuilder createSafeDocumentBuilder() throws ParserConfigurationException {
final DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
docFactory.setNamespaceAware(false);

// These features are used to disable processing external entities in the DocumentBuilderFactory to protect against XXE attacks
final String DISALLOW_DOCTYPES = "http://apache.org/xml/features/disallow-doctype-decl";
final String ALLOW_EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";
final String ALLOW_EXTERNAL_PARAM_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";
final String ALLOW_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd";

// Disable DTDs and external entities to protect against XXE
docFactory.setAttribute(DISALLOW_DOCTYPES, true);
docFactory.setAttribute(ALLOW_EXTERNAL_DTD, false);
docFactory.setAttribute(ALLOW_EXTERNAL_GENERAL_ENTITIES, false);
docFactory.setAttribute(ALLOW_EXTERNAL_PARAM_ENTITIES, false);
docFactory.setXIncludeAware(false);
docFactory.setExpandEntityReferences(false);

return docFactory.newDocumentBuilder();
}

/**
* Loads the Notification Services from the given XML configuration file.
*
Expand Down Expand Up @@ -123,9 +143,7 @@ public void setMaxNotificationAttempts(final int maxAttempts) {
* @throws SAXException if unable to parse the given file properly
*/
public void loadNotificationServices(final File servicesFile) throws IOException, ParserConfigurationException, SAXException {
final DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance();
docBuilderFactory.setNamespaceAware(false);
final DocumentBuilder docBuilder = docBuilderFactory.newDocumentBuilder();
final DocumentBuilder docBuilder = createSafeDocumentBuilder();

final Map<String, ConfiguredNotificationService> serviceMap = new HashMap<>();
try (final InputStream fis = new FileInputStream(servicesFile);
Expand Down
39 changes: 36 additions & 3 deletions nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/xml/XmlUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,28 @@
package org.apache.nifi.security.xml;

import java.io.InputStream;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.parsers.SAXParser;
import javax.xml.parsers.SAXParserFactory;
import javax.xml.stream.XMLInputFactory;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamReader;
import javax.xml.transform.stream.StreamSource;
import javax.xml.validation.Schema;
import org.xml.sax.ContentHandler;
import org.xml.sax.SAXException;
import org.xml.sax.XMLReader;

public class XmlUtils {

// These features are used to disable processing external entities in the DocumentBuilderFactory to protect against XXE attacks
private static final String DISALLOW_DOCTYPES = "http://apache.org/xml/features/disallow-doctype-decl";
private static final String ALLOW_EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";
private static final String ALLOW_EXTERNAL_PARAM_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";
private static final String ALLOW_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd";

public static XMLStreamReader createSafeReader(InputStream inputStream) throws XMLStreamException {
if (inputStream == null) {
throw new IllegalArgumentException("The provided input stream cannot be null");
Expand Down Expand Up @@ -57,13 +66,37 @@ public static XMLReader createSafeSaxReader(SAXParserFactory saxParserFactory, C
throw new IllegalArgumentException("The provided SAX content handler cannot be null");
}

saxParserFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);

SAXParser saxParser = saxParserFactory.newSAXParser();
XMLReader xmlReader = saxParser.getXMLReader();
xmlReader.setFeature(DISALLOW_DOCTYPES, true);
xmlReader.setFeature(ALLOW_EXTERNAL_GENERAL_ENTITIES, false);
xmlReader.setFeature(ALLOW_EXTERNAL_PARAM_ENTITIES, false);
xmlReader.setContentHandler(contentHandler);

return xmlReader;
}

public static DocumentBuilder createSafeDocumentBuilder(Schema schema, boolean isNamespaceAware) throws ParserConfigurationException {
final DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
docFactory.setSchema(schema);
docFactory.setNamespaceAware(isNamespaceAware);

// Disable DTDs and external entities to protect against XXE
docFactory.setAttribute(DISALLOW_DOCTYPES, true);
docFactory.setAttribute(ALLOW_EXTERNAL_DTD, false);
docFactory.setAttribute(ALLOW_EXTERNAL_GENERAL_ENTITIES, false);
docFactory.setAttribute(ALLOW_EXTERNAL_PARAM_ENTITIES, false);
docFactory.setXIncludeAware(false);
docFactory.setExpandEntityReferences(false);

return docFactory.newDocumentBuilder();
}

public static DocumentBuilder createSafeDocumentBuilder(Schema schema) throws ParserConfigurationException {
return createSafeDocumentBuilder(schema, true);
}

public static DocumentBuilder createSafeDocumentBuilder(boolean isNamespaceAware) throws ParserConfigurationException {
return createSafeDocumentBuilder(null, isNamespaceAware);
}
}
21 changes: 20 additions & 1 deletion ...mons/nifi-security-utils/src/test/groovy/org/apache/nifi/security/xml/XmlUtilsTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import org.junit.runner.RunWith
import org.junit.runners.JUnit4
import org.slf4j.Logger
import org.slf4j.LoggerFactory
import org.xml.sax.SAXParseException

import javax.xml.bind.JAXBContext
import javax.xml.bind.UnmarshalException
Expand All @@ -32,6 +33,7 @@ import javax.xml.bind.annotation.XmlAccessType
import javax.xml.bind.annotation.XmlAccessorType
import javax.xml.bind.annotation.XmlAttribute
import javax.xml.bind.annotation.XmlRootElement
import javax.xml.parsers.DocumentBuilder
import javax.xml.stream.XMLStreamReader

import static groovy.test.GroovyAssert.shouldFail
Expand Down Expand Up @@ -77,9 +79,26 @@ class XmlUtilsTest {
logger.expected(msg)
assert msg =~ "XMLStreamException: ParseError "
}

@Test
void testShouldHandleXXEInDocumentBuilder() {
// Arrange
final String XXE_TEMPLATE_FILEPATH = "src/test/resources/local_xxe_file.xml"
DocumentBuilder documentBuilder = XmlUtils.createSafeDocumentBuilder(null)

// Act
def msg = shouldFail(SAXParseException) {
def parsedFlow = documentBuilder.parse(new File(XXE_TEMPLATE_FILEPATH))
logger.info("Parsed ${parsedFlow.toString()}")
}

// Assert
logger.expected(msg)
assert msg =~ "SAXParseException.* DOCTYPE is disallowed when the feature"
}
}

@XmlAccessorType( XmlAccessType.NONE )
@XmlAccessorType(XmlAccessType.NONE)
@XmlRootElement(name = "object")
class XmlObject {
@XmlAttribute
Expand Down
6 changes: 6 additions & 0 deletions nifi-framework-api/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,11 @@
<artifactId>nifi-api</artifactId>
<version>1.12.0-SNAPSHOT</version>
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-security-utils</artifactId>
<version>1.12.0-SNAPSHOT</version>
<scope>provided</scope>
</dependency>
</dependencies>
</project>
34 changes: 16 additions & 18 deletions ...mework-api/src/main/java/org/apache/nifi/authorization/AbstractPolicyBasedAuthorizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,25 @@
*/
package org.apache.nifi.authorization;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.StringWriter;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.util.Set;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.stream.XMLOutputFactory;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamWriter;
import org.apache.nifi.authorization.exception.AuthorizationAccessException;
import org.apache.nifi.authorization.exception.AuthorizerCreationException;
import org.apache.nifi.authorization.exception.AuthorizerDestructionException;
import org.apache.nifi.authorization.exception.UninheritableAuthorizationsException;
import org.apache.nifi.security.xml.XmlUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.w3c.dom.Document;
Expand All @@ -28,29 +43,12 @@
import org.w3c.dom.NodeList;
import org.xml.sax.SAXException;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.stream.XMLOutputFactory;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamWriter;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.StringWriter;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.util.Set;

/**
* An Authorizer that provides management of users, groups, and policies.
*/
public abstract class AbstractPolicyBasedAuthorizer implements ManagedAuthorizer {
private static final Logger logger = LoggerFactory.getLogger(AbstractPolicyBasedAuthorizer.class);

static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY = DocumentBuilderFactory.newInstance();
static final XMLOutputFactory XML_OUTPUT_FACTORY = XMLOutputFactory.newInstance();

static final String USER_ELEMENT = "user";
Expand Down Expand Up @@ -415,7 +413,7 @@ private PoliciesUsersAndGroups parsePoliciesUsersAndGroups(final String fingerpr

final byte[] fingerprintBytes = fingerprint.getBytes(StandardCharsets.UTF_8);
try (final ByteArrayInputStream in = new ByteArrayInputStream(fingerprintBytes)) {
final DocumentBuilder docBuilder = DOCUMENT_BUILDER_FACTORY.newDocumentBuilder();
final DocumentBuilder docBuilder = XmlUtils.createSafeDocumentBuilder(null);
final Document document = docBuilder.parse(in);
final Element rootElement = document.getDocumentElement();

Expand Down
54 changes: 26 additions & 28 deletions ...dle/nifi-evtx-processors/src/test/java/org/apache/nifi/processors/evtx/ParseEvtxTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,30 @@

package org.apache.nifi.processors.evtx;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.isA;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.stream.XMLStreamException;
import org.apache.nifi.flowfile.FlowFile;
import org.apache.nifi.flowfile.attributes.CoreAttributes;
import org.apache.nifi.logging.ComponentLog;
Expand All @@ -28,6 +52,7 @@
import org.apache.nifi.processors.evtx.parser.MalformedChunkException;
import org.apache.nifi.processors.evtx.parser.Record;
import org.apache.nifi.processors.evtx.parser.bxml.RootNode;
import org.apache.nifi.security.xml.XmlUtils;
import org.apache.nifi.util.MockFlowFile;
import org.apache.nifi.util.TestRunner;
import org.apache.nifi.util.TestRunners;
Expand All @@ -42,35 +67,8 @@
import org.w3c.dom.NodeList;
import org.xml.sax.SAXException;

import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.stream.XMLStreamException;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.isA;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

@RunWith(MockitoJUnitRunner.class)
public class ParseEvtxTest {
public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY = DocumentBuilderFactory.newInstance();
public static final String USER_DATA = "UserData";
public static final String EVENT_DATA = "EventData";
public static final Set DATA_TAGS = new HashSet<>(Arrays.asList(EVENT_DATA, USER_DATA));
Expand Down Expand Up @@ -477,7 +475,7 @@ private int validateFlowFiles(List<MockFlowFile> successFlowFiles) throws SAXExc
int totalSize = 0;
for (MockFlowFile successFlowFile : successFlowFiles) {
// Verify valid XML output
Document document = DOCUMENT_BUILDER_FACTORY.newDocumentBuilder().parse(new ByteArrayInputStream(successFlowFile.toByteArray()));
Document document = XmlUtils.createSafeDocumentBuilder(false).parse(new ByteArrayInputStream(successFlowFile.toByteArray()));
Element documentElement = document.getDocumentElement();
assertEquals(XmlRootNodeHandler.EVENTS, documentElement.getTagName());
NodeList eventNodes = documentElement.getChildNodes();
Expand Down
Loading

0 comments on commit 7f0416e

Please sign in to comment.