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

Add support for $2y blowfish fix #175

Closed
MitchellMcKenna opened this issue Jul 15, 2013 · 12 comments

Comments

@MitchellMcKenna
Copy link

commented Jul 15, 2013

Password hashes generated using the new "2y" version (hashes that start with $2y$...") of the algorithm are not working with this library. Which means password hashes generated from PHP or Python using the 2y version don't validate as true using this library.

The "2y" format specific to the crypt_blowfish BCrypt implementation is identical to "2a", except I believe it solves a problem with unicode input strings.

Python support for it: http://pythonhosted.org/passlib/lib/passlib.hash.bcrypt.html
PHP support for it: http://www.php.net/manual/en/function.crypt.php

@defunctzombie

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2013

Is there a c implementation that fixes this?

@rizidoro

This comment has been minimized.

Copy link

commented Oct 22, 2013

+1 for this... My passwords was created with password_hash from PHP5, but when I try to compare against, the lib is returning false...

@defunctzombie

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2013

Please read this:
http://security.stackexchange.com/questions/20541/insecure-versions-of-crypt-hashes

It seems that these $2y$ hashes are an issue with crypt_blowfish and not the implementation we used. Your hashes are identical to our hashes except they use $2y$ in the version. You can work about our library not supporting $2y$ by changing the 'y' to an 'a' yourself.

@ncb000gt What do you think of allowing the 'y' to be treated like 'a' versions? This does beg the question of what true version of bcrypt we support since the other implementations could have issues. In the case of 'y' vs 'a' it seems that it is just a replacement and we are ok.

@rizidoro

This comment has been minimized.

Copy link

commented Oct 22, 2013

@defunctzombie that works.... change the $2y$ to $2a$ and now it's returning TRUE... thanks =D

@ncb000gt

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2013

@defunctzombie I have no problem with that. But we may want to check for the x version and throw a warning to the developer that says something along the lines of "these were generated with a faulty version, upgrade the data" or something along those lines in case someone is coming from an older/different system which has been using 'x'.

@defunctzombie

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2013

The 'x' is only faulty if created with that other version of bcrypt. Our module creates non faulty 'x' versions (as far as I know but who can really say ;)

I am not sure there is a good detection path for the faulty x version and if there was, I am not sure implementing it would not expose some other issue. I think the best thing is to just allow 'y' versions as if they are 'x'. I will let this marinate in my mind for a day and then do it if I still think it right.

@ncb000gt

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2013

Right, but the 'x' version is only representative of a faulty version (at least, that's how I understand it). So, I don't want to implement a "fix" for it, just notify people who have faulty hashes that they do and that they should get those users to re-submit their information or "update" their passwords...

@defunctzombie

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2013

Oh, I see, I was mistyping 'a' and 'x', oops! Yea, if a 'x' is passed we could do something, but my feeling is to actually ignore 'x' entirely. What I meant to say was to treat 'y' like 'a' :)

@ncb000gt

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2013

Yes, and in that case I agree. ;D

@defunctzombie

This comment has been minimized.

Copy link
Collaborator

commented Dec 7, 2013

Will not fix. This was an issue with the PHP version and they decided to muck with things.

@reggi

This comment has been minimized.

Copy link

commented Mar 16, 2016

I'm coming new to this really old issue, thanks for having this issue documented (#349 & #213). I'm dealing with an old database with $2y$ hashes. I've dug into this a bit, also stumbled on the stack overflow post as well

I'm able to generate them using this website.

helloworld:$2y$10$tRM7x9gGKhcAmpeqKEdhj.qRWCr4qoV1FU9se0Crx2hkMVNL2ktEW

Seems the module has no way of validating $2y$ hashes.

var Promise = require('bluebird')
var bcrypt = require('bcrypt')

var string = 'helloworld'

Promise.promisifyAll(bcrypt)

// bcrypt.genSalt(10, function(err, salt) {
//   bcrypt.hash(string, salt, function(err, hash) {
//     console.log(hash)
//   })
// })

var hashesGeneratedUsingBcryptModule = [
  '$2a$10$6ppmIdlNEPwxWJskPaQ7l.d2fblh.GO6JomzrcpiD/hxGPOXA3Bsq',
  '$2a$10$YmpoYCDHzdAPMbd9B8l48.hkSnylnAPbOym367FKIEPa0ixY.o4b.',
  '$2a$10$Xfy3OPurrZEmbmmO0x1wGuFMdRTlmOgEMS0geg4wTj1vKcvXXjk06',
  '$2a$10$mYgwmdPZjiEncp7Yh5UB1uyPkoyavxrYcOIzzY4mzSniGpI9RbhL.',
  '$2a$10$dkBVTe2A2DAn24PUq1GZYe7AqL8WQqwOi8ZWBJAauOg60sk44DkOC'
]

var hashesGeneratedUsingAspirineDotOrg = [
  '$2y$10$MKgpAXLJkwx5tpijWX99Qek2gf/irwvp5iSfxuFoDswIjMIbj2.Ma',
  '$2y$10$tRM7x9gGKhcAmpeqKEdhj.qRWCr4qoV1FU9se0Crx2hkMVNL2ktEW'
]

var hashesGeneratedUsingAspirineDotOrgSwippedYForA = [
  '$2a$10$MKgpAXLJkwx5tpijWX99Qek2gf/irwvp5iSfxuFoDswIjMIbj2.Ma',
  '$2a$10$tRM7x9gGKhcAmpeqKEdhj.qRWCr4qoV1FU9se0Crx2hkMVNL2ktEW'
]

hashesGeneratedUsingBcryptModule = hashesGeneratedUsingBcryptModule.map(hash => bcrypt.compareAsync(string, hash))
hashesGeneratedUsingAspirineDotOrg = hashesGeneratedUsingAspirineDotOrg.map(hash => bcrypt.compareAsync(string, hash))
hashesGeneratedUsingAspirineDotOrgSwippedYForA = hashesGeneratedUsingAspirineDotOrgSwippedYForA.map(hash => bcrypt.compareAsync(string, hash))


Promise.all(hashesGeneratedUsingBcryptModule)
.tap(() => console.log('hashesGeneratedUsingBcryptModule'))
.then(console.log)

Promise.all(hashesGeneratedUsingAspirineDotOrg)
.tap(() => console.log('hashesGeneratedUsingAspirineDotOrg'))
.then(console.log)

Promise.all(hashesGeneratedUsingAspirineDotOrgSwippedYForA)
.tap(() => console.log('hashesGeneratedUsingAspirineDotOrgSwippedYForA'))
.then(console.log)

Here are the results:

// hashesGeneratedUsingAspirineDotOrg
// [ false, false ]
// hashesGeneratedUsingBcryptModule
// [ true, true, true, true, true ]
// hashesGeneratedUsingAspirineDotOrgSwippedYForA
// [ false, false ]

I'm stumped on how I can compare $2y$ hashes in node. Would love ideas :)

@reggi

This comment has been minimized.

Copy link

commented Mar 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.