Project

General

Profile

Actions

Patch #25240

closed

Use SHA256 for attachment digest computation

Added by Jens Krämer about 7 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
Normal
Category:
Attachments
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

As discussed in #25215 we should change the digest method used for attachment checksums to something stronger. The attached patches

  • widen the attachments.digest column to 64 chars
  • change the digest used for new attachments to SHA256
  • provide a rake task for upgrading the digest value of existing attachments

I also changed the label of the 'MD5' column header in the files list to 'Checksum'.


Files


Related issues

Blocks Redmine - Patch #25215: Re-use existing identical disk files for new attachmentsClosedJean-Philippe Lang

Actions
Actions #1

Updated by Go MAEDA about 7 years ago

  • Blocks Patch #25215: Re-use existing identical disk files for new attachments added
Actions #2

Updated by Go MAEDA about 7 years ago

Thanks for the patch.
But I encountered the following error while running "rake redmine:attachments:update_digest_to_sha256" on my test environment.

rake aborted!
Errno::ENOENT: No such file or directory @ rb_sysopen - /Users/maeda/redmines/redmine-trunk/files/2006/07/060719210727_error281.txt

I think that Attachment#update_digest_to_sha256! should simply ignore the record if the corresponding file for the record is not exists.

Actions #3

Updated by Jens Krämer about 7 years ago

Yes. Here's the updated patch no. 2 with the readable? check added.

Actions #4

Updated by Go MAEDA about 7 years ago

Jens Krämer wrote:

Yes. Here's the updated patch no. 2 with the readable? check added.

Thanks, now it works fine for me.

I have a suggestion. What about adding "MD5: " or "SHA256: " before hash values in app/views/files/index.html.erb? In the current implementation, it is a little bit difficult for users to know what hash algorithm is used to calculate the checksum.

diff --git a/app/views/files/index.html.erb b/app/views/files/index.html.erb
index 05fe37a..f111744 100644
--- a/app/views/files/index.html.erb
+++ b/app/views/files/index.html.erb
@@ -31,7 +31,7 @@
     <td class="created_on"><%= format_time(file.created_on) %></td>
     <td class="filesize"><%= number_to_human_size(file.filesize) %></td>
     <td class="downloads"><%= file.downloads %></td>
-    <td class="digest"><%= file.digest %></td>
+    <td class="digest"><%= file.digest.size < 64 ? "MD5" : "SHA256" %>: <%= file.digest %></td>
     <td class="buttons">
     <%= link_to(image_tag('delete.png'), attachment_path(file),
                                          :data => {:confirm => l(:text_are_you_sure)}, :method => :delete) if delete_allowed %>

Actions #5

Updated by Jens Krämer about 7 years ago

Yes, that makes sense. The check for length of the digest to find out which it is isn't particularly nice (I know I did the same in my query for finding the attachments to upgrade) but I'm still not sure adding the hashing algorithm as a field to the Attachment model is any better. Maybe we could have a method named Attachment#digest_type to at least clean up the view?

Actions #6

Updated by Go MAEDA about 7 years ago

Jens Krämer wrote:

Maybe we could have a method named Attachment#digest_type to at least clean up the view?

Yes, absolutely agree.

Actions #7

Updated by Jean-Philippe Lang about 7 years ago

  • Target version set to 3.4.0
Actions #8

Updated by Jens Krämer about 7 years ago

here's the updated patch 3 showing the digest used for each file in the files list

Actions #9

Updated by Jean-Philippe Lang almost 7 years ago

  • Status changed from New to Closed
  • Assignee set to Jean-Philippe Lang

Patches are committed, thanks Jens. I've made a few changes to the patches and changed the fixture used in the test to a binary file (possible failure due to \r\n EOLs).

Actions #10

Updated by Shane Coles almost 3 years ago

I know this issue is really old at this point, but if anyone is still watching it by chance I could use some help. I am migrating a Redmine server to a FIPS validated server and running into issues because of the MD5 validation. I found this issue and it sounds like it could solve the problem. Unfortunately when I tried to run the Patches, it prompted me for which files I would like patched, and the answer is that I do now know.
If these patches can make it so that Redmine attachments/repos can be viewed on a FIPS server that would be great, and any instructions to that end would also be nice.
Thanks!

Actions #11

Updated by Pavel Rosický almost 3 years ago

IIRC, even require 'digest/md5' is a problem on FIPS.

but Redmine use it in many other places
https://github.com/redmine/redmine/search?q=require+%27digest%2Fmd5%27

it's usually for cache keys calculations or gravatars, which is safe from a security perspective, but since the algorithm itself isn't allowed, the app won't work.

try to replace these occurrences with a different algorithm, but it may introduce incompatibilities. Do you know a way how to reliably test it? (I don't have a FIPS SW available)

you should open a new ticket if you want to discuss further since this one is already closed.

Actions #12

Updated by Shane Coles almost 3 years ago

Thanks for the response. I will open a new ticket. I'm not a server expert at all, we've just always had this and I need to move it and ran into this. Hopefully someone on the new ticket will know how to do it.

Actions

Also available in: Atom PDF