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

Comments

Projects
None yet
5 participants
@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 from Checking MIME type by extension is a security risk to Checking MIME type by extension is a potential security risk Jan 7, 2015

@JangoSteve

This comment has been minimized.

Show comment
Hide comment
@JangoSteve

JangoSteve Jan 12, 2015

Contributor

@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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@JangoSteve

JangoSteve Jan 12, 2015

Contributor

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
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@d4rky-pl

d4rky-pl 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 :)

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

This comment has been minimized.

Show comment
Hide comment
@JangoSteve

JangoSteve Jan 13, 2015

Contributor

@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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@d4rky-pl

d4rky-pl 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.

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

This comment has been minimized.

Show comment
Hide comment
@bensie

bensie Mar 3, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@seb-sykio

seb-sykio Jul 16, 2017

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

seb-sykio commented Jul 16, 2017

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

@kevinjom

This comment has been minimized.

Show comment
Hide comment
@kevinjom

kevinjom 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/..

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