プロジェクト

全般

プロフィール

Vote #78286

完了

Use SHA256 for attachment digest computation

Admin Redmine さんが約4年前に追加. 約4年前に更新.

ステータス:
Closed
優先度:
通常
担当者:
-
カテゴリ:
Attachments_19
対象バージョン:
開始日:
2022/05/09
期日:
進捗率:

0%

予定工数:
category_id:
19
version_id:
119
issue_org_id:
25240
author_id:
40856
assigned_to_id:
1
comments:
12
status_id:
5
tracker_id:
3
plus1:
0
affected_version:
closed_on:
affected_version_id:
ステータス-->[Closed]

説明

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


journals

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

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

I think that Attachment#update_digest_to_sha256! should simply ignore the record if the corresponding file for the record is not exists.
--------------------------------------------------------------------------------
Yes. Here's the updated patch no. 2 with the @readable?@ check 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.

<pre><code class="diff">
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 %>
</code></pre>

!add-algorithm-name.png!
--------------------------------------------------------------------------------
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?
--------------------------------------------------------------------------------
Jens Krämer wrote:
> Maybe we could have a method named @Attachment#digest_type@ to at least clean up the view?

Yes, absolutely agree.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
here's the updated patch 3 showing the digest used for each file in the files list
--------------------------------------------------------------------------------
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).
--------------------------------------------------------------------------------
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!
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------


related_issues

blocks,Closed,25215,Re-use existing identical disk files for new attachments

Admin Redmine さんが約4年前に更新

  • カテゴリAttachments_19 にセット
  • 対象バージョン3.4.0_119 にセット

他の形式にエクスポート: Atom PDF

いいね!0
いいね!0