Skip to content

Commit 3f68cd6

Browse files
rstoyanchevsnicoll
rstoyanchev
authored and
snicoll
committed
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
1 parent e1d6826 commit 3f68cd6

File tree

2 files changed

+214
-28
lines changed

2 files changed

+214
-28
lines changed

‎spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java

Lines changed: 136 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@
1818

1919
import java.io.IOException;
2020
import java.io.InputStream;
21+
import java.io.UnsupportedEncodingException;
22+
import java.net.URLDecoder;
2123
import java.util.List;
24+
2225
import javax.activation.FileTypeMap;
2326
import javax.activation.MimetypesFileTypeMap;
2427
import javax.servlet.ServletException;
@@ -27,14 +30,15 @@
2730

2831
import org.apache.commons.logging.Log;
2932
import org.apache.commons.logging.LogFactory;
30-
3133
import org.springframework.beans.factory.InitializingBean;
3234
import org.springframework.core.io.ClassPathResource;
3335
import org.springframework.core.io.Resource;
36+
import org.springframework.core.io.UrlResource;
3437
import org.springframework.http.MediaType;
3538
import org.springframework.util.Assert;
3639
import org.springframework.util.ClassUtils;
3740
import org.springframework.util.CollectionUtils;
41+
import org.springframework.util.ResourceUtils;
3842
import org.springframework.util.StreamUtils;
3943
import org.springframework.util.StringUtils;
4044
import org.springframework.web.HttpRequestHandler;
@@ -159,25 +163,50 @@ protected Resource getResource(HttpServletRequest request) {
159163
throw new IllegalStateException("Required request attribute '" +
160164
HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE + "' is not set");
161165
}
162-
166+
path = processPath(path);
163167
if (!StringUtils.hasText(path) || isInvalidPath(path)) {
164168
if (logger.isDebugEnabled()) {
165169
logger.debug("Ignoring invalid resource path [" + path + "]");
166170
}
167171
return null;
168172
}
169-
173+
if (path.contains("%")) {
174+
try {
175+
// Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars
176+
if (isInvalidPath(URLDecoder.decode(path, "UTF-8"))) {
177+
if (logger.isTraceEnabled()) {
178+
logger.trace("Ignoring invalid resource path with escape sequences [" + path + "].");
179+
}
180+
return null;
181+
}
182+
}
183+
catch (UnsupportedEncodingException e) {
184+
// ignore: shouldn't happen
185+
}
186+
catch (IllegalArgumentException ex) {
187+
// ignore
188+
}
189+
}
170190
for (Resource location : this.locations) {
171191
try {
172192
if (logger.isDebugEnabled()) {
173193
logger.debug("Trying relative path [" + path + "] against base location: " + location);
174194
}
175195
Resource resource = location.createRelative(path);
176196
if (resource.exists() && resource.isReadable()) {
177-
if (logger.isDebugEnabled()) {
178-
logger.debug("Found matching resource: " + resource);
197+
if (isResourceUnderLocation(resource, location)) {
198+
if (logger.isDebugEnabled()) {
199+
logger.debug("Found matching resource: " + resource);
200+
}
201+
return resource;
202+
}
203+
else {
204+
if (logger.isTraceEnabled()) {
205+
logger.trace("resource=\"" + resource + "\" was successfully resolved " +
206+
"but is not under the location=\"" + location);
207+
}
208+
return null;
179209
}
180-
return resource;
181210
}
182211
else if (logger.isTraceEnabled()) {
183212
logger.trace("Relative resource doesn't exist or isn't readable: " + resource);
@@ -191,14 +220,110 @@ else if (logger.isTraceEnabled()) {
191220
}
192221

193222
/**
194-
* Validates the given path: returns {@code true} if the given path is not a valid resource path.
195-
* <p>The default implementation rejects paths containing "WEB-INF" or "META-INF" as well as paths
196-
* with relative paths ("../") that result in access of a parent directory.
223+
* Process the given resource path to be used.
224+
* <p>The default implementation replaces any combination of leading '/' and
225+
* control characters (00-1F and 7F) with a single "/" or "". For example
226+
* {@code " // /// //// foo/bar"} becomes {@code "/foo/bar"}.
227+
* @since 3.2.12
228+
*/
229+
protected String processPath(String path) {
230+
boolean slash = false;
231+
for (int i = 0; i < path.length(); i++) {
232+
if (path.charAt(i) == '/') {
233+
slash = true;
234+
}
235+
else if (path.charAt(i) > ' ' && path.charAt(i) != 127) {
236+
if (i == 0 || (i == 1 && slash)) {
237+
return path;
238+
}
239+
path = slash ? "/" + path.substring(i) : path.substring(i);
240+
if (logger.isTraceEnabled()) {
241+
logger.trace("Path trimmed for leading '/' and control characters: " + path);
242+
}
243+
return path;
244+
}
245+
}
246+
return (slash ? "/" : "");
247+
}
248+
249+
/**
250+
* Identifies invalid resource paths. By default rejects:
251+
* <ul>
252+
* <li>Paths that contain "WEB-INF" or "META-INF"
253+
* <li>Paths that contain "../" after a call to
254+
* {@link org.springframework.util.StringUtils#cleanPath}.
255+
* <li>Paths that represent a {@link org.springframework.util.ResourceUtils#isUrl
256+
* valid URL} or would represent one after the leading slash is removed.
257+
* </ul>
258+
* <p><strong>Note:</strong> this method assumes that leading, duplicate '/'
259+
* or control characters (e.g. white space) have been trimmed so that the
260+
* path starts predictably with a single '/' or does not have one.
197261
* @param path the path to validate
198-
* @return {@code true} if the path has been recognized as invalid, {@code false} otherwise
262+
* @return {@code true} if the path is invalid, {@code false} otherwise
199263
*/
200264
protected boolean isInvalidPath(String path) {
201-
return (path.contains("WEB-INF") || path.contains("META-INF") || StringUtils.cleanPath(path).startsWith(".."));
265+
if (logger.isTraceEnabled()) {
266+
logger.trace("Applying \"invalid path\" checks to path: " + path);
267+
}
268+
if (path.contains("WEB-INF") || path.contains("META-INF")) {
269+
if (logger.isTraceEnabled()) {
270+
logger.trace("Path contains \"WEB-INF\" or \"META-INF\".");
271+
}
272+
return true;
273+
}
274+
if (path.contains(":/")) {
275+
String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path);
276+
if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) {
277+
if (logger.isTraceEnabled()) {
278+
logger.trace("Path represents URL or has \"url:\" prefix.");
279+
}
280+
return true;
281+
}
282+
}
283+
if (path.contains("../")) {
284+
path = StringUtils.cleanPath(path);
285+
if (path.contains("../")) {
286+
if (logger.isTraceEnabled()) {
287+
logger.trace("Path contains \"../\" after call to StringUtils#cleanPath.");
288+
}
289+
return true;
290+
}
291+
}
292+
return false;
293+
}
294+
295+
private boolean isResourceUnderLocation(Resource resource, Resource location) throws IOException {
296+
if (!resource.getClass().equals(location.getClass())) {
297+
return false;
298+
}
299+
String resourcePath;
300+
String locationPath;
301+
if (resource instanceof ClassPathResource) {
302+
resourcePath = ((ClassPathResource) resource).getPath();
303+
locationPath = ((ClassPathResource) location).getPath();
304+
}
305+
else if (resource instanceof UrlResource) {
306+
resourcePath = resource.getURL().toExternalForm();
307+
locationPath = location.getURL().toExternalForm();
308+
}
309+
else {
310+
resourcePath = resource.getURL().getPath();
311+
locationPath = location.getURL().getPath();
312+
}
313+
locationPath = (locationPath.endsWith("/") || locationPath.isEmpty() ? locationPath : locationPath + "/");
314+
if (!resourcePath.startsWith(locationPath)) {
315+
return false;
316+
}
317+
if (resourcePath.contains("%")) {
318+
// Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars
319+
if (URLDecoder.decode(resourcePath, "UTF-8").contains("../")) {
320+
if (logger.isTraceEnabled()) {
321+
logger.trace("Resolved resource path contains \"../\" after decoding: " + resourcePath);
322+
}
323+
return false;
324+
}
325+
}
326+
return true;
202327
}
203328

204329
/**

‎spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java

Lines changed: 78 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2012 the original author or authors.
2+
* Copyright 2002-2014 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -16,6 +16,10 @@
1616

1717
package org.springframework.web.servlet.resource;
1818

19+
import static org.junit.Assert.assertEquals;
20+
import static org.junit.Assert.assertSame;
21+
import static org.junit.Assert.assertTrue;
22+
1923
import java.util.ArrayList;
2024
import java.util.Arrays;
2125
import java.util.List;
@@ -26,14 +30,13 @@
2630
import org.junit.Test;
2731
import org.springframework.core.io.ClassPathResource;
2832
import org.springframework.core.io.Resource;
33+
import org.springframework.core.io.UrlResource;
2934
import org.springframework.mock.web.test.MockHttpServletRequest;
3035
import org.springframework.mock.web.test.MockHttpServletResponse;
3136
import org.springframework.mock.web.test.MockServletContext;
3237
import org.springframework.web.HttpRequestMethodNotSupportedException;
3338
import org.springframework.web.servlet.HandlerMapping;
3439

35-
import static org.junit.Assert.*;
36-
3740
/**
3841
* @author Keith Donald
3942
* @author Jeremy Grelle
@@ -124,28 +127,76 @@ public void getResourceFromSubDirectoryOfAlternatePath() throws Exception {
124127
assertEquals("function foo() { console.log(\"hello world\"); }", response.getContentAsString());
125128
}
126129

130+
127131
@Test
128-
public void getResourceViaDirectoryTraversal() throws Exception {
132+
public void invalidPath() throws Exception {
129133
MockHttpServletRequest request = new MockHttpServletRequest();
130134
request.setMethod("GET");
131-
132-
request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "../testsecret/secret.txt");
133135
MockHttpServletResponse response = new MockHttpServletResponse();
134-
handler.handleRequest(request, response);
135-
assertEquals(404, response.getStatus());
136136

137-
request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "test/../../testsecret/secret.txt");
137+
Resource location = new ClassPathResource("test/", getClass());
138+
this.handler.setLocations(Arrays.asList(location));
139+
140+
testInvalidPath(location, "../testsecret/secret.txt", request, response);
141+
testInvalidPath(location, "test/../../testsecret/secret.txt", request, response);
142+
testInvalidPath(location, ":/../../testsecret/secret.txt", request, response);
143+
144+
location = new UrlResource(getClass().getResource("./test/"));
145+
this.handler.setLocations(Arrays.asList(location));
146+
Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt"));
147+
String secretPath = secretResource.getURL().getPath();
148+
149+
testInvalidPath(location, "file:" + secretPath, request, response);
150+
testInvalidPath(location, "/file:" + secretPath, request, response);
151+
testInvalidPath(location, "url:" + secretPath, request, response);
152+
testInvalidPath(location, "/url:" + secretPath, request, response);
153+
testInvalidPath(location, "/" + secretPath, request, response);
154+
testInvalidPath(location, "////../.." + secretPath, request, response);
155+
testInvalidPath(location, "/%2E%2E/testsecret/secret.txt", request, response);
156+
testInvalidPath(location, "/%2e%2e/testsecret/secret.txt", request, response);
157+
testInvalidPath(location, " " + secretPath, request, response);
158+
testInvalidPath(location, "/ " + secretPath, request, response);
159+
testInvalidPath(location, "url:" + secretPath, request, response);
160+
}
161+
162+
@Test
163+
public void ignoreInvalidEscapeSequence() throws Exception {
164+
MockHttpServletRequest request = new MockHttpServletRequest();
165+
request.setMethod("GET");
166+
MockHttpServletResponse response = new MockHttpServletResponse();
167+
request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "/%foo%/bar.txt");
138168
response = new MockHttpServletResponse();
139-
handler.handleRequest(request, response);
169+
this.handler.handleRequest(request, response);
140170
assertEquals(404, response.getStatus());
171+
}
141172

142-
handler.setLocations(Arrays.<Resource>asList(new ClassPathResource("testsecret/", getClass())));
143-
request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "secret.txt");
144-
response = new MockHttpServletResponse();
145-
handler.handleRequest(request, response);
146-
assertEquals(200, response.getStatus());
147-
assertEquals("text/plain", response.getContentType());
148-
assertEquals("big secret", response.getContentAsString());
173+
@Test
174+
public void processPath() throws Exception {
175+
assertSame("/foo/bar", this.handler.processPath("/foo/bar"));
176+
assertSame("foo/bar", this.handler.processPath("foo/bar"));
177+
178+
// leading whitespace control characters (00-1F)
179+
assertEquals("/foo/bar", this.handler.processPath(" /foo/bar"));
180+
assertEquals("/foo/bar", this.handler.processPath((char) 1 + "/foo/bar"));
181+
assertEquals("/foo/bar", this.handler.processPath((char) 31 + "/foo/bar"));
182+
assertEquals("foo/bar", this.handler.processPath(" foo/bar"));
183+
assertEquals("foo/bar", this.handler.processPath((char) 31 + "foo/bar"));
184+
185+
// leading control character 0x7F (DEL)
186+
assertEquals("/foo/bar", this.handler.processPath((char) 127 + "/foo/bar"));
187+
assertEquals("/foo/bar", this.handler.processPath((char) 127 + "/foo/bar"));
188+
189+
// leading control and '/' characters
190+
assertEquals("/foo/bar", this.handler.processPath(" / foo/bar"));
191+
assertEquals("/foo/bar", this.handler.processPath(" / / foo/bar"));
192+
assertEquals("/foo/bar", this.handler.processPath(" // /// //// foo/bar"));
193+
assertEquals("/foo/bar", this.handler.processPath((char) 1 + " / " + (char) 127 + " // foo/bar"));
194+
195+
// root ot empty path
196+
assertEquals("", this.handler.processPath(" "));
197+
assertEquals("/", this.handler.processPath("/"));
198+
assertEquals("/", this.handler.processPath("///"));
199+
assertEquals("/", this.handler.processPath("/ / / "));
149200
}
150201

151202
@Test
@@ -219,6 +270,16 @@ public void resourceNotFound() throws Exception {
219270
assertEquals(404, response.getStatus());
220271
}
221272

273+
private void testInvalidPath(Resource location, String requestPath,
274+
MockHttpServletRequest request, MockHttpServletResponse response) throws Exception {
275+
276+
request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, requestPath);
277+
response = new MockHttpServletResponse();
278+
this.handler.handleRequest(request, response);
279+
assertTrue(location.createRelative(requestPath).exists());
280+
assertEquals(404, response.getStatus());
281+
}
282+
222283

223284
private static class TestServletContext extends MockServletContext {
224285

0 commit comments

Comments
 (0)