Patch #25240
closed
Use SHA256 for attachment digest computation
Added by Jens Krämer about 7 years ago. Updated almost 3 years ago.
0%
Description
As discussed in #25215 we should change the digest method used for attachment checksums to something stronger. The attached patches
- widen the
attachments.digestcolumn 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
| 0002-adds-a-rake-task-to-convert-the-digests-of-existing-.patch (2.62 KB) 0002-adds-a-rake-task-to-convert-the-digests-of-existing-.patch | Jens Krämer, 2017-03-02 04:27 | ||
| 0001-changes-the-digest-used-for-attachments-to-SHA256.patch (5.49 KB) 0001-changes-the-digest-used-for-attachments-to-SHA256.patch | Jens Krämer, 2017-03-02 04:27 | ||
| 0003-change-MD5-table-header-to-Checksum.patch (1.28 KB) 0003-change-MD5-table-header-to-Checksum.patch | Jens Krämer, 2017-03-02 04:27 | ||
| 0002-adds-a-rake-task-to-convert-the-digests-of-existing-.patch (2.66 KB) 0002-adds-a-rake-task-to-convert-the-digests-of-existing-.patch | updated version of patch 2 that doesn't error on missing files | Jens Krämer, 2017-03-02 07:16 | |
| add-algorithm-name.png (65.3 KB) add-algorithm-name.png | Go MAEDA, 2017-03-02 13:45 | ||
| 0003-change-MD5-table-header-to-Checksum.patch (2.43 KB) 0003-change-MD5-table-header-to-Checksum.patch | updated version of patch 3 including the display of digest type for each file | Jens Krämer, 2017-03-12 00:56 |
Related issues
Updated by Go MAEDA about 7 years ago
- Blocks Patch #25215: Re-use existing identical disk files for new attachments added
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.
Updated by Jens Krämer about 7 years ago
- File 0002-adds-a-rake-task-to-convert-the-digests-of-existing-.patch 0002-adds-a-rake-task-to-convert-the-digests-of-existing-.patch added
Yes. Here's the updated patch no. 2 with the readable? check added.
Updated by Go MAEDA about 7 years ago
- File add-algorithm-name.png add-algorithm-name.png added
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 %>
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?
Updated by Go MAEDA about 7 years ago
Jens Krämer wrote:
Maybe we could have a method named
Attachment#digest_typeto at least clean up the view?
Yes, absolutely agree.
Updated by Jens Krämer about 7 years ago
- File 0003-change-MD5-table-header-to-Checksum.patch 0003-change-MD5-table-header-to-Checksum.patch added
here's the updated patch 3 showing the digest used for each file in the files list
Updated by Jean-Philippe Lang about 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).
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!
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.
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.