From 3f68cd633f03370d33c2603a6496e81273782601 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 11 Nov 2014 06:31:57 +0100 Subject: [PATCH] Apply extra checks to static resource handling - remove leading '/' and control chars - improve url and relative path checks - account for URL encoding - add isResourceUnderLocation final verification Issue: SPR-12354 --- .../resource/ResourceHttpRequestHandler.java | 147 ++++++++++++++++-- .../ResourceHttpRequestHandlerTests.java | 95 +++++++++-- 2 files changed, 214 insertions(+), 28 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java index cdb0453ed85..59a34108e33 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java @@ -18,7 +18,10 @@ package org.springframework.web.servlet.resource; import java.io.IOException; import java.io.InputStream; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; import java.util.List; + import javax.activation.FileTypeMap; import javax.activation.MimetypesFileTypeMap; import javax.servlet.ServletException; @@ -27,14 +30,15 @@ import javax.servlet.http.HttpServletResponse; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; - import org.springframework.beans.factory.InitializingBean; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.Resource; +import org.springframework.core.io.UrlResource; import org.springframework.http.MediaType; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.CollectionUtils; +import org.springframework.util.ResourceUtils; import org.springframework.util.StreamUtils; import org.springframework.util.StringUtils; import org.springframework.web.HttpRequestHandler; @@ -159,14 +163,30 @@ public class ResourceHttpRequestHandler extends WebContentGenerator implements H throw new IllegalStateException("Required request attribute '" + HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE + "' is not set"); } - + path = processPath(path); if (!StringUtils.hasText(path) || isInvalidPath(path)) { if (logger.isDebugEnabled()) { logger.debug("Ignoring invalid resource path [" + path + "]"); } return null; } - + if (path.contains("%")) { + try { + // Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars + if (isInvalidPath(URLDecoder.decode(path, "UTF-8"))) { + if (logger.isTraceEnabled()) { + logger.trace("Ignoring invalid resource path with escape sequences [" + path + "]."); + } + return null; + } + } + catch (UnsupportedEncodingException e) { + // ignore: shouldn't happen + } + catch (IllegalArgumentException ex) { + // ignore + } + } for (Resource location : this.locations) { try { if (logger.isDebugEnabled()) { @@ -174,10 +194,19 @@ public class ResourceHttpRequestHandler extends WebContentGenerator implements H } Resource resource = location.createRelative(path); if (resource.exists() && resource.isReadable()) { - if (logger.isDebugEnabled()) { - logger.debug("Found matching resource: " + resource); + if (isResourceUnderLocation(resource, location)) { + if (logger.isDebugEnabled()) { + logger.debug("Found matching resource: " + resource); + } + return resource; + } + else { + if (logger.isTraceEnabled()) { + logger.trace("resource=\"" + resource + "\" was successfully resolved " + + "but is not under the location=\"" + location); + } + return null; } - return resource; } else if (logger.isTraceEnabled()) { logger.trace("Relative resource doesn't exist or isn't readable: " + resource); @@ -191,14 +220,110 @@ public class ResourceHttpRequestHandler extends WebContentGenerator implements H } /** - * Validates the given path: returns {@code true} if the given path is not a valid resource path. - *

The default implementation rejects paths containing "WEB-INF" or "META-INF" as well as paths - * with relative paths ("../") that result in access of a parent directory. + * Process the given resource path to be used. + *

The default implementation replaces any combination of leading '/' and + * control characters (00-1F and 7F) with a single "/" or "". For example + * {@code " // /// //// foo/bar"} becomes {@code "/foo/bar"}. + * @since 3.2.12 + */ + protected String processPath(String path) { + boolean slash = false; + for (int i = 0; i < path.length(); i++) { + if (path.charAt(i) == '/') { + slash = true; + } + else if (path.charAt(i) > ' ' && path.charAt(i) != 127) { + if (i == 0 || (i == 1 && slash)) { + return path; + } + path = slash ? "/" + path.substring(i) : path.substring(i); + if (logger.isTraceEnabled()) { + logger.trace("Path trimmed for leading '/' and control characters: " + path); + } + return path; + } + } + return (slash ? "/" : ""); + } + + /** + * Identifies invalid resource paths. By default rejects: + *

+ *

Note: this method assumes that leading, duplicate '/' + * or control characters (e.g. white space) have been trimmed so that the + * path starts predictably with a single '/' or does not have one. * @param path the path to validate - * @return {@code true} if the path has been recognized as invalid, {@code false} otherwise + * @return {@code true} if the path is invalid, {@code false} otherwise */ protected boolean isInvalidPath(String path) { - return (path.contains("WEB-INF") || path.contains("META-INF") || StringUtils.cleanPath(path).startsWith("..")); + if (logger.isTraceEnabled()) { + logger.trace("Applying \"invalid path\" checks to path: " + path); + } + if (path.contains("WEB-INF") || path.contains("META-INF")) { + if (logger.isTraceEnabled()) { + logger.trace("Path contains \"WEB-INF\" or \"META-INF\"."); + } + return true; + } + if (path.contains(":/")) { + String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path); + if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) { + if (logger.isTraceEnabled()) { + logger.trace("Path represents URL or has \"url:\" prefix."); + } + return true; + } + } + if (path.contains("../")) { + path = StringUtils.cleanPath(path); + if (path.contains("../")) { + if (logger.isTraceEnabled()) { + logger.trace("Path contains \"../\" after call to StringUtils#cleanPath."); + } + return true; + } + } + return false; + } + + private boolean isResourceUnderLocation(Resource resource, Resource location) throws IOException { + if (!resource.getClass().equals(location.getClass())) { + return false; + } + String resourcePath; + String locationPath; + if (resource instanceof ClassPathResource) { + resourcePath = ((ClassPathResource) resource).getPath(); + locationPath = ((ClassPathResource) location).getPath(); + } + else if (resource instanceof UrlResource) { + resourcePath = resource.getURL().toExternalForm(); + locationPath = location.getURL().toExternalForm(); + } + else { + resourcePath = resource.getURL().getPath(); + locationPath = location.getURL().getPath(); + } + locationPath = (locationPath.endsWith("/") || locationPath.isEmpty() ? locationPath : locationPath + "/"); + if (!resourcePath.startsWith(locationPath)) { + return false; + } + if (resourcePath.contains("%")) { + // Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars + if (URLDecoder.decode(resourcePath, "UTF-8").contains("../")) { + if (logger.isTraceEnabled()) { + logger.trace("Resolved resource path contains \"../\" after decoding: " + resourcePath); + } + return false; + } + } + return true; } /** diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java index 8dd54a643fc..ae809141b66 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,6 +16,10 @@ package org.springframework.web.servlet.resource; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -26,14 +30,13 @@ import org.junit.Before; import org.junit.Test; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.Resource; +import org.springframework.core.io.UrlResource; import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.mock.web.test.MockHttpServletResponse; import org.springframework.mock.web.test.MockServletContext; import org.springframework.web.HttpRequestMethodNotSupportedException; import org.springframework.web.servlet.HandlerMapping; -import static org.junit.Assert.*; - /** * @author Keith Donald * @author Jeremy Grelle @@ -124,28 +127,76 @@ public class ResourceHttpRequestHandlerTests { assertEquals("function foo() { console.log(\"hello world\"); }", response.getContentAsString()); } + @Test - public void getResourceViaDirectoryTraversal() throws Exception { + public void invalidPath() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest(); request.setMethod("GET"); - - request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "../testsecret/secret.txt"); MockHttpServletResponse response = new MockHttpServletResponse(); - handler.handleRequest(request, response); - assertEquals(404, response.getStatus()); - request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "test/../../testsecret/secret.txt"); + Resource location = new ClassPathResource("test/", getClass()); + this.handler.setLocations(Arrays.asList(location)); + + testInvalidPath(location, "../testsecret/secret.txt", request, response); + testInvalidPath(location, "test/../../testsecret/secret.txt", request, response); + testInvalidPath(location, ":/../../testsecret/secret.txt", request, response); + + location = new UrlResource(getClass().getResource("./test/")); + this.handler.setLocations(Arrays.asList(location)); + Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt")); + String secretPath = secretResource.getURL().getPath(); + + testInvalidPath(location, "file:" + secretPath, request, response); + testInvalidPath(location, "/file:" + secretPath, request, response); + testInvalidPath(location, "url:" + secretPath, request, response); + testInvalidPath(location, "/url:" + secretPath, request, response); + testInvalidPath(location, "/" + secretPath, request, response); + testInvalidPath(location, "////../.." + secretPath, request, response); + testInvalidPath(location, "/%2E%2E/testsecret/secret.txt", request, response); + testInvalidPath(location, "/%2e%2e/testsecret/secret.txt", request, response); + testInvalidPath(location, " " + secretPath, request, response); + testInvalidPath(location, "/ " + secretPath, request, response); + testInvalidPath(location, "url:" + secretPath, request, response); + } + + @Test + public void ignoreInvalidEscapeSequence() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setMethod("GET"); + MockHttpServletResponse response = new MockHttpServletResponse(); + request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "/%foo%/bar.txt"); response = new MockHttpServletResponse(); - handler.handleRequest(request, response); + this.handler.handleRequest(request, response); assertEquals(404, response.getStatus()); + } - handler.setLocations(Arrays.asList(new ClassPathResource("testsecret/", getClass()))); - request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "secret.txt"); - response = new MockHttpServletResponse(); - handler.handleRequest(request, response); - assertEquals(200, response.getStatus()); - assertEquals("text/plain", response.getContentType()); - assertEquals("big secret", response.getContentAsString()); + @Test + public void processPath() throws Exception { + assertSame("/foo/bar", this.handler.processPath("/foo/bar")); + assertSame("foo/bar", this.handler.processPath("foo/bar")); + + // leading whitespace control characters (00-1F) + assertEquals("/foo/bar", this.handler.processPath(" /foo/bar")); + assertEquals("/foo/bar", this.handler.processPath((char) 1 + "/foo/bar")); + assertEquals("/foo/bar", this.handler.processPath((char) 31 + "/foo/bar")); + assertEquals("foo/bar", this.handler.processPath(" foo/bar")); + assertEquals("foo/bar", this.handler.processPath((char) 31 + "foo/bar")); + + // leading control character 0x7F (DEL) + assertEquals("/foo/bar", this.handler.processPath((char) 127 + "/foo/bar")); + assertEquals("/foo/bar", this.handler.processPath((char) 127 + "/foo/bar")); + + // leading control and '/' characters + assertEquals("/foo/bar", this.handler.processPath(" / foo/bar")); + assertEquals("/foo/bar", this.handler.processPath(" / / foo/bar")); + assertEquals("/foo/bar", this.handler.processPath(" // /// //// foo/bar")); + assertEquals("/foo/bar", this.handler.processPath((char) 1 + " / " + (char) 127 + " // foo/bar")); + + // root ot empty path + assertEquals("", this.handler.processPath(" ")); + assertEquals("/", this.handler.processPath("/")); + assertEquals("/", this.handler.processPath("///")); + assertEquals("/", this.handler.processPath("/ / / ")); } @Test @@ -219,6 +270,16 @@ public class ResourceHttpRequestHandlerTests { assertEquals(404, response.getStatus()); } + private void testInvalidPath(Resource location, String requestPath, + MockHttpServletRequest request, MockHttpServletResponse response) throws Exception { + + request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, requestPath); + response = new MockHttpServletResponse(); + this.handler.handleRequest(request, response); + assertTrue(location.createRelative(requestPath).exists()); + assertEquals(404, response.getStatus()); + } + private static class TestServletContext extends MockServletContext {