Skip to content

Commit

Permalink
Apply extra checks to static resource handling
Browse files Browse the repository at this point in the history 
- remove leading '/' and control chars
- improve url and relative path checks
- account for URL encoding
- add isResourceUnderLocation final verification

Issue: SPR-12354
  • Loading branch information
@rstoyanchev @snicoll
rstoyanchev authored and snicoll committed Nov 11, 2014
1 parent e42e233 commit 9beae9a
Show file tree
Hide file tree
Showing 2 changed files with 214 additions and 28 deletions.
147 changes: 136 additions & 11 deletions ...vc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@

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;
Expand All @@ -27,14 +30,15 @@

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;
Expand Down Expand Up @@ -161,25 +165,50 @@ protected Resource getResource(HttpServletRequest request) {
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()) {
logger.debug("Trying relative path [" + path + "] against base location: " + location);
}
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);
Expand All @@ -193,14 +222,110 @@ else if (logger.isTraceEnabled()) {
}

/**
* Validates the given path: returns {@code true} if the given path is not a valid resource path.
* <p>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.
* <p>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:
* <ul>
* <li>Paths that contain "WEB-INF" or "META-INF"
* <li>Paths that contain "../" after a call to
* {@link org.springframework.util.StringUtils#cleanPath}.
* <li>Paths that represent a {@link org.springframework.util.ResourceUtils#isUrl
* valid URL} or would represent one after the leading slash is removed.
* </ul>
* <p><strong>Note:</strong> 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;
}

/**
Expand Down
95 changes: 78 additions & 17 deletions ...c/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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;
Expand All @@ -26,14 +30,13 @@
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
Expand Down Expand Up @@ -124,28 +127,76 @@ public void getResourceFromSubDirectoryOfAlternatePath() throws Exception {
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.<Resource>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
Expand Down Expand Up @@ -219,6 +270,16 @@ public void resourceNotFound() throws Exception {
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 {

Expand Down

0 comments on commit 9beae9a

Please sign in to comment.