Skip to content

Commit 03f547e

Browse files
rstoyanchevsnicoll
rstoyanchev
authored and
snicoll
committed
Protect against RFD exploits
Issue: SPR-13548
1 parent daada71 commit 03f547e

File tree

8 files changed

+195
-15
lines changed

8 files changed

+195
-15
lines changed

‎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
@@ -128,6 +128,10 @@ public void setUseJaf(boolean useJaf) {
128128
this.useJaf = useJaf;
129129
}
130130

131+
private boolean isUseJafTurnedOff() {
132+
return (this.useJaf != null && !this.useJaf);
133+
}
134+
131135
/**
132136
* Indicate whether a request parameter should be used to determine the
133137
* requested media type with the <em>2nd highest priority</em>, i.e.
@@ -184,7 +188,7 @@ public void afterPropertiesSet() {
184188

185189
if (this.favorPathExtension) {
186190
PathExtensionContentNegotiationStrategy strategy;
187-
if (this.servletContext != null) {
191+
if (this.servletContext != null && !isUseJafTurnedOff()) {
188192
strategy = new ServletPathExtensionContentNegotiationStrategy(this.servletContext, this.mediaTypes);
189193
}
190194
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
@@ -63,7 +63,7 @@ public class PathExtensionContentNegotiationStrategy extends AbstractMappingCont
6363
urlPathHelper.setUrlDecode(false);
6464
}
6565

66-
private boolean useJaf = JAF_PRESENT;
66+
private boolean useJaf = true;
6767

6868

6969
/**
@@ -109,7 +109,7 @@ protected void handleMatch(String extension, MediaType mediaType) {
109109

110110
@Override
111111
protected MediaType handleNoMatch(NativeWebRequest webRequest, String extension) {
112-
if (this.useJaf) {
112+
if (this.useJaf && JAF_PRESENT) {
113113
MediaType jafMediaType = JafMediaTypeFactory.getMediaType("file." + extension);
114114
if (jafMediaType != null && !MediaType.APPLICATION_OCTET_STREAM.equals(jafMediaType)) {
115115
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
@@ -709,20 +709,23 @@ public static String extractFilenameFromUrlPath(String urlPath) {
709709
}
710710

711711
/**
712-
* Extract the full URL filename (including file extension) from the given request URL path.
713-
* Correctly resolves nested paths such as "/products/view.html" as well.
712+
* Extract the full URL filename (including file extension) from the given
713+
* request URL path. Correctly resolve nested paths such as
714+
* "/products/view.html" and remove any path and or query parameters.
714715
* @param urlPath the request URL path (e.g. "/products/index.html")
715716
* @return the extracted URI filename (e.g. "index.html")
716717
*/
717718
public static String extractFullFilenameFromUrlPath(String urlPath) {
718-
int end = urlPath.indexOf(';');
719+
int end = urlPath.indexOf('?');
719720
if (end == -1) {
720-
end = urlPath.indexOf('?');
721+
end = urlPath.indexOf('#');
721722
if (end == -1) {
722723
end = urlPath.length();
723724
}
724725
}
725726
int begin = urlPath.lastIndexOf('/', end) + 1;
727+
int paramIndex = urlPath.indexOf(';', begin);
728+
end = (paramIndex != -1 && paramIndex < end ? paramIndex : end);
726729
return urlPath.substring(begin, end);
727730
}
728731

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

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626
import org.junit.Test;
2727
import org.springframework.http.MediaType;
2828
import org.springframework.mock.web.test.MockHttpServletRequest;
29+
import org.springframework.mock.web.test.MockServletContext;
30+
import org.springframework.util.StringUtils;
2931
import org.springframework.web.context.request.NativeWebRequest;
3032
import org.springframework.web.context.request.ServletWebRequest;
3133

