diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java index 02d916f29c..0468e4aee8 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java @@ -306,13 +306,17 @@ public void setAliasService(AliasService as) { public void deployTopology(Topology t){ try { + File topology = new File(topologiesDirectory.getAbsolutePath() + "/" + t.getName() + ".xml"); + if (!topology.getCanonicalFile().toPath().startsWith(topologiesDirectory.getCanonicalFile().toPath())) { + throw new IOException("Resolved topology path escapes managed directory: " + t.getName()); + } + File temp = new File(topologiesDirectory.getAbsolutePath() + "/" + t.getName() + ".xml.temp"); Marshaller mr = jaxbContext.createMarshaller(); mr.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true); mr.marshal(t, temp); - File topology = new File(topologiesDirectory.getAbsolutePath() + "/" + t.getName() + ".xml"); if(!temp.renameTo(topology)) { FileUtils.forceDelete(temp); throw new IOException("Could not rename temp file"); @@ -716,8 +720,15 @@ private static boolean writeConfig(File dest, String name, String content) { File destFile = new File(dest, name); try { - FileUtils.writeStringToFile(destFile, content, StandardCharsets.UTF_8); - log.wroteConfigurationFile(destFile.getAbsolutePath()); + final File canonicalDest = dest.getCanonicalFile(); + final File canonicalFile = destFile.getCanonicalFile(); + if (!canonicalFile.toPath().startsWith(canonicalDest.toPath())) { + log.failedToWriteConfigurationFile(destFile.getAbsolutePath(), + new IOException("Resolved path escapes managed directory: " + name)); + return false; + } + FileUtils.writeStringToFile(canonicalFile, content, StandardCharsets.UTF_8); + log.wroteConfigurationFile(canonicalFile.getAbsolutePath()); result = true; } catch (IOException e) { log.failedToWriteConfigurationFile(destFile.getAbsolutePath(), e); diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/topology/monitor/ZkRemoteConfigurationMonitorService.java b/gateway-server/src/main/java/org/apache/knox/gateway/topology/monitor/ZkRemoteConfigurationMonitorService.java index 0d8e34c57c..2c6d89a389 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/topology/monitor/ZkRemoteConfigurationMonitorService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/topology/monitor/ZkRemoteConfigurationMonitorService.java @@ -212,14 +212,14 @@ public void stop() throws ServiceLifecycleException { @Override public boolean createProvider(String name, String content) { - String entryPath = "/knox/config/shared-providers/" + name; + String entryPath = "/knox/config/shared-providers/" + FilenameUtils.getName(name); client.createEntry(entryPath, content); return (client.getEntryData(entryPath) != null); } @Override public boolean createDescriptor(String name, String content) { - String entryPath = "/knox/config/descriptors/" + name; + String entryPath = "/knox/config/descriptors/" + FilenameUtils.getName(name); client.createEntry(entryPath, content); return (client.getEntryData(entryPath) != null); } diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java index bf7cf976c0..e428d1e84f 100644 --- a/gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java +++ b/gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java @@ -595,6 +595,45 @@ public void testConfigurationCRUDAPI() throws Exception { } } + @Test + public void testDeployRejectsPathTraversal() throws Exception { + File dir = createDir(); + File topologyDir = new File(dir, "topologies"); + topologyDir.mkdirs(); + + File descriptorsDir = new File(dir, "descriptors"); + descriptorsDir.mkdirs(); + + File sharedProvidersDir = new File(dir, "shared-providers"); + sharedProvidersDir.mkdirs(); + + try { + TopologyService ts = new DefaultTopologyService(); + Map c = new HashMap<>(); + + GatewayConfig config = EasyMock.createNiceMock(GatewayConfig.class); + EasyMock.expect(config.getReadOnlyOverrideTopologyNames()).andReturn(Collections.emptyList()).anyTimes(); + EasyMock.expect(config.getGatewayTopologyDir()).andReturn(topologyDir.getAbsolutePath()).anyTimes(); + EasyMock.expect(config.getGatewayConfDir()).andReturn(descriptorsDir.getParentFile().getAbsolutePath()).anyTimes(); + EasyMock.replay(config); + + ts.init(config, c); + + final String traversalName = "../../evil.json"; + final File escapeTarget = new File(dir.getParentFile(), "evil.json"); + assertFalse("precondition: escape target must not pre-exist", escapeTarget.exists()); + + assertFalse("deployProviderConfiguration must reject a path-traversal name", + ts.deployProviderConfiguration(traversalName, "malicious")); + assertFalse("deployDescriptor must reject a path-traversal name", + ts.deployDescriptor(traversalName, "malicious")); + + assertFalse("no file may be written outside the managed directory", escapeTarget.exists()); + } finally { + FileUtils.deleteQuietly(dir); + } + } + @Test public void testProviderParamsOrderIsPreserved() { diff --git a/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologiesResource.java b/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologiesResource.java index 2c66faafd2..98279a5798 100644 --- a/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologiesResource.java +++ b/gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologiesResource.java @@ -43,6 +43,7 @@ import javax.ws.rs.PathParam; import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; +import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.Context; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; @@ -55,8 +56,6 @@ import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; -import java.net.URLDecoder; -import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collection; import java.util.Comparator; @@ -90,7 +89,7 @@ public class TopologiesResource { private static final String SINGLE_DESCRIPTOR_API_PATH = DESCRIPTORS_API_PATH + "/{name}"; private static final int RESOURCE_NAME_LENGTH_MAX = 100; - private static final Pattern RESOURCE_NAME_PATTERN = Pattern.compile("^[\\w-/.]+$"); + private static final Pattern RESOURCE_NAME_PATTERN = Pattern.compile("^[\\w.-]+$"); private static GatewaySpiMessages log = MessagesFactory.get(GatewaySpiMessages.class); @@ -171,12 +170,6 @@ public SimpleTopologyWrapper getTopologies() { public Topology uploadTopology(@PathParam("id") String id, Topology t) { Topology result = null; - try { - id = URLDecoder.decode(id, StandardCharsets.UTF_8.name()); - } catch (Exception e) { - // Ignore - } - if (!isValidResourceName(id)) { log.invalidResourceName(id); throw new BadRequestException("Invalid topology name: " + id); @@ -188,6 +181,17 @@ public Topology uploadTopology(@PathParam("id") String id, Topology t) { t.setName(id); TopologyService ts = gs.getService(ServiceType.TOPOLOGY_SERVICE); + GatewayConfig config = + (GatewayConfig) request.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE); + if (config != null && + config.getReadOnlyOverrideTopologyNames().contains(FilenameUtils.getBaseName(id))) { + log.disallowedOverwritingReadOnlyTopology(id); + throw new WebApplicationException( + status(Response.Status.FORBIDDEN) + .entity("{ \"error\" : \"Cannot overwrite read-only topology: " + id + "\" }") + .build()); + } + // Check for existing topology with the same name, to see if it had been generated boolean existingGenerated = false; for (org.apache.knox.gateway.topology.Topology existingTopology : ts.getTopologies()) { @@ -338,12 +342,6 @@ public Response deleteSimpleDescriptor(@PathParam("name") String name) { public Response uploadProviderConfiguration(@PathParam("name") String name, @Context HttpHeaders headers, String content) { Response response = null; - try { - name = URLDecoder.decode(name, StandardCharsets.UTF_8.name()); - } catch (Exception e) { - // Ignore - } - if (!isValidResourceName(name)) { log.invalidResourceName(name); throw new BadRequestException("Invalid provider configuration name: " + name); @@ -393,12 +391,6 @@ public Response uploadSimpleDescriptor(@PathParam("name") String name, String content) { Response response = null; - try { - name = URLDecoder.decode(name, StandardCharsets.UTF_8.name()); - } catch (Exception e) { - // Ignore - } - if (!isValidResourceName(name)) { log.invalidResourceName(name); throw new BadRequestException("Invalid descriptor name: " + name); @@ -554,8 +546,9 @@ private File getExistingConfigFile(Collection existing, String candidateNa return result; } - private static boolean isValidResourceName(final String name) { + static boolean isValidResourceName(final String name) { return name != null && name.length() <= RESOURCE_NAME_LENGTH_MAX && + !name.contains("..") && RESOURCE_NAME_PATTERN.matcher(name).matches(); } diff --git a/gateway-service-admin/src/test/java/org/apache/knox/gateway/service/admin/TopologyResourceTest.java b/gateway-service-admin/src/test/java/org/apache/knox/gateway/service/admin/TopologyResourceTest.java index e49015e1f9..d0e225ed13 100644 --- a/gateway-service-admin/src/test/java/org/apache/knox/gateway/service/admin/TopologyResourceTest.java +++ b/gateway-service-admin/src/test/java/org/apache/knox/gateway/service/admin/TopologyResourceTest.java @@ -19,7 +19,18 @@ import org.apache.knox.gateway.topology.Topology; import org.apache.knox.gateway.config.GatewayConfig; +import org.apache.knox.gateway.services.GatewayServices; +import org.apache.knox.gateway.services.ServiceType; +import org.apache.knox.gateway.services.topology.TopologyService; + +import javax.servlet.ServletContext; import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.core.Response; + +import java.lang.reflect.Field; +import java.util.Collections; +import java.util.List; import org.easymock.EasyMock; import org.junit.Test; @@ -27,6 +38,9 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; public class TopologyResourceTest { @@ -174,6 +188,28 @@ public void testTopologyURLMethods(){ } + @Test + public void testResourceNameValidation() { + assertTrue(TopologiesResource.isValidResourceName("foo")); + assertTrue(TopologiesResource.isValidResourceName("foo.json")); + assertTrue(TopologiesResource.isValidResourceName("my-provider_1")); + assertTrue(TopologiesResource.isValidResourceName("a.b.c")); + + assertFalse(TopologiesResource.isValidResourceName("../../etc/passwd")); + assertFalse(TopologiesResource.isValidResourceName("..")); + assertFalse(TopologiesResource.isValidResourceName("foo/bar")); + assertFalse(TopologiesResource.isValidResourceName("a/../../b")); + assertFalse(TopologiesResource.isValidResourceName("foo..bar")); + + assertFalse(TopologiesResource.isValidResourceName("%2f")); + assertFalse(TopologiesResource.isValidResourceName("%252f")); + + assertFalse(TopologiesResource.isValidResourceName(null)); + assertFalse(TopologiesResource.isValidResourceName("")); + assertFalse(TopologiesResource.isValidResourceName("a".repeat(101))); + assertTrue(TopologiesResource.isValidResourceName("a".repeat(100))); + } + private void setDefaultExpectations(HttpServletRequest request){ EasyMock.expect( request.getPathInfo() ).andReturn( pathInfo ).anyTimes(); EasyMock.expect( request.getContextPath() ).andReturn( reqContext ).anyTimes(); @@ -186,4 +222,76 @@ private void setMockRequestHeader(HttpServletRequest request, String header, Str EasyMock.expect( request.getHeader( header ) ).andReturn( expected ).anyTimes(); } + @Test + public void testUploadTopologyRefusesReadOnlyOverride() throws Exception { + TopologyService ts = EasyMock.createMock(TopologyService.class); + + GatewayServices gs = EasyMock.createNiceMock(GatewayServices.class); + EasyMock.expect(gs.getService(ServiceType.TOPOLOGY_SERVICE)).andReturn(ts).anyTimes(); + + GatewayConfig config = EasyMock.createNiceMock(GatewayConfig.class); + EasyMock.expect(config.getReadOnlyOverrideTopologyNames()) + .andReturn(List.of("manager")).anyTimes(); + + HttpServletRequest request = mockRequest(gs, config); + + EasyMock.replay(ts, gs, config, request); + + TopologiesResource res = new TopologiesResource(); + setRequestField(res, request); + + try { + res.uploadTopology("manager", new org.apache.knox.gateway.service.admin.beans.Topology()); + fail("Expected WebApplicationException for read-only override topology"); + } catch (WebApplicationException e) { + assertEquals(Response.Status.FORBIDDEN.getStatusCode(), e.getResponse().getStatus()); + } + + EasyMock.verify(ts); + } + + @Test + public void testUploadTopologyAllowedWhenNotReadOnly() throws Exception { + TopologyService ts = EasyMock.createMock(TopologyService.class); + EasyMock.expect(ts.getTopologies()).andReturn(Collections.emptyList()).anyTimes(); + ts.deployTopology(EasyMock.anyObject(Topology.class)); + EasyMock.expectLastCall().once(); + + GatewayServices gs = EasyMock.createNiceMock(GatewayServices.class); + EasyMock.expect(gs.getService(ServiceType.TOPOLOGY_SERVICE)).andReturn(ts).anyTimes(); + + GatewayConfig config = EasyMock.createNiceMock(GatewayConfig.class); + EasyMock.expect(config.getReadOnlyOverrideTopologyNames()) + .andReturn(Collections.emptyList()).anyTimes(); + + HttpServletRequest request = mockRequest(gs, config); + + EasyMock.replay(ts, gs, config, request); + + TopologiesResource res = new TopologiesResource(); + setRequestField(res, request); + + res.uploadTopology("sandbox", new org.apache.knox.gateway.service.admin.beans.Topology()); + + EasyMock.verify(ts); + } + + private HttpServletRequest mockRequest(GatewayServices gs, GatewayConfig config) { + ServletContext context = EasyMock.createNiceMock(ServletContext.class); + EasyMock.expect(context.getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE)) + .andReturn(gs).anyTimes(); + EasyMock.expect(context.getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE)) + .andReturn(config).anyTimes(); + HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class); + EasyMock.expect(request.getServletContext()).andReturn(context).anyTimes(); + EasyMock.replay(context); + return request; + } + + private void setRequestField(TopologiesResource res, HttpServletRequest request) throws Exception { + Field f = TopologiesResource.class.getDeclaredField("request"); + f.setAccessible(true); + f.set(res, request); + } + } diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/i18n/GatewaySpiMessages.java b/gateway-spi/src/main/java/org/apache/knox/gateway/i18n/GatewaySpiMessages.java index 7217499706..a067af3b53 100644 --- a/gateway-spi/src/main/java/org/apache/knox/gateway/i18n/GatewaySpiMessages.java +++ b/gateway-spi/src/main/java/org/apache/knox/gateway/i18n/GatewaySpiMessages.java @@ -76,6 +76,9 @@ public interface GatewaySpiMessages { @Message( level = MessageLevel.ERROR, text = "Topology {0} cannot be manually overwritten because it was generated from a simple descriptor." ) void disallowedOverwritingGeneratedTopology(String topologyName); + @Message( level = MessageLevel.INFO, text = "Read-only topology {0} cannot be overwritten." ) + void disallowedOverwritingReadOnlyTopology(String topologyName); + @Message( level = MessageLevel.INFO, text = "Read-only descriptor {0} cannot be overwritten." ) void disallowedOverwritingGeneratedDescriptor(String name); 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 4928ed920b..417ab5949a 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 @@ -1806,7 +1806,8 @@ public void testPutDescriptorWithValidEncodedName() throws Exception { String newDescriptorJSON = createDescriptor(clusterName); // Attempt to PUT the descriptor - given().auth().preemptive().basic(username, password) + given().urlEncodingEnabled(false) + .auth().preemptive().basic(username, password) .header("Content-type", MediaType.APPLICATION_JSON) .body(newDescriptorJSON.getBytes(StandardCharsets.UTF_8.name())) .then()