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

Fix a possible security issue with spoofing #2

Open
wants to merge 1,050 commits into
base: v.2.7.0
Choose a base branch
from
Open

Conversation

psergus
Copy link

@psergus psergus commented Jun 12, 2018

This is a patch that addresses this problem: https://robots.thoughtbot.com/paperclip-security-release

The description is the following:

There is an issue where if an HTML file is uploaded with a .html
extension, but the content type is listed as being `image/jpeg`, this
will bypass a validation checking for images. But it will also pass the
spoof check, because a file named .html and containing actual HTML
passes the spoof check.

This change makes it so that we also check the supplied content type. So
even if the file contains HTML and ends with .html, it doesn't match the
content type of `image/jpeg` and so it fails.

The patch was inspired by the commit to a recent versions: thoughtbot@9aee411

kesha-antonov and others added 30 commits October 3, 2015 18:09
update README.me: add missing 'end'
Before the error message was sometimes millions of characters long.

> [paperclip] Content Type Spoof: Filename a.csv
> (application/octet-stream from Headers,
> [#<MIME::Type::Columnar:0x007f9f90f89fa8
> @container=#<MIME::Types:0x007f9f90b09d98 ... snip millions of
> characters of output here ...>], @content_type="text/csv",
> @raw_media_type="text", @raw_sub_type="csv", @simplified="text/csv",
> @i18n_key="text.csv", @media_type="text", @sub_type="csv",
> @extensions=["csv"]>] from Extension), content type discovered from
> file command: application/zip. See documentation to allow this
> combination.

Now becomes:

> [paperclip] Content Type Spoof: Filename a.csv
> (application/octet-stream from Headers,
> ["text/comma-separated-values", "text/csv"] from Extension), content
> type discovered from file command: application/zip. See documentation
> to allow this combination.

[fixes thoughtbot#2017]
added the comma in line 54
* Regression fix
* Add specs for intermediate_files var in Paperclip::Attachment

[fixes thoughtbot#1908]
by using the attachment name (symbol) and Class as keys we reduce the number of Strings created before hitting the cache
this commit primarily uses frozen strings to reduce object creation during interpolation.
the :basename method now uses File.basename(file, ".*") rather than a Regexp. basename may be called multiple times.
the name string is used multiple times in interpolation so storing it reduces object creation
something in aruba 0.10.x is breaking the cucumber specs
Resolve broken CI tests due to upstream gem changes
something in aruba 0.10.x is breaking the cucumber specs
…g_v4.3

Cache interpolator methods and reduce memory allocations - rebased on v4.3
Includes memory usage adjustments.

Conflicts:
	lib/paperclip/storage/s3.rb
Update README with version requirement for aws-sdk
akihikodaki and others added 27 commits May 8, 2018 18:53
* Add deprecation notice to README

Addresses new projects, existing projects, issues, and PRs.
"key" is a reserved keyword in MariaDB, so the SQL statement fails.
Surrounding the keyword in backticks fixes the error.
This cause to erease previous image when the id change to above
`999_999_999`, for example:

```
2.3.6 :010 > id
 => 1000602578
2.3.6 :011 > ("%09d".freeze % id).scan(/\d{3}/).join("/".freeze)
 => "100/060/257"
```
Since the support version is Ruby 2.0 or later, magic comment is unnecessary.

> The UTF-8 default encoding, which make many magic comments omissible

https://www.ruby-lang.org/en/news/2013/02/24/ruby-2-0-0-p0-is-released/
Encountered an issue where the URI was returning header with
content-disposition where the filename value wasn't enclosed in the
double quotes. Turns out that this is a valid grammar according to
RFC6266. Also made the logic more robust to account for spaces and
uppercase letters.
This validation was causing issue when there is no content_type column
in the database, as the content type of the attachment would be empty.
There should be no need to check attachment's validity because this
`#reprocess!` method is running internally on the server.

Fix thoughtbot#2078
While using the Paperclip gem, we noticed during some ad-hoc testing
that if you do not supply an extension when uploading a file, Paperclip
effectively skipped it's spoofing check, which allowed potentially
dangerous files to slip through into your application.

This addresses that by moving the checks around a little bit and only
testing against the extension when there is one.
As described in thoughtbot#2118, `OpenURI::Meta#content_type` will return
`application/octet-stream` when no `content-type` header is set. Using
the original `meta` fixes this issue.
@psergus psergus changed the base branch from master to v.2.7.0 June 12, 2018 22:15
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.