KNOX-1308 - Implement safeguards against XML entity injection/expansion in the Admin API
Project: http://git-wip-us.apache.org/repos/asf/knox/repo Commit: http://git-wip-us.apache.org/repos/asf/knox/commit/7439e696 Tree: http://git-wip-us.apache.org/repos/asf/knox/tree/7439e696 Diff: http://git-wip-us.apache.org/repos/asf/knox/diff/7439e696 Branch: refs/heads/master Commit: 7439e6968d5ad9241ca242b04248080ea7e8c996 Parents: b83a8bc Author: Phil Zampino <[email protected]> Authored: Mon May 14 14:12:59 2018 -0400 Committer: Phil Zampino <[email protected]> Committed: Mon May 14 14:12:59 2018 -0400 ---------------------------------------------------------------------- .../service/admin/TopologyMarshaller.java | 47 ++++-- .../gateway/GatewayAdminTopologyFuncTest.java | 165 +++++++++++++++++++ 2 files changed, 202 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/knox/blob/7439e696/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologyMarshaller.java ---------------------------------------------------------------------- diff --git a/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologyMarshaller.java b/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologyMarshaller.java index 6d25982..dd95af9 100644 --- a/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologyMarshaller.java +++ b/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologyMarshaller.java @@ -40,6 +40,10 @@ import javax.xml.bind.JAXBContext; import javax.xml.bind.JAXBException; import javax.xml.bind.Unmarshaller; import javax.xml.bind.Marshaller; +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.XMLStreamReader; +import javax.xml.transform.stream.StreamSource; @Provider @Consumes({MediaType.APPLICATION_XML, MediaType.APPLICATION_JSON}) @@ -57,7 +61,13 @@ public class TopologyMarshaller implements MessageBodyWriter<Topology>, MessageB } @Override - public void writeTo(Topology instance, Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType, MultivaluedMap<String, Object> httpHeaders, OutputStream entityStream) throws IOException, WebApplicationException { + public void writeTo(Topology instance, + Class<?> type, + Type genericType, + Annotation[] annotations, + MediaType mediaType, + MultivaluedMap<String, Object> httpHeaders, + OutputStream entityStream) throws IOException, WebApplicationException { try { Map<String, Object> properties = new HashMap<>(1); properties.put( JAXBContextProperties.MEDIA_TYPE, mediaType.toString()); @@ -75,26 +85,43 @@ public class TopologyMarshaller implements MessageBodyWriter<Topology>, MessageB /// @Override public boolean isReadable(Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType) { - boolean readable = (type == Topology.class); - return readable; + return (type == Topology.class); } @Override - public Topology readFrom(Class<Topology> type, Type genericType, Annotation[] annotations, MediaType mediaType, MultivaluedMap<String, String> httpHeaders, InputStream entityStream) throws IOException, WebApplicationException { + public Topology readFrom(Class<Topology> type, + Type genericType, + Annotation[] annotations, + MediaType mediaType, + MultivaluedMap<String, String> httpHeaders, + InputStream entityStream) throws IOException, WebApplicationException { + Topology topology = null; + try { - if(isReadable(type, genericType, annotations, mediaType)) { + if (isReadable(type, genericType, annotations, mediaType)) { Map<String, Object> properties = Collections.emptyMap(); JAXBContext context = JAXBContext.newInstance(new Class[]{Topology.class}, properties); - InputStream is = entityStream; + Unmarshaller u = context.createUnmarshaller(); u.setProperty(UnmarshallerProperties.MEDIA_TYPE, mediaType.getType() + "/" + mediaType.getSubtype()); - Topology topology = (Topology)u.unmarshal(is); - return topology; + + if (mediaType.isCompatible(MediaType.APPLICATION_XML_TYPE)) { + // Safeguard against entity injection (KNOX-1308) + XMLInputFactory xif = XMLInputFactory.newFactory(); + xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + xif.setProperty(XMLInputFactory.SUPPORT_DTD, false); + xif.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false); + XMLStreamReader xsr = xif.createXMLStreamReader(new StreamSource(entityStream)); + topology = (Topology) u.unmarshal(xsr); + } else { + topology = (Topology) u.unmarshal(entityStream); + } } - } catch (JAXBException e) { + } catch (XMLStreamException | JAXBException e) { throw new IOException(e); } - return null; + + return topology; } } http://git-wip-us.apache.org/repos/asf/knox/blob/7439e696/gateway-test/src/test/java/org/apache/knox/gateway/GatewayAdminTopologyFuncTest.java ---------------------------------------------------------------------- diff --git a/gateway-test/src/test/java/org/apache/knox/gateway/GatewayAdminTopologyFuncTest.java b/gateway-test/src/test/java/org/apache/knox/gateway/GatewayAdminTopologyFuncTest.java index ae9371d..b7eefb1 100644 --- a/gateway-test/src/test/java/org/apache/knox/gateway/GatewayAdminTopologyFuncTest.java +++ b/gateway-test/src/test/java/org/apache/knox/gateway/GatewayAdminTopologyFuncTest.java @@ -72,6 +72,7 @@ import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.xml.HasXPath.hasXPath; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; @@ -732,6 +733,170 @@ public class GatewayAdminTopologyFuncTest { } @Test( timeout = TestUtils.LONG_TIMEOUT ) + public void testPutTopologyWithEntityInjection() throws Exception { + LOG_ENTER() ; + + final String MALICIOUS_PARAM_NAME = "exposed"; + + final String XML_WITH_INJECTION = + "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" + + "<!DOCTYPE foo [<!ENTITY xxeiltvf SYSTEM \"file:///etc/hosts\"> ]>\n" + + "<topology>\n" + + " <gateway>\n" + + " <provider>\n" + + " <role>authentication</role>\n" + + " <name>ShiroProvider</name>\n" + + " <enabled>true</enabled>\n" + + " <param>\n" + + " <name>" + MALICIOUS_PARAM_NAME + "</name>\n" + + " <value>&xxeiltvf;</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>sessionTimeout</name>\n" + + " <value>30</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm</name>\n" + + " <value>org.apache.knox.gateway.shirorealm.KnoxLdapRealm</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapContextFactory</name>\n" + + " <value>org.apache.knox.gateway.shirorealm.KnoxLdapContextFactory</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm.contextFactory</name>\n" + + " <value>$ldapContextFactory</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm.userDnTemplate</name>\n" + + " <value>uid={0},ou=people,dc=hadoop,dc=apache,dc=org</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm.contextFactory.url</name>\n" + + " <value>ldap://localhost:33389</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm.contextFactory.authenticationMechanism</name>\n" + + " <value>simple</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>urls./**</name>\n" + + " <value>authcBasic</value>\n" + + " </param>\n" + + " </provider>\n" + + " </gateway>\n" + + " <service>\n" + + " <role>NAMENODE</role>\n" + + " <url>hdfs://localhost:8020</url>\n" + + " </service>\n" + + "</topology>"; + + String username = "admin"; + String password = "admin-password"; + String url = clusterUrl + "/api/v1/topologies/test-put-with-entity-injection"; + + // Should get a response with missing entity reference value because of the entity injection prevention safeguard + String XML_RESPONSE = given().auth().preemptive().basic(username, password) + .contentType(MediaType.APPLICATION_XML) + .header("Accept", MediaType.APPLICATION_XML) + .body(XML_WITH_INJECTION) + .then() + .statusCode(HttpStatus.SC_OK) + .when().put(url).getBody().asString(); + + Document doc = XmlUtils.readXml(new InputSource(new StringReader(XML_RESPONSE))); + assertNotNull(doc); + + assertThat(doc, hasXPath("/topology/gateway/provider[1]/param/name", containsString("exposed"))); + assertThat(doc, hasXPath("/topology/gateway/provider[1]/param[\"" + MALICIOUS_PARAM_NAME + "\"]/value", is(""))); + + LOG_EXIT(); + } + + + @Test( timeout = TestUtils.LONG_TIMEOUT ) + public void testPutTopologyWithEntityExpansion() throws Exception { + LOG_ENTER() ; + + final String MALICIOUS_PARAM_NAME = "expanded"; + + final String XML_WITH_INJECTION = + "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" + + "<!DOCTYPE foo [<!ENTITY xeevowya0 \"b68et\"><!ENTITY xeevowya1 \"&xeevowya0;&xeevowya0;\"><!ENTITY xeevowya2 \"&xeevowya1;&xeevowya1;\"><!ENTITY xeevowya3 \"&xeevowya2;&xeevowya2;\">]>\n" + + "<topology>\n" + + " <gateway>\n" + + " <provider>\n" + + " <role>authentication</role>\n" + + " <name>ShiroProvider</name>\n" + + " <enabled>true</enabled>\n" + + " <param>\n" + + " <name>" + MALICIOUS_PARAM_NAME + "</name>\n" + + " <value>&xeevowya3;</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>sessionTimeout</name>\n" + + " <value>30</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm</name>\n" + + " <value>org.apache.knox.gateway.shirorealm.KnoxLdapRealm</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapContextFactory</name>\n" + + " <value>org.apache.knox.gateway.shirorealm.KnoxLdapContextFactory</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm.contextFactory</name>\n" + + " <value>$ldapContextFactory</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm.userDnTemplate</name>\n" + + " <value>uid={0},ou=people,dc=hadoop,dc=apache,dc=org</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm.contextFactory.url</name>\n" + + " <value>ldap://localhost:33389</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>main.ldapRealm.contextFactory.authenticationMechanism</name>\n" + + " <value>simple</value>\n" + + " </param>\n" + + " <param>\n" + + " <name>urls./**</name>\n" + + " <value>authcBasic</value>\n" + + " </param>\n" + + " </provider>\n" + + " </gateway>\n" + + " <service>\n" + + " <role>NAMENODE</role>\n" + + " <url>hdfs://localhost:8020</url>\n" + + " </service>\n" + + "</topology>"; + + String username = "admin"; + String password = "admin-password"; + String url = clusterUrl + "/api/v1/topologies/test-put-with-entity-injection"; + + // Should get a HTTP 500 response because of the entity injection prevention safeguard + String XML_RESPONSE = given().auth().preemptive().basic(username, password) + .contentType(MediaType.APPLICATION_XML) + .header("Accept", MediaType.APPLICATION_XML) + .body(XML_WITH_INJECTION) + .then() + .statusCode(HttpStatus.SC_OK) + .when().put(url).getBody().asString(); + + Document doc = XmlUtils.readXml(new InputSource(new StringReader(XML_RESPONSE))); + assertNotNull(doc); + + assertThat(doc, hasXPath("/topology/gateway/provider[1]/param/name", containsString("expanded"))); + assertThat(doc, hasXPath("/topology/gateway/provider[1]/param[\"" + MALICIOUS_PARAM_NAME + "\"]/value", is(""))); + + LOG_EXIT(); + } + + + @Test( timeout = TestUtils.LONG_TIMEOUT ) public void testXForwardedHeaders() { LOG_ENTER();
