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.

Sign up for GitHub

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

Jump to bottom

Support bcrypt $2b$ and $2y$ revisions #56086

Closed

CactoT wants to merge 1 commit into elastic:master from CactoT:bcrypt-support-2b-and-2y-revisions

CactoT commented May 3, 2020

Although we don't claim support for $2b$ and $2y$ revisions, this
change keeps our bcrypt implementation more in line with OpenBSD's
implementation, that is considered the canonical one and supports
$2b$ besides $2a$, and also other common implementations using $2y$
hashes.

This commit moves specific bcrypt code from Hasher to BCrypt in order
to improve responsibilities separation. We could refactor and move out
other hash specific code in the future (for PBKDF2 and SHA1, for
instance) so Hasher could have only one responsibility.

BCrypt tests were missing, so the commit adds tests for code moved
into it and also some tests for hash generation and verification, but
we should cover the rest of it.

Please note that BCrypt.generateHash still has code to support $2$,
which was already dropped by OpenBSD. We should evaluate if this needs
to be removed in the future.

Closes #51132


          Support bcrypt $2b$ and $2y$ revisions

3e2053a

Although we don't claim support for $2b$ and $2y$ revisions, this
change keeps our bcrypt implementation more in line with OpenBSD's
implementation, that is considered the canonical one and supports
$2b$ besides $2a$, and also other common implementations using $2y$
hashes.

This commit moves specific bcrypt code from Hasher to BCrypt in order
to improve responsibilities separation. We could refactor and move out
other hash specific code in the future (for PBKDF2 and SHA1, for
instance) so Hasher could have only one responsibility.

BCrypt tests were missing, so the commit adds tests for code moved
into it and also some tests for hash generation and verification, but
we should cover the rest of it.

Please note that BCrypt.generateHash still has code to support $2$,
which was already dropped by OpenBSD. We should evaluate if this needs
to be removed in the future.

Closes elastic#51132

danielmitterdorfer added the :Security/Authentication label

Collaborator

elasticmachine commented May 4, 2020

Pinging @elastic/es-security (:Security/Authentication)

elasticmachine added the Team:Security label

tvernum self-requested a review

June 4, 2020 06:37

tvernum self-assigned this

tvernum reviewed

View reviewed changes

Contributor

tvernum left a comment •

edited

Hi @CactoT - I just noticed that I've had review comments queued on this for the last 6 months and I never submitted them.

I'm very sorry that we let this stall for as long as we did. Are you still interested in working on this?

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/BCrypt.java

    
            @@ -666,7 +672,7 @@ public static String hashpw(SecureString password, String salt) {
          
                          off = 3;

                      else {

                          minor = salt.charAt(2);

                          if (minor != 'a' || salt.charAt(3) != '$')

                          if ((REVISIONS.contains(minor) == false) || (salt.charAt(3) != '$'))

                              throw new IllegalArgumentException ("Invalid salt revision");

Contributor

tvernum Jun 4, 2020

Suggested change

      
                            throw new IllegalArgumentException ("Invalid salt revision");
          
                            throw new IllegalArgumentException ("Invalid salt revision [" + minor + "]");

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/BCrypt.java

@@ @@ -652,7 +658,7 @@ private void ekskey(byte data[], byte key[]) { @@
                    *                  using BCrypt.gensalt)
                    * @return          the hashed password
                    */
-                  public static String hashpw(SecureString password, String salt) {
+                  public static String generateHash(SecureString password, String salt) {

Contributor

tvernum Jun 4, 2020

I'd prefer that we didn't rename these methods.
Although the names aren't consistent with how we typically name things in ES, this file is based on source code from outside of Elastic and it is easier to for us if we minimize unnecessary changes to it.

Author

CactoT Feb 10, 2021

@tvernum I missed the copyright notice. Agree on minimizing changes to it then. Besides not renaming any methods, it could be desirable to leave the original source as is, except for the specific check for new revisions:

- if (minor != 'a' || salt.charAt(3) != '$')
+ if ((minor != 'a' && minor != 'b' && minor != 'y') || salt.charAt(3) != '$')

This means that some supporting bcrypt code refactored from Hasher should return there or create a wrapper in order to better separate responsibilities while also maintaining the BCrypt class as close as the original as possible.

What do you think?

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/BCrypt.java

-                      // this method previously took a string and did its own constant time comparison
-                      return CharArrays.constantTimeEquals(hashed, hashpw(plaintext, hashed));
-                      /*************************** ES CHANGE END *************************/
+                  public static boolean verifyHash(SecureString plaintext, char[] hash) {

Contributor

tvernum Jun 4, 2020

Is there are reason to change this from String to char[]?
It means that you convert the char[] to a string twice (in isPrefixValid and then again inside the method), for no obvious gain.

Author

CactoT Feb 10, 2021

Should be a leftover of a refactor from Hasher that uses char[] instead of String, but agree with you. No reason for the change.

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/BCrypt.java

+                  public static boolean verifyHash(SecureString plaintext, char[] hash) {
+                      if (isPrefixValid(hash) == false) {
+                          return false;
+                      }

Contributor

tvernum Jun 4, 2020

I don't follow why we need this block.
It means that attempting to verify a password against a $2x$ hash will always return false rather than throw an exception, but I don't follow why that's a change we want to make.
It turns an error (invalid password hash format) into a silent failure.

Author

CactoT Feb 10, 2021

It was refactored from Hasher.verifyBcryptHash, the only client for BCrypt.checkpw. Not a good idea to change bevahior, taking into account your previous comment related to BCrypt being from an outside source.

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/BCrypt.java

+                      if (hash == null) {
+                          return false;
+                      }
+                      return new String(hash).matches(STRING_WITH_VALID_PREFIX_REGEX);

Contributor

tvernum Jun 4, 2020

If we do keep this check, we definitely won't want to do it with matches. This should be a very simple check that doesn't need the overhead of a regular expression.

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/BCrypt.java Show resolved Hide resolved

tvernum added a commit to tvernum/elasticsearch that referenced this pull request


          Import tests from elastic#56086

15ed58e

tvernum mentioned this pull request

Support 2b and 2y prefixes in bcrypt #76083

Merged

Contributor

tvernum commented Aug 5, 2021

I've opened a new PR (#76083) incorporating the changes from this PR against the current state of master.

tvernum closed this in #76083

tvernum added a commit that referenced this pull request


          Support 2b and 2y prefixes in bcrypt (#76083)

98d2989

Adds support for parsing "$2b$" and "$2y$" prefixes in bcrypt hashes.

These non standard prefixes are commonly used by some other tools
(e.g. python defaults to "$2b$" and PHP defaults to "$2y$").

Our implementation will continue to generate "$2a$" hashes, but will
support hashes imported from tools that use these other prefixes.

Closes: #56086, #51132

Co-authored-by: cactot aprus@protonmail.com

elasticsearchmachine pushed a commit to elasticsearchmachine/elasticsearch that referenced this pull request


          Support 2b and 2y prefixes in bcrypt (elastic#76083)

ab3eb2e

Adds support for parsing "$2b$" and "$2y$" prefixes in bcrypt hashes.

These non standard prefixes are commonly used by some other tools
(e.g. python defaults to "$2b$" and PHP defaults to "$2y$").

Our implementation will continue to generate "$2a$" hashes, but will
support hashes imported from tools that use these other prefixes.

Closes: elastic#56086, elastic#51132

Co-authored-by: cactot aprus@protonmail.com

tvernum added a commit to tvernum/elasticsearch that referenced this pull request


          Support 2b and 2y prefixes in bcrypt

390b33a

Adds support for parsing "$2b$" and "$2y$" prefixes in bcrypt hashes.

These non standard prefixes are commonly used by some other tools
(e.g. python defaults to "$2b$" and PHP defaults to "$2y$").

Our implementation will continue to generate "$2a$" hashes, but will
support hashes imported from tools that use these other prefixes.

Closes: elastic#56086, elastic#51132

Co-authored-by: cactot aprus@protonmail.com

Backport of: elastic#76083

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment