Change MD5 Fingerprints to SHA1 Fingerprints #1708

teliosdev · 2014-11-22T19:51:59Z

MD5 can collide. SHA1 is better.

teliosdev · 2014-11-22T20:12:47Z

I'm not sure why the paperclip spec for fog is failing...

clemensg · 2014-12-04T18:47:26Z

👍

@jyurek Please merge this change. On modern hardware SHA1 is even faster than MD5 and at the same time it is less prone to collision attacks.

The travis error has nothing to do with this change. According to the Travis build history the fog test failure did also occur with other PRs.

jyurek · 2014-12-04T20:14:38Z

This seems fine. But this is a pretty big change, given that people currently have fingerprints in MD5 format.

But, anyway, collisions are problems with hashing functions in general. Are collisions actually something you've had problems with in Paperclip?

clemensg · 2014-12-04T20:36:38Z

It's known for a long time that MD5 is vulnerable to collision attacks. For SHA1 on the other hand, there are still no known collisions, although that might just be a matter of time.

I did not have problems yet, but I am using it in a critical environment, so I'd rather be safe than sorry. I don't fear the probability of accidentally colliding filenames, but a deliberate collision attack by a malicious user. (Maybe I am a little paranoid :D)

Regarding backwards-compatibility:
It is likely that some paperclip users relied on the fact that it uses MD5 for the fingerprint.

What do you think about letting the user choose the hash algorithm?
I'd use MD5 as a default, so existing apps won't be affected. The rails generator could then add the SHA1 setting for new apps.

teliosdev · 2014-12-04T20:53:32Z

@jyurek With a carefully crafted file, an attacker cam create two distinct files that make the same MD5, and given the nature of paperclip, I think it would be best to move to SHA1.

@clemensg I don't like the idea of making MD5 default. The Rails Way™ is convention over configuration, so they should have to choose to use MD5; not to say that the compatibility thing isn't an issue, though.

Maybe have a converter that updates the hash to SHA1 whenever it can?

clemensg · 2014-12-04T21:23:07Z

Maybe a rake task to regenerate all fingerprints and a warning in the README and NEWS file?

@jyurek Do you expect any issues with backwards-compatibility?
What is your opinion on the configurability of the hash function and how do you want to proceed?

jyurek · 2014-12-05T19:33:11Z

Yes, I expect issues with backwards compatibility! If we're going to add this, then an option for which Digest class to use should be in place, but I'm not 100% sure how/where to add it yet. The fingerprint method is in lib/paperclip/io_adapters/abstract_adaptor.rb, which doesn't have access to the Paperclip options.

maclover7 · 2015-03-10T20:53:38Z

Hi everybody! Is this still an issue? If so, can you please rebase your PR and make sure everything is passing? Thanks!

teliosdev · 2015-03-10T22:50:13Z

Yeah, I'll rebase this, give me a little while.

teliosdev · 2015-03-10T23:30:21Z

Updated, but I'm not sure what's going on with travis...

maclover7 · 2015-03-11T14:08:28Z

This looks set to go.

paging @jferris @jyurek

teliosdev · 2015-03-18T17:32:39Z

Any progress on the status of this?

maclover7 · 2015-03-21T01:26:32Z

Hi @medcat! Currently just waiting on @jyurek or @jferris to say if this is ready to be merged into Paperclip. I think it's ready, but we need to wait for one of them (I don't have write permissions to the repo).

clemensg · 2015-03-21T14:28:06Z

Hi! What about existing projects who update to this version.. shouldn't there be a way to automatically migrate their MD5 fingerprints to SHA1? A rake task to force the regeneration of all fingerprints would be nice.

maclover7 · 2015-03-24T20:44:32Z

paging @jferris @jyurek @mike-burns

jyurek · 2015-04-24T18:03:42Z

This isn't going to get merged without a way to either automatically update the hashes or a way to switch between which class is doing the digesting. As I said before, there are a LOT of people already using MD5 hashes and this would break every single one. Please address this or it won't get merged. Thanks.

houndci-bot · 2015-04-24T20:09:58Z

lib/paperclip/attachment.rb

@@ -94,7 +95,7 @@ def initialize(name, instance, options = {})
    # attachment:
    #   new_user.avatar = old_user.avatar
    def assign(uploaded_file)
-      @file = Paperclip.io_adapters.for(uploaded_file)
+      @file = Paperclip.io_adapters.for(uploaded_file, @options[:adapter_options])


