Skip to content

Commit 2bd1daa

Browse files
rstoyanchevsnicoll
rstoyanchev
authored and
snicoll
committed
Protect against RFD exploits
Issue: SPR-13548
1 parent 161fd98 commit 2bd1daa

File tree

20 files changed

+465
-109
lines changed

20 files changed

+465
-109
lines changed

‎spring-web/src/main/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverter.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ protected void writePrefix(JsonGenerator generator, Object object) throws IOExce
9595
String jsonpFunction =
9696
(object instanceof MappingJacksonValue ? ((MappingJacksonValue) object).getJsonpFunction() : null);
9797
if (jsonpFunction != null) {
98+
generator.writeRaw("/**/");
9899
generator.writeRaw(jsonpFunction + "(");
99100
}
100101
}

‎spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,16 @@ public List<String> resolveFileExtensions(MediaType mediaType) {
120120
return new ArrayList<String>(result);
121121
}
122122

123+
/**
124+
* {@inheritDoc}
125+
* <p>At startup this method returns extensions explicitly registered with
126+
* either {@link PathExtensionContentNegotiationStrategy} or
127+
* {@link ParameterContentNegotiationStrategy}. At runtime if there is a
128+
* "path extension" strategy and its
129+
* {@link PathExtensionContentNegotiationStrategy#setUseJaf(boolean)
130+
* useJaf} property is set to "true", the list of extensions may
131+
* increase as file extensions are resolved via JAF and cached.
132+
*/
123133
@Override
124134
public List<String> getAllFileExtensions() {
125135
Set<String> result = new LinkedHashSet<String>();

‎spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,10 @@ public void setUseJaf(boolean useJaf) {
178178
this.useJaf = useJaf;
179179
}
180180

181+
private boolean isUseJafTurnedOff() {
182+
return (this.useJaf != null && !this.useJaf);
183+
}
184+
181185
/**
182186
* Whether a request parameter ("format" by default) should be used to
183187
* determine the requested media type. For this option to work you must
@@ -240,7 +244,7 @@ public void afterPropertiesSet() {
240244

241245
if (this.favorPathExtension) {
242246
PathExtensionContentNegotiationStrategy strategy;
243-
if (this.servletContext != null) {
247+
if (this.servletContext != null && !isUseJafTurnedOff()) {
244248
strategy = new ServletPathExtensionContentNegotiationStrategy(
245249
this.servletContext, this.mediaTypes);
246250
}
@@ -272,7 +276,6 @@ public void afterPropertiesSet() {
272276
this.contentNegotiationManager = new ContentNegotiationManager(strategies);
273277
}
274278

275-
276279
@Override
277280
public ContentNegotiationManager getObject() {
278281
return this.contentNegotiationManager;

‎spring-web/src/main/java/org/springframework/web/accept/PathExtensionContentNegotiationStrategy.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,8 @@ public class PathExtensionContentNegotiationStrategy
6666
PATH_HELPER.setUrlDecode(false);
6767
}
6868

69-
private boolean useJaf = JAF_PRESENT;
69+
70+
private boolean useJaf = true;
7071

7172
private boolean ignoreUnknownExtensions = true;
7273

@@ -89,8 +90,7 @@ public PathExtensionContentNegotiationStrategy() {
8990

9091
/**
9192
* Whether to use the Java Activation Framework to look up file extensions.
92-
* <p>By default if this property is not set JAF is present on the
93-
* classpath it will be used.
93+
* <p>By default this is set to "true" but depends on JAF being present.
9494
*/
9595
public void setUseJaf(boolean useJaf) {
9696
this.useJaf = useJaf;
@@ -123,7 +123,7 @@ protected String getMediaTypeKey(NativeWebRequest webRequest) {
123123
protected MediaType handleNoMatch(NativeWebRequest webRequest, String extension)
124124
throws HttpMediaTypeNotAcceptableException {
125125

126-
if (this.useJaf) {
126+
if (this.useJaf && JAF_PRESENT) {
127127
MediaType mediaType = JafMediaTypeFactory.getMediaType("file." + extension);
128128
if (mediaType != null && !MediaType.APPLICATION_OCTET_STREAM.equals(mediaType)) {
129129
return mediaType;

‎spring-web/src/main/java/org/springframework/web/util/UrlPathHelper.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,7 @@ private String decodeAndCleanUriString(HttpServletRequest request, String uri) {
438438
* @see java.net.URLDecoder#decode(String)
439439
*/
440440
public String decodeRequestString(HttpServletRequest request, String source) {
441-
if (this.urlDecode) {
441+
if (this.urlDecode && source != null) {
442442
return decodeInternal(request, source);
443443
}
444444
return source;

‎spring-web/src/main/java/org/springframework/web/util/WebUtils.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -723,20 +723,23 @@ public static String extractFilenameFromUrlPath(String urlPath) {
723723
}
724724

725725
/**
726-
* Extract the full URL filename (including file extension) from the given request URL path.
727-
* Correctly resolves nested paths such as "/products/view.html" as well.
726+
* Extract the full URL filename (including file extension) from the given
727+
* request URL path. Correctly resolve nested paths such as
728+
* "/products/view.html" and remove any path and or query parameters.
728729
* @param urlPath the request URL path (e.g. "/products/index.html")
729730
* @return the extracted URI filename (e.g. "index.html")
730731
*/
731732
public static String extractFullFilenameFromUrlPath(String urlPath) {
732-
int end = urlPath.indexOf(';');
733+
int end = urlPath.indexOf('?');
733734
if (end == -1) {
734-
end = urlPath.indexOf('?');
735+
end = urlPath.indexOf('#');
735736
if (end == -1) {
736737
end = urlPath.length();
737738
}
738739
}
739740
int begin = urlPath.lastIndexOf('/', end) + 1;
741+
int paramIndex = urlPath.indexOf(';', begin);
742+
end = (paramIndex != -1 && paramIndex < end ? paramIndex : end);
740743
return urlPath.substring(begin, end);
741744
}
742745

‎spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ public void jsonp() throws Exception {
291291
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
292292
this.converter.writeInternal(jacksonValue, null, outputMessage);
293293

294-
assertEquals("callback(\"foo\");", outputMessage.getBodyAsString(Charset.forName("UTF-8")));
294+
assertEquals("/**/callback(\"foo\");", outputMessage.getBodyAsString(Charset.forName("UTF-8")));
295295
}
296296

297297
@Test
@@ -308,7 +308,7 @@ public void jsonpAndJsonView() throws Exception {
308308
this.converter.writeInternal(jacksonValue, null, outputMessage);
309309

310310
String result = outputMessage.getBodyAsString(Charset.forName("UTF-8"));
311-
assertThat(result, startsWith("callback("));
311+
assertThat(result, startsWith("/**/callback("));
312312
assertThat(result, endsWith(");"));
313313
assertThat(result, containsString("\"withView1\":\"with\""));
314314
assertThat(result, not(containsString("\"withView2\":\"with\"")));

‎spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java

Lines changed: 65 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2014 the original author or authors.
2+
* Copyright 2002-2015 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.
@@ -15,7 +15,6 @@
1515
*/
1616
package org.springframework.web.accept;
1717

18-
import java.util.Arrays;
1918
import java.util.Collections;
2019
import java.util.HashMap;
2120
import java.util.Map;
@@ -25,11 +24,13 @@
2524

2625
import org.springframework.http.MediaType;
2726
import org.springframework.mock.web.test.MockHttpServletRequest;
27+
import org.springframework.mock.web.test.MockServletContext;
28+
import org.springframework.util.StringUtils;
2829
import org.springframework.web.HttpMediaTypeNotAcceptableException;
2930
import org.springframework.web.context.request.NativeWebRequest;
3031
import org.springframework.web.context.request.ServletWebRequest;
3132

32-
import static org.junit.Assert.*;
33+
import static org.junit.Assert.assertEquals;
3334

3435
/**
3536
* Test fixture for {@link ContentNegotiationManagerFactoryBean} tests.
@@ -46,7 +47,10 @@ public class ContentNegotiationManagerFactoryBeanTests {
4647

4748
@Before
4849
public void setup() {
49-
this.servletRequest = new MockHttpServletRequest();
50+
TestServletContext servletContext = new TestServletContext();
51+
servletContext.getMimeTypes().put("foo", "application/foo");
52+
53+
this.servletRequest = new MockHttpServletRequest(servletContext);
5054
this.webRequest = new ServletWebRequest(this.servletRequest);
5155

5256
this.factoryBean = new ContentNegotiationManagerFactoryBean();
@@ -62,7 +66,7 @@ public void defaultSettings() throws Exception {
6266
this.servletRequest.setRequestURI("/flower.gif");
6367

6468
assertEquals("Should be able to resolve file extensions by default",
65-
Arrays.asList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
69+
Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
6670

6771
this.servletRequest.setRequestURI("/flower.xyz");
6872

@@ -79,26 +83,46 @@ public void defaultSettings() throws Exception {
7983
this.servletRequest.addHeader("Accept", MediaType.IMAGE_GIF_VALUE);
8084

8185
assertEquals("Should resolve Accept header by default",
82-
Arrays.asList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
86+
Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
8387
}
8488

8589
@Test
86-
public void addMediaTypes() throws Exception {
87-
Map<String, MediaType> mediaTypes = new HashMap<>();
88-
mediaTypes.put("json", MediaType.APPLICATION_JSON);
89-
this.factoryBean.addMediaTypes(mediaTypes);
90+
public void favorPath() throws Exception {
91+
this.factoryBean.setFavorPathExtension(true);
92+
this.factoryBean.addMediaTypes(Collections.singletonMap("bar", new MediaType("application", "bar")));
93+
this.factoryBean.afterPropertiesSet();
94+
ContentNegotiationManager manager = this.factoryBean.getObject();
95+
96+
this.servletRequest.setRequestURI("/flower.foo");
97+
assertEquals(Collections.singletonList(new MediaType("application", "foo")),
98+
manager.resolveMediaTypes(this.webRequest));
99+
100+
this.servletRequest.setRequestURI("/flower.bar");
101+
assertEquals(Collections.singletonList(new MediaType("application", "bar")),
102+
manager.resolveMediaTypes(this.webRequest));
103+
104+
this.servletRequest.setRequestURI("/flower.gif");
105+
assertEquals(Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
106+
}
90107

108+
@Test
109+
public void favorPathWithJafTurnedOff() throws Exception {
110+
this.factoryBean.setFavorPathExtension(true);
111+
this.factoryBean.setUseJaf(false);
91112
this.factoryBean.afterPropertiesSet();
92113
ContentNegotiationManager manager = this.factoryBean.getObject();
93114

94-
this.servletRequest.setRequestURI("/flower.json");
95-
assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
115+
this.servletRequest.setRequestURI("/flower.foo");
116+
assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest));
117+
118+
this.servletRequest.setRequestURI("/flower.gif");
119+
assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest));
96120
}
97121

98122
// SPR-10170
99123

100124
@Test(expected = HttpMediaTypeNotAcceptableException.class)
101-
public void favorPathExtensionWithUnknownMediaType() throws Exception {
125+
public void favorPathWithIgnoreUnknownPathExtensionTurnedOff() throws Exception {
102126
this.factoryBean.setFavorPathExtension(true);
103127
this.factoryBean.setIgnoreUnknownPathExtensions(false);
104128
this.factoryBean.afterPropertiesSet();
@@ -124,7 +148,8 @@ public void favorParameter() throws Exception {
124148
this.servletRequest.setRequestURI("/flower");
125149
this.servletRequest.addParameter("format", "json");
126150

127-
assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
151+
assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
152+
manager.resolveMediaTypes(this.webRequest));
128153
}
129154

130155
// SPR-10170
@@ -159,26 +184,48 @@ public void setDefaultContentType() throws Exception {
159184
this.factoryBean.afterPropertiesSet();
160185
ContentNegotiationManager manager = this.factoryBean.getObject();
161186

162-
assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
187+
assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
188+
manager.resolveMediaTypes(this.webRequest));
163189

164190
// SPR-10513
165191

166192
this.servletRequest.addHeader("Accept", MediaType.ALL_VALUE);
167193

168-
assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
194+
assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
195+
manager.resolveMediaTypes(this.webRequest));
169196
}
170197

171198
// SPR-12286
199+
172200
@Test
173201
public void setDefaultContentTypeWithStrategy() throws Exception {
174202
this.factoryBean.setDefaultContentTypeStrategy(new FixedContentNegotiationStrategy(MediaType.APPLICATION_JSON));
175203
this.factoryBean.afterPropertiesSet();
176204
ContentNegotiationManager manager = this.factoryBean.getObject();
177205

178-
assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
206+
assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
207+
manager.resolveMediaTypes(this.webRequest));
179208

180209
this.servletRequest.addHeader("Accept", MediaType.ALL_VALUE);
181-
assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
210+
assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
211+
manager.resolveMediaTypes(this.webRequest));
212+
}
213+
214+
215+
private static class TestServletContext extends MockServletContext {
216+
217+
private final Map<String, String> mimeTypes = new HashMap<>();
218+
219+
220+
public Map<String, String> getMimeTypes() {
221+
return this.mimeTypes;
222+
}
223+
224+
@Override
225+
public String getMimeType(String filePath) {
226+
String extension = StringUtils.getFilenameExtension(filePath);
227+
return getMimeTypes().get(extension);
228+
}
182229
}
183230

184231
}

‎spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,17 @@ public void extractFullFilenameFromUrlPath() {
7070
assertEquals("index.html", WebUtils.extractFullFilenameFromUrlPath("index.html"));
7171
assertEquals("index.html", WebUtils.extractFullFilenameFromUrlPath("/index.html"));
7272
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html"));
73+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/a"));
74+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/path/a"));
75+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/path/a.do"));
7376
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=a"));
7477
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a"));
7578
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a.do"));
79+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a#/path/a"));
80+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a.do#/path/a.do"));
81+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html?param=/path/a.do"));
82+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html;r=22?param=/path/a.do"));
83+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html;r=22;s=33?param=/path/a.do"));
7684
}
7785

7886
@Test

‎spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractJsonpResponseBodyAdvice.java

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2014 the original author or authors.
2+
* Copyright 2002-2015 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,8 +16,12 @@
1616

1717
package org.springframework.web.servlet.mvc.method.annotation;
1818

19+
import java.util.regex.Pattern;
1920
import javax.servlet.http.HttpServletRequest;
2021

22+
import org.apache.commons.logging.Log;
23+
import org.apache.commons.logging.LogFactory;
24+
2125
import org.springframework.core.MethodParameter;
2226
import org.springframework.http.MediaType;
2327
import org.springframework.http.converter.json.MappingJacksonValue;
@@ -44,6 +48,14 @@
4448
*/
4549
public abstract class AbstractJsonpResponseBodyAdvice extends AbstractMappingJacksonResponseBodyAdvice {
4650

51+
/**
52+
* Pattern for validating jsonp callback parameter values.
53+
*/
54+
private static final Pattern CALLBACK_PARAM_PATTERN = Pattern.compile("[0-9A-Za-z_\\.]*");
55+
56+
57+
private final Log logger = LogFactory.getLog(getClass());
58+
4759
private final String[] jsonpQueryParamNames;
4860

4961

@@ -62,14 +74,31 @@ protected void beforeBodyWriteInternal(MappingJacksonValue bodyContainer, MediaT
6274
for (String name : this.jsonpQueryParamNames) {
6375
String value = servletRequest.getParameter(name);
6476
if (value != null) {
77+
if (!isValidJsonpQueryParam(value)) {
78+
if (logger.isDebugEnabled()) {
79+
logger.debug("Ignoring invalid jsonp parameter value: " + value);
80+
}
81+
continue;
82+
}
6583
MediaType contentTypeToUse = getContentType(contentType, request, response);
6684
response.getHeaders().setContentType(contentTypeToUse);
6785
bodyContainer.setJsonpFunction(value);
68-
return;
86+
break;
6987
}
7088
}
7189
}
7290

91+
/**
92+
* Validate the jsonp query parameter value. The default implementation
93+
* returns true if it consists of digits, letters, or "_" and ".".
94+
* Invalid parameter values are ignored.
95+
* @param value the query param value, never {@code null}
96+
* @since 4.1.8
97+
*/
98+
protected boolean isValidJsonpQueryParam(String value) {
99+
return CALLBACK_PARAM_PATTERN.matcher(value).matches();
100+
}
101+
73102
/**
74103
* Return the content type to set the response to.
75104
* This implementation always returns "application/javascript".

0 commit comments

Comments
 (0)