Restricted Content-Types #141

ccbrown · 2018-02-09T22:08:50Z

For security purposes, we need to ensure that the proxy is only used to serve images. Serving non-image content opens up a lot of attack surface for phishing, XSS, SSRF, and other nasty tricks. Being labeled "imageproxy", I was surprised to find out that our proxies were happily serving any arbitrary HTML that was thrown at them.

I'm putting this PR in hoping that we can make the proxy more secure by default and bring it up to par with atmos/camo in this regard.

Our ~~upcoming~~ 4.7 release of Mattermost was all set to include built-in support for proxying user-posted images (via this or Camo), but we have a lot of very security-minded customers, so this put a slight wrinkle in our plans.

I've read over all of the related discussion I could find (namely this) and I think this addresses the big concerns except for one:

initially, I think the default should be to not check (see next point), so that this doesn't become a breaking change for anyone.

I strongly believe this should be enabled by default. It shouldn't be viewed as simply being a breaking change. It's a security patch. It's supposed to prevent people from doing things that they could do before.

The Changes

By default, the proxy will never return a Content-Type that isn't one of Camo's whitelisted image types. Their whitelist hasn't changed in 4 years and seems like a good, stable reference point.
If users are relying on the ability to proxy non-image types, they can specify them via the new -contentTypes flag. Shell pattern matching is used, so you can do things like -contentTypes image/*,video/mp4 if you'd like. Or you can just enable everything with wildcards to get today's behavior, but probably no one should do this.

This alters the behavior at the last moment, right before sending the response back to the client. This does not do sniffing. As stated in some of the other discussion, sniffing is pointless in the context of security. The only thing that matters is how browsers interpret the content. And browsers decide that based on the Content-Type and X-Content-Type-Options headers, which are now both set and strictly controlled.

mikesimons · 2018-03-22T14:48:16Z

👍 to this. If you have not set a host whitelist then imageproxy may happily proxy content from the network in which it is running too. For example http://my-site/images/200/http://169.254.169.254/latest/meta-data/ on AWS.

julianzur · 2018-03-26T14:21:27Z

Really looking forward for this PR to get merged. This will be a huge plus for security. Thanks, @ccbrown!

willnorris

Apologies for the delay in reviewing, but thanks for this change... it definitely looks to be on the right track. A few suggested changes and questions below.

willnorris · 2018-06-21T05:13:19Z

imageproxy.go

-	copyHeader(w.Header(), resp.Header, "Content-Length", "Content-Type")
+	if contentType := p.allowedContentType(resp.Header.Get("Content-Type")); contentType != "" {
+		w.Header().Set("Content-Type", contentType)
+		w.Header().Set("X-Content-Type-Options", "nosniff")


Doesn't whatwg/fetch#395 suggest that you shouldn't use nosniff with images?

And can this content type check simply be moved into the existing allowed() method?

My current understanding is that it's fine (and more secure) as long you also specify the correct Content-Type. I should research and test that a bit more though.

I think the reason it's not in the existing allowed method is because it has to happen after we fetch the remote image.

willnorris · 2018-06-21T05:15:53Z

imageproxy.go

@@ -211,6 +225,41 @@ func (p *Proxy) allowed(r *Request) error {
 	return fmt.Errorf("request does not contain an allowed host or valid signature: %v", r)
 }

+// allowedContentType returns an allowed content type string to use in responses or "" if the
+// content type cannot be used.
+func (p *Proxy) allowedContentType(contentType string) string {


Why return a string? It doesn't look like the return value is really needed. Instead, perhaps rename this method to validContentType and return a bool like the other valid* methods?

I feel like there was a good reason for this, but I can't remember it. I'll refactor if it doesn't come to me.

willnorris · 2018-06-21T05:21:12Z

imageproxy.go

+
+	if len(p.ContentTypes) == 0 {
+		switch mediaType {
+		case "image/bmp", "image/cgm", "image/g3fax", "image/gif", "image/ief", "image/jp2",


Having to list out all of these images types seems really messy and error prone. You've already added support for image/* style wildcards below, so instead of treating an empty ContentTypes as special, just have the default flag value explicitly be image/*. That results in safe default behavior (just images), but still allows for the existing behavior (all types), by simply passing an empty value for the contentTypes flag.

Or is there some other image/* subtype that is deemed unsafe that you're specifically trying to avoid?

Since this is a security PR, I feel a lot better about using a more restrictive whitelist that's already been put to the test in other applications.

I can't say with much certainty that there are not dangerous image/* mimetypes. But perhaps more importantly, we can't say there will never be a dangerous image/* mimetype in the future (if there isn't already one).

So while there's nothing specifically that I'm aware of needing to avoid, I think it would be a security best practice to use a whitelist such as this one.

where did this whitelist come from?

Camo: https://github.com/atmos/camo/blob/master/mime-types.json

willnorris · 2018-06-21T05:21:34Z

imageproxy_test.go

@@ -299,20 +299,20 @@ func (t testTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	var raw string

 	switch req.URL.Path {
-	case "/ok":
+	case "/plain":


with the behavior I suggested above (namely that an empty ContentTypes value is not treated special), then this change should be reverted, as well as the corresponding StatusForbidden below.

ccbrown · 2018-06-22T05:03:02Z

Thanks for taking a look @willnorris. I'll try to re-familiarize myself with this PR and at least look into the nosniff thing / allowedContentType signature tomorrow.

ccbrown · 2018-06-22T21:42:34Z

@willnorris I made the requested allowedContentType -> validContentType refactor.

As for using "nosniff", the fact that Camo uses it by default combined with the fact that many very large and popular websites use "nosniff" for images has me convinced that there is not a compatibility issue with modern browsers. And I think there's a very real risk of some browsers sniffing proxied resources into non-image types if the header is omitted.

hmhealey · 2018-09-14T14:47:47Z

Is there any update on the status of this PR? We've renewed our focus on adding a built-in image proxy to Mattermost, and this feature would be very important for that

willnorris · 2018-09-15T05:40:52Z

@ccbrown's changes merged in 39a4e18, with a few additional changes in 0370572. Thanks @ccbrown for your work on this!

This was referenced Feb 12, 2018

ABC-258: Remove willnorris/imageproxy from sysadmin console mattermost/mattermost-webapp#777

Merged

ABC-258: Remove willnorris/imageproxy support mattermost/mattermost#8250

Merged

ccbrown mentioned this pull request Jun 20, 2018

fix XSS and potential SSRF #152

Closed

willnorris requested changes Jun 21, 2018

View reviewed changes

ccbrown added 4 commits June 22, 2018 16:10

content-type checking

f9a49e1

fix flag parsing

0056c7a

go 1.8 compatibility

67342bf

requested refactor

b0bdbd9

ccbrown force-pushed the content-type-checking branch from d7889d1 to b0bdbd9 Compare June 22, 2018 21:35

willnorris closed this in 0370572 Sep 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restricted Content-Types #141

Restricted Content-Types #141

ccbrown commented Feb 9, 2018 •

edited

Loading

mikesimons commented Mar 22, 2018

julianzur commented Mar 26, 2018

willnorris left a comment

willnorris Jun 21, 2018

ccbrown Jun 22, 2018

willnorris Jun 21, 2018

ccbrown Jun 22, 2018

willnorris Jun 21, 2018

ccbrown Jun 22, 2018 •

edited

Loading

willnorris Jun 22, 2018

ccbrown Jun 22, 2018

willnorris Jun 21, 2018

ccbrown commented Jun 22, 2018

ccbrown commented Jun 22, 2018

hmhealey commented Sep 14, 2018

willnorris commented Sep 15, 2018

Restricted Content-Types #141

Restricted Content-Types #141

Conversation

ccbrown commented Feb 9, 2018 • edited Loading

mikesimons commented Mar 22, 2018

julianzur commented Mar 26, 2018

willnorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccbrown Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccbrown commented Jun 22, 2018

ccbrown commented Jun 22, 2018

hmhealey commented Sep 14, 2018

willnorris commented Sep 15, 2018

ccbrown commented Feb 9, 2018 •

edited

Loading

ccbrown Jun 22, 2018 •

edited

Loading