diff --git a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java index 4278dd6dee..0e684b015b 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java +++ b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java @@ -128,6 +128,10 @@ public class ContentNegotiationManagerFactoryBean this.useJaf = useJaf; } + private boolean isUseJafTurnedOff() { + return (this.useJaf != null && !this.useJaf); + } + /** * Indicate whether a request parameter should be used to determine the * requested media type with the 2nd highest priority, i.e. @@ -184,7 +188,7 @@ public class ContentNegotiationManagerFactoryBean if (this.favorPathExtension) { PathExtensionContentNegotiationStrategy strategy; - if (this.servletContext != null) { + if (this.servletContext != null && !isUseJafTurnedOff()) { strategy = new ServletPathExtensionContentNegotiationStrategy(this.servletContext, this.mediaTypes); } else { diff --git a/spring-web/src/main/java/org/springframework/web/accept/PathExtensionContentNegotiationStrategy.java b/spring-web/src/main/java/org/springframework/web/accept/PathExtensionContentNegotiationStrategy.java index c3c24fe85b..92209600bf 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/PathExtensionContentNegotiationStrategy.java +++ b/spring-web/src/main/java/org/springframework/web/accept/PathExtensionContentNegotiationStrategy.java @@ -63,7 +63,7 @@ public class PathExtensionContentNegotiationStrategy extends AbstractMappingCont urlPathHelper.setUrlDecode(false); } - private boolean useJaf = JAF_PRESENT; + private boolean useJaf = true; /** @@ -109,7 +109,7 @@ public class PathExtensionContentNegotiationStrategy extends AbstractMappingCont @Override protected MediaType handleNoMatch(NativeWebRequest webRequest, String extension) { - if (this.useJaf) { + if (this.useJaf && JAF_PRESENT) { MediaType jafMediaType = JafMediaTypeFactory.getMediaType("file." + extension); if (jafMediaType != null && !MediaType.APPLICATION_OCTET_STREAM.equals(jafMediaType)) { return jafMediaType; diff --git a/spring-web/src/main/java/org/springframework/web/util/UrlPathHelper.java b/spring-web/src/main/java/org/springframework/web/util/UrlPathHelper.java index a9e6adb5ad..63cd869392 100644 --- a/spring-web/src/main/java/org/springframework/web/util/UrlPathHelper.java +++ b/spring-web/src/main/java/org/springframework/web/util/UrlPathHelper.java @@ -405,7 +405,7 @@ public class UrlPathHelper { * @see java.net.URLDecoder#decode(String) */ public String decodeRequestString(HttpServletRequest request, String source) { - if (this.urlDecode) { + if (this.urlDecode && source != null) { return decodeInternal(request, source); } return source; diff --git a/spring-web/src/main/java/org/springframework/web/util/WebUtils.java b/spring-web/src/main/java/org/springframework/web/util/WebUtils.java index b33c6ce6f1..7bf5fd2562 100644 --- a/spring-web/src/main/java/org/springframework/web/util/WebUtils.java +++ b/spring-web/src/main/java/org/springframework/web/util/WebUtils.java @@ -709,20 +709,23 @@ public abstract class WebUtils { } /** - * Extract the full URL filename (including file extension) from the given request URL path. - * Correctly resolves nested paths such as "/products/view.html" as well. + * Extract the full URL filename (including file extension) from the given + * request URL path. Correctly resolve nested paths such as + * "/products/view.html" and remove any path and or query parameters. * @param urlPath the request URL path (e.g. "/products/index.html") * @return the extracted URI filename (e.g. "index.html") */ public static String extractFullFilenameFromUrlPath(String urlPath) { - int end = urlPath.indexOf(';'); + int end = urlPath.indexOf('?'); if (end == -1) { - end = urlPath.indexOf('?'); + end = urlPath.indexOf('#'); if (end == -1) { end = urlPath.length(); } } int begin = urlPath.lastIndexOf('/', end) + 1; + int paramIndex = urlPath.indexOf(';', begin); + end = (paramIndex != -1 && paramIndex < end ? paramIndex : end); return urlPath.substring(begin, end); } diff --git a/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java b/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java index f7924b840e..5a8fdd59f6 100644 --- a/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java +++ b/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java @@ -26,6 +26,8 @@ import org.junit.Before; import org.junit.Test; import org.springframework.http.MediaType; import org.springframework.mock.web.test.MockHttpServletRequest; +import org.springframework.mock.web.test.MockServletContext; +import org.springframework.util.StringUtils; import org.springframework.web.context.request.NativeWebRequest; import org.springframework.web.context.request.ServletWebRequest; @@ -43,7 +45,10 @@ public class ContentNegotiationManagerFactoryBeanTests { @Before public void setup() { - this.servletRequest = new MockHttpServletRequest(); + TestServletContext servletContext = new TestServletContext(); + servletContext.getMimeTypes().put("foo", "application/foo"); + + this.servletRequest = new MockHttpServletRequest(servletContext); this.webRequest = new ServletWebRequest(this.servletRequest); this.factoryBean = new ContentNegotiationManagerFactoryBean(); @@ -74,16 +79,36 @@ public class ContentNegotiationManagerFactoryBeanTests { } @Test - public void addMediaTypes() throws Exception { - Map mediaTypes = new HashMap(); - mediaTypes.put("json", MediaType.APPLICATION_JSON); - this.factoryBean.addMediaTypes(mediaTypes); - + public void favorPath() throws Exception { + this.factoryBean.setFavorPathExtension(true); + this.factoryBean.addMediaTypes(Collections.singletonMap("bar", new MediaType("application", "bar"))); this.factoryBean.afterPropertiesSet(); ContentNegotiationManager manager = this.factoryBean.getObject(); - this.servletRequest.setRequestURI("/flower.json"); - assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest)); + this.servletRequest.setRequestURI("/flower.foo"); + assertEquals(Collections.singletonList(new MediaType("application", "foo")), + manager.resolveMediaTypes(this.webRequest)); + + this.servletRequest.setRequestURI("/flower.bar"); + assertEquals(Collections.singletonList(new MediaType("application", "bar")), + manager.resolveMediaTypes(this.webRequest)); + + this.servletRequest.setRequestURI("/flower.gif"); + assertEquals(Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest)); + } + + @Test + public void favorPathWithJafTurnedOff() throws Exception { + this.factoryBean.setFavorPathExtension(true); + this.factoryBean.setUseJaf(false); + this.factoryBean.afterPropertiesSet(); + ContentNegotiationManager manager = this.factoryBean.getObject(); + + this.servletRequest.setRequestURI("/flower.foo"); + assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest)); + + this.servletRequest.setRequestURI("/flower.gif"); + assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest)); } @Test @@ -130,4 +155,21 @@ public class ContentNegotiationManagerFactoryBeanTests { assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest)); } + + private static class TestServletContext extends MockServletContext { + + private final Map mimeTypes = new HashMap<>(); + + + public Map getMimeTypes() { + return this.mimeTypes; + } + + @Override + public String getMimeType(String filePath) { + String extension = StringUtils.getFilenameExtension(filePath); + return getMimeTypes().get(extension); + } + } + } diff --git a/spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java b/spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java index 437e1db604..f6edf65ffe 100644 --- a/spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java +++ b/spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java @@ -62,9 +62,17 @@ public class WebUtilsTests { assertEquals("index.html", WebUtils.extractFullFilenameFromUrlPath("index.html")); assertEquals("index.html", WebUtils.extractFullFilenameFromUrlPath("/index.html")); assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html")); + assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/a")); + assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/path/a")); + assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/path/a.do")); assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=a")); assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a")); assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a.do")); + assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a#/path/a")); + assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a.do#/path/a.do")); + assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html?param=/path/a.do")); + assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html;r=22?param=/path/a.do")); + assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html;r=22;s=33?param=/path/a.do")); } @Test diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java index 83c564bc23..a67ebe0813 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java @@ -18,26 +18,32 @@ package org.springframework.web.servlet.mvc.method.annotation; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; +import java.util.Locale; import java.util.Set; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.springframework.core.MethodParameter; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpOutputMessage; import org.springframework.http.MediaType; import org.springframework.http.converter.HttpMessageConverter; import org.springframework.http.server.ServletServerHttpRequest; import org.springframework.http.server.ServletServerHttpResponse; import org.springframework.util.CollectionUtils; +import org.springframework.util.StringUtils; import org.springframework.web.HttpMediaTypeNotAcceptableException; import org.springframework.web.accept.ContentNegotiationManager; import org.springframework.web.context.request.NativeWebRequest; import org.springframework.web.context.request.ServletWebRequest; import org.springframework.web.method.support.HandlerMethodReturnValueHandler; import org.springframework.web.servlet.HandlerMapping; +import org.springframework.web.util.UrlPathHelper; /** * Extends {@link AbstractMessageConverterMethodArgumentResolver} with the ability to handle @@ -52,8 +58,24 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe private static final MediaType MEDIA_TYPE_APPLICATION = new MediaType("application"); + private static final UrlPathHelper RAW_URL_PATH_HELPER = new UrlPathHelper(); + + private static final UrlPathHelper DECODING_URL_PATH_HELPER = new UrlPathHelper(); + + static { + RAW_URL_PATH_HELPER.setRemoveSemicolonContent(false); + RAW_URL_PATH_HELPER.setUrlDecode(false); + } + + /* Extensions associated with the built-in message converters */ + private static final Set WHITELISTED_EXTENSIONS = new HashSet(Arrays.asList( + "txt", "text", "json", "xml", "atom", "rss", "png", "jpe", "jpeg", "jpg", "gif", "wbmp", "bmp")); + + private final ContentNegotiationManager contentNegotiationManager; + private final Set safeExtensions = new HashSet(); + protected AbstractMessageConverterMethodProcessor(List> messageConverters) { this(messageConverters, null); @@ -64,6 +86,8 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe super(messageConverters); this.contentNegotiationManager = (manager != null ? manager : new ContentNegotiationManager()); + this.safeExtensions.addAll(this.contentNegotiationManager.getAllFileExtensions()); + this.safeExtensions.addAll(WHITELISTED_EXTENSIONS); } @@ -140,6 +164,7 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe selectedMediaType = selectedMediaType.removeQualityValue(); for (HttpMessageConverter messageConverter : this.messageConverters) { if (messageConverter.canWrite(returnValueClass, selectedMediaType)) { + addContentDispositionHeader(inputMessage, outputMessage); ((HttpMessageConverter) messageConverter).write(returnValue, selectedMediaType, outputMessage); if (logger.isDebugEnabled()) { logger.debug("Written [" + returnValue + "] as \"" + selectedMediaType + "\" using [" + @@ -194,4 +219,48 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe return (MediaType.SPECIFICITY_COMPARATOR.compare(acceptType, produceTypeToUse) <= 0 ? acceptType : produceTypeToUse); } + /** + * Check if the path has a file extension and whether the extension is either + * {@link #WHITELISTED_EXTENSIONS whitelisted} or + * {@link ContentNegotiationManager#getAllFileExtensions() explicitly + * registered}. If not add a 'Content-Disposition' header with a safe + * attachment file name ("f.txt") to prevent RFD exploits. + */ + private void addContentDispositionHeader(ServletServerHttpRequest request, + ServletServerHttpResponse response) { + + HttpHeaders headers = response.getHeaders(); + if (headers.containsKey("Content-Disposition")) { + return; + } + + HttpServletRequest servletRequest = request.getServletRequest(); + String requestUri = RAW_URL_PATH_HELPER.getOriginatingRequestUri(servletRequest); + + int index = requestUri.lastIndexOf('/') + 1; + String filename = requestUri.substring(index); + String pathParams = ""; + + index = filename.indexOf(';'); + if (index != -1) { + pathParams = filename.substring(index); + filename = filename.substring(0, index); + } + + filename = DECODING_URL_PATH_HELPER.decodeRequestString(servletRequest, filename); + String ext = StringUtils.getFilenameExtension(filename); + + pathParams = DECODING_URL_PATH_HELPER.decodeRequestString(servletRequest, pathParams); + String extInPathParams = StringUtils.getFilenameExtension(pathParams); + + if (!isSafeExtension(ext) || !isSafeExtension(extInPathParams)) { + headers.add("Content-Disposition", "attachment;filename=f.txt"); + } + } + + private boolean isSafeExtension(String extension) { + return (!StringUtils.hasText(extension) || + this.safeExtensions.contains(extension.toLowerCase(Locale.ENGLISH))); + } + } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java index 1e287af60e..88147c5c40 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java @@ -19,6 +19,7 @@ package org.springframework.web.servlet.mvc.method.annotation; import java.io.Serializable; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import org.junit.Before; @@ -35,6 +36,7 @@ import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.mock.web.test.MockHttpServletResponse; import org.springframework.util.MultiValueMap; import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; +import org.springframework.web.accept.ContentNegotiationManagerFactoryBean; import org.springframework.web.bind.WebDataBinder; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.support.WebDataBinderFactory; @@ -42,6 +44,7 @@ import org.springframework.web.context.request.NativeWebRequest; import org.springframework.web.context.request.ServletWebRequest; import org.springframework.web.method.HandlerMethod; import org.springframework.web.method.support.ModelAndViewContainer; +import org.springframework.web.util.WebUtils; import static org.junit.Assert.*; @@ -224,6 +227,57 @@ public class RequestResponseBodyMethodProcessorTests { assertEquals("text/plain;charset=UTF-8", servletResponse.getHeader("Content-Type")); } + @Test + public void addContentDispositionHeader() throws Exception { + + ContentNegotiationManagerFactoryBean factory = new ContentNegotiationManagerFactoryBean(); + factory.addMediaType("pdf", new MediaType("application", "pdf")); + factory.afterPropertiesSet(); + + RequestResponseBodyMethodProcessor processor = new RequestResponseBodyMethodProcessor( + Collections.>singletonList(new StringHttpMessageConverter()), + factory.getObject()); + + assertContentDisposition(processor, false, "/hello.json", "whitelisted extension"); + assertContentDisposition(processor, false, "/hello.pdf", "registered extension"); + assertContentDisposition(processor, true, "/hello.dataless", "uknown extension"); + + // path parameters + assertContentDisposition(processor, false, "/hello.json;a=b", "path param shouldn't cause issue"); + assertContentDisposition(processor, true, "/hello.json;a=b;setup.dataless", "uknown ext in path params"); + assertContentDisposition(processor, true, "/hello.dataless;a=b;setup.json", "uknown ext in filename"); + assertContentDisposition(processor, false, "/hello.json;a=b;setup.json", "whitelisted extensions"); + + // encoded dot + assertContentDisposition(processor, true, "/hello%2Edataless;a=b;setup.json", "encoded dot in filename"); + assertContentDisposition(processor, true, "/hello.json;a=b;setup%2Edataless", "encoded dot in path params"); + assertContentDisposition(processor, true, "/hello.dataless%3Bsetup.bat", "encoded dot in path params"); + + this.servletRequest.setAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE, "/hello.bat"); + assertContentDisposition(processor, true, "/bonjour", "forwarded URL"); + this.servletRequest.removeAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE); + } + + private void assertContentDisposition(RequestResponseBodyMethodProcessor processor, + boolean expectContentDisposition, String requestURI, String comment) throws Exception { + + this.servletRequest.setRequestURI(requestURI); + processor.handleReturnValue("body", this.returnTypeString, this.mavContainer, this.webRequest); + + String header = servletResponse.getHeader("Content-Disposition"); + if (expectContentDisposition) { + assertEquals("Expected 'Content-Disposition' header. Use case: '" + comment + "'", + "attachment;filename=f.txt", header); + } + else { + assertNull("Did not expect 'Content-Disposition' header. Use case: '" + comment + "'", header); + } + + this.servletRequest = new MockHttpServletRequest(); + this.servletResponse = new MockHttpServletResponse(); + this.webRequest = new ServletWebRequest(servletRequest, servletResponse); + } + public String handle( @RequestBody List list,