chore: [ANDROSDK-2059] replace MD5 password hashing with PBKDF2 - #2703
chore: [ANDROSDK-2059] replace MD5 password hashing with PBKDF2#2703taridepaco wants to merge 3 commits into
Conversation
Resolve the overlap between the PBKDF2 password hash migration and the login refactor landed on develop: - loginOnline writes a fresh PBKDF2 hash when the credentials carry a secret and otherwise preserves the stored one, keeping develop's fix that stops a token re-login from wiping the PIN hash. - verifyPinAgainstStoredHash is gone: develop moved the PIN check for token accounts into tryLoginOffline, which already verifies the hash. - tryLoginOffline verifies instead of comparing and upgrades an outdated hash in place, reporting failures through develop's wrongLocalCredentialsError. - Tests adopt develop's givenExistingDatabase helper and stub the current hash format; the legacy MD5 path keeps its own dedicated tests.
|
| * The result is salted and therefore different on every call, so it must never be compared: | ||
| * use [matches] to verify a secret against an already stored hash. | ||
| */ | ||
| fun newPasswordHash(): String? { |
There was a problem hiding this comment.
I would avoid adding the new word in a method name because after some versions it won't be new anymore.
Why don't keep the same name as before? This method is replacing it.
| fun matches(storedHash: String?): HashVerification { | ||
| val secret = passwordOrPin | ||
| return when { | ||
| secret == null && storedHash == null -> HashVerification.Match(needsUpgrade = false) |
There was a problem hiding this comment.
In case the existing hash was null, I am not sure if we should let the user in by providing a null value. If the hash is null, it means that the user didn't set up a PIN code.
| deriveKey(parsed.algorithm, secret, parsed.salt, parsed.iterations) | ||
| } catch (_: NoSuchAlgorithmException) { | ||
| // The hash was written on a device running a newer Android version. It cannot be | ||
| // verified here, so the account has to be authenticated online again. |
There was a problem hiding this comment.
We would have to check if this restriction is important when exporting/importing a DB from a newer Android version. In version 3.6 we should move to use a different password for DB encryption and then force the user the do an online login in case they want to synchronize. I think it won't be so important if we implement this workflow, but it might be important if we keep the current one.
2 participants
UserHelper.md5()is no longer used to store password hashes. Credentials are now hashed with PBKDF2-HMAC-SHA256 (210,000 iterations, 16-byte random salt) through a new internalPasswordHasher, andCredentials.getHash()is replaced bynewPasswordHash()for the write path andmatches()for the read path, so a salted hash is never compared for equality.Existing accounts migrate transparently rather than being logged out. Hashes are stored in a self-describing
$pbkdf2-sha256$i=<iterations>$<salt>$<hash>format: legacy MD5 values are still verified (verify-only, never produced again) and are rewritten in place on the next successful login, while the plaintext secret is still at hand. The same mechanism allows raising the iteration count in a later release without invalidating anything already stored.UserHelper.md5is kept as@Deprecatedbecause it is published API and the legacy verification path still needs it; removal is left for the next major. Note thatPBKDF2WithHmacSHA256is only available from API 26, so devices below that fall back toPBKDF2WithHmacSHA1, which is recorded in the stored value and upgraded when the account is next verified on a capable device.Related task: ANDROSDK-2059