search

Cisco-Talos / clamav

ClamAV - Documentation is here: https://docs.clamav.net
https://www.clamav.net/
GNU General Public License v2.0
4.43k stars 706 forks source link

Use SHA-2 hash over MD5 #1320

Closed yunimoo closed 2 months ago

yunimoo commented 3 months ago

I would like to keep this draft PR open to leverage the community for advice / guidance on supporting FIPS-140 through deprecating MD5 in favor of SHA-2. I think that it would be best to work towards the short term solution to unblock users with FIPS-140 requirements safely, while longer term solutions are in the works.

In reference to: https://github.com/Cisco-Talos/clamav/issues/564

yunimoo commented 3 months ago

Just an update. So I was able to get my hands on a FIPS enabled RHEL9 box. After building on the main branch, I get the same error of "Can't allocate memory" given the issue with MD5.

I have tested with this PR's changes and was able to get a different error which was shown in the pipeline: "Can't verify database integrity". Upon closer inspection it seems like the CVD use MD5, so SHA256 will not work as expected. I believe that the error for failed memory allocation happens while the system tries to obtain the MD5 checksum (FIPS restricts this). If that is the case, then it seems that changes in this branch are successfully reading the SHA256 but can't validate it with the current CVD internal (as it's MD5).

What I have currently tried out is by modifying unit_tests/input/freshclam_testfiles/test-1.cvd to use a completely different MD5 hash. When I tested against main branch I get the failed memory allocation, which is leading me to believe that this check from FIPS is restricted prior to obtaining the checksum. So... This could mean that changes in this PR are allowing SHA256 checksums to be obtained just fine, without FIPS complaining; as expected.

If so, I'd like to test a SHA256 CVD, but I am not quite sure where to begin with this. Changing the checksum to be SHA256 inside the CVD does not work, so any advice on where I can find the relevant documentation would be greatly appreciated. I was hoping to perhaps redo the freshclam_testfiles since all ctests are failing due to those files.

TLDR: SHA256 changes in branch seem to read, but fail because current CVD are all MD5

micahsnyder commented 3 months ago

Your changes make sense, if we want to simply replace MD5 in the existing signing process with SHA256. But there is a lot more work that would need to be done.

Our internal CVD signing code has the logic to be able to switch to SHA256 so that's also doable. The problem is that we'll need to continue to distribute the MD5 signed CVD's for older ClamAV versions, and also the newer SHA256 signed CVD's (new name?), and we'll have to put in additional changes in the readdb.c code (I think?) and freshclam code to differentiate. We'd then have to change our SigManager code to build and publish both versions. I think our web team will also need to change some stuff in cloudflare to host both and enable the same rate limiting rules, etc.

If we're going through all that, then I really want to switch to a better signing mechanism that is more standard / less custom, and also allows us to rotate signing keys, etc. Stuff I started brainstorming here: https://github.com/Cisco-Talos/clamav/issues/564#issuecomment-2078262495

yunimoo commented 3 months ago

Hi, thanks for the update on the changes for this PR.

I agree that distributing the old MD5 is still necessary... And moving everything to SHA256 is not the complete solution as users with older clamav will fail to work. Looking at your comment in I am wondering if it is possible to include both SHA256 and MD5 for a CVD, to facilitate the burden of having two separate builds? I'm not sure how logical it is for clamav to see if FIPS is enabled, then to use SHA256 - if not, default to MD5?

If we're going through all that, then I really want to switch to a better signing mechanism that is more standard / less custom, and also allows us to rotate signing keys, etc.

I'm personally in agreement that having a better signing mechanism can enhance security, and not a waste of time. I can try to pursue a solution for this, but I have limiting knowledge on this and will need some support.

I'm not sure. Is it even a good idea to store public keys for verifying CVD2's in the same directory? If not, why not?

I think that it should be fine to store the public keys for verifying the CVD2's (If we plan to use this new format) in the same directory. I'm curious to know if there are security concerns or edge cases where this might not be the best idea. If this helps unblock users with FIPS requirements, then please let me know if this seems fine to proceed. I imagine that the rotating keys can be in a separate PR?

micahsnyder commented 3 months ago

There's no reason to prefer MD5 over SHA256. The only reason to continue publishing with MD5's is for backwards compatibility with older clamav versions. If we sign the CVD's with MD5 and also with SHA256 while retaining compatibility with older CVD loading code, this would be a big win.

Sadly, I also cannot share the CVD signing software for you to work on. I am not allowed to open source it. And I don't think it is reasonable to ask you to solve it without being able to do test signing and test changes to the signing process yourself.

For what it's worth, I also want to resolve this issue. We have competing priorities and I haven't been able to dedicate dev time to it, yet.

yunimoo commented 3 months ago

If we sign the CVD's with MD5 and also with SHA256 while retaining compatibility with older CVD loading code, this would be a big win.

Yes, I agree with this as it might be the best approach for compatibility and lessens the maintenance required.

Sadly, I also cannot share the CVD signing software for you to work on. I am not allowed to open source it. And I don't think it is reasonable to ask you to solve it without being able to do test signing and test changes to the signing process yourself.

Which is perfectly reasonable. Thank you for the clarification on this.

We have competing priorities and I haven't been able to dedicate dev time to it, yet.

Would it be fine for me to leave this PR open until the CVD's have been updated? I can retest this branch again once the new files are promoted.

micahsnyder commented 3 months ago

Would it be fine for me to leave this PR open until the CVD's have been updated? I can retest this branch again once the new files are promoted.

It doesn't bother me. Leaving it open is a visual reminder, and an indication that I do consider this work to be important.