From aec4092cc718b61998c1de221c9c728f377cd430 Mon Sep 17 00:00:00 2001 From: Lachlan Date: Thu, 13 May 2021 01:13:30 +1000 Subject: [PATCH] Fixes #6263 - Review URI encoding in ConcatServlet & WelcomeFilter. Review URI encoding in ConcatServlet & WelcomeFilter and improve testing. Signed-off-by: Lachlan Roberts Signed-off-by: Simone Bordet Co-authored-by: Simone Bordet --- .../eclipse/jetty/server/ResourceService.java | 4 +- .../eclipse/jetty/servlets/ConcatServlet.java | 4 +- .../eclipse/jetty/servlets/WelcomeFilter.java | 8 +- .../jetty/servlets/ConcatServletTest.java | 83 ++++++---- .../jetty/servlets/WelcomeFilterTest.java | 143 ++++++++++++++++++ .../webapp/WebAppDefaultServletTest.java | 142 +++++++++++++++++ 6 files changed, 353 insertions(+), 31 deletions(-) create mode 100644 jetty-servlets/src/test/java/org/eclipse/jetty/servlets/WelcomeFilterTest.java create mode 100644 jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppDefaultServletTest.java diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java index 3d3f05c..5ce24b4 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java @@ -236,7 +236,7 @@ public class ResourceService // Find the content content=_contentFactory.getContent(pathInContext,response.getBufferSize()); if (LOG.isDebugEnabled()) - LOG.info("content={}",content); + LOG.debug("content={}", content); // Not found? if (content==null || !content.getResource().exists()) @@ -422,7 +422,7 @@ public class ResourceService return; } - RequestDispatcher dispatcher=context.getRequestDispatcher(welcome); + RequestDispatcher dispatcher = context.getRequestDispatcher(URIUtil.encodePath(welcome)); if (dispatcher!=null) { // Forward to the index diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/ConcatServlet.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/ConcatServlet.java index a4b7df0..f1d8e57 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/ConcatServlet.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/ConcatServlet.java @@ -62,6 +62,7 @@ import org.eclipse.jetty.util.URIUtil; * appropriate. This means that when not in development mode, the servlet must be * restarted before changed content will be served.

*/ +@Deprecated public class ConcatServlet extends HttpServlet { private boolean _development; @@ -126,7 +127,8 @@ public class ConcatServlet extends HttpServlet } } - RequestDispatcher dispatcher = getServletContext().getRequestDispatcher(path); + // Use the original string and not the decoded path as the Dispatcher will decode again. + RequestDispatcher dispatcher = getServletContext().getRequestDispatcher(part); if (dispatcher != null) dispatchers.add(dispatcher); } diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/WelcomeFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/WelcomeFilter.java index e67a067..492a8ca 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/WelcomeFilter.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/WelcomeFilter.java @@ -28,6 +28,8 @@ import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import org.eclipse.jetty.util.URIUtil; + /* ------------------------------------------------------------ */ /** Welcome Filter * This filter can be used to server an index file for a directory @@ -42,6 +44,7 @@ import javax.servlet.http.HttpServletRequest; * * Requests to "/some/directory" will be redirected to "/some/directory/". */ +@Deprecated public class WelcomeFilter implements Filter { private String welcome; @@ -63,7 +66,10 @@ public class WelcomeFilter implements Filter { String path=((HttpServletRequest)request).getServletPath(); if (welcome!=null && path.endsWith("/")) - request.getRequestDispatcher(path+welcome).forward(request,response); + { + String uriInContext = URIUtil.encodePath(URIUtil.addPaths(path, welcome)); + request.getRequestDispatcher(uriInContext).forward(request, response); + } else chain.doFilter(request, response); } diff --git a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/ConcatServletTest.java b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/ConcatServletTest.java index 3fcb9af..b815b35 100644 --- a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/ConcatServletTest.java +++ b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/ConcatServletTest.java @@ -32,6 +32,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.stream.Stream; import javax.servlet.RequestDispatcher; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; @@ -48,7 +49,12 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.startsWith; public class ConcatServletTest { private Server server; @@ -114,7 +120,7 @@ public class ConcatServletTest } @Test - public void testWEBINFResourceIsNotServed() throws Exception + public void testDirectoryNotAccessible() throws Exception { File directoryFile = MavenTestingUtils.getTargetTestingDir(); Path directoryPath = directoryFile.toPath(); @@ -136,9 +142,8 @@ public class ConcatServletTest // Verify that I can get the file programmatically, as required by the spec. assertNotNull(context.getServletContext().getResource("/WEB-INF/one.js")); - // Having a path segment and then ".." triggers a special case - // that the ConcatServlet must detect and avoid. - String uri = contextPath + concatPath + "?/trick/../WEB-INF/one.js"; + // Make sure ConcatServlet cannot see file system files. + String uri = contextPath + concatPath + "?/trick/../../" + directoryFile.getName(); String request = "" + "GET " + uri + " HTTP/1.1\r\n" + "Host: localhost\r\n" + @@ -146,35 +151,59 @@ public class ConcatServletTest "\r\n"; String response = connector.getResponse(request); assertTrue(response.startsWith("HTTP/1.1 404 ")); + } - // Make sure ConcatServlet behaves well if it's case insensitive. - uri = contextPath + concatPath + "?/trick/../web-inf/one.js"; - request = "" + - "GET " + uri + " HTTP/1.1\r\n" + - "Host: localhost\r\n" + - "Connection: close\r\n" + - "\r\n"; - response = connector.getResponse(request); - assertTrue(response.startsWith("HTTP/1.1 404 ")); + public static Stream webInfTestExamples() + { + return Stream.of( + // Cannot access WEB-INF. + Arguments.of("?/WEB-INF/", "HTTP/1.1 404 "), + Arguments.of("?/WEB-INF/one.js", "HTTP/1.1 404 "), + + // Having a path segment and then ".." triggers a special case that the ConcatServlet must detect and avoid. + Arguments.of("?/trick/../WEB-INF/one.js", "HTTP/1.1 404 "), + + // Make sure ConcatServlet behaves well if it's case insensitive. + Arguments.of("?/trick/../web-inf/one.js", "HTTP/1.1 404 "), + + // Make sure ConcatServlet behaves well if encoded. + Arguments.of("?/trick/..%2FWEB-INF%2Fone.js", "HTTP/1.1 404 "), + Arguments.of("?/%2557EB-INF/one.js", "HTTP/1.1 500 "), + Arguments.of("?/js/%252e%252e/WEB-INF/one.js", "HTTP/1.1 500 ") + ); + } - // Make sure ConcatServlet behaves well if encoded. - uri = contextPath + concatPath + "?/trick/..%2FWEB-INF%2Fone.js"; - request = "" + - "GET " + uri + " HTTP/1.1\r\n" + - "Host: localhost\r\n" + - "Connection: close\r\n" + - "\r\n"; - response = connector.getResponse(request); - assertTrue(response.startsWith("HTTP/1.1 404 ")); + @ParameterizedTest + @MethodSource("webInfTestExamples") + public void testWEBINFResourceIsNotServed(String querystring, String expectedStatus) throws Exception + { + File directoryFile = MavenTestingUtils.getTargetTestingDir(); + Path directoryPath = directoryFile.toPath(); + Path hiddenDirectory = directoryPath.resolve("WEB-INF"); + Files.createDirectories(hiddenDirectory); + Path hiddenResource = hiddenDirectory.resolve("one.js"); + try (OutputStream output = Files.newOutputStream(hiddenResource)) + { + output.write("function() {}".getBytes(StandardCharsets.UTF_8)); + } + + String contextPath = ""; + WebAppContext context = new WebAppContext(server, directoryPath.toString(), contextPath); + server.setHandler(context); + String concatPath = "/concat"; + context.addServlet(ConcatServlet.class, concatPath); + server.start(); - // Make sure ConcatServlet cannot see file system files. - uri = contextPath + concatPath + "?/trick/../../" + directoryFile.getName(); - request = "" + + // Verify that I can get the file programmatically, as required by the spec. + assertNotNull(context.getServletContext().getResource("/WEB-INF/one.js")); + + String uri = contextPath + concatPath + querystring; + String request = "GET " + uri + " HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: close\r\n" + "\r\n"; - response = connector.getResponse(request); - assertTrue(response.startsWith("HTTP/1.1 404 ")); + String response = connector.getResponse(request); + assertThat(response, startsWith(expectedStatus)); } } diff --git a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/WelcomeFilterTest.java b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/WelcomeFilterTest.java new file mode 100644 index 0000000..65e6503 --- /dev/null +++ b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/WelcomeFilterTest.java @@ -0,0 +1,143 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.servlets; + +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.EnumSet; +import java.util.stream.Stream; +import javax.servlet.DispatcherType; + +import org.eclipse.jetty.server.LocalConnector; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.servlet.FilterHolder; +import org.eclipse.jetty.toolchain.test.MavenTestingUtils; +import org.eclipse.jetty.webapp.WebAppContext; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +public class WelcomeFilterTest +{ + private Server server; + private LocalConnector connector; + + @BeforeEach + public void prepareServer() throws Exception + { + server = new Server(); + connector = new LocalConnector(server); + server.addConnector(connector); + + Path directoryPath = MavenTestingUtils.getTargetTestingDir().toPath(); + Files.createDirectories(directoryPath); + Path welcomeResource = directoryPath.resolve("welcome.html"); + try (OutputStream output = Files.newOutputStream(welcomeResource)) + { + output.write("