Line is too long. [82/80]

houndci-bot · 2015-08-02T21:30:03Z

spec/paperclip/io_adapters/abstract_adapter_spec.rb

+    end
+
+    context "SHA1" do
+      let(:options) { { :hash_digest => Digest::SHA1 } }


Use the new Ruby 1.9 hash syntax.

teliosdev · 2015-08-02T21:49:00Z

I believe I am content with it.

jyurek · 2015-09-01T18:48:48Z

lib/paperclip/attachment.rb

@@ -32,6 +32,7 @@ def self.default_options
        :use_timestamp         => true,
        :whiny                 => Paperclip.options[:whiny] || Paperclip.options[:whiny_thumbnails],
        :validate_media_type   => true,
+        :adapter_options       => { :hash_digest => Digest::SHA1 },


I'm sorry to have to repeat this, but SHA1 cannot be the default. At least, not until we have a major version bump. This should remain MD5, and changing it to SHA1 (or anything else provided by Digest) should be well documented. There can (and should) still be tests for SHA1, but it should be specified as an option in those tests, not assumed that it's the default.

Adapters now accept an options parameter, that currently specifies the type of hash digest to use. The default value is SHA1, but can be specified to be any OpenSSL-supported digest. The specs are modified to reflect that. The task just reassigns all of the attachments, thereby regenerating their fingerprints.

houndci-bot · 2015-12-08T01:12:45Z

spec/paperclip/io_adapters/attachment_adapter_spec.rb

@@ -2,7 +2,8 @@

 describe Paperclip::AttachmentAdapter do
  before do
-    rebuild_model path: "tmp/:class/:attachment/:style/:filename", styles: {thumb: '50x50'}
+    rebuild_model path: "tmp/:class/:attachment/:style/:filename",
+      styles: {thumb: '50x50'}


Align the elements of a hash literal if they span more than one line.
Space inside { missing.
Space inside } missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2015-12-08T02:39:32Z

spec/paperclip/io_adapters/attachment_adapter_spec.rb

@@ -65,7 +65,7 @@

      @attachment.assign(@file)
      @attachment.save
-      @subject = Paperclip.io_adapters.for(@attachment)
+      @subject = Paperclip.io_adapters.for(@attachment, hash_digest: Digest::SHA1)


Line is too long. [82/80]

teliosdev · 2015-12-08T02:41:38Z

It defaults to MD5 now.

bdewater · 2016-03-07T13:20:26Z