@@ -43,7 +45,10 @@ public class ContentNegotiationManagerFactoryBeanTests {
4345

4446
@Before
4547
public void setup() {
46-
this.servletRequest = new MockHttpServletRequest();
48+
TestServletContext servletContext = new TestServletContext();
49+
servletContext.getMimeTypes().put("foo", "application/foo");
50+
51+
this.servletRequest = new MockHttpServletRequest(servletContext);
4752
this.webRequest = new ServletWebRequest(this.servletRequest);
4853

4954
this.factoryBean = new ContentNegotiationManagerFactoryBean();
@@ -74,16 +79,36 @@ public void defaultSettings() throws Exception {
7479
}
7580

7681
@Test
77-
public void addMediaTypes() throws Exception {
78-
Map<String, MediaType> mediaTypes = new HashMap<String, MediaType>();
79-
mediaTypes.put("json", MediaType.APPLICATION_JSON);
80-
this.factoryBean.addMediaTypes(mediaTypes);
82+
public void favorPath() throws Exception {
83+
this.factoryBean.setFavorPathExtension(true);
84+
this.factoryBean.addMediaTypes(Collections.singletonMap("bar", new MediaType("application", "bar")));
85+
this.factoryBean.afterPropertiesSet();
86+
ContentNegotiationManager manager = this.factoryBean.getObject();
8187

88+
this.servletRequest.setRequestURI("/flower.foo");
89+
assertEquals(Collections.singletonList(new MediaType("application", "foo")),
90+
manager.resolveMediaTypes(this.webRequest));
91+
92+
this.servletRequest.setRequestURI("/flower.bar");
93+
assertEquals(Collections.singletonList(new MediaType("application", "bar")),
94+
manager.resolveMediaTypes(this.webRequest));
95+
96+
this.servletRequest.setRequestURI("/flower.gif");
97+
assertEquals(Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
98+
}
99+
100+
@Test
101+
public void favorPathWithJafTurnedOff() throws Exception {
102+
this.factoryBean.setFavorPathExtension(true);
103+
this.factoryBean.setUseJaf(false);
82104
this.factoryBean.afterPropertiesSet();
83105
ContentNegotiationManager manager = this.factoryBean.getObject();
84106

85-
this.servletRequest.setRequestURI("/flower.json");
86-
assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
107+
this.servletRequest.setRequestURI("/flower.foo");
108+
assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest));
109+
110+
this.servletRequest.setRequestURI("/flower.gif");
111+
assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest));
87112
}
88113

89114
@Test
@@ -130,4 +155,21 @@ public void setDefaultContentType() throws Exception {
130155
assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
131156
}
132157

158+
159+
private static class TestServletContext extends MockServletContext {
160+
161+
private final Map<String, String> mimeTypes = new HashMap<>();
162+
163+
164+
public Map<String, String> getMimeTypes() {
165+
return this.mimeTypes;
166+
}
167+
168+
@Override
169+
public String getMimeType(String filePath) {
170+
String extension = StringUtils.getFilenameExtension(filePath);
171+
return getMimeTypes().get(extension);
172+
}
173+
}
174+
133175
}

‎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
@@ -62,9 +62,17 @@ public void extractFullFilenameFromUrlPath() {
6262
assertEquals("index.html", WebUtils.extractFullFilenameFromUrlPath("index.html"));
6363
assertEquals("index.html", WebUtils.extractFullFilenameFromUrlPath("/index.html"));
6464
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html"));
65+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/a"));
66+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/path/a"));
67+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/path/a.do"));
6568
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=a"));
6669
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a"));
6770
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a.do"));
71+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a#/path/a"));
72+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a.do#/path/a.do"));
73+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html?param=/path/a.do"));
74+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html;r=22?param=/path/a.do"));
75+
assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html;r=22;s=33?param=/path/a.do"));
6876
}
6977

7078
@Test

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,26 +18,32 @@
1818

1919
import java.io.IOException;
2020
import java.util.ArrayList;
21+
import java.util.Arrays;
2122
import java.util.Collections;
23+
import java.util.HashSet;
2224
import java.util.LinkedHashSet;
2325
import java.util.List;
26+
import java.util.Locale;
2427
import java.util.Set;
2528
import javax.servlet.http.HttpServletRequest;
2629
import javax.servlet.http.HttpServletResponse;
2730

2831
import org.springframework.core.MethodParameter;
32+
import org.springframework.http.HttpHeaders;
2933
import org.springframework.http.HttpOutputMessage;
3034
import org.springframework.http.MediaType;
3135
import org.springframework.http.converter.HttpMessageConverter;
3236
import org.springframework.http.server.ServletServerHttpRequest;
3337
import org.springframework.http.server.ServletServerHttpResponse;
3438
import org.springframework.util.CollectionUtils;
39+
import org.springframework.util.StringUtils;
3540
import org.springframework.web.HttpMediaTypeNotAcceptableException;
3641
import org.springframework.web.accept.ContentNegotiationManager;
3742
import org.springframework.web.context.request.NativeWebRequest;
3843
import org.springframework.web.context.request.ServletWebRequest;
3944
import org.springframework.web.method.support.HandlerMethodReturnValueHandler;
4045
import org.springframework.web.servlet.HandlerMapping;
46+
import org.springframework.web.util.UrlPathHelper;
4147

