packages/jetty
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
jetty/CVE-2021-28169.patch

529 lines
22 KiB
Diff
Raw Normal View History

fix CVE-2021-28169
2021-06-23 20:40:09 +08:00
From aec4092cc718b61998c1de221c9c728f377cd430 Mon Sep 17 00:00:00 2001
From: Lachlan <lachlan@webtide.com>
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 <lachlan@webtide.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-by: Simone Bordet <simone.bordet@gmail.com>
---
.../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.</p>
*/
+@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<Arguments> 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("<h1>welcome page</h1>".getBytes(StandardCharsets.UTF_8));
+ }
+
+ Path otherResource = directoryPath.resolve("other.html");
+ try (OutputStream output = Files.newOutputStream(otherResource))
+ {
+ output.write("<h1>other resource</h1>".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<Arguments> argumentsStream()
+ {
+ return Stream.of(
+ // Normal requests for the directory are redirected to the welcome page.
+ Arguments.of("/", new String[]{"HTTP/1.1 200 ", "<h1>welcome page</h1>"}),
+
+ // Try a normal resource (will bypass the filter).
+ Arguments.of("/other.html", new String[]{"HTTP/1.1 200 ", "<h1>other resource</h1>"}),
+
+ // 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("<h1>welcome page</h1>".getBytes(StandardCharsets.UTF_8));
+ }
+
+ Path otherResource = directoryPath.resolve("other.html");
+ try (OutputStream output = Files.newOutputStream(otherResource))
+ {
+ output.write("<h1>other resource</h1>".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<Arguments> 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 ", "<h1>welcome page</h1>"}),
+
+ // We can be served other resources.
+ Arguments.of("/other.html", new String[]{"HTTP/1.1 200 ", "<h1>other resource</h1>"}),
+
+ // 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
Reference in New Issue Copy Permalink