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

Add a file types whitelist #42

Closed
wants to merge 8 commits into from
Closed

Conversation

toubib
Copy link
Contributor

@toubib toubib commented Sep 2, 2015

No description provided.

@willnorris
Copy link
Owner

is this still needed, even with the new(-ish) signature support?

@toubib
Copy link
Contributor Author

toubib commented Dec 8, 2015

It should be for the case where you don't use the signature option.
Even with the whitelist option, it's maybe not a good idea to deliver everything like html pages or video or whatever. It's more about reducing potentials attack and security breach vectors ... Maybe a simpler option would be to defined a cold list somewhere of every file type imageproxy should support ?

@willnorris
Copy link
Owner

I'm still not completely sure of the best approach here, but here are some thoughts while it's on my mind:

One of the simplest fixes is to update TransformingTransport.RoundTrip to return the error it receives from Transform rather than simply logging it and returning the original response bytes. This will eliminate any non-image responses that include any transformation options, since the image.Decode call will return an error.

The above will not work for requests that don't contain any transformation options, since the Transport method intentionally returns early in that case. One option would be to move that return to after the image.Decode call. That would be a complete waste of CPU cycles though in a lot of cases, so not really a great option.

A better option would be to call http.DetectContentType to sniff the content type and bail if it's not an image.

We could also just look at the declared content-type in the response headers, but if the point here is to prevent abuse, we shouldn't really trust those headers. Content sniffing is probably the best approach.

Trying to do any kind of whitelisting based on file extension is not really going to be robust enough.

@toubib toubib mentioned this pull request Dec 9, 2015
@toubib
Copy link
Contributor Author

toubib commented Dec 9, 2015

I agree than file extension check can leads to complications and can be easily bypass for security, the use of http.DetectContentType looks like the best and more robust option.

See #53 for this.

@toubib toubib closed this Dec 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants