Mitigate against CVE-2016-3714 (ImageTragick)

[locriani]

• Use the MimeMagic gem to detect content types via file headers
• Fallback to Mime::Types if the file does not exist
• Make the ruby.gif file actually a GIF, so blacklist tests pass (was previously a png masquerading as a gif)
• Update the README to detail all required steps to mitigate against CVE-2016-3714

[locriani]

Ref: #1933

[locriani]

Issues:

• This only allows the end user to mitigate by specifying a white list of allowable content types. Should this be addressed in the readme, or by adding a default white list? Updated the README to include instructions for complete mitigation.
• I do not know the code path that's resolved at line 268, this could be a vector for an inaccurate mime type Upon further investigation, it appears this code path is for files already uploaded with mime type information stored.

[namxam]

Regarding issue 1, I would vote for a default whitelist. Having a secure setup should always be default. But there should be some info in the README how to change it and why those defaults have been chosen.

[thiagofm]

@locriani can you please remove the images changes on your commit and preferably backport it to 0.11(which I'll generate a release asap)?

Cheers

[locriani]

@thiagofm I can't remove the change to the ruby.gif image without breaking these specs:

• spec/uploader/content_type_blacklist_spec.rb:33
• spec/uploader/content_type_whitelist_spec.rb:27

With the changes to the mime type validation, the ruby.gif image no longer parses with a mime type of image/gif (which is spoofed based on the extension), but as an image/png (which is what it actually was). The image change I made converted it into a gif, so the new file header based mime-type detection would still continue to read it as an image/gif.

Would you prefer those tests be changed?

[thiagofm]

@locriani yeah, perhaps might be useful adapt the tests.

[locriani]

@thiagofm Done. I created a new PR for the backport, as well.

[thiagofm]

As this is the master branch, I guess it's worth to have a "lasting" solution for this problem. As it's very common for people to have outdated versions of imagemagick(given that there's a fix for the CVE), I think we'll always have to handle this.

@locriani how this approach contrasts with https://github.com/DarthSim/carrierwave-bombshelter ? (uses fastimage gem)

I've merged the backport and will generate a new release for carrierwave 0.11(this is what most of the people are probably/should be using).

[locriani]

@thiagofm That's reasonable.

I think it would make sense to make a default whitelist of acceptable mime types, which could include the common image types (but would need to exclude SVG, by default, because of the MSVG issue). This doesn't look like it would be too complex to accomplish with the carrierwave code, and I think it could be a good approach.

The problems I see here from a security perspective are:

1. denial of service from overly-large images
2. script execution via extension names
3. improper / unexpected processing of files as images (this CVE)

I'm not sure 1 would be in scope for carrierwave to handle, as it's most effectively mitigated at the webserver (apache / nginx) level. 2 is already possible to mitigate with carrierwave, but there are no defaults, and 3 is not possible to mitigate until there's inspection of the file headers built into carrierwave.

Bombshelter's approach is to do validation of the content-type of the file in a validation before it even hits CarrierWave. The gem used for header validation there may be better and easier to use than mimemagic, though.

[thiagofm]

@locriani I was also thinking about something in that direction(default whitelist of mimetypes).

I think the item 1 is definitely out of context.

Might be worth doing a comparison between the two gems(if possible), or perhaps pushing that protection to the RMagick/Minimagick side.

[locriani]

Makes sense to me. I'll spend some time comparing the two gems later today when I have some free time.

[thiagofm]

Thanks a lot for putting your time to it!

[locriani]

It looks like FastImage is actually very limited and unable to detect multiple filetypes that RMagick / ImageMagick supports.

We can't push it over to RMagick / ImageMagick for identification with identify, unfortunately, because the identify command has the same RCE flaws.

[rgueldem]

It seems like there is still an issue if mimemagic doesn't know the mimetype. In that case it falls back to the possibly spoofed content type...

[locriani]

@rgueldem After reviewing the POC and the code again, you're absolutely right. I'll create (yet another) fix for 0.11.0 and revisit this code here.

[pjg]

There is a typo here. extension_type_whitelist should be just extension_whitelist.

[FergusonSean]

Was this ever fixed in 1.0? I'm able to get around my content type whitelist by changing file extensions.

[lukeasrodgers]

@FergusonSean it is not, AFAICT. Looks like the last commit in common between mainline (current release 1.2.2) and 0.11.2 is 3a0978a. Only the v0.11.2 line has the imagetragick fix.

[birthdaycorp]

Hi all! Quick question. The doc says:

and you MUST either disable ImageMagick's default SVG delegate or use the RSVG delegate for SVG processing.

How does one go about doing this? The first part of the solution is just adding the whitelist, but this part sort of eludes me.