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

Bcrypt Key Stretcher used incorrectly #16

patrickfav opened this issue Jul 13, 2018 · 1 comment

Bcrypt Key Stretcher used incorrectly #16

patrickfav opened this issue Jul 13, 2018 · 1 comment


Copy link

@patrickfav patrickfav commented Jul 13, 2018

return Bytes.from(BCrypt.hashpw(String.valueOf(password) + Bytes.wrap(salt).encodeHex(), generateSalt(salt, logRounds))).array();

Unfortunately the jBcrypt API was used totally incorrectly (to be fair, the API does not have any fail safes to warn the user):

  1. The password is encoded with hex and the salt is appended:
    String.valueOf(password) + Bytes.wrap(salt).encodeHex()
    First of all, I dont't know why I appended the salt. Second, bcrypt only supports 72 bytes of PW by encoding the UTF-8 byte array as hex the PW length was additionally shortened and maybe results in premature truncated passwords. So the actually maximum length is 72/2 = 36 bytes (which is probably long enough for most practically used PW, but still bad)
  2. The salt was generated like this:
    saltBuilder.append(Bytes.wrap(HKDF.fromHmacSha256().expand(salt, "bcrypt".getBytes(), 16)).encodeHex());
    Due to the 'creative' way the jBcrypt API generates a salt, I implemented my own salt method to be able to pass a custom salt, but did it incorrectly. The salt SHOULD be 16 bytes encoded as Bcrypt Radix64 (= 22 bytes), but the 16 bytes where encoded to 32 characters hex. The jBcrypt impl then cuts 10 of those characters cutting 5 bytes of entropy making it a 11 byte hash

All this was discovered while trying to update the bcrypt impl with mine

Going forward:

  • I will replace the current Bcrypt stretcher with the new, correctly implemented one, that means that old encrypted preferences cannot be read anymore
  • The old bcrypt impl will still be in the code base, but deprecated, the data is still accessible
  • I will change the "change password" feature to also accept a new Keystrecht impl, so this issue can be migrated
@patrickfav patrickfav added this to the v0.6.0 milestone Jul 13, 2018
patrickfav added a commit that referenced this issue Jul 14, 2018
For this the new bcrypt lib (at.favre.bcrypt) was used. There is an
deprecated fallback impl for migration.

refs #16
Copy link
Owner Author

@patrickfav patrickfav commented Jul 18, 2018

Fixed by #17

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

No branches or pull requests

1 participant