Enhance pbkdf2 password hashing

[adkharat]

Description

CWE : 759
Using SHA-256 (256 bit) cipher for PBKDF2 while fallback option with SHA1 (160 bit) cipher

Motivation and Context

SHA-1 is no longer considered secure for modern cryptographic applications due to vulnerabilities that allow attackers to perform collision attacks or brute force the hash more efficiently. Security standards (e.g., NIST guidelines) recommend moving away from SHA-1 to SHA-256.

Impact

Release Notes

== RELEASE NOTES ==

Security Changes
* Improve pbkdf2 hashing using SHA-256 cipher in response to `CWE-759 <https://cwe.mitre.org/data/definitions/759.htm>`_. :pr:`24132`

[agrawalreetika]

If we make changes in the HMAC hash algorithm, will it be backward compatible with the existing password hash of the earlier deployments that are getting used with HMAC-SHA1?

[adkharat]

@agrawalreetika
I think No, changing HMAC hash algorithm from SHA1 to SHA256 in PBKDF2 will not be backward compatible with existing password hashes created using the earlier algorithm (HMAC-SHA1) because the generated hash is a direct product of the selected HMAC, the password, the salt, and the iteration count.

outputHash = PBKDF2(HMAC, inputPassword, Salt, Iteration, keyLength)

If we change the HMAC from SHA-1 to SHA-256, output hash will differ even for the same inputPassword and other input parameters.

[agrawalreetika]

@agrawalreetika I think No, changing HMAC hash algorithm from SHA1 to SHA256 in PBKDF2 will not be backward compatible with existing password hashes created using the earlier algorithm (HMAC-SHA1) because the generated hash is a direct product of the selected HMAC, the password, the salt, and the iteration count.

outputHash = PBKDF2(HMAC, inputPassword, Salt, Iteration, keyLength)

If we change the HMAC from SHA-1 to SHA-256, output hash will differ even for the same inputPassword and other input parameters.

Thanks for confirming.
Then I think this would be a breaking change for existing deployments using Password authentication. They need to regenerate their respective password hash

[adkharat]

@agrawalreetika

Right, Password authentication for users who have hashed their passwords with SHA-1 will fail unless we migrate their existing hash to SHA-256.

I am proposing below approach to migrate users with SHA-1 password to SHA-256

(1) Detecting the Hash Algorithm:

• When a user logs in, check the stored hash to determine whether it uses SHA-1 or SHA-256 based on hash length (20 bytes for SHA-1, 32 bytes for SHA-256)

(2) Migration Logic:

• If the hash is using SHA-1, rehash the password with SHA-256 and Update the user password hash where it was stored originally. (E.g. Database, etc...)
• If hash is already with SHA-256 then continue with normal authentication.

@agrawalreetika please let me know if we implement above migration approach then will it break Password authentication?

[anandamideShakyan]

@agrawalreetika

Right, Password authentication for users who have hashed their passwords with SHA-1 will fail unless we migrate their existing hash to SHA-256.

I am proposing below approach to migrate users with SHA-1 password to SHA-256

(1) Detecting the Hash Algorithm:

• When a user logs in, check the stored hash to determine whether it uses SHA-1 or SHA-256.

(2) Migration Logic:

• If the hash is using SHA-1, rehash the password with SHA-256 and Update the user password hash where it was stored originally. (E.g. Database, etc...)
• If hash is already with SHA-256 then continue with normal authentication.

@agrawalreetika please let me know if we implement above migration approach then will it break Password authentication?

@adkharat For the solution that you are proposing for migration, what additional changes need to be done to the code?

[adkharat]

@anandamideShakyan , @agrawalreetika

We will need to update method and class in : EncryptionUtil.java

• Update method : doesPBKDF2PasswordMatch()
• Add new method : migrateSHA1toSHA256()
• Update class : PBKDF2Password

Unknow to me is, What is the storage location where user password hash is stored? and how can I store back migrated hash?

1. Update method : doesPBKDF2PasswordMatch()

public static boolean doesPBKDF2PasswordMatch(String inputPassword, String hashedPassword)
    {
        PBKDF2Password password = PBKDF2Password.fromString(hashedPassword);

        if (password.isSHA1()) {
            String migratedHash = migrateSHA1toSHA256(inputPassword, password);
            updateUserPasswordInDatabase(migratedHash); //TODO ? How ?
            return true;
        }

        try {
            KeySpec spec = new PBEKeySpec(inputPassword.toCharArray(), password.salt(), password.iterations(), password.hash().length * 8);
            SecretKeyFactory key = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
            byte[] inputHash = key.generateSecret(spec).getEncoded();

            if (password.hash().length != inputHash.length) {
                throw new HashedPasswordException("PBKDF2 password input is malformed");
            }
            return MessageDigest.isEqual(password.hash(), inputHash);
        }
        catch (NoSuchAlgorithmException | InvalidKeySpecException e) {
            throw new HashedPasswordException("Invalid PBKDF2 password", e);
        }
    }

2. Add new method : migrateSHA1toSHA256()

