From 8437193708e4bf6b2861a7953dc472f9dad49111 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Thu, 19 Nov 2015 16:17:38 +0000 Subject: [PATCH] Move the functionality that provides redirects for context roots and directories where a trailing / is added from the Mapper to the DefaultServlet. This enables such requests to be processed by any configured Valves and Filters before the redirect is made. This behaviour is configurable via the mapperContextRootRedirectEnabled and mapperDirectoryRedirectEnabled attributes of the Context which may be used to restore the previous behaviour. This is part 1 of 3 of the fix for CVE-2015-5345 git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1715206 13f79535-47bb-0310-9956-ffa450edef68 --- java/org/apache/catalina/Context.java | 40 ++++++++++++ .../apache/catalina/core/StandardContext.java | 41 ++++++++++++ .../catalina/core/mbeans-descriptors.xml | 8 +++ java/org/apache/catalina/mapper/Mapper.java | 10 +-- .../catalina/servlets/DefaultServlet.java | 11 ++++ .../catalina/startup/FailedContext.java | 18 +++++- .../apache/catalina/core/TesterContext.java | 16 +++++ .../catalina/mapper/TestMapperWebapps.java | 64 +++++++++++++++++++ .../catalina/startup/TomcatBaseTest.java | 3 +- .../valves/rewrite/TestRewriteValve.java | 3 + webapps/docs/changelog.xml | 10 +++ webapps/docs/config/context.xml | 16 +++++ 12 files changed, 232 insertions(+), 8 deletions(-) diff --git a/java/org/apache/catalina/Context.java b/java/org/apache/catalina/Context.java index e19e854fec16..c2c255f50038 100644 --- a/java/org/apache/catalina/Context.java +++ b/java/org/apache/catalina/Context.java @@ -1714,4 +1714,44 @@ Set addServletSecurity(ServletRegistration.Dynamic registration, * false} */ public boolean getValidateClientProvidedNewSessionId(); + + /** + * If enabled, requests for a web application context root will be + * redirected (adding a trailing slash) by the Mapper. This is more + * efficient but has the side effect of confirming that the context path is + * valid. + * + * @param mapperContextRootRedirectEnabled Should the redirects be enabled? + */ + public void setMapperContextRootRedirectEnabled(boolean mapperContextRootRedirectEnabled); + + /** + * Determines if requests for a web application context root will be + * redirected (adding a trailing slash) by the Mapper. This is more + * efficient but has the side effect of confirming that the context path is + * valid. + * + * @return {@code true} if the Mapper level redirect is enabled for this + * Context. + */ + public boolean getMapperContextRootRedirectEnabled(); + + /** + * If enabled, requests for a directory will be redirected (adding a + * trailing slash) by the Mapper. This is more efficient but has the + * side effect of confirming that the directory is valid. + * + * @param mapperDirectoryRedirectEnabled Should the redirects be enabled? + */ + public void setMapperDirectoryRedirectEnabled(boolean mapperDirectoryRedirectEnabled); + + /** + * Determines if requests for a directory will be redirected (adding a + * trailing slash) by the Mapper. This is more efficient but has the + * side effect of confirming that the directory is valid. + * + * @return {@code true} if the Mapper level redirect is enabled for this + * Context. + */ + public boolean getMapperDirectoryRedirectEnabled(); } diff --git a/java/org/apache/catalina/core/StandardContext.java b/java/org/apache/catalina/core/StandardContext.java index d8c8425118db..a251a620ae37 100644 --- a/java/org/apache/catalina/core/StandardContext.java +++ b/java/org/apache/catalina/core/StandardContext.java @@ -816,13 +816,53 @@ public void unbind() {} private boolean validateClientProvidedNewSessionId = true; + boolean mapperContextRootRedirectEnabled = false; + + boolean mapperDirectoryRedirectEnabled = false; + + // ----------------------------------------------------- Context Properties + @Override + public void setMapperContextRootRedirectEnabled(boolean mapperContextRootRedirectEnabled) { + this.mapperContextRootRedirectEnabled = mapperContextRootRedirectEnabled; + } + + + /** + * {@inheritDoc} + *

