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

SEC-3121: Support for bcrypt revision $2b$ #3320

Closed
spring-projects-issues opened this issue Oct 7, 2015 · 23 comments
Closed

SEC-3121: Support for bcrypt revision $2b$ #3320

spring-projects-issues opened this issue Oct 7, 2015 · 23 comments
Assignees
Labels
type: enhancement A general enhancement type: jira An issue that was migrated from JIRA
Milestone

Comments

@spring-projects-issues
Copy link

Peter De Wachter (Migrated from SEC-3121) said:

In February 2014 a new revision of the bcrypt algorithm was published, which Spring Security does not yet implement. This is starting to cause interoperability problems, as various tools now generate bcrypt hashes that are not supported by Spring Security.

@spring-projects-issues
Copy link
Author

Cory Berg said:

Interestingly, I found that the both the Spring bcrypt and the mindrot version seem to still have a bcrypt prefix of $2a$ which causes interoperability problems with, among other things, PHP (which uses $2y$, see http://php.net/manual/en/function.password-hash.php). That means one cannot validate passwords generated in something like PHP Laravel with Spring, without hand-changing the prefix in the Spring version. After I did this manual change, I was able to validate against $2y$ prefixed hashes from PHP. Perhaps someone who is more familiar with the theory of bcrypt could comment?

@spring-projects-issues spring-projects-issues added Open type: enhancement A general enhancement type: jira An issue that was migrated from JIRA labels Feb 5, 2016
@jeanbza
Copy link

jeanbza commented Feb 6, 2016

This is kind of interesting. What's the general approach for this? Support both through config + defaults, switch to supporting the new one, etc?

@pdewacht
Copy link

pdewacht commented Feb 7, 2016

The reference URL got lost in the migration. This message explains the change:
https://marc.info/?l=openbsd-misc&m=139320023202696

@icey502
Copy link

icey502 commented Feb 18, 2016

You guys are welcome to my version of the bcrypt code, it is essentially the same except I replaced the hard-coded version string usage with something that I could modify to comply with the PHP stack I was up against at the time. Trivial workaround once I realized what was happening. I could imagine a "real" solution involving either a configuration setting or some kind of apriori inspection of the revision, possibly both. To give an idea (there are several areas that I had to touch to make it work, but the approach is the same):

private static final char SALT_REVISION_HACK_PHP = 'y';
...
in gensalt(...):
....
//rs.append("$2a$"); <-- original
rs.append("$2"+SALT_REVISION_HACK_PHP+"$"); <-- yucky but working change

Gross, but operational :-) Alas I am not a Bcrypt expert by any stretch, so alternate validation is probably a good idea.

@neuhaus
Copy link

neuhaus commented Jul 1, 2016

The webpage for the openwall crypt_blowfish library which also implements bcrypt has some information and pointers regarding $2b$ and $2y$.

@rwinch
Copy link
Member

rwinch commented Sep 19, 2017

The problem with just parsing these patterns is that they all behave slightly differently. This is the reason that the different prefixes exist in the first place. This means if we run into the edge case where there was a bug, then the password will not verify. So just changing the identifier may work in a large number of cases, but it won't for all.

I don't think we want to pretend that this will work with the bad implementations of BCrypt.

If we want to update the version of BCrypt, then we need mindrot to update their implementation so that we know whatever version we are parsing with is actually compatible with this implementation. Alternatively, we would consider using bouncy castle

@kopax
Copy link

kopax commented Oct 8, 2017

Any update on this ?

I have the same issue using https://github.com/wclarie/openldap-bcrypt module for OpenLDAP.
The BCrypt password doesn't pass the test:

image

@rwinch
Copy link
Member

rwinch commented Oct 9, 2017

@kopax No updates from my side.

We cannot just change the prefix for the reasons outlined in #3320 (comment) This means we need to find a credible crypto library that supports the additional prefixes.

You could investigate if bouncy castle supports the additional prefixes. If it does, then sending a PR that delegates to Bouncy Castle would get merged in. If not, then you need to try and get Bouncy Castle or Mindrot to update their implementations.

@bytor99999
Copy link

