From c704b8100a1ccc36f4bb8b80a96f3375dde8d182 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 12 May 2021 18:29:17 +1000 Subject: [PATCH 1/2] Review URI encoding in ConcatServlet & WelcomeFilter and improve testing. Signed-off-by: Lachlan Roberts --- .../eclipse/jetty/server/ResourceService.java | 2 +- .../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, 352 insertions(+), 30 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 1edfd83141bb..eca99c781219 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 @@ -430,7 +430,7 @@ protected void sendWelcome(HttpContent content, String pathInContext, boolean en 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 f6dde9468afb..55700b47febd 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 @@ -61,6 +61,7 @@ * 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; @@ -125,7 +126,8 @@ else if (!type.equals(t)) } } - 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 9a2553823a73..3caa85a99c7a 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 @@ -27,6 +27,8 @@ 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 @@ -41,6 +43,7 @@ * * Requests to "/some/directory" will be redirected to "/some/directory/". */ +@Deprecated public class WelcomeFilter implements Filter { private String welcome; @@ -61,7 +64,10 @@ public void doFilter(ServletRequest request, { 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 f8ea08753be2..5cb9c89f4160 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 @@ -26,6 +26,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; @@ -41,7 +42,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; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -112,7 +118,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t } @Test - public void testWEBINFResourceIsNotServed() throws Exception + public void testDirectoryNotAccessible() throws Exception { File directoryFile = MavenTestingUtils.getTargetTestingDir(); Path directoryPath = directoryFile.toPath(); @@ -134,9 +140,8 @@ public void testWEBINFResourceIsNotServed() throws Exception // 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" + @@ -144,35 +149,59 @@ public void testWEBINFResourceIsNotServed() throws Exception "\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)); + } - // Make sure ConcatServlet cannot see file system files. - uri = contextPath + concatPath + "?/trick/../../" + directoryFile.getName(); - request = + String contextPath = ""; + WebAppContext context = new WebAppContext(server, directoryPath.toString(), contextPath); + server.setHandler(context); + String concatPath = "/concat"; + context.addServlet(ConcatServlet.class, concatPath); + server.start(); + + // 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 000000000000..65e6503ed6ac --- /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 000000000000..933bb7aaf715 --- /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)); + } + } +} From 5d57248380268e1d594ae6350fe12f49c804bcf9 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 12 May 2021 16:10:52 +0200 Subject: [PATCH 2/2] Issue #6263 - Review URI encoding in ConcatServlet & WelcomeFilter. Fixed log statement. Signed-off-by: Simone Bordet --- .../src/main/java/org/eclipse/jetty/server/ResourceService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 eca99c781219..c908e8d271c1 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 @@ -240,7 +240,7 @@ public boolean doGet(HttpServletRequest request, HttpServletResponse response) // 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())