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 header causes download in browser for Spring Boot Actuator endpoints [SPR-13587] #18164

Closed
spring-issuemaster opened this issue Oct 19, 2015 · 12 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Oct 19, 2015

Rossen Stoyanchev opened SPR-13587 and commented

The fix to protect against RFD exploits (#18124) introduced a "Content-Disposition:attachment;filename=f.txt" response header for @ResponseBody methods where the URL appears to have an extension that is neither whitelisted by default nor explicitly registered by the application.

Spring Boot Actuator exposes many endpoints that naturally contain dots and do not represent an extension. When such a URL is typed in a browser it causes content to be downloaded as "f.txt" rather than rendered.

Several example mappings in Boot:

/metrics/{name:.*}
/env/{name:.*}
/diff/{fromVersion}/{toVersion}

We need to consider ways to make the fix for RFD more flexible with this case in mind (and possible others that might yet be reported), without compromising the security of the application. For once it looks like Spring Boot metrics aren't exposed to RFD since the metric name in the URL has to match a known metric so for example appending a random extension should result in a 404.

Note this issue was originally reported under Spring Boot ticket #4220.


Affects: 4.1.8, 4.2.2

Issue Links:

  • #18224 Content Disposition header being added on some urls...did not behave this way in 4.2.1 ("is duplicated by")
  • #18207 Content-Disposition added for @ResponseBody methods explicitly mapped to ".html" or other extensions
  • #18124 Protect against RFD exploits
  • #18220 Content-Disposition with fixed file name "f.txt" causes confusion
  • #18165 Skip Content-Disposition header when status != 2xx

Referenced from: commits 1489e29, 3a919a4, 92ca537

Backported to: 4.1.9, 3.2.16

1 votes, 7 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 19, 2015

Rossen Stoyanchev commented

We can check via JAF (also via ServletContext#getMimeType) if the "extension" is even considered a file extension before adding a Content-Disposition header. This should significantly reduce the impact for URLs that naturally have a dot (like the Boot's Actuator endpoint URLs) although there is still a possibility for a clash with what appears as a valid file extension (e.g. ".air", ".com", ".frame", ".install", and others).

On the flip side we should be careful to double-check that a file extension is consistently recognized as such. For example ServletContext#getMimeType resolves ".bat" on Tomcat but not on Jetty. So we should probably use JAF at a minimum (also to be verified) and ServletContext#getMimeType as an extra check.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 19, 2015

Rob Winch commented

A point of consideration is that we should check for other scripting languages like PERL, Python, etc to see if JAF / ServletContext recognizes those).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 31, 2015

Martin Frey commented

Also happens to me after update to 4.2.2 with an ordinary controller that renders the page internally and returns it as a ResponseBody:

@RequestMapping(value = "/index.html", method = RequestMethod.GET, produces = MediaType.TEXT_HTML_VALUE)
@ResponseBody
public String getIndex() throws IOException {
    return indexHtmlText;
}

Is there a specific reason why html is not in safeExtensions?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 1, 2015

Rossen Stoyanchev commented

Martin Frey I've created a separate ticket #18207. Please follow the discussion there.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 6, 2015

Rossen Stoyanchev commented

Here is a further update.

To understand why Spring Boot Actuator endpoints need to be protected I asked Rob Winch to demonstrate RFD with an Actuator endpoint and the RFD fix suppressed. It took him very little time to find one. For example "/trace" displays recent requests. Start with:

curl -H "user-agent:\"||calc||" http://localhost:8080/

And then http://localhost:8080/trace.bat returns the embedded call to Windows calculator:

[{"timestamp":1446847240258,"info":{"method":"GET","path":"/","headers":{"request":{"host":"localhost:8080","accept":"*/*","user-agent":"\"||calc||"},"response":{"X-Application-Context":"application:dev","status":"200"}}}}]

Note the point here is not to find which are vulnerable and allow the rest. The point is that it is very difficult to asses if an endpoint is vulnerable and exposed to RFD or not so simply providing an option to declare specific URLs "safe" isn't feasible.

A couple of further points. Note that even today adding a "/" at the end of the URL works. In other words http://localhost:8080/metrics/mem.free/ will show in the browser while http://localhost:8080/metrics/mem.free will force a download. This works because trailing slash pattern matching is on and it stabilizes the URL path at the end (i.e. there is no extension).

Yet another thought especially with the fix for #18207 is that Spring Boot Actuator endpoints may have an additional mapping added with a stable ending. For example on MetricsMvcEndpoint:

@RequestMapping(value = {"/{name:.*}", "/{name:.*}.json"}, method = RequestMethod.GET, produces = MediaType.APPLICATION_JSON_VALUE)

Note the additional ".json". So now I can enter http://localhost:8080/metrics/mem.free.json and that will display in the browser. Or along similar lines perhaps an additional mapping such as "/{name:.*}/browser". Or perhaps the top level "/metrics" URL can optionally accept the metric name as a query parameter.

/cc Andy Wilkinson, Phil Webb, Dave Syer

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 7, 2015

Dave Syer commented

That's a cute example. Doesn't it show (together with the remark that .json works) that we are in dnager of putting the cart before the horse? I really don't want or need /trace.bat to be handled by Spring MVC at all, in any shape or form - /trace is the endpoint and that's the only path I need to respond to. But IIRC I can only switch off extension mapping for the whole application, not just for a set of endpoints that I choose. That is what we should be fixing isn't it?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 7, 2015

Rossen Stoyanchev commented

The example was fully proven out with IE from end to end. Instead of calculator an attacker could restart a person's chrome browser with CORS turned off and do countless other things. I think of that as more scary than cute. As for the suffix pattern matching, Spring Boot Actuator has its own RequestMappingHandlerMapping so you should be able to turn suffix pattern matching off there without affecting the rest of the application.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 7, 2015

Rossen Stoyanchev commented

BTW just to be clear ".json" doesn't work as-is. You have to add an explicit mapping with ".json" in it as I showed above or some other mapping with a fixed ending. I see this is more as a way to provide safe access to such URLs in a browser, not something that should dictate REST API design (i.e. putting the horse before the cart.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 7, 2015

Dave Syer commented

you should be able to turn suffix pattern matching off there without affecting the rest of the application

Good point. spring-projects/spring-boot#4402

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 7, 2015

Dave Syer commented

".json" doesn't work as-is. You have to add an explicit mapping with ".json" in it

This defeats the object of content negotiation being invisible to endpoints doesn't it? We don't want to explicitly map all the Actuator endpoints (and possible future or user provided ones) with all possible media types. The HttpMessageConverters are orthogonal and they should decide what content types are supported.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 7, 2015

Rossen Stoyanchev commented

I didn't suggest changing the existing mappings, rather to create additional ones. Let me try to explain again.

MetricsMvcEndpoint and EnvironmentMvcEndpoint are mapped to "/{name:.*}". The URLs contain dots and that ends with a Content-Disposition header. However, if the mapping had a fixed ending or an explicit extension the Content-Disposition wouldn't be added. An example of a fixed ending is "/{name:.*}/browser", or perhaps "/metrics" with the metric name becoming a query parameter. An example of an explicitly mapped extension is "/{name:.*}.json".

My idea is that any one of these would be added as alternatives so they can be used in a browser, for example:

@RequestMapping(path = {"/{name:.*}", "/{name:.*}.json"}, ...)

Note also that adding a suffix "/" also works (i.e. no Content-Disposition header is added) even today. In any case I'm just raising an idea. Perhaps some sort of a consistent way of entering actuator endpoints in a browser might be one alternative to consider.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 11, 2015

Rossen Stoyanchev commented

RFC 6266 defines two disposition types. Switching to "inline" doesn't force the download and that suits our purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.