Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change PBKDF2 hash from SHA512 to SHA256 (according to specification) #176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kjetilkl
Copy link

I have been looking more deeply into the Java version of Crypt4GH created by Dmytro Titov (@dtitov) many years ago,
and especially the KDF functions that stretch the password used to encrypt the private key file.

I compared the Java version to the Python implementation and this Go implementation to see if the KDFs produce the same hash values and verify that private key files created with the different versions are compatible with each other.
The good news is that all three versions produce the same hash for SCrypt, which is the preferred and default KDF. For the BCrypt alternative, the Python and Go implementations are compatible, but the Java version is not. I am looking into that.

For PBKDF2, the Java and Python versions are compatible but the Go version is not. PBKDF2 combines the input with a salt using a hash function and repeats this process multiple times. And from the Crypt4GH documentation, it seems clear that PBKDF2 should use SHA256 as its hash function. It is even part of the name for the KDF: pbkdf2_hmac_sha256.

But for some reason, the Go implementation is using SHA512, even if it still retains the KDF name pbkdf2_hmac_sha256.
I don't know the reason for this. Perhaps Dmytro decided to use SHA512 because it is more secure, but this should not really matter much anyway, since PBKDF2 should not really be used in the first place, and it is only an alternative in case the first two options, SCrypt and BCrypt, are not available.

My PR changes the hash function back to SHA256 (which is the correct choice according to the Crypt4GH specification) and makes it compatible with the other two implementations of Crypt4GH.

The new hash value in the kdf_test.go file is the one that is returned by the Python implementation using SHA256.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.49%. Comparing base (35584b2) to head (6d7085f).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #176   +/-   ##
=======================================
  Coverage   50.49%   50.49%           
=======================================
  Files           9        9           
  Lines        1717     1717           
=======================================
  Hits          867      867           
  Misses        731      731           
  Partials      119      119           
Flag Coverage Δ
unittests 50.49% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dtitov
Copy link
Contributor

dtitov commented Dec 20, 2024

@kjetilkl, very nice findings and analysis! Probably it was just an oversight of mine. I don’t know if I’m still entitled to approve PRs here, but since technically I can do it - I approved :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants