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

Checking MIME type by extension is a potential security risk #1543

Closed
d4rky-pl opened this issue Jan 7, 2015 · 8 comments
Closed

Checking MIME type by extension is a potential security risk #1543

d4rky-pl opened this issue Jan 7, 2015 · 8 comments

Comments

@d4rky-pl
Copy link

@d4rky-pl d4rky-pl commented Jan 7, 2015

Nothing stops the user from spoofing the file extension and uploading potentially dangerous file.

Paperclip solves this using UNIX file command. If you believe this is a viable solution, I can prepare a pull request.

@d4rky-pl d4rky-pl changed the title Checking MIME type by extension is a security risk Checking MIME type by extension is a potential security risk Jan 7, 2015
@JangoSteve
Copy link
Contributor

@JangoSteve JangoSteve commented Jan 12, 2015

@d4rky-pl FYI, you can use the built-in mime_types processor to do more extensive MIME type testing:

require 'carrierwave/processing/mime_types'
class MyUploader < CarrierWave::Uploader::Base
  include CarrierWave::MimeTypes
  process :set_content_type
end

Then you could validate based on the content_type.

@JangoSteve
Copy link
Contributor

@JangoSteve JangoSteve commented Jan 12, 2015

Also, here's the original discussion on why Carrierwave uses the extension to determine the mime type by default.

EDIT: It seems now that the mime_types processor is deprecated though, as the mime_types gem has now been made a hard-dependency after all in the sanitized_file.rb, so you can get to the content type directly. I would think then that you could add a validation like this:

class Upload < ActiveRecord::Base
  mount_uploader :file, MyUploader
  validate :valid_content_type

  private
  def valid_content_type
    errors.add(:file, "is an invalid file type") unless %w(image/jpeg image/png).include? file.sanitized_file.content_type
  end
end
@d4rky-pl
Copy link
Author

@d4rky-pl d4rky-pl commented Jan 12, 2015

The problem with mime_types gem is that it doesn't do any actual checking.

# sanitized_file.rb:246

    def content_type
      return @content_type if @content_type
      if @file.respond_to?(:content_type) and @file.content_type
        @content_type = @file.content_type.to_s.chomp
      elsif path
        @content_type = ::MIME::Types.type_for(path).first.to_s
      end
    end
# mime/types.rb:151

  # Return the list of MIME::Types which belongs to the file based on its
  # filename extension. If there is no extension, the filename will be used
  # as the matching criteria on its own.

The link you've provided to the discussion does not explain why Carrierwave uses the extension to determine the mime type, I think you've got the wrong link :)

@JangoSteve
Copy link
Contributor

@JangoSteve JangoSteve commented Jan 13, 2015

@d4rky-pl That's a good point. I could have sworn that the mime_types gem actually introspected the file content to make sure it matched the content type, but looking through the source code, I'm not seeing it. Now that I think about it, I think the problem the mime_types processor solved was simply having a more complete dictionary of mime-types.

It seems that the ruby-filemagic gem has the bindings to libmagic to do what you're talking about (or, like you said, you could just farm it out to the unix command).

Might consider making it a processor though in the same vein as my link.

I thought we had discussed why it wasn't there by default, and why I added the mime_types as a processor in that pull request, but I'm now thinking maybe we had that discussion offline. The main reason was to minimize gem dependencies. A processor allows you to add extra functionality that may depend additional gems, without then requiring that gem as a hard dependency for people who don't need the functionality.

@d4rky-pl
Copy link
Author

@d4rky-pl d4rky-pl commented Feb 10, 2015

I feel a bit unease about using ruby-filemagic, considering it haven't been updated for the past 4.5 years :) Farming it from file command is probably the best idea right now, with fallback to extension-based MIME checking on Windows (or any other solution that is possible on this system). If we do it this way, there won't be any additional gems dependencies.

The choice on how to proceed is yours though, I can devote some time to this problem on some weekend.

@bensie
Copy link
Member

@bensie bensie commented Mar 3, 2015

File introspection to determine mime types is outside the scope of what we'll include in core. Like @JangoSteve mentioned though, adding a new processor capable of this would be great.

@seb-sykio
Copy link

@seb-sykio seb-sykio commented Jul 16, 2017

any update about checking mime type with file and not extension ?

@kevinjom
Copy link

@kevinjom kevinjom commented Oct 10, 2017

both ruby-filemagics and mimetypes gems work poorly, they are not working well with MS offices files. and as mentioned above, they are not updated frequently and PRs are not processed.

The problem with content-type check is its not reliable, you can rename a shell script with an image file extention, and the browser (from what I saw) will set the content type to image/..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.