Skip to content

Commit a95c3d8

Browse files
rstoyanchevsnicoll
rstoyanchev
authored and
snicoll
committed
Protect against RFD exploits
Issue: SPR-13548
1 parent 271e5fd commit a95c3d8

File tree

19 files changed

+419
-95
lines changed

19 files changed

+419
-95
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
@@ -96,6 +96,7 @@ protected void writePrefix(JsonGenerator generator, Object object) throws IOExce
9696
String jsonpFunction =
9797
(object instanceof MappingJacksonValue ? ((MappingJacksonValue) object).getJsonpFunction() : null);
9898
if (jsonpFunction != null) {
99+
generator.writeRaw("/**/");
99100
generator.writeRaw(jsonpFunction + "(");
100101
}
101102
}

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,10 @@ public void setUseJaf(boolean useJaf) {
141141
this.useJaf = useJaf;
142142
}
143143

144+
private boolean isUseJafTurnedOff() {
145+
return (this.useJaf != null && !this.useJaf);
146+
}
147+
144148
/**
145149
* Indicate whether a request parameter should be used to determine the
146150
* requested media type with the <em>2nd highest priority</em>, i.e.
@@ -211,7 +215,7 @@ public void afterPropertiesSet() {
211215

212216
if (this.favorPathExtension) {
213217
PathExtensionContentNegotiationStrategy strategy;
214-
if (this.servletContext != null) {
218+
if (this.servletContext != null && !isUseJafTurnedOff()) {
215219
strategy = new ServletPathExtensionContentNegotiationStrategy(this.servletContext, this.mediaTypes);
216220
}
217221
else {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public class PathExtensionContentNegotiationStrategy extends AbstractMappingCont
6464
urlPathHelper.setUrlDecode(false);
6565
}
6666

67-
private boolean useJaf = JAF_PRESENT;
67+
private boolean useJaf = true;
6868

6969
private boolean ignoreUnknownExtensions = true;
7070

@@ -130,7 +130,7 @@ protected void handleMatch(String extension, MediaType mediaType) {
130130
protected MediaType handleNoMatch(NativeWebRequest webRequest, String extension)
131131
throws HttpMediaTypeNotAcceptableException {
132132

133-
if (this.useJaf) {
133+
if (this.useJaf && JAF_PRESENT) {
134134
MediaType jafMediaType = JafMediaTypeFactory.getMediaType("file." + extension);
135135
if (jafMediaType != null && !MediaType.APPLICATION_OCTET_STREAM.equals(jafMediaType)) {
136136
return jafMediaType;

‎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
@@ -405,7 +405,7 @@ private String decodeAndCleanUriString(HttpServletRequest request, String uri) {
405405
* @see java.net.URLDecoder#decode(String)
406406
*/
407407
public String decodeRequestString(HttpServletRequest request, String source) {
408-
if (this.urlDecode) {
408+
if (this.urlDecode && source != null) {
409409
return decodeInternal(request, source);
410410
}
411411
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
@@ -728,20 +728,23 @@ public static String extractFilenameFromUrlPath(String urlPath) {
728728
}
729729

730730
/**
731-
* Extract the full URL filename (including file extension) from the given request URL path.
732-
* Correctly resolves nested paths such as "/products/view.html" as well.
731+
* Extract the full URL filename (including file extension) from the given
732+
* request URL path. Correctly resolve nested paths such as
733+
* "/products/view.html" and remove any path and or query parameters.
733734
* @param urlPath the request URL path (e.g. "/products/index.html")
734735
* @return the extracted URI filename (e.g. "index.html")
735736
*/
736737
public static String extractFullFilenameFromUrlPath(String urlPath) {
737-
int end = urlPath.indexOf(';');
738+
int end = urlPath.indexOf('?');
738739
if (end == -1) {
739-
end = urlPath.indexOf('?');
740+
end = urlPath.indexOf('#');
740741
if (end == -1) {
741742
end = urlPath.length();
742743
}
743744
}
744745
int begin = urlPath.lastIndexOf('/', end) + 1;
746+
int paramIndex = urlPath.indexOf(';', begin);
747+
end = (paramIndex != -1 && paramIndex < end ? paramIndex : end);
745748
return urlPath.substring(begin, end);
746749
}
747750

‎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
@@ -267,7 +267,7 @@ public void jsonp() throws Exception {
267267
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
268268
this.converter.writeInternal(jacksonValue, outputMessage);
269269

270-
assertEquals("callback(\"foo\");", outputMessage.getBodyAsString(Charset.forName("UTF-8")));
270+
assertEquals("/**/callback(\"foo\");", outputMessage.getBodyAsString(Charset.forName("UTF-8")));
271271
}
272272

273273
@Test
@@ -284,7 +284,7 @@ public void jsonpAndJsonView() throws Exception {
284284
this.converter.writeInternal(jacksonValue, outputMessage);
285285

286286
String result = outputMessage.getBodyAsString(Charset.forName("UTF-8"));
287-
assertThat(result, startsWith("callback("));
287+
assertThat(result, startsWith("/**/callback("));
288288
assertThat(result, endsWith(");"));
289289
assertThat(result, containsString("\"withView1\":\"with\""));
290290
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: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -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)