From 2bd1daa75ee0b8ec33608ca6ab065ef3e1815543 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Sun, 4 Oct 2015 21:25:41 -0400 Subject: [PATCH] Protect against RFD exploits Issue: SPR-13548 --- .../MappingJackson2HttpMessageConverter.java | 1 + .../web/accept/ContentNegotiationManager.java | 10 +++ .../ContentNegotiationManagerFactoryBean.java | 7 +- ...thExtensionContentNegotiationStrategy.java | 8 +- .../web/util/UrlPathHelper.java | 2 +- .../springframework/web/util/WebUtils.java | 11 ++- ...pingJackson2HttpMessageConverterTests.java | 4 +- ...entNegotiationManagerFactoryBeanTests.java | 83 +++++++++++++++---- .../web/util/WebUtilsTests.java | 8 ++ .../AbstractJsonpResponseBodyAdvice.java | 33 +++++++- ...stractMessageConverterMethodProcessor.java | 82 +++++++++++++++++- .../view/json/MappingJackson2JsonView.java | 30 ++++++- .../RequestMappingHandlerAdapterTests.java | 77 +++++++++++++---- ...questResponseBodyMethodProcessorTests.java | 62 +++++++++++++- .../json/MappingJackson2JsonViewTests.java | 65 +++++++-------- .../sockjs/support/AbstractSockJsService.java | 17 +++- .../AbstractHttpSendingTransportHandler.java | 16 +++- .../handler/JsonpPollingTransportHandler.java | 2 +- .../sockjs/support/SockJsServiceTests.java | 1 + .../HttpSendingTransportHandlerTests.java | 55 ++++++++---- 20 files changed, 465 insertions(+), 109 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverter.java index 6f7594aaa603..726671ddd19c 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverter.java @@ -95,6 +95,7 @@ protected void writePrefix(JsonGenerator generator, Object object) throws IOExce String jsonpFunction = (object instanceof MappingJacksonValue ? ((MappingJacksonValue) object).getJsonpFunction() : null); if (jsonpFunction != null) { + generator.writeRaw("/**/"); generator.writeRaw(jsonpFunction + "("); } } diff --git a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java index babf45c68e3a..eeba53f7d9c5 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java +++ b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java @@ -120,6 +120,16 @@ public List resolveFileExtensions(MediaType mediaType) { return new ArrayList(result); } + /** + * {@inheritDoc} + *

At startup this method returns extensions explicitly registered with + * either {@link PathExtensionContentNegotiationStrategy} or + * {@link ParameterContentNegotiationStrategy}. At runtime if there is a + * "path extension" strategy and its + * {@link PathExtensionContentNegotiationStrategy#setUseJaf(boolean) + * useJaf} property is set to "true", the list of extensions may + * increase as file extensions are resolved via JAF and cached. + */ @Override public List getAllFileExtensions() { Set result = new LinkedHashSet(); 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 ec62ae516ece..48275f399167 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 @@ -178,6 +178,10 @@ public void setUseJaf(boolean useJaf) { this.useJaf = useJaf; } + private boolean isUseJafTurnedOff() { + return (this.useJaf != null && !this.useJaf); + } + /** * Whether a request parameter ("format" by default) should be used to * determine the requested media type. For this option to work you must @@ -240,7 +244,7 @@ public void afterPropertiesSet() { if (this.favorPathExtension) { PathExtensionContentNegotiationStrategy strategy; - if (this.servletContext != null) { + if (this.servletContext != null && !isUseJafTurnedOff()) { strategy = new ServletPathExtensionContentNegotiationStrategy( this.servletContext, this.mediaTypes); } @@ -272,7 +276,6 @@ public void afterPropertiesSet() { this.contentNegotiationManager = new ContentNegotiationManager(strategies); } - @Override public ContentNegotiationManager getObject() { return this.contentNegotiationManager; 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 a64292823a8e..9b19c27154c1 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 @@ -66,7 +66,8 @@ public class PathExtensionContentNegotiationStrategy PATH_HELPER.setUrlDecode(false); } - private boolean useJaf = JAF_PRESENT; + + private boolean useJaf = true; private boolean ignoreUnknownExtensions = true; @@ -89,8 +90,7 @@ public PathExtensionContentNegotiationStrategy() { /** * Whether to use the Java Activation Framework to look up file extensions. - *

By default if this property is not set JAF is present on the - * classpath it will be used. + *

By default this is set to "true" but depends on JAF being present. */ public void setUseJaf(boolean useJaf) { this.useJaf = useJaf; @@ -123,7 +123,7 @@ protected String getMediaTypeKey(NativeWebRequest webRequest) { protected MediaType handleNoMatch(NativeWebRequest webRequest, String extension) throws HttpMediaTypeNotAcceptableException { - if (this.useJaf) { + if (this.useJaf && JAF_PRESENT) { MediaType mediaType = JafMediaTypeFactory.getMediaType("file." + extension); if (mediaType != null && !MediaType.APPLICATION_OCTET_STREAM.equals(mediaType)) { return mediaType; 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 5d0b207b72d9..1a7a4fe3221f 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 @@ -438,7 +438,7 @@ private String decodeAndCleanUriString(HttpServletRequest request, String uri) { * @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 9a263bbed15c..76c5dd042e27 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 @@ -723,20 +723,23 @@ public static String extractFilenameFromUrlPath(String urlPath) { } /** - * 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/http/converter/json/MappingJackson2HttpMessageConverterTests.java b/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java index 6b1cfbbb779e..51a04ee6025e 100644 --- a/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java +++ b/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java @@ -291,7 +291,7 @@ public void jsonp() throws Exception { MockHttpOutputMessage outputMessage = new MockHttpOutputMessage(); this.converter.writeInternal(jacksonValue, null, outputMessage); - assertEquals("callback(\"foo\");", outputMessage.getBodyAsString(Charset.forName("UTF-8"))); + assertEquals("/**/callback(\"foo\");", outputMessage.getBodyAsString(Charset.forName("UTF-8"))); } @Test @@ -308,7 +308,7 @@ public void jsonpAndJsonView() throws Exception { this.converter.writeInternal(jacksonValue, null, outputMessage); String result = outputMessage.getBodyAsString(Charset.forName("UTF-8")); - assertThat(result, startsWith("callback(")); + assertThat(result, startsWith("/**/callback(")); assertThat(result, endsWith(");")); assertThat(result, containsString("\"withView1\":\"with\"")); assertThat(result, not(containsString("\"withView2\":\"with\""))); 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 a093cf3140f9..e9d95c2a13ac 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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. @@ -15,7 +15,6 @@ */ package org.springframework.web.accept; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -25,11 +24,13 @@ 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.HttpMediaTypeNotAcceptableException; import org.springframework.web.context.request.NativeWebRequest; import org.springframework.web.context.request.ServletWebRequest; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; /** * Test fixture for {@link ContentNegotiationManagerFactoryBean} tests. @@ -46,7 +47,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(); @@ -62,7 +66,7 @@ public void defaultSettings() throws Exception { this.servletRequest.setRequestURI("/flower.gif"); assertEquals("Should be able to resolve file extensions by default", - Arrays.asList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest)); + Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest)); this.servletRequest.setRequestURI("/flower.xyz"); @@ -79,26 +83,46 @@ public void defaultSettings() throws Exception { this.servletRequest.addHeader("Accept", MediaType.IMAGE_GIF_VALUE); assertEquals("Should resolve Accept header by default", - Arrays.asList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest)); + Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest)); } @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.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.json"); - assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest)); + this.servletRequest.setRequestURI("/flower.foo"); + assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest)); + + this.servletRequest.setRequestURI("/flower.gif"); + assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest)); } // SPR-10170 @Test(expected = HttpMediaTypeNotAcceptableException.class) - public void favorPathExtensionWithUnknownMediaType() throws Exception { + public void favorPathWithIgnoreUnknownPathExtensionTurnedOff() throws Exception { this.factoryBean.setFavorPathExtension(true); this.factoryBean.setIgnoreUnknownPathExtensions(false); this.factoryBean.afterPropertiesSet(); @@ -124,7 +148,8 @@ public void favorParameter() throws Exception { this.servletRequest.setRequestURI("/flower"); this.servletRequest.addParameter("format", "json"); - assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest)); + assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON), + manager.resolveMediaTypes(this.webRequest)); } // SPR-10170 @@ -159,26 +184,48 @@ public void setDefaultContentType() throws Exception { this.factoryBean.afterPropertiesSet(); ContentNegotiationManager manager = this.factoryBean.getObject(); - assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest)); + assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON), + manager.resolveMediaTypes(this.webRequest)); // SPR-10513 this.servletRequest.addHeader("Accept", MediaType.ALL_VALUE); - assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest)); + assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON), + manager.resolveMediaTypes(this.webRequest)); } // SPR-12286 + @Test public void setDefaultContentTypeWithStrategy() throws Exception { this.factoryBean.setDefaultContentTypeStrategy(new FixedContentNegotiationStrategy(MediaType.APPLICATION_JSON)); this.factoryBean.afterPropertiesSet(); ContentNegotiationManager manager = this.factoryBean.getObject(); - assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest)); + assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON), + manager.resolveMediaTypes(this.webRequest)); this.servletRequest.addHeader("Accept", MediaType.ALL_VALUE); - assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest)); + assertEquals(Collections.singletonList(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 8f83ea7fb055..1ad98fd255be 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 @@ -70,9 +70,17 @@ public void extractFullFilenameFromUrlPath() { 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/AbstractJsonpResponseBodyAdvice.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractJsonpResponseBodyAdvice.java index a8d983d38592..0a383c6c57e1 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractJsonpResponseBodyAdvice.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractJsonpResponseBodyAdvice.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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,8 +16,12 @@ package org.springframework.web.servlet.mvc.method.annotation; +import java.util.regex.Pattern; import javax.servlet.http.HttpServletRequest; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + import org.springframework.core.MethodParameter; import org.springframework.http.MediaType; import org.springframework.http.converter.json.MappingJacksonValue; @@ -44,6 +48,14 @@ */ public abstract class AbstractJsonpResponseBodyAdvice extends AbstractMappingJacksonResponseBodyAdvice { + /** + * Pattern for validating jsonp callback parameter values. + */ + private static final Pattern CALLBACK_PARAM_PATTERN = Pattern.compile("[0-9A-Za-z_\\.]*"); + + + private final Log logger = LogFactory.getLog(getClass()); + private final String[] jsonpQueryParamNames; @@ -62,14 +74,31 @@ protected void beforeBodyWriteInternal(MappingJacksonValue bodyContainer, MediaT for (String name : this.jsonpQueryParamNames) { String value = servletRequest.getParameter(name); if (value != null) { + if (!isValidJsonpQueryParam(value)) { + if (logger.isDebugEnabled()) { + logger.debug("Ignoring invalid jsonp parameter value: " + value); + } + continue; + } MediaType contentTypeToUse = getContentType(contentType, request, response); response.getHeaders().setContentType(contentTypeToUse); bodyContainer.setJsonpFunction(value); - return; + break; } } } + /** + * Validate the jsonp query parameter value. The default implementation + * returns true if it consists of digits, letters, or "_" and ".". + * Invalid parameter values are ignored. + * @param value the query param value, never {@code null} + * @since 4.1.8 + */ + protected boolean isValidJsonpQueryParam(String value) { + return CALLBACK_PARAM_PATTERN.matcher(value).matches(); + } + /** * Return the content type to set the response to. * This implementation always returns "application/javascript". 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 93a45c948377..eeccf439d590 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 @@ -19,15 +19,19 @@ import java.io.IOException; import java.lang.reflect.Type; 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.HttpEntity; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpOutputMessage; import org.springframework.http.MediaType; import org.springframework.http.converter.GenericHttpMessageConverter; @@ -36,12 +40,14 @@ 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 @@ -56,24 +62,52 @@ 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(); + + /** + * Constructor with list of converters only. + */ protected AbstractMessageConverterMethodProcessor(List> converters) { this(converters, null); } + /** + * Constructor with list of converters and ContentNegotiationManager. + */ protected AbstractMessageConverterMethodProcessor(List> converters, ContentNegotiationManager contentNegotiationManager) { this(converters, contentNegotiationManager, null); } + /** + * Constructor with list of converters and ContentNegotiationManager as well + * as request/response body advice instances. + */ protected AbstractMessageConverterMethodProcessor(List> converters, ContentNegotiationManager manager, List requestResponseBodyAdvice) { super(converters, requestResponseBodyAdvice); this.contentNegotiationManager = (manager != null ? manager : new ContentNegotiationManager()); + this.safeExtensions.addAll(this.contentNegotiationManager.getAllFileExtensions()); + this.safeExtensions.addAll(WHITELISTED_EXTENSIONS); } @@ -164,6 +198,7 @@ else if (mediaType.equals(MediaType.ALL) || mediaType.equals(MEDIA_TYPE_APPLICAT (Class>) messageConverter.getClass(), inputMessage, outputMessage); if (returnValue != null) { + addContentDispositionHeader(inputMessage, outputMessage); ((GenericHttpMessageConverter) messageConverter).write(returnValue, returnValueType, selectedMediaType, outputMessage); if (logger.isDebugEnabled()) { @@ -179,6 +214,7 @@ else if (messageConverter.canWrite(returnValueClass, selectedMediaType)) { (Class>) messageConverter.getClass(), inputMessage, outputMessage); if (returnValue != null) { + addContentDispositionHeader(inputMessage, outputMessage); ((HttpMessageConverter) messageConverter).write(returnValue, selectedMediaType, outputMessage); if (logger.isDebugEnabled()) { @@ -225,7 +261,7 @@ private Type getGenericType(MethodParameter returnType) { /** * @see #getProducibleMediaTypes(HttpServletRequest, Class, Type) */ - @SuppressWarnings("unchecked") + @SuppressWarnings({"unchecked", "unused"}) protected List getProducibleMediaTypes(HttpServletRequest request, Class returnValueClass) { return getProducibleMediaTypes(request, returnValueClass, null); } @@ -278,4 +314,48 @@ private MediaType getMostSpecificMediaType(MediaType acceptType, MediaType produ 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(HttpHeaders.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(HttpHeaders.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/main/java/org/springframework/web/servlet/view/json/MappingJackson2JsonView.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/view/json/MappingJackson2JsonView.java index afc3067e02fd..2039e8c0ea7d 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/view/json/MappingJackson2JsonView.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/view/json/MappingJackson2JsonView.java @@ -23,6 +23,7 @@ import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; +import java.util.regex.Pattern; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -70,6 +71,12 @@ public class MappingJackson2JsonView extends AbstractJackson2View { */ public static final String DEFAULT_JSONP_CONTENT_TYPE = "application/javascript"; + /** + * Pattern for validating jsonp callback parameter values. + */ + private static final Pattern CALLBACK_PARAM_PATTERN = Pattern.compile("[0-9A-Za-z_\\.]*"); + + private String jsonPrefix; private Set modelKeys; @@ -170,14 +177,32 @@ private String getJsonpParameterValue(HttpServletRequest request) { if (this.jsonpParameterNames != null) { for (String name : this.jsonpParameterNames) { String value = request.getParameter(name); - if (!StringUtils.isEmpty(value)) { - return value; + if (StringUtils.isEmpty(value)) { + continue; + } + if (!isValidJsonpQueryParam(value)) { + if (logger.isDebugEnabled()) { + logger.debug("Ignoring invalid jsonp parameter value: " + value); + } + continue; } + return value; } } return null; } + /** + * Validate the jsonp query parameter value. The default implementation + * returns true if it consists of digits, letters, or "_" and ".". + * Invalid parameter values are ignored. + * @param value the query param value, never {@code null} + * @since 4.1.8 + */ + protected boolean isValidJsonpQueryParam(String value) { + return CALLBACK_PARAM_PATTERN.matcher(value).matches(); + } + /** * Filter out undesired attributes from the given model. * The return value can be either another {@link Map} or a single value object. @@ -228,6 +253,7 @@ protected void writePrefix(JsonGenerator generator, Object object) throws IOExce jsonpFunction = ((MappingJacksonValue) object).getJsonpFunction(); } if (jsonpFunction != null) { + generator.writeRaw("/**/"); generator.writeRaw(jsonpFunction + "(" ); } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java index acbb8fc73e25..dcbb9554ecd7 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java @@ -19,6 +19,7 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -53,7 +54,8 @@ import org.springframework.web.servlet.FlashMap; import org.springframework.web.servlet.ModelAndView; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; /** * Unit tests for {@link RequestMappingHandlerAdapter}. @@ -128,7 +130,7 @@ public void setAlwaysUseRedirectAttributes() throws Exception { HandlerMethodReturnValueHandler viewHandler = new ViewNameMethodReturnValueHandler(); this.handlerAdapter.setArgumentResolvers(Arrays.asList(redirectAttributesResolver, modelResolver)); - this.handlerAdapter.setReturnValueHandlers(Arrays.asList(viewHandler)); + this.handlerAdapter.setReturnValueHandlers(Collections.singletonList(viewHandler)); this.handlerAdapter.setIgnoreDefaultModelOnRedirect(true); this.handlerAdapter.afterPropertiesSet(); @@ -143,7 +145,7 @@ public void setAlwaysUseRedirectAttributes() throws Exception { @Test public void setCustomArgumentResolvers() throws Exception { HandlerMethodArgumentResolver resolver = new ServletRequestMethodArgumentResolver(); - this.handlerAdapter.setCustomArgumentResolvers(Arrays.asList(resolver)); + this.handlerAdapter.setCustomArgumentResolvers(Collections.singletonList(resolver)); this.handlerAdapter.afterPropertiesSet(); assertTrue(this.handlerAdapter.getArgumentResolvers().contains(resolver)); @@ -153,7 +155,7 @@ public void setCustomArgumentResolvers() throws Exception { @Test public void setArgumentResolvers() throws Exception { HandlerMethodArgumentResolver resolver = new ServletRequestMethodArgumentResolver(); - this.handlerAdapter.setArgumentResolvers(Arrays.asList(resolver)); + this.handlerAdapter.setArgumentResolvers(Collections.singletonList(resolver)); this.handlerAdapter.afterPropertiesSet(); assertMethodProcessorCount(1, INIT_BINDER_RESOLVER_COUNT, HANDLER_COUNT); @@ -162,7 +164,7 @@ public void setArgumentResolvers() throws Exception { @Test public void setInitBinderArgumentResolvers() throws Exception { HandlerMethodArgumentResolver resolver = new ServletRequestMethodArgumentResolver(); - this.handlerAdapter.setInitBinderArgumentResolvers(Arrays.asList(resolver)); + this.handlerAdapter.setInitBinderArgumentResolvers(Collections.singletonList(resolver)); this.handlerAdapter.afterPropertiesSet(); assertMethodProcessorCount(RESOLVER_COUNT, 1, HANDLER_COUNT); @@ -171,7 +173,7 @@ public void setInitBinderArgumentResolvers() throws Exception { @Test public void setCustomReturnValueHandlers() { HandlerMethodReturnValueHandler handler = new ViewNameMethodReturnValueHandler(); - this.handlerAdapter.setCustomReturnValueHandlers(Arrays.asList(handler)); + this.handlerAdapter.setCustomReturnValueHandlers(Collections.singletonList(handler)); this.handlerAdapter.afterPropertiesSet(); assertTrue(this.handlerAdapter.getReturnValueHandlers().contains(handler)); @@ -181,7 +183,7 @@ public void setCustomReturnValueHandlers() { @Test public void setReturnValueHandlers() { HandlerMethodReturnValueHandler handler = new ModelMethodProcessor(); - this.handlerAdapter.setReturnValueHandlers(Arrays.asList(handler)); + this.handlerAdapter.setReturnValueHandlers(Collections.singletonList(handler)); this.handlerAdapter.afterPropertiesSet(); assertMethodProcessorCount(RESOLVER_COUNT, INIT_BINDER_RESOLVER_COUNT, 1); @@ -240,20 +242,37 @@ public void responseBodyAdvice() throws Exception { this.handlerAdapter.setMessageConverters(converters); this.webAppContext.registerSingleton("rba", ResponseCodeSuppressingAdvice.class); - this.webAppContext.registerSingleton("ja", JsonpAdvice.class); this.webAppContext.refresh(); this.request.addHeader("Accept", MediaType.APPLICATION_JSON_VALUE); this.request.setParameter("c", "callback"); - HandlerMethod handlerMethod = handlerMethod(new SimpleController(), "handleWithResponseEntity"); + HandlerMethod handlerMethod = handlerMethod(new SimpleController(), "handleBadRequest"); this.handlerAdapter.afterPropertiesSet(); this.handlerAdapter.handle(this.request, this.response, handlerMethod); assertEquals(200, this.response.getStatus()); - assertEquals("callback({\"status\":400,\"message\":\"body\"});", this.response.getContentAsString()); + assertEquals("{\"status\":400,\"message\":\"body\"}", this.response.getContentAsString()); } + @Test + public void jsonpResponseBodyAdvice() throws Exception { + + List> converters = new ArrayList<>(); + converters.add(new MappingJackson2HttpMessageConverter()); + this.handlerAdapter.setMessageConverters(converters); + + this.webAppContext.registerSingleton("jsonpAdvice", JsonpAdvice.class); + this.webAppContext.refresh(); + + testJsonp("callback", true); + testJsonp("_callback", true); + testJsonp("_Call.bAcK", true); + testJsonp("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_.", true); + + testJsonp("