welcome page

".getBytes(StandardCharsets.UTF_8)); + } + + Path otherResource = directoryPath.resolve("other.html"); + try (OutputStream output = Files.newOutputStream(otherResource)) + { + output.write("

other resource

".getBytes(StandardCharsets.UTF_8)); + } + + Path hiddenDirectory = directoryPath.resolve("WEB-INF"); + Files.createDirectories(hiddenDirectory); + Path hiddenResource = hiddenDirectory.resolve("one.js"); + try (OutputStream output = Files.newOutputStream(hiddenResource)) + { + output.write("CONFIDENTIAL".getBytes(StandardCharsets.UTF_8)); + } + + Path hiddenWelcome = hiddenDirectory.resolve("index.html"); + try (OutputStream output = Files.newOutputStream(hiddenWelcome)) + { + output.write("CONFIDENTIAL".getBytes(StandardCharsets.UTF_8)); + } + + WebAppContext context = new WebAppContext(server, directoryPath.toString(), "/"); + server.setHandler(context); + String concatPath = "/*"; + + FilterHolder filterHolder = new FilterHolder(new WelcomeFilter()); + filterHolder.setInitParameter("welcome", "welcome.html"); + context.addFilter(filterHolder, concatPath, EnumSet.of(DispatcherType.REQUEST)); + server.start(); + + // Verify that I can get the file programmatically, as required by the spec. + assertNotNull(context.getServletContext().getResource("/WEB-INF/one.js")); + } + + @AfterEach + public void destroy() throws Exception + { + if (server != null) + server.stop(); + } + + public static Stream argumentsStream() + { + return Stream.of( + // Normal requests for the directory are redirected to the welcome page. + Arguments.of("/", new String[]{"HTTP/1.1 200 ", "

welcome page

"}), + + // Try a normal resource (will bypass the filter). + Arguments.of("/other.html", new String[]{"HTTP/1.1 200 ", "

other resource

"}), + + // Cannot access files in WEB-INF. + Arguments.of("/WEB-INF/one.js", new String[]{"HTTP/1.1 404 "}), + + // Cannot serve welcome from WEB-INF. + Arguments.of("/WEB-INF/", new String[]{"HTTP/1.1 404 "}), + + // Try to trick the filter into serving a protected resource. + Arguments.of("/WEB-INF/one.js#/", new String[]{"HTTP/1.1 404 "}), + Arguments.of("/js/../WEB-INF/one.js#/", new String[]{"HTTP/1.1 404 "}), + + // Test the URI is not double decoded in the dispatcher. + Arguments.of("/%2557EB-INF/one.js%23/", new String[]{"HTTP/1.1 404 "}) + ); + } + + @ParameterizedTest + @MethodSource("argumentsStream") + public void testWelcomeFilter(String uri, String[] contains) throws Exception + { + String request = + "GET " + uri + " HTTP/1.1\r\n" + + "Host: localhost\r\n" + + "Connection: close\r\n" + + "\r\n"; + String response = connector.getResponse(request); + for (String s : contains) + { + assertThat(response, containsString(s)); + } + } +} diff --git a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppDefaultServletTest.java b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppDefaultServletTest.java new file mode 100644 index 0000000..933bb7a --- /dev/null +++ b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppDefaultServletTest.java @@ -0,0 +1,142 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.webapp; + +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.stream.Stream; + +import org.eclipse.jetty.server.LocalConnector; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.toolchain.test.MavenTestingUtils; +import org.eclipse.jetty.util.IO; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +public class WebAppDefaultServletTest +{ + private Server server; + private LocalConnector connector; + + @BeforeEach + public void prepareServer() throws Exception + { + server = new Server(); + connector = new LocalConnector(server); + server.addConnector(connector); + + Path directoryPath = MavenTestingUtils.getTargetTestingDir().toPath(); + IO.delete(directoryPath.toFile()); + Files.createDirectories(directoryPath); + Path welcomeResource = directoryPath.resolve("index.html"); + try (OutputStream output = Files.newOutputStream(welcomeResource)) + { + output.write("

welcome page

".getBytes(StandardCharsets.UTF_8)); + } + + Path otherResource = directoryPath.resolve("other.html"); + try (OutputStream output = Files.newOutputStream(otherResource)) + { + output.write("

other resource

".getBytes(StandardCharsets.UTF_8)); + } + + Path hiddenDirectory = directoryPath.resolve("WEB-INF"); + Files.createDirectories(hiddenDirectory); + Path hiddenResource = hiddenDirectory.resolve("one.js"); + try (OutputStream output = Files.newOutputStream(hiddenResource)) + { + output.write("this is confidential".getBytes(StandardCharsets.UTF_8)); + } + + // Create directory to trick resource service. + Path hackPath = directoryPath.resolve("%57EB-INF/one.js#/"); + Files.createDirectories(hackPath); + try (OutputStream output = Files.newOutputStream(hackPath.resolve("index.html"))) + { + output.write("this content does not matter".getBytes(StandardCharsets.UTF_8)); + } + + Path standardHashDir = directoryPath.resolve("welcome#"); + Files.createDirectories(standardHashDir); + try (OutputStream output = Files.newOutputStream(standardHashDir.resolve("index.html"))) + { + output.write("standard hash dir welcome".getBytes(StandardCharsets.UTF_8)); + } + + WebAppContext context = new WebAppContext(server, directoryPath.toString(), "/"); + server.setHandler(context); + server.start(); + + // Verify that I can get the file programmatically, as required by the spec. + assertNotNull(context.getServletContext().getResource("/WEB-INF/one.js")); + } + + @AfterEach + public void destroy() throws Exception + { + if (server != null) + server.stop(); + } + + public static Stream argumentsStream() + { + return Stream.of( + Arguments.of("/WEB-INF/", new String[]{"HTTP/1.1 404 "}), + Arguments.of("/welcome%23/", new String[]{"HTTP/1.1 200 ", "standard hash dir welcome"}), + + // Normal requests for the directory are redirected to the welcome page. + Arguments.of("/", new String[]{"HTTP/1.1 200 ", "

welcome page

"}), + + // We can be served other resources. + Arguments.of("/other.html", new String[]{"HTTP/1.1 200 ", "

other resource

"}), + + // The ContextHandler will filter these ones out as as WEB-INF is a protected target. + Arguments.of("/WEB-INF/one.js#/", new String[]{"HTTP/1.1 404 "}), + Arguments.of("/js/../WEB-INF/one.js#/", new String[]{"HTTP/1.1 404 "}), + + // Test the URI is not double decoded by the dispatcher that serves the welcome file (we get index.html not one.js). + Arguments.of("/%2557EB-INF/one.js%23/", new String[]{"HTTP/1.1 200 ", "this content does not matter"}) + ); + } + + @ParameterizedTest + @MethodSource("argumentsStream") + public void testResourceService(String uri, String[] contains) throws Exception + { + String request = + "GET " + uri + " HTTP/1.1\r\n" + + "Host: localhost\r\n" + + "Connection: close\r\n" + + "\r\n"; + String response = connector.getResponse(request); + for (String s : contains) + { + assertThat(response, containsString(s)); + } + } +} -- 2.23.0