private static String migrateSHA1toSHA256(String inputPassword, PBKDF2Password oldPassword)
 {
     try {
         SecretKeyFactory key = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
         KeySpec spec = new PBEKeySpec(inputPassword.toCharArray(), oldPassword.salt(), PBKDF2_MIN_ITERATIONS, 256);
         byte[] newHash = key.generateSecret(spec).getEncoded();
         return PBKDF2_MIN_ITERATIONS + ":" + base16().lowerCase().encode(oldPassword.salt()) + ":" + base16().lowerCase().encode(newHash);
     } catch (NoSuchAlgorithmException | InvalidKeySpecException e) {
         throw new RuntimeException(e);
     }
 }

3. Update class : PBKDF2Password

private static class PBKDF2Password
    {
        private final int iterations;
        private final byte[] salt;
        private final byte[] hash;
        private final boolean isSHA1;

        private PBKDF2Password(int iterations, byte[] salt, byte[] hash, boolean isSHA1)
        {
            this.iterations = iterations;
            this.salt = requireNonNull(salt, "salt is null");
            this.hash = requireNonNull(hash, "hash is null");
            this.isSHA1 = isSHA1;
        }

        public int iterations()
        {
            return iterations;
        }

        public byte[] salt()
        {
            return salt;
        }

        public byte[] hash()
        {
            return hash;
        }

        public boolean isSHA1() 
        {
            return isSHA1;
        }

        public static PBKDF2Password fromString(String password)
        {
            try {
                List<String> parts = Splitter.on(":").splitToList(password);
                checkArgument(parts.size() == 3, "wrong part count");

                int iterations = Integer.parseInt(parts.get(0));
                byte[] salt = base16().lowerCase().decode(parts.get(1));
                byte[] hash = base16().lowerCase().decode(parts.get(2));
                boolean isSHA1 = (hash.length == 20);

                return new PBKDF2Password(iterations, salt, hash, isSHA1);
            }
            catch (IllegalArgumentException e) {
                throw new HashedPasswordException("Invalid PBKDF2 password");
            }
        }
    }

[agrawalreetika]

In my opinion, Presto shouldn't be responsible for migrating the password hashing.
Instead, we can have a fallback even for the SHA1 hash, mark it as deprecated, and enter the details about it in the documentation as well. Eventually, we can remove SHA1 hash support.

We can hear from others in the community about the options to handle this.

[adkharat]

Thank @agrawalreetika .
Maintaining a fallback for SHA-1 while marking it as deprecated looks correct approach to ensure backward compatibility.
I have pushed the changes with fall back to SHA-1. Can you please review the changes.

[agrawalreetika]

@adkharat Can you please add/update the documentation as well?

[adkharat]

@agrawalreetika Do you have any example to share on how to add documentation.

[steveburnett]

@agrawalreetika Do you have any example to share on how to add documentation.

I can help you get started with that, I hope.

You will need to update an existing doc file, or create a new doc file, in
https://github.com/prestodb/presto/tree/master/presto-docs/src/main/sphinx

Presto documentation is written in reStructuredText format.

See the presto-docs README for information on how to locally build and view the doc to test.

Add the doc change in your branch for this PR as a new commit.

If you have other questions about the Presto documentation, please ask, and thank you in advance for the doc!

[adkharat]

@steveburnett @agrawalreetika I have added documentation. Can you please review it.

[steveburnett]

Thanks for the release note entry! Just a couple of nits addressed in the draft below:

== RELEASE NOTES ==

Security Changes
* Improve pbkdf2 hashing using SHA-256 cipher in response to `CWE-759 <https://cwe.mitre.org/data/definitions/759.htm>`_. :pr:`24132`

[adkharat]

Thanks for the release note entry! Just a couple of nits addressed in the draft below:

== RELEASE NOTES ==

Security Changes
* Improve pbkdf2 hashing using SHA-256 cipher in response to `CWE-759 <https://cwe.mitre.org/data/definitions/759.htm>`_. :pr:`24132`

Done

[steveburnett]

Thanks for the documentation!

In general, documentation describes the current state of the software without too much emphasis on a change event. The phrasing of this doc feels like it fits a "news" topic, such as the Highlights section of the Release Notes for the upcoming release. (Note: when the release notes are written for the release this code change is included in, I believe it should be mentioned in the Highlights for the release as valuable information for Presto users.)

I suggest that you consider revising the text slightly for the tone I am describing. I’ve made a few suggestions, but based on this draft's quality I'll trust you to revise this documentation for a consistent tone if I miss something.

[agrawalreetika]

@adkharat Did you check the existing Password File Authentication document? Should it go in the same? cc: @steveburnett

[steveburnett]

@adkharat Did you check the existing Password File Authentication document? Should it go in the same? cc: @steveburnett

Thank you, @agrawalreetika!

@adkharat, consider if your new topic can be added to the existing Password File Authentication documentation instead of a separate page.

[adkharat]

Thanks @steveburnett @agrawalreetika .
I have updated existing Password File Authentication documentation instead of a separate page. Can you please review it.

[steveburnett]

Thanks for the revision using the existing doc page! A few minor edits and suggestions for you to consider.

[steveburnett]

Thanks for the quick revision!

Pull updated branch, new local doc build, reviewed new local doc build, everything looks good.

[steveburnett]

LGTM! (docs)

Pull updated branch, new local doc build, reviewed local build, looks good. Thanks!

Needs a test

[adkharat]

@tdcmeehan @agrawalreetika updated test cases.