cve-2016-3714 "magic byte" validation

[jglass]

In this article, an ImageMagick exploit is described, and the suggested temporary fixes are to use a policy file to limit the possible attack vectors and also preferably to validate the "magic bytes" at the beginning of each image file to make sure that they are indeed image files and not maliciously uploaded content.

Although is of course this is an ImageMagick issue, it would be great if paperclip could validate that the first few bytes match what is expected as an extra security measure. Is this something that is already in the works or that a pull request would be welcome for?

[bramswenson]

👍

[tute]

Paperclip checks for that: https://github.com/thoughtbot/paperclip/blob/v4.3.6/lib/paperclip/content_type_detector.rb#L69-L72

We are preparing a blog post to announce that this vulnerability does not affect paperclip.

Thank you for reporting.

[jglass]

Thanks so much!

[magnusvk]

@tute Does that really protect us here? This code looks like it falls back to running file if it can't detect the file type from the Mime magic bytes. Wouldn't that leave us exposed?

[tute]

The file command takes into account the "magic numbers", so that branch is safe as well.

[mahemoff]

Just wondering, is there any test case for this as it could help with confidence level?

[mike-burns]

We have this test -- https://github.com/thoughtbot/paperclip/blob/master/spec/paperclip/media_type_spoof_detector_spec.rb -- and we are certainly not opposed to more security-related tests.

If you open a PR with at least an outline of what the test would look like, we'd be happy to help fill it in and make it pass.

[magnusvk]

Thanks for the quick updates.

[O-I]

What about versions of paperclip <4.3?

[tute]

We don't actively maintain v3 and earlier versions. While mime type checks were present, the attack could succeed, because the check was not enforced.

[tute]

Cc @jyurek, who implemented this fix for v4: can you add context on this functionality for earlier versions?

[mike-burns]

Paperclip before 4.2.2 was vulnerable to CVE-2015-2963: https://robots.thoughtbot.com/paperclip-security-release

[Nitrodist]

@mike-burns can you add that info to the post? Seems important.

[mike-burns]

@tute since you're still at work: ^

[tute]

I will add a paragraph and a link to this issue this afternoon.

[tute]

Blog post is in https://robots.thoughtbot.com/imagemagick-vulnerability-and-paperclip

---

UPDATE: after #2190 (comment) we published that it is affected: https://robots.thoughtbot.com/paperclip-is-vulnerable-to-the-imagetragick-vulnerability

[awj]

The wording on that post is confusing. How is Paperclip < 4.3 not vulnerable to this? That's the implication I'm getting from this statement:

Paperclip versions 4.2.2 and newer don’t have known vulnerabilities (versions earlier than 4.2.2 are vulnerable to CVE-2015-2963). There is no need to upgrade Paperclip in light of CVE-2016–3714.

[tute]

From version 4.0.0 we enforce the validation of mime types with the file utility. v4.3 uses the mimemagic gem.

[awj]

Ahh, yeah, I can see that now. Thanks for the explanation.

[sparksp]

@tute the paperclip spoofing protection is not a panacea, and does not protect users from SVGs containing the exploit. Users of paperclip must be advised to apply the policy.xml fix to their systems; they are not able to prevent SVGs from being uploaded if they're expecting images.

With a model containing the following code, an SVG file may be uploaded as an image and it will be processed with ImageMagick:

class Avatar < ActiveRecord::Base
  has_attached_file :media,
    default_style: :thumbnail,
    styles: { thumbnail: ["54x54#", :png] }

  validates_attachment :media,
    content_type: { content_type: /^image/ },
    size: { less_than: 5.megabytes }
end

Furthermore, because paperclip does not reject a file that is named .jpg and identifies as PNG, or in fact a file that is named .png and identifies as SVG (tested locally), spoofing protection is not a saving grace. Uploading an SVG named .png records the content type as "image/png" and continues processing. Using explicit content types, as shown below, will not prevent an SVG spoofed as another image type from being uploaded.

class Avatar < ActiveRecord::Base
  has_attached_file :media,
    default_style: :thumbnail,
    styles: { thumbnail: ["54x54#", :png] }

  validates_attachment :media,
    content_type: { content_type: ["image/jpeg", "image/png"] }
    size: { less_than: 5.megabytes }
end

All local tests conducted using Paperclip 4.3.5.

[tute]

Thanks so much for your input, @sparksp. We'll make this announcement soon.

[tute]

https://robots.thoughtbot.com/paperclip-is-vulnerable-to-the-imagetragick-vulnerability

Thank you, @sparksp

[baron]

@tute I think you should update the original blog post with a big fat warning on top, just in case.

https://robots.thoughtbot.com/imagemagick-vulnerability-and-paperclip

[tute]

@tute I think you should update the original blog post with a big fat warning on top, just in case.

Indeed, updated. Thanks for the nudge.

[tute]

This is all taken care of, closing the issue. Thank you all.

[rememberlenny]

