Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content-Disposition with fixed file name "f.txt" causes confusion [SPR-13643] #18220

Closed
spring-projects-issues opened this issue Nov 4, 2015 · 16 comments
Assignees
Labels
in: web status: backported type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Nov 4, 2015

Dave Syer opened SPR-13643 and commented

$ curl -v localhost:8888/foo/bar/master/application.yml
* Hostname was NOT found in DNS cache
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8888 (#0)
> GET /foo/bar/master/application.yml HTTP/1.1
> User-Agent: curl/7.35.0
> Host: localhost:8888
> Accept: */*
> 
< HTTP/1.1 200 OK
* Server Apache-Coyote/1.1 is not blacklisted
< Server: Apache-Coyote/1.1
< X-Application-Context: configserver:8888
< Content-Disposition: attachment;filename=f.txt
< Content-Type: text/plain;charset=UTF-8
< Content-Length: 120
< Date: Wed, 04 Nov 2015 14:48:09 GMT
< 
...

Users expect the filename would be "application.yml". It looks weird in the browser when you get a download of a file that has the wrong name.

Here's the endpoint:

@RequestMapping("/{name}/{profile}/{label}/{path:.*}")

From https://github.com/spring-cloud/spring-cloud-config/blob/c0ddcd8/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/ResourceController.java#L63


Affects: 4.1.8, 4.2.2

Issue Links:

  • #18124 Protect against RFD exploits
  • #18164 Content-Disposition header causes download in browser for Spring Boot Actuator endpoints

Referenced from: commits a3168fd, 92ca537, 1489e29, 3a919a4, f0464e8, f5f57e9, 71a9eb7

Backported to: 4.1.9, 3.2.16

0 votes, 7 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 4, 2015

Dave Syer commented

I looked at AbstractMessageConverterMethodProcessor so I can see that ".yml" is not on the default whitelist of extensions so that explains why the header is massacred. I still think that "f.txt" is completely wrong as a default file name. As a user, the first time I saw it I thought "crap, I'm being hacked, I asked for 'application.yml' and it's downloading 'f.txt' without even asking me". Maybe 'application.yml.txt' would be a better default (i.e. re-use the request URI path)?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 4, 2015

Dave Syer commented

Aside: I can't find anything in the Spring User Guide about this (maybe I'm just not good at searching). I assume it's possible to add ".yml" to the whitelist, but I don't know how at this point.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 4, 2015

Rossen Stoyanchev commented

Point taken that the docs need to have proper explanation for Content-Disposition with "f.txt". For now see section on configuring content negotiation.

I'll get back to you with a better summary about "f.txt" (discussing that with Rob Winch as well). Briefly however a fixed file name like that is recommended for example in the RFD paper ("1.txt") and also the Rosetta flash ( when possible where URLs are not expected to be used in the browser anyway. It reduces the attack surface of what an attacker can do by playing with the file name (e.g. "foo.bat.bat", "foo;foo.bat", "foo%2Ebat", etc.) and what browsers might do with an attachment such as "foo.bat.txt" now or in the future. The "f.txt" name seems to be adopted in various sites. It's a bit hard finding examples but here is one.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 4, 2015

Rossen Stoyanchev commented

Marking as 4.2.3 for now since we know we'll at least update the docs but also to decide what else we should do.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 4, 2015

Rossen Stoyanchev commented

Second action item (besides documentation) we should expand the whitelist to include "yml" out of the box. Also other extensions such as "properties" and "csv" that may potentially be rendered with the String message converter and other message converters that can match to a range of extensions such as XML and images.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 4, 2015

Rob Winch commented

As demonstrated in #18124 reflecting user input is quite risky. This is especially true for HTTP headers. As previously mentioned, allowing user defined input to control the filename increases the attack surface which tends to lead to vulnerabilities.

In this instance, allowing the user to specify the file name can lead to attacks like HTTP Response Splitting. There is an excellent white paper on Content-Disposition and HTTP Response splitting.

More concretely a malicious user could craft a URL that looks like:

https://example.com/run.bat%22%0D%0A%0D%0Anotepad%0D%0A%0D%0A

This would produce an HTTP response that looks like:

HTTP/1.1 200 OK
..
Content-Disposition: attachment; filename="run.bat"

notepad

.txt" # this is the suffix we tried to append to the file name we computed
1. remainder of HTTP headers and the body

This leads us right back to where we started.

A few important things to highlight:

  • We could whitelist the possible characters that the filename could have
  • Many modern application servers protect against HTTP Response splitting. For example, tomcat replaces new line characters with a space.

While the example I provided does not produce a valid exploit (if done properly), it does lead to quite a bit more code and increases the attack surface of the application. For these reasons, I think we are better off leaving the file name a constant.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 6, 2015

Rossen Stoyanchev commented

In summary we will keep "f.txt" for now and try to minimize its impact.

The following is done already (see commit):

  • add "yml", "properties", and "csv" to the extensions whitelist
  • add section on RFD to the reference
  • refer to RFD docs in Javadoc where extensions are registered

Separately I have prepared this commit which significantly expands the range of whitelisted extensions by looking up the media type for an extension and effectively whitelisting it if it is associated with image, audio, video, and xml. This is pending a final review from Rob Winch tomorrow. In the mean time I will close this ticket as the steps taken already address the issue.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 6, 2015

Rob Winch commented

A note that is good for historical records...

I spoke to John Melton, a respected security expert, offline. He favored using a constant as well.

He also pointed out that a constant filename will make searching for a solution easier. Specifically, searching for "spring f.txt" will result in a quick answer. However, if the filename was dynamic the search might be more challenging.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 6, 2015

Rossen Stoyanchev commented

FYI that the additional commit for images, audio, video, and +xml media types is now in master.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 7, 2015

Dave Syer commented

Maybe we should give "+json" the same treatment as "+xml"?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 7, 2015

Rossen Stoyanchev commented

Main reason I didn't add it is because I couldn't find much evidence of extensions linked to such media types. For example in the apache httpd list there many +xml and just 1 or 2 rather unknown +json ones.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 7, 2015

Dave Syer commented

I think we should support at least HAL media types (and any others provided by Spring Data REST). They might not be on the JAF list I guess.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 7, 2015

Rossen Stoyanchev commented

Are they associated with any file extensions though?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 7, 2015

Dave Syer commented

Are they associated with any file extensions though?

I guess not.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 11, 2015

Rossen Stoyanchev commented

RFC 6266 apparently defines two disposition types. The "inline" type doesn't enforce the Save As dialog so that should avoid the "f.txt" confusion more definitively.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 12, 2015

Sébastien Deleuze commented

Awesome (y)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web status: backported type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants