patrickfav / armadillo

A shared preference implementation for confidential data in Android. Per default uses AES-GCM, BCrypt and HKDF as cryptographic primitives. Uses the concept of device fingerprinting combined with optional user provided passwords and strong password hashes.
https://favr.dev/opensource/armadillo
Apache License 2.0
280 stars 52 forks source link

Bcrypt Key Stretcher used incorrectly #16

Closed patrickfav closed 6 years ago

patrickfav commented 6 years ago

https://github.com/patrickfav/armadillo/blob/1761d0f21f0abdcc678b3cdc66b160fe1afdb080/armadillo/src/main/java/at/favre/lib/armadillo/BcryptKeyStretcher.java#L63

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 https://github.com/patrickfav/bcrypt.

Going forward:

patrickfav commented 6 years ago

Fixed by #17