@tute Would you mind updating your post above. The comment saying the Paperclip is NOT vulnerable is misleading for people who dont scroll down.

Re: #2190 (comment)
Re: #2190 (comment)

[tute]

The older blog post is updated, but now the comment is as well.

[rememberlenny]

Thank you!

Leonard Bogdonoff
rememberlenny@gmail.com
Blog http://blog.rememberlenny.com | Portfolio http://rememberlenny.com/
| @rememberlenny http://twitter.com/rememberlenny
949-322-8496

On Mon, May 9, 2016 at 11:09 AM, Tute Costa notifications@github.com
wrote:

The older blog post is updated, but now the comment is as well.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#2190 (comment)

[jrochkind]

I don't understand what's going on -- if you can upload an svg even with your content types explicitly limited to ["image/jpeg", "image/png"], as suggested in a previous comment -- is this a bug in Paperclip? How is it getting by the content type validation, where it's said to be required to be a jpeg or png, but it lets an svg through anyway?

Is it not possible to tell the difference with 'magic bytes'? Is it possible, but the routines paperclip uses don't? Are the routines paperclip uses accurately identifying it as an svg, but it's slipping through content type validation via other means?

Information on this would be helpful in understanding how to use paperclip efficiently and securely.

(Is it related to the bug I already filed, coincidentally a couple weeks before the imagemagick vuln announcement, on post-process hooks being fired even if content type validation fails? #2178)

[jrochkind]

If the vulnerability reproduction pointed out by @sparksp here still reproduces, I would suggest a separate issue should be created to track it.

[mmrko]

@jrochkind Not sure if the vulnerability can be fully mitigated without adding a policy.xml or upgrading ImageMagick. See https://github.com/ImageTragick/PoCs for testing your system against the known attack vectors.

[jrochkind]

Right, I understand that may be the case.

However, I would expect that paperclip's built-in spoof detection would detect and flag an SVG uploaded pretending to be a PNG. Paperclip has all the tools to do so, but does not, as @sparksp pointed out.

However, investigating it, I see that MediaTypeSpoofDetector intentionally only checks the first half of the content-type, the "media type", the "image" in "image/jpeg". The code is written this way, and it appears to be intentional, there is an existing spec for does not reject a file that is named .jpg and identifies as PNG

And in fact, I now see the README does say (or imply) this: "This will prevent HTML documents from being uploaded as JPEGs, but will not prevent GIFs from being uploaded with a .jpg extension."

I am not sure why it would have been intentionally decided to test for media-type spoofing, but not full content_type spoofing. The commit just says "Add media-type spoof detection" -- okay, that's what it does, now that I know what "media type" means (not full content-type).

The spoof detector could be easily changed to flag all content_type spoofs, the tools are there for it. But I guess there's a reason for it not to. Okay then. That seems to limit it's utility. I wonder if the original author thought the only "dangerous" type of spoofing was at the media-type level, and didn't realize an SVG pretending to be a PNG might be dangerous.

[sparksp]

@jrochkind I'm with you on this - I would expect the media type checking to take place, for it to set the correct content-type for the uploaded file so that content type validation (which is separate from the media type anti-spoofing) can still restrict the attachments. It sounds like this should be a separate issue though.

[jrochkind]

@sparksp I think it would actually be potentially dangerous to automatically set the correct content_type. Consider that many apps serve assets with their original file name as uploaded (and saved by paperclip). If the thing is really an svg but has a filename ending in ".png", and paperclip sets the database content type to svg -- the db content type is typically ignored by most apps after the point of upload, and the filename ending in ".png" will likely still be served with a png content type by the web server/AWS/CDN/whatever based on the suffix.

Then we could imagine paperclip automatically changing the suffix too. But I think this points out that there are all sorts of potential gotchas here, and the best thing to do for now is keep doing what paperclip is doing -- simply error if the asset appears to have a spoofed content type.

The current paperclip spoof-detection code actually has all the info it needs to correctly detect an svg pretending to be a png (I tested it). The code just intentionally ignores it, and only pays attention to the media-type (before the / in the full content-type) in deciding whether to flag it.

So it would be easy to change. You could fairly easily write your own validation that detected spoofing on full content_type, and add it into your app. The existing media-type spoofing code would still run too (there's no way to disable it), so there'd be kind of redundant code running, but it would work.

[sparksp]

@jrochkind hmm, good point. I still think this is probably best raised as another issue.

[Dharaniradhakrishnan]

Paperclip checks for that: https://github.com/thoughtbot/paperclip/blob/v4.3.6/lib/paperclip/content_type_detector.rb#L69-L72

We are preparing a blog post to announce that this vulnerability does not affect paperclip.

Thank you for reporting.

@tute I am also trying to update paperclip from 4.2.4 to 4.3 .please send me the blog related to this.

[tute]

Hi @Dharaniradhakrishnan! You must have already seen https://github.com/thoughtbot/paperclip/#deprecated

Within this comment is the link you were seeking: #2190 (comment)