+ * The default value for this implementation is {@code false}. + */ + @Override + public boolean getMapperContextRootRedirectEnabled() { + return mapperContextRootRedirectEnabled; + } + + + @Override + public void setMapperDirectoryRedirectEnabled(boolean mapperDirectoryRedirectEnabled) { + this.mapperDirectoryRedirectEnabled = mapperDirectoryRedirectEnabled; + } + + + /** + * {@inheritDoc} + *

+ * The default value for this implementation is {@code false}. + */ + @Override + public boolean getMapperDirectoryRedirectEnabled() { + return mapperDirectoryRedirectEnabled; + } + + @Override public void setValidateClientProvidedNewSessionId(boolean validateClientProvidedNewSessionId) { this.validateClientProvidedNewSessionId = validateClientProvidedNewSessionId; } + /** * {@inheritDoc} *

@@ -833,6 +873,7 @@ public boolean getValidateClientProvidedNewSessionId() { return validateClientProvidedNewSessionId; } + @Override public void setCookieProcessor(CookieProcessor cookieProcessor) { if (cookieProcessor == null) { diff --git a/java/org/apache/catalina/core/mbeans-descriptors.xml b/java/org/apache/catalina/core/mbeans-descriptors.xml index bafd270e593f..2893730d7116 100644 --- a/java/org/apache/catalina/core/mbeans-descriptors.xml +++ b/java/org/apache/catalina/core/mbeans-descriptors.xml @@ -177,6 +177,14 @@ description="Associated manager." type="org.apache.catalina.Manager" /> + + + + diff --git a/java/org/apache/catalina/mapper/Mapper.java b/java/org/apache/catalina/mapper/Mapper.java index e37ff8f26b89..f22f47a313cb 100644 --- a/java/org/apache/catalina/mapper/Mapper.java +++ b/java/org/apache/catalina/mapper/Mapper.java @@ -883,7 +883,8 @@ private final void internalMapWrapper(ContextVersion contextVersion, } } - if(mappingData.wrapper == null && noServletPath) { + if(mappingData.wrapper == null && noServletPath && + mappingData.context.getMapperContextRootRedirectEnabled()) { // The path is empty, redirect to "/" mappingData.redirectPath.setChars (path.getBuffer(), pathOffset, pathEnd-pathOffset); @@ -1001,9 +1002,9 @@ private final void internalMapWrapper(ContextVersion contextVersion, char[] buf = path.getBuffer(); if (contextVersion.resources != null && buf[pathEnd -1 ] != '/') { String pathStr = path.toString(); - WebResource file = - contextVersion.resources.getResource(pathStr); - if (file != null && file.isDirectory()) { + WebResource file = contextVersion.resources.getResource(pathStr); + if (file != null && file.isDirectory() && + mappingData.context.getMapperDirectoryRedirectEnabled()) { // Note: this mutates the path: do not do any processing // after this (since we set the redirectPath, there // shouldn't be any) @@ -1020,7 +1021,6 @@ private final void internalMapWrapper(ContextVersion contextVersion, path.setOffset(pathOffset); path.setEnd(pathEnd); - } diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java b/java/org/apache/catalina/servlets/DefaultServlet.java index e2ecd6fbd6ed..6119de91166c 100644 --- a/java/org/apache/catalina/servlets/DefaultServlet.java +++ b/java/org/apache/catalina/servlets/DefaultServlet.java @@ -816,6 +816,17 @@ protected void serveResource(HttpServletRequest request, long contentLength = -1L; if (resource.isDirectory()) { + if (!path.endsWith("/")) { + StringBuilder location = new StringBuilder(request.getRequestURI()); + location.append('/'); + if (request.getQueryString() != null) { + location.append('?'); + location.append(request.getQueryString()); + } + response.sendRedirect(response.encodeRedirectURL(location.toString())); + return; + } + // Skip directory listings if we have been configured to // suppress them if (!listings) { diff --git a/java/org/apache/catalina/startup/FailedContext.java b/java/org/apache/catalina/startup/FailedContext.java index f2383ff5a42f..9a0407b6deca 100644 --- a/java/org/apache/catalina/startup/FailedContext.java +++ b/java/org/apache/catalina/startup/FailedContext.java @@ -764,9 +764,25 @@ public void setCookieProcessor(CookieProcessor cookieProcessor) { /* NO-OP */ } @Override public void setValidateClientProvidedNewSessionId(boolean validateClientProvidedNewSessionId) { - //NO-OP + // NO-OP } @Override public boolean getValidateClientProvidedNewSessionId() { return false; } + + @Override + public void setMapperContextRootRedirectEnabled(boolean mapperContextRootRedirectEnabled) { + // NO-OP + } + + @Override + public boolean getMapperContextRootRedirectEnabled() { return false; } + + @Override + public void setMapperDirectoryRedirectEnabled(boolean mapperDirectoryRedirectEnabled) { + // NO-OP + } + + @Override + public boolean getMapperDirectoryRedirectEnabled() { return false; } } \ No newline at end of file diff --git a/test/org/apache/catalina/core/TesterContext.java b/test/org/apache/catalina/core/TesterContext.java index d544c408dfb1..cdc5f78ce918 100644 --- a/test/org/apache/catalina/core/TesterContext.java +++ b/test/org/apache/catalina/core/TesterContext.java @@ -1234,4 +1234,20 @@ public void setValidateClientProvidedNewSessionId(boolean validateClientProvided @Override public boolean getValidateClientProvidedNewSessionId() { return false; } + + @Override + public void setMapperContextRootRedirectEnabled(boolean mapperContextRootRedirectEnabled) { + // NO-OP + } + + @Override + public boolean getMapperContextRootRedirectEnabled() { return false; } + + @Override + public void setMapperDirectoryRedirectEnabled(boolean mapperDirectoryRedirectEnabled) { + // NO-OP + } + + @Override + public boolean getMapperDirectoryRedirectEnabled() { return false; } } diff --git a/test/org/apache/catalina/mapper/TestMapperWebapps.java b/test/org/apache/catalina/mapper/TestMapperWebapps.java index 51e725c7aa2d..236eff2c8f55 100644 --- a/test/org/apache/catalina/mapper/TestMapperWebapps.java +++ b/test/org/apache/catalina/mapper/TestMapperWebapps.java @@ -18,6 +18,7 @@ import java.io.File; import java.io.IOException; +import java.net.HttpURLConnection; import java.util.HashMap; import java.util.List; @@ -33,7 +34,10 @@ import org.apache.catalina.core.StandardContext; import org.apache.catalina.startup.Tomcat; import org.apache.catalina.startup.TomcatBaseTest; +import org.apache.catalina.valves.RemoteAddrValve; import org.apache.tomcat.util.buf.ByteChunk; +import org.apache.tomcat.util.descriptor.web.SecurityCollection; +import org.apache.tomcat.util.descriptor.web.SecurityConstraint; import org.apache.tomcat.websocket.server.WsContextListener; /** @@ -225,6 +229,66 @@ public void testWelcomeFileStrict() throws Exception { Assert.assertEquals(HttpServletResponse.SC_NOT_FOUND, rc); } + @Test + public void testRedirect() throws Exception { + // Disable the following of redirects for this test only + boolean originalValue = HttpURLConnection.getFollowRedirects(); + HttpURLConnection.setFollowRedirects(false); + try { + Tomcat tomcat = getTomcatInstance(); + + // Use standard test webapp as ROOT + File rootDir = new File("test/webapp"); + org.apache.catalina.Context root = + tomcat.addWebapp(null, "", rootDir.getAbsolutePath()); + + // Add a security constraint + SecurityConstraint constraint = new SecurityConstraint(); + SecurityCollection collection = new SecurityCollection(); + collection.addPattern("/welcome-files/*"); + collection.addPattern("/welcome-files"); + constraint.addCollection(collection); + constraint.addAuthRole("foo"); + root.addConstraint(constraint); + + // Also make examples available + File examplesDir = new File(getBuildDirectory(), "webapps/examples"); + org.apache.catalina.Context examples = tomcat.addWebapp( + null, "/examples", examplesDir.getAbsolutePath()); + // Then block access to the examples to test redirection + RemoteAddrValve rav = new RemoteAddrValve(); + rav.setDeny(".*"); + rav.setDenyStatus(404); + examples.getPipeline().addValve(rav); + + tomcat.start(); + + // Redirects within a web application + doRedirectTest("/welcome-files", 401); + doRedirectTest("/welcome-files/", 401); + + doRedirectTest("/jsp", 302); + doRedirectTest("/jsp/", 404); + + doRedirectTest("/WEB-INF", 404); + doRedirectTest("/WEB-INF/", 404); + + // Redirects between web applications + doRedirectTest("/examples", 404); + doRedirectTest("/examples/", 404); + } finally { + HttpURLConnection.setFollowRedirects(originalValue); + } + } + + + private void doRedirectTest(String path, int expected) throws IOException { + ByteChunk bc = new ByteChunk(); + int rc = getUrl("http://localhost:" + getPort() + path, bc, null); + Assert.assertEquals(expected, rc); + } + + /** * Prepare a string to search in messages that contain a timestamp, when it * is known that the timestamp was printed between {@code timeA} and diff --git a/test/org/apache/catalina/startup/TomcatBaseTest.java b/test/org/apache/catalina/startup/TomcatBaseTest.java index ab8ee6e8bfcc..cb82bf297f9c 100644 --- a/test/org/apache/catalina/startup/TomcatBaseTest.java +++ b/test/org/apache/catalina/startup/TomcatBaseTest.java @@ -647,8 +647,7 @@ public static int methodUrl(String path, ByteChunk out, int readTimeout, String method) throws IOException { URL url = new URL(path); - HttpURLConnection connection = - (HttpURLConnection) url.openConnection(); + HttpURLConnection connection = (HttpURLConnection) url.openConnection(); connection.setUseCaches(false); connection.setReadTimeout(readTimeout); connection.setRequestMethod(method); diff --git a/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java b/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java index 060351581581..47f9440f0012 100644 --- a/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java +++ b/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java @@ -20,6 +20,7 @@ import org.junit.Test; import org.apache.catalina.Context; +import org.apache.catalina.servlets.DefaultServlet; import org.apache.catalina.startup.Tomcat; import org.apache.catalina.startup.TomcatBaseTest; import org.apache.tomcat.util.buf.ByteChunk; @@ -75,6 +76,8 @@ private void doTestRewrite(String config, String request, String expectedURI) th Tomcat.addServlet(ctx, "snoop", new SnoopServlet()); ctx.addServletMapping("/a/%255A", "snoop"); ctx.addServletMapping("/c/*", "snoop"); + Tomcat.addServlet(ctx, "default", new DefaultServlet()); + ctx.addServletMapping("/", "default"); tomcat.start(); diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 5d6dd080766c..4c9669a80ed9 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -52,6 +52,16 @@ Mapper used is the Mapper associated with the Service for which the listener was created. (markt) + + Move the functionality that provides redirects for context roots and + directories where a trailing / is added from the Mapper to + the DefaultServlet. This enables such requests to be + processed by any configured Valves and Filters before the redirect is + made. This behaviour is configurable via the + mapperContextRootRedirectEnabled and + mapperDirectoryRedirectEnabled attributes of the Context + which may be used to restore the previous behaviour. (markt) + diff --git a/webapps/docs/config/context.xml b/webapps/docs/config/context.xml index 8b20e6dd16ee..22d8c66f111a 100644 --- a/webapps/docs/config/context.xml +++ b/webapps/docs/config/context.xml @@ -360,6 +360,22 @@ default value of false is used.

+ +

If enabled, requests for a web application context root will be + redirected (adding a trailing slash) if necessary by the Mapper rather + than the default Servlet. This is more efficient but has the side effect + of confirming that the context path exists. If not specified, the + default value of false is used.

+ + + +

If enabled, requests for a web application directory will be + redirected (adding a trailing slash) if necessary by the Mapper rather + than the default Servlet. This is more efficient but has the side effect + of confirming that the directory is exists. If not specified, the + default value of false is used.

+ +

Set to true to ignore any settings in both the global or Host default contexts. By default, settings