4248
/**
4349
* Extends {@link AbstractMessageConverterMethodArgumentResolver} with the ability to handle
@@ -52,8 +58,24 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
5258

5359
private static final MediaType MEDIA_TYPE_APPLICATION = new MediaType("application");
5460

61+
private static final UrlPathHelper RAW_URL_PATH_HELPER = new UrlPathHelper();
62+
63+
private static final UrlPathHelper DECODING_URL_PATH_HELPER = new UrlPathHelper();
64+
65+
static {
66+
RAW_URL_PATH_HELPER.setRemoveSemicolonContent(false);
67+
RAW_URL_PATH_HELPER.setUrlDecode(false);
68+
}
69+
70+
/* Extensions associated with the built-in message converters */
71+
private static final Set<String> WHITELISTED_EXTENSIONS = new HashSet<String>(Arrays.asList(
72+
"txt", "text", "json", "xml", "atom", "rss", "png", "jpe", "jpeg", "jpg", "gif", "wbmp", "bmp"));
73+
74+
5575
private final ContentNegotiationManager contentNegotiationManager;
5676

77+
private final Set<String> safeExtensions = new HashSet<String>();
78+
5779

5880
protected AbstractMessageConverterMethodProcessor(List<HttpMessageConverter<?>> messageConverters) {
5981
this(messageConverters, null);
@@ -64,6 +86,8 @@ protected AbstractMessageConverterMethodProcessor(List<HttpMessageConverter<?>>
6486

6587
super(messageConverters);
6688
this.contentNegotiationManager = (manager != null ? manager : new ContentNegotiationManager());
89+
this.safeExtensions.addAll(this.contentNegotiationManager.getAllFileExtensions());
90+
this.safeExtensions.addAll(WHITELISTED_EXTENSIONS);
6791
}
6892

6993

@@ -140,6 +164,7 @@ else if (mediaType.equals(MediaType.ALL) || mediaType.equals(MEDIA_TYPE_APPLICAT
140164
selectedMediaType = selectedMediaType.removeQualityValue();
141165
for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
142166
if (messageConverter.canWrite(returnValueClass, selectedMediaType)) {
167+
addContentDispositionHeader(inputMessage, outputMessage);
143168
((HttpMessageConverter<T>) messageConverter).write(returnValue, selectedMediaType, outputMessage);
144169
if (logger.isDebugEnabled()) {
145170
logger.debug("Written [" + returnValue + "] as \"" + selectedMediaType + "\" using [" +
@@ -194,4 +219,48 @@ private MediaType getMostSpecificMediaType(MediaType acceptType, MediaType produ
194219
return (MediaType.SPECIFICITY_COMPARATOR.compare(acceptType, produceTypeToUse) <= 0 ? acceptType : produceTypeToUse);
195220
}
196221

222+
/**
223+
* Check if the path has a file extension and whether the extension is either
224+
* {@link #WHITELISTED_EXTENSIONS whitelisted} or
225+
* {@link ContentNegotiationManager#getAllFileExtensions() explicitly
226+
* registered}. If not add a 'Content-Disposition' header with a safe
227+
* attachment file name ("f.txt") to prevent RFD exploits.
228+
*/
229+
private void addContentDispositionHeader(ServletServerHttpRequest request,
230+
ServletServerHttpResponse response) {
231+
232+
HttpHeaders headers = response.getHeaders();
233+
if (headers.containsKey("Content-Disposition")) {
234+
return;
235+
}
236+
237+
HttpServletRequest servletRequest = request.getServletRequest();
238+
String requestUri = RAW_URL_PATH_HELPER.getOriginatingRequestUri(servletRequest);
239+
240+
int index = requestUri.lastIndexOf('/') + 1;
241+
String filename = requestUri.substring(index);
242+
String pathParams = "";
243+
244+
index = filename.indexOf(';');
245+
if (index != -1) {
246+
pathParams = filename.substring(index);
247+
filename = filename.substring(0, index);
248+
}
249+
250+
filename = DECODING_URL_PATH_HELPER.decodeRequestString(servletRequest, filename);
251+
String ext = StringUtils.getFilenameExtension(filename);
252+
253+
pathParams = DECODING_URL_PATH_HELPER.decodeRequestString(servletRequest, pathParams);
254+
String extInPathParams = StringUtils.getFilenameExtension(pathParams);
255+
256+
if (!isSafeExtension(ext) || !isSafeExtension(extInPathParams)) {
257+
headers.add("Content-Disposition", "attachment;filename=f.txt");
258+
}
259+
}
260+
261+
private boolean isSafeExtension(String extension) {
262+
return (!StringUtils.hasText(extension) ||
263+
this.safeExtensions.contains(extension.toLowerCase(Locale.ENGLISH)));
264+
}
265+
197266
}

‎spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.io.Serializable;
2020
import java.lang.reflect.Method;
2121
import java.util.ArrayList;
22+
import java.util.Collections;
2223
import java.util.List;
2324

2425
import org.junit.Before;
@@ -35,13 +36,15 @@
3536
import org.springframework.mock.web.test.MockHttpServletResponse;
3637
import org.springframework.util.MultiValueMap;
3738
import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean;
39+
import org.springframework.web.accept.ContentNegotiationManagerFactoryBean;
3840
import org.springframework.web.bind.WebDataBinder;
3941
import org.springframework.web.bind.annotation.RequestBody;
4042
import org.springframework.web.bind.support.WebDataBinderFactory;
4143
import org.springframework.web.context.request.NativeWebRequest;
4244
import org.springframework.web.context.request.ServletWebRequest;
4345
import org.springframework.web.method.HandlerMethod;
4446
import org.springframework.web.method.support.ModelAndViewContainer;
47+
import org.springframework.web.util.WebUtils;
4548

4649
import static org.junit.Assert.*;
4750

@@ -224,6 +227,57 @@ public void handleReturnValueStringAcceptCharset() throws Exception {
224227
assertEquals("text/plain;charset=UTF-8", servletResponse.getHeader("Content-Type"));
225228
}
226229

230+
@Test
231+
public void addContentDispositionHeader() throws Exception {
232+
233+
ContentNegotiationManagerFactoryBean factory = new ContentNegotiationManagerFactoryBean();
234+
factory.addMediaType("pdf", new MediaType("application", "pdf"));
235+
factory.afterPropertiesSet();
236+
237+
RequestResponseBodyMethodProcessor processor = new RequestResponseBodyMethodProcessor(
238+
Collections.<HttpMessageConverter<?>>singletonList(new StringHttpMessageConverter()),
239+
factory.getObject());
240+
241+
assertContentDisposition(processor, false, "/hello.json", "whitelisted extension");
242+
assertContentDisposition(processor, false, "/hello.pdf", "registered extension");
243+
assertContentDisposition(processor, true, "/hello.dataless", "uknown extension");
244+
245+
// path parameters
246+
assertContentDisposition(processor, false, "/hello.json;a=b", "path param shouldn't cause issue");
247+
assertContentDisposition(processor, true, "/hello.json;a=b;setup.dataless", "uknown ext in path params");
248+
assertContentDisposition(processor, true, "/hello.dataless;a=b;setup.json", "uknown ext in filename");
249+
assertContentDisposition(processor, false, "/hello.json;a=b;setup.json", "whitelisted extensions");
250+
251+
// encoded dot
252+
assertContentDisposition(processor, true, "/hello%2Edataless;a=b;setup.json", "encoded dot in filename");
253+
assertContentDisposition(processor, true, "/hello.json;a=b;setup%2Edataless", "encoded dot in path params");
254+
assertContentDisposition(processor, true, "/hello.dataless%3Bsetup.bat", "encoded dot in path params");
255+
256+
this.servletRequest.setAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE, "/hello.bat");
257+
assertContentDisposition(processor, true, "/bonjour", "forwarded URL");
258+
this.servletRequest.removeAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE);
259+
}
260+
261+
private void assertContentDisposition(RequestResponseBodyMethodProcessor processor,
262+
boolean expectContentDisposition, String requestURI, String comment) throws Exception {
263+
264+
this.servletRequest.setRequestURI(requestURI);
265+
processor.handleReturnValue("body", this.returnTypeString, this.mavContainer, this.webRequest);
266+
267+
String header = servletResponse.getHeader("Content-Disposition");
268+
if (expectContentDisposition) {
269+
assertEquals("Expected 'Content-Disposition' header. Use case: '" + comment + "'",
270+
"attachment;filename=f.txt", header);
271+
}
272+
else {
273+
assertNull("Did not expect 'Content-Disposition' header. Use case: '" + comment + "'", header);
274+
}
275+
276+
this.servletRequest = new MockHttpServletRequest();
277+
this.servletResponse = new MockHttpServletResponse();
278+
this.webRequest = new ServletWebRequest(servletRequest, servletResponse);
279+
}
280+
227281

228282
public String handle(
229283
@RequestBody List<SimpleBean> list,

0 commit comments

Comments
 (0)