Skip to content

Commit

Permalink
Fixes #6263 - Review URI encoding in ConcatServlet & WelcomeFilter.
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
lachlan-roberts and sbordet committed May 12, 2021
1 parent 9cb9343 commit 1c05b0b
Show file tree
Hide file tree
Showing 6 changed files with 353 additions and 31 deletions.
Expand Up @@ -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())
Expand Down Expand Up @@ -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
Expand Down
Expand Up @@ -61,6 +61,7 @@
* 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;
Expand Down Expand Up @@ -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);
}
Expand Down
Expand Up @@ -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
Expand All @@ -41,6 +43,7 @@
*
* Requests to "/some/directory" will be redirected to "/some/directory/".
*/
@Deprecated
public class WelcomeFilter implements Filter
{
private String welcome;
Expand All @@ -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);
}
Expand Down
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -134,45 +140,68 @@ 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" +
"Connection: close\r\n" +
"\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));
}

// 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));
}
}
@@ -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));
}
}
}

0 comments on commit 1c05b0b

Please sign in to comment.