I believe defaulting to MD5 is a mistake with this change possibly getting released with Paperclip 5.0 (#1991, #2049), where you can 'get away' with breaking changes. Keep it around as an option for existing users to add so they can upgrade the hashes at their own pace, but keep it secure by default. Go with SHA2, based on https://gist.github.com/tqbf/be58d2d39690c3b366ad

clemensg · 2016-03-07T13:26:45Z

I second that. SHA-256 seems to be a safe default for Paperclip 5.0.
The upgrade rake task makes the update process easy.

I think it is a mistake to switch to SHA1 which has several known flaws. If we update then let's go straight to SHA2 (with 256 bits). SHA2-512 and SHA3 are probably overkill.

This PR is sitting around since the end of 2014. Any remaining objections from the maintainers?

@jyurek @tute @mike-burns @maclover7

tute · 2016-03-12T19:24:58Z

Deferring to @jyurek.

tute · 2016-05-09T18:59:26Z

I agree that if we change the hashing algorithm, it should be SHA2 by default, and it should be configurable so users upgrading can continue to use MD5 until needed. We can make this decision until we release the next major version, which is soon upcoming.

We want to comply with the coding standards and Hound comments for changed code, but leave alone the preexisting code. That way the changeset is smaller and easier to review. If there are only stylistic changes and you can undo them, please do. Thanks!

Can you please rebase on top of latest master? Thank you all for your input and work.

bdewater · 2016-06-11T14:00:39Z

Unless somebody beats me to it, I'm willing to work the next week on rebasing this PR with SHA256 as default, incorporating the feedback from Hound and the above comment from @tute, so this can ship in Paperclip 5.0.

tute · 2016-08-16T14:09:14Z

Closing in favor of @bdewater's #2229. Thank you.

teliosdev force-pushed the sha1 branch from 6662597 to 79a3716 Compare March 10, 2015 23:27

maclover7 mentioned this pull request Mar 23, 2015

Paperclip Meta-Issue #1806

Closed

jyurek added the Needs More Work label Apr 24, 2015

houndci-bot reviewed Apr 24, 2015
View reviewed changes

houndci-bot reviewed Aug 2, 2015
View reviewed changes

jyurek reviewed Sep 1, 2015
View reviewed changes

tute removed this from the v4.4.0 milestone Sep 10, 2015

teliosdev added 2 commits December 7, 2015 18:08

Change MD5 Fingerprints to SHA1 Fingerprints

163d6b1

teliosdev force-pushed the sha1 branch from 79a687c to 4290b7f Compare December 8, 2015 01:12

houndci-bot reviewed Dec 8, 2015
View reviewed changes

Use MD5 Digest as Default Hashing Function

02b7148

teliosdev force-pushed the sha1 branch from 4290b7f to 02b7148 Compare December 8, 2015 02:39

houndci-bot reviewed Dec 8, 2015
View reviewed changes

tute force-pushed the master branch from 017fead to e60f000 Compare March 12, 2016 18:57

tute assigned jyurek Mar 12, 2016

tute mentioned this pull request May 10, 2016

Feature request: Option to use SHA-256 hash instead of MD5 #2187

Closed

bdewater mentioned this pull request Jun 14, 2016

Make fingerprints configurable to prepare for a new default in the future #2229

Merged

tute closed this Aug 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change MD5 Fingerprints to SHA1 Fingerprints #1708

Change MD5 Fingerprints to SHA1 Fingerprints #1708

teliosdev commented Nov 22, 2014

teliosdev commented Nov 22, 2014

clemensg commented Dec 4, 2014

jyurek commented Dec 4, 2014

clemensg commented Dec 4, 2014

teliosdev commented Dec 4, 2014

clemensg commented Dec 4, 2014

jyurek commented Dec 5, 2014

maclover7 commented Mar 10, 2015

teliosdev commented Mar 10, 2015

teliosdev commented Mar 10, 2015

maclover7 commented Mar 11, 2015

teliosdev commented Mar 18, 2015

maclover7 commented Mar 21, 2015

clemensg commented Mar 21, 2015

maclover7 commented Mar 24, 2015

jyurek commented Apr 24, 2015

houndci-bot Apr 24, 2015

houndci-bot Aug 2, 2015

teliosdev commented Aug 2, 2015

jyurek Sep 1, 2015

houndci-bot Dec 8, 2015

houndci-bot Dec 8, 2015

teliosdev commented Dec 8, 2015

bdewater commented Mar 7, 2016

clemensg commented Mar 7, 2016

tute commented Mar 12, 2016

tute commented May 9, 2016

bdewater commented Jun 11, 2016

tute commented Aug 16, 2016

Change MD5 Fingerprints to SHA1 Fingerprints #1708

Change MD5 Fingerprints to SHA1 Fingerprints #1708

Conversation

teliosdev commented Nov 22, 2014

teliosdev commented Nov 22, 2014

clemensg commented Dec 4, 2014

jyurek commented Dec 4, 2014

clemensg commented Dec 4, 2014

teliosdev commented Dec 4, 2014

clemensg commented Dec 4, 2014

jyurek commented Dec 5, 2014

maclover7 commented Mar 10, 2015

teliosdev commented Mar 10, 2015

teliosdev commented Mar 10, 2015

maclover7 commented Mar 11, 2015

teliosdev commented Mar 18, 2015

maclover7 commented Mar 21, 2015

clemensg commented Mar 21, 2015

maclover7 commented Mar 24, 2015

jyurek commented Apr 24, 2015

houndci-bot Apr 24, 2015

Choose a reason for hiding this comment

houndci-bot Aug 2, 2015

Choose a reason for hiding this comment

teliosdev commented Aug 2, 2015

jyurek Sep 1, 2015

Choose a reason for hiding this comment

houndci-bot Dec 8, 2015

Choose a reason for hiding this comment

houndci-bot Dec 8, 2015

Choose a reason for hiding this comment

teliosdev commented Dec 8, 2015

bdewater commented Mar 7, 2016

clemensg commented Mar 7, 2016

tute commented Mar 12, 2016

tute commented May 9, 2016

bdewater commented Jun 11, 2016

tute commented Aug 16, 2016