So our encrypting passwords and then checking for passwords are using the exact same code in Spring Security and for some users the first password is randomly generated, then sent to the encrypt code and saved to our users table. The user gets an email with the generated password for them to use to login. They go to our app and try to login and it fails because of this issue. We are not using any alternative encryptor or generator, it is all the same, so why would the creation of the encryption when we generate the password have these different patterns?

@bytor99999
Copy link

Any more updates on this? It is getting to be a real problem. More and more encryptors are using other variations and we are getting tons of calls from our users saying they can't login to our application and we spend 30 minutes trying to get a password to work for them, when we should be working on programming our application more. As I mentioned in my comment in November, Spring Security is even creating encrypted Strings with the different variations.

@nidi3
Copy link

nidi3 commented Jan 5, 2018

As per https://security.stackexchange.com/questions/20541/insecure-versions-of-crypt-hashes I understand that 2a and 2y are identical and 2y exists only to mark the fix of an implementation problem in crypt_blowfish.
So at least 2y could be accepted by Spring Security to allow interoperability with PHP.

@bytor99999
Copy link

bytor99999 commented Jan 5, 2018

I think the big problem we are having right now @rwinch is that it appears that the BCryptEncoder is returning encoding that do not match the Regex anymore.

Such that if you call encode with say "Hello" and then call matches with "Hello" and the resulting string from the encoding call, then matches can sometimes return false. In one test yesterday 4 out of 5 encode calls returned String encodings that started with "$2b" and not "$2A" which would match the regex that is used in checking the second parameter in matches call.

public interface PasswordEncoder {
    String encode(CharSequence rawPassword);
    boolean matches(CharSequence rawPassword, String encodedPassword);

This is a huge production problem for us now. We get 10-20 calls a day from lawyers that can't login to do their work in our application, and that takes away our support team and dev team to work on those instead of the tons of other work on our plates.

I am going to write up a quick Unit Test that generates hundreds of encodings, then pass them to matches and get a better sample set for you.

UPDATE: I created an Integration test, so that I could use our application code that creates a random password, that then calls encode. I take the returned String and passed the same random password and encoded String to matches. I loop 1000 times to do the test with 1000 random passwords. Then I also created a unit test that used Spring's BCryptEncoder directly, but the same random password generator and same calls to encode and match methods, also 1000 times. After running both tests 3 times, so total 6000 random generated passwords and match checks. NOT a single one failed. Meaning encode and matches worked perfectly. So I have no idea why in our application which is using the same code is failing so many times.

Here is the Unit test minus package statement (Sorry, I am using Groovy)

import org.apache.commons.lang.RandomStringUtils
import org.junit.Test
import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder
import org.springframework.security.crypto.password.PasswordEncoder

import java.util.regex.Pattern

class BCryptEncoderTest {

    private Pattern BCRYPT_PATTERN = Pattern.compile("\\A\\\$2a?\\\$\\d\\d\\\$[./0-9A-Za-z]{53}");


    PasswordEncoder passwordEncoder = new BCryptPasswordEncoder()


    @Test
    void testEncoderApi(){


        for (int i = 0; i < 1000; i++) {
            String randomPassword = RandomStringUtils.randomAlphanumeric(8)
            println "Generated Password: $randomPassword"
            String encodedPassword = passwordEncoder.encode(randomPassword)
            if (!BCRYPT_PATTERN.matcher(encodedPassword).matches()) {
                println "Encoded password does not look like BCrypt: $encodedPassword";
            } else {
                println "Encoded Password: $encodedPassword"
            }

            boolean matches = passwordEncoder.matches(randomPassword, encodedPassword)
            if (!matches) {
                println "*********************************"
                println "*********************************"
                println "TRUE FAILURE MATCHING RESET LOGIC"
                println "*********************************"
                println "*********************************"
            }
        }

    }
}

@rwinch
Copy link
Member

rwinch commented Jan 26, 2018

@JSteven Any chance you could advise on this? My concerns with just adding support for bcrypt revision $2b$ are outlined in #3320 (comment) Any advice on how we should proceed? Thanks!

@jkeruzec
Copy link

jkeruzec commented Apr 26, 2018

It would be very interesting to have the other implementation of Bcrypt inside Spring !
+1

Please if you could add $2b$ implementation also.

Thanks

@rwinch
Copy link
Member

rwinch commented Apr 26, 2018

@JSteven Bump

@m1spl4c3ds0ul
Copy link

I know, I know. I'm a bad person. I'll get on this.

@rubensa
Copy link
Contributor

rubensa commented May 17, 2018

I just found this: https://github.com/codebazz/jbcrypt
Do you think this could be a good starting point?

@rwinch
Copy link
Member

rwinch commented May 17, 2018

@rubensa Thanks for the pointer. I'm hesitant to pick just any code and place it in Spring Security. The Spring Security team doesn't believe we're in a position to determine if the code is secure, so we are waiting for someone we trust to advise us on how best to proceed.

If you are confident in an approach, working around the problem is as simple as implementing an interface with two methods, so it should be pretty straight forward to work around in the meantime.

Thank you for your patience as we take the time necessary to ensure we maintain the quality you and the rest of our community expect.

@rubensa
Copy link
Contributor

rubensa commented May 18, 2018

@rwinch you are right

I just checked the implementation and is the same algorithm only changing the prefix 'a' with 'y'.

@rubensa
Copy link
Contributor

rubensa commented May 18, 2018

For anyone interested this is my implementation of Spring PasswordEncoder that uses Bouncy Castle OpenBSDBCrypt (tested with bcprov-jdk15on-1.59.jar).

https://gist.github.com/rubensa/f998c068d5a0bb0fcfa4f60401482fdb

Update: Updated to allow specify encoding version to generated passwords

@m1spl4c3ds0ul
Copy link

All,

I outlined the need to store/parse meta-data associated with stored credential material in SEC-1932. This issue will impact both a single family of approaches (brcypt) in an implementation and configuration-specific way, as well as should differentiate in those circumstances where credentials w/in a single repository are stored using heterogeneous means. Why would credentials be stored using different algo's or rounds or salt lengths? A variety of valid reasons: an algorithm is adjusted, its implementation found flawed, or the parameters of an adaptive function are updated to meet rising threat capabilities. Regardless of the reason, this will result in one of two scenarios: a) rolling upgrades of the actual stored and protected credential data (as users log in, the stored format is updated after successful verification) or b) wrapping. Either way, supporting credentials stored with different means of protection is warranted.

So the library code should be cognizant of this formatting, and respond accordingly. Accepting this has nuanced implications to the code base, test suite, and for handling in-place production upgrades. I've worked with organizations to specify out this behavior, but haven't had time to back out something robust enough to retrofit in SpringSecurity+tests+production utility support. I will commit to doing that, but the time frame won't be immediate.

In the meantime, the difference in the brcypt prefix designations represents implementation updates as described by SecurityStackExchange. To be through, SpringSecurity may want to emit a logged warning for admins when a "x" or "a" is encountered, and make the distinction/generate 'y'-spec output when generating hashes by its own auspices.

There's a fair amount of (ranty) material out there on how impactful the problem actually is, from a security perspective, but I haven't parsed that sufficiently to summarize here yet.

@MaciejWysocki
Copy link

My hack for the issue when using jdbc based authentication:

@Override
protected void configure(AuthenticationManagerBuilder auth) throws Exception {
    auth
        .jdbcAuthentication()
        .dataSource(dataSource)
        .usersByUsernameQuery("select email, '$2a'||substring(password from 4) as password, 1 AS enabled from users where email=?")
        .passwordEncoder(new BCryptPasswordEncoder());
}

@rwinch rwinch self-assigned this Oct 22, 2018
@rwinch rwinch added this to the 5.2.0.M1 milestone Oct 22, 2018
@rwinch rwinch closed this as completed in 388a7b6 Oct 22, 2018
rwinch added a commit that referenced this issue Oct 22, 2018
@rwinch
Copy link
Member

rwinch commented Oct 22, 2018

Thanks to @lin199231 who submitted a PR for this! Support for BCrypt revisions is now merged into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement type: jira An issue that was migrated from JIRA
Projects
None yet
Development

No branches or pull requests