diff --git a/core/src/main/java/org/hisp/dhis/android/core/arch/helpers/UserHelper.kt b/core/src/main/java/org/hisp/dhis/android/core/arch/helpers/UserHelper.kt index 95c1be82fd0..c2fcf4e1a07 100644 --- a/core/src/main/java/org/hisp/dhis/android/core/arch/helpers/UserHelper.kt +++ b/core/src/main/java/org/hisp/dhis/android/core/arch/helpers/UserHelper.kt @@ -59,10 +59,15 @@ object UserHelper { /** * Encode the given username and password to a MD5 [String]. * + * MD5 is not suitable for hashing secrets. The SDK no longer uses it to store password hashes, + * it only reads the values written by previous versions. It will be removed in a future major + * release. + * * @param username The username of the user account. * @param password The password of the user account. * @return An encoded MD5 [String]. */ + @Deprecated("MD5 is cryptographically weak and must not be used to hash secrets.") fun md5(username: String, password: String): String { return try { val credentials = usernameAndPassword(username, password) diff --git a/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/Credentials.kt b/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/Credentials.kt index d38e0824405..328cc500296 100644 --- a/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/Credentials.kt +++ b/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/Credentials.kt @@ -28,7 +28,6 @@ package org.hisp.dhis.android.core.arch.storage.internal import net.openid.appauth.AuthState -import org.hisp.dhis.android.core.arch.helpers.UserHelper import org.hisp.dhis.android.core.common.AuthorizationType import org.hisp.dhis.android.core.user.oauth2.OAuth2State @@ -45,8 +44,27 @@ internal data class Credentials( val passwordOrPin: String? get() = password ?: pin - fun getHash(): String? { - return passwordOrPin?.let { UserHelper.md5(username, it) } + /** + * Derives a new hash for the stored secret, to be persisted in the AuthenticatedUser table. + * 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? { + return passwordOrPin?.let { PasswordHasher.hash(it) } + } + + /** + * Verifies the stored secret against [storedHash], which may be in either the current or the + * legacy MD5 format. Accounts without a secret (token based accounts with no PIN) are expected + * to have no stored hash either. + */ + fun matches(storedHash: String?): HashVerification { + val secret = passwordOrPin + return when { + secret == null && storedHash == null -> HashVerification.Match(needsUpgrade = false) + secret == null || storedHash == null -> HashVerification.Mismatch + else -> PasswordHasher.verify(username, secret, storedHash) + } } override fun equals(other: Any?) = diff --git a/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/HashVerification.kt b/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/HashVerification.kt new file mode 100644 index 00000000000..72b0e9d1eb6 --- /dev/null +++ b/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/HashVerification.kt @@ -0,0 +1,37 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * Neither the name of the HISP project nor the names of its contributors may + * be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.android.core.arch.storage.internal + +/** + * Outcome of verifying a secret against a stored password hash. + */ +internal sealed interface HashVerification { + + data class Match(val needsUpgrade: Boolean) : HashVerification + data object Mismatch : HashVerification +} diff --git a/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasher.kt b/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasher.kt new file mode 100644 index 00000000000..9ddf2d64f70 --- /dev/null +++ b/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasher.kt @@ -0,0 +1,202 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * Neither the name of the HISP project nor the names of its contributors may + * be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.android.core.arch.storage.internal + +import okio.ByteString.Companion.decodeBase64 +import okio.ByteString.Companion.toByteString +import org.hisp.dhis.android.core.arch.helpers.UserHelper +import java.security.MessageDigest +import java.security.NoSuchAlgorithmException +import java.security.SecureRandom +import javax.crypto.SecretKeyFactory +import javax.crypto.spec.PBEKeySpec + +/** + * Derives and verifies the password hash stored in the AuthenticatedUser table. + * + * Hashes are stored in a self-describing, PHC-like format so that the algorithm and its parameters + * travel with the value: `$pbkdf2-sha256$i=210000$$`. + * + * Values that do not start with the separator are legacy MD5 digests of `"username:secret"` written + * by previous SDK versions. They can still be verified, but they are never produced again: a + * successful legacy verification reports [HashVerification.Match.needsUpgrade], so the caller + * rewrites the row with a current hash. The same mechanism allows raising the iteration count later + * without invalidating anything already stored. + */ +internal object PasswordHasher { + + private const val PHC_SEPARATOR = '$' + private const val ITERATIONS_PREFIX = "i=" + private const val PHC_SEGMENTS = 5 + private const val ALGORITHM_SEGMENT = 1 + private const val ITERATIONS_SEGMENT = 2 + private const val SALT_SEGMENT = 3 + private const val HASH_SEGMENT = 4 + private const val SALT_LENGTH_BYTES = 16 + private const val KEY_LENGTH_BITS = 256 + + /** + * Cost factor applied to newly derived hashes. + */ + private const val ITERATIONS = 210_000 + + /** + * Supported PBKDF2 variants, declared from strongest to weakest: the order defines which stored + * hashes are considered outdated. PBKDF2WithHmacSHA256 is only available from API 26, so devices + * below that fall back to SHA-1, available since API 10. + */ + private enum class Pbkdf2Algorithm( + val id: String, + val jcaName: String, + val iterations: Int, + ) { + SHA256("pbkdf2-sha256", "PBKDF2WithHmacSHA256", ITERATIONS), + SHA1("pbkdf2-sha1", "PBKDF2WithHmacSHA1", ITERATIONS), + ; + + fun isWeakerThan(other: Pbkdf2Algorithm) = ordinal > other.ordinal + + companion object { + fun byId(id: String) = entries.firstOrNull { it.id == id } + } + } + + private val currentAlgorithm: Pbkdf2Algorithm by lazy { + Pbkdf2Algorithm.entries.firstOrNull { it.isAvailable() } + ?: throw AssertionError("No PBKDF2 implementation available") + } + + /** + * Derives a new hash for the given secret using the current algorithm and parameters. Every call + * generates a fresh random salt, so the result is never the same twice: it is only valid for the + * write path, never for comparing. + */ + fun hash(secret: String): String { + val algorithm = currentAlgorithm + val salt = ByteArray(SALT_LENGTH_BYTES).also { SecureRandom().nextBytes(it) } + val derived = deriveKey(algorithm, secret, salt, algorithm.iterations) + return listOf( + "", + algorithm.id, + ITERATIONS_PREFIX + algorithm.iterations, + salt.base64(), + derived.base64(), + ).joinToString(PHC_SEPARATOR.toString()) + } + + /** + * Verifies [secret] against [storedHash]. [username] is only used by the legacy MD5 path, which + * hashes `"username:secret"` instead of the secret alone. + */ + fun verify(username: String, secret: String, storedHash: String): HashVerification { + return if (storedHash.startsWith(PHC_SEPARATOR)) { + verifyPbkdf2(secret, storedHash) + } else { + verifyLegacyMd5(username, secret, storedHash) + } + } + + @Suppress("ReturnCount") + private fun verifyPbkdf2(secret: String, storedHash: String): HashVerification { + val parsed = parse(storedHash) ?: return HashVerification.Mismatch + val candidate = try { + 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. + return HashVerification.Mismatch + } + + return if (MessageDigest.isEqual(candidate, parsed.hash)) { + HashVerification.Match( + needsUpgrade = parsed.algorithm.isWeakerThan(currentAlgorithm) || + parsed.iterations < parsed.algorithm.iterations, + ) + } else { + HashVerification.Mismatch + } + } + + @Suppress("DEPRECATION") + private fun verifyLegacyMd5(username: String, secret: String, storedHash: String): HashVerification { + val candidate = UserHelper.md5(username, secret) + return if (MessageDigest.isEqual(candidate.encodeToByteArray(), storedHash.encodeToByteArray())) { + HashVerification.Match(needsUpgrade = true) + } else { + HashVerification.Mismatch + } + } + + @Suppress("ReturnCount") + private fun parse(storedHash: String): ParsedHash? { + val segments = storedHash.split(PHC_SEPARATOR) + if (segments.size != PHC_SEGMENTS) return null + + val algorithm = Pbkdf2Algorithm.byId(segments[ALGORITHM_SEGMENT]) ?: return null + val iterationsSegment = segments[ITERATIONS_SEGMENT] + if (!iterationsSegment.startsWith(ITERATIONS_PREFIX)) return null + val iterations = iterationsSegment.removePrefix(ITERATIONS_PREFIX) + .toIntOrNull()?.takeIf { it > 0 } ?: return null + val salt = segments[SALT_SEGMENT].decodeBase64()?.toByteArray() ?: return null + val hash = segments[HASH_SEGMENT].decodeBase64()?.toByteArray() ?: return null + + return ParsedHash(algorithm, iterations, salt, hash) + } + + private fun deriveKey( + algorithm: Pbkdf2Algorithm, + secret: String, + salt: ByteArray, + iterations: Int, + ): ByteArray { + val spec = PBEKeySpec(secret.toCharArray(), salt, iterations, KEY_LENGTH_BITS) + return try { + SecretKeyFactory.getInstance(algorithm.jcaName).generateSecret(spec).encoded + } finally { + spec.clearPassword() + } + } + + private fun Pbkdf2Algorithm.isAvailable(): Boolean { + return try { + SecretKeyFactory.getInstance(jcaName) + true + } catch (_: NoSuchAlgorithmException) { + false + } + } + + private fun ByteArray.base64(): String = toByteString().base64() + + private class ParsedHash( + val algorithm: Pbkdf2Algorithm, + val iterations: Int, + val salt: ByteArray, + val hash: ByteArray, + ) +} diff --git a/core/src/main/java/org/hisp/dhis/android/core/user/internal/LogInCall.kt b/core/src/main/java/org/hisp/dhis/android/core/user/internal/LogInCall.kt index a433b71e0e8..1040888a84c 100644 --- a/core/src/main/java/org/hisp/dhis/android/core/user/internal/LogInCall.kt +++ b/core/src/main/java/org/hisp/dhis/android/core/user/internal/LogInCall.kt @@ -32,6 +32,7 @@ import org.hisp.dhis.android.core.arch.api.executors.internal.CoroutineAPICallEx import org.hisp.dhis.android.core.arch.api.internal.ServerURLWrapper import org.hisp.dhis.android.core.arch.storage.internal.Credentials import org.hisp.dhis.android.core.arch.storage.internal.CredentialsSecureStore +import org.hisp.dhis.android.core.arch.storage.internal.HashVerification import org.hisp.dhis.android.core.arch.storage.internal.UserIdInMemoryStore import org.hisp.dhis.android.core.common.AuthorizationType import org.hisp.dhis.android.core.configuration.internal.DatabaseConfigurationHelper @@ -190,7 +191,7 @@ internal class LogInCall( val existingUser = authenticatedUserStore.selectFirst() val authenticatedUser = AuthenticatedUser.builder() .user(user.uid()) - .hash(credentials.getHash() ?: existingUser?.hash()) + .hash(credentials.newPasswordHash() ?: existingUser?.hash()) .build() authenticatedUserStore.updateOrInsertWhere(authenticatedUser) @@ -215,14 +216,30 @@ internal class LogInCall( } val existingUser = authenticatedUserStore.selectFirst() ?: throw exceptions.noUserOfflineError() - if (credentials.getHash() != existingUser.hash()) { - throw wrongLocalCredentialsError(credentials) + when (val verification = credentials.matches(existingUser.hash())) { + is HashVerification.Mismatch -> throw wrongLocalCredentialsError(credentials) + + is HashVerification.Match -> + if (verification.needsUpgrade) { + upgradeStoredHash(existingUser, credentials) + } } credentialsSecureStore.set(credentials) userIdStore.set(existingUser.user()!!) return userStore.selectByUid(existingUser.user()!!)!! } + /** + * Rewrites a hash that was verified successfully but is stored in an outdated format, typically + * the legacy MD5 one. It happens transparently while the plaintext secret is still at hand, so + * the user is never asked to authenticate again. + */ + private suspend fun upgradeStoredHash(existingUser: AuthenticatedUser, credentials: Credentials) { + authenticatedUserStore.updateOrInsertWhere( + existingUser.toBuilder().hash(credentials.newPasswordHash()).build(), + ) + } + @Suppress("TooGenericExceptionCaught") private suspend fun importDB(serverUrl: String, credentials: Credentials): User { try { diff --git a/core/src/main/java/org/hisp/dhis/android/core/user/oauth2/internal/OAuth2HandlerImpl.kt b/core/src/main/java/org/hisp/dhis/android/core/user/oauth2/internal/OAuth2HandlerImpl.kt index 50adf8c51df..d7a1dcb7fb5 100644 --- a/core/src/main/java/org/hisp/dhis/android/core/user/oauth2/internal/OAuth2HandlerImpl.kt +++ b/core/src/main/java/org/hisp/dhis/android/core/user/oauth2/internal/OAuth2HandlerImpl.kt @@ -268,7 +268,7 @@ internal class OAuth2HandlerImpl( Result.Failure(logInExceptions.noAuthenticatedUserPersistedError()) } else { authenticatedUserStore.updateOrInsertWhere( - existing.toBuilder().hash(updated.getHash()).build(), + existing.toBuilder().hash(updated.newPasswordHash()).build(), ) Result.Success(Unit) } diff --git a/core/src/main/java/org/hisp/dhis/android/core/user/openid/OpenIDConnectHandlerImpl.kt b/core/src/main/java/org/hisp/dhis/android/core/user/openid/OpenIDConnectHandlerImpl.kt index 80554eebeff..788fa342ab6 100644 --- a/core/src/main/java/org/hisp/dhis/android/core/user/openid/OpenIDConnectHandlerImpl.kt +++ b/core/src/main/java/org/hisp/dhis/android/core/user/openid/OpenIDConnectHandlerImpl.kt @@ -123,7 +123,7 @@ internal class OpenIDConnectHandlerImpl( Result.Failure(logInExceptions.noAuthenticatedUserPersistedError()) } else { authenticatedUserStore.updateOrInsertWhere( - existing.toBuilder().hash(updated.getHash()).build(), + existing.toBuilder().hash(updated.newPasswordHash()).build(), ) Result.Success(Unit) } diff --git a/core/src/test/java/org/hisp/dhis/android/core/arch/helpers/UserHelperShould.kt b/core/src/test/java/org/hisp/dhis/android/core/arch/helpers/UserHelperShould.kt index b6d74b25e62..c40aba2041d 100644 --- a/core/src/test/java/org/hisp/dhis/android/core/arch/helpers/UserHelperShould.kt +++ b/core/src/test/java/org/hisp/dhis/android/core/arch/helpers/UserHelperShould.kt @@ -33,6 +33,7 @@ import org.junit.runner.RunWith import org.junit.runners.JUnit4 @RunWith(JUnit4::class) +@Suppress("DEPRECATION") class UserHelperShould { @Test fun md5_evaluate_same_string() { diff --git a/core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/CredentialsShould.kt b/core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/CredentialsShould.kt index 52dbf9ae2f7..e40fb18059e 100644 --- a/core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/CredentialsShould.kt +++ b/core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/CredentialsShould.kt @@ -93,19 +93,67 @@ class CredentialsShould { @Test fun build_hash_from_pin_when_there_is_no_password() { val credentials = Credentials("user", "https://dhis2.org", null, PIN, null, oauth2State) - assertThat(credentials.getHash()).isEqualTo(UserHelper.md5("user", PIN)) + assertThat(credentials.matches(credentials.newPasswordHash())) + .isEqualTo(HashVerification.Match(needsUpgrade = false)) } @Test fun build_hash_from_password_when_password_is_set() { val credentials = Credentials("user", "https://dhis2.org", "password", null, null, null) - assertThat(credentials.getHash()).isEqualTo(UserHelper.md5("user", "password")) + assertThat(credentials.matches(credentials.newPasswordHash())) + .isEqualTo(HashVerification.Match(needsUpgrade = false)) + } + + @Test + fun build_a_different_hash_on_every_call() { + val credentials = Credentials("user", "https://dhis2.org", "password", null, null, null) + assertThat(credentials.newPasswordHash()).isNotEqualTo(credentials.newPasswordHash()) } @Test fun return_null_hash_when_neither_password_nor_pin() { val credentials = Credentials("user", "https://dhis2.org", null, null, null, oauth2State) - assertThat(credentials.getHash()).isNull() + assertThat(credentials.newPasswordHash()).isNull() + } + + @Test + fun match_a_legacy_md5_hash_and_ask_for_an_upgrade() { + val credentials = Credentials("user", "https://dhis2.org", "password", null, null, null) + + @Suppress("DEPRECATION") + val legacyHash = UserHelper.md5("user", "password") + + assertThat(credentials.matches(legacyHash)).isEqualTo(HashVerification.Match(needsUpgrade = true)) + } + + @Test + fun not_match_a_hash_that_belongs_to_a_different_secret() { + val credentials = Credentials("user", "https://dhis2.org", "password", null, null, null) + val other = Credentials("user", "https://dhis2.org", "another-password", null, null, null) + + assertThat(credentials.matches(other.newPasswordHash())).isEqualTo(HashVerification.Mismatch) + } + + @Test + fun match_when_neither_the_credentials_nor_the_stored_hash_have_a_secret() { + val credentials = Credentials("user", "https://dhis2.org", null, null, null, oauth2State) + + assertThat(credentials.matches(null)).isEqualTo(HashVerification.Match(needsUpgrade = false)) + } + + @Test + fun not_match_when_a_secret_is_given_but_nothing_is_stored() { + val credentials = Credentials("user", "https://dhis2.org", null, PIN, null, oauth2State) + + assertThat(credentials.matches(null)).isEqualTo(HashVerification.Mismatch) + } + + @Test + fun not_match_when_a_hash_is_stored_but_no_secret_is_given() { + val withPin = Credentials("user", "https://dhis2.org", null, PIN, null, oauth2State) + val withoutPin = Credentials("user", "https://dhis2.org", null, null, null, oauth2State) + + assertThat(withoutPin.matches(withPin.newPasswordHash())).isEqualTo(HashVerification.Mismatch) } @Test diff --git a/core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasherShould.kt b/core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasherShould.kt new file mode 100644 index 00000000000..5700c7857b0 --- /dev/null +++ b/core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasherShould.kt @@ -0,0 +1,151 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * Neither the name of the HISP project nor the names of its contributors may + * be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.android.core.arch.storage.internal + +import com.google.common.truth.Truth.assertThat +import okio.ByteString.Companion.toByteString +import org.hisp.dhis.android.core.arch.helpers.UserHelper +import org.junit.Test +import javax.crypto.SecretKeyFactory +import javax.crypto.spec.PBEKeySpec + +@Suppress("DEPRECATION") +class PasswordHasherShould { + + @Test + fun produce_a_self_describing_hash() { + val hash = PasswordHasher.hash(SECRET) + + val segments = hash.split("$") + assertThat(segments).hasSize(5) + assertThat(segments[0]).isEmpty() + assertThat(segments[1]).isEqualTo("pbkdf2-sha256") + assertThat(segments[2]).isEqualTo("i=210000") + } + + @Test + fun produce_a_different_hash_on_every_call() { + assertThat(PasswordHasher.hash(SECRET)).isNotEqualTo(PasswordHasher.hash(SECRET)) + } + + @Test + fun verify_a_hash_it_produced_without_requiring_an_upgrade() { + val verification = PasswordHasher.verify(USERNAME, SECRET, PasswordHasher.hash(SECRET)) + + assertThat(verification).isEqualTo(HashVerification.Match(needsUpgrade = false)) + } + + @Test + fun reject_a_wrong_secret() { + val verification = PasswordHasher.verify(USERNAME, "wrong", PasswordHasher.hash(SECRET)) + + assertThat(verification).isEqualTo(HashVerification.Mismatch) + } + + @Test + fun ignore_the_username_when_verifying_a_current_hash() { + val verification = PasswordHasher.verify("someone-else", SECRET, PasswordHasher.hash(SECRET)) + + assertThat(verification).isEqualTo(HashVerification.Match(needsUpgrade = false)) + } + + @Test + fun verify_a_legacy_md5_hash_and_ask_for_an_upgrade() { + val legacy = UserHelper.md5(USERNAME, SECRET) + + val verification = PasswordHasher.verify(USERNAME, SECRET, legacy) + + assertThat(verification).isEqualTo(HashVerification.Match(needsUpgrade = true)) + } + + @Test + fun reject_a_legacy_md5_hash_that_belongs_to_a_different_secret() { + val legacy = UserHelper.md5(USERNAME, "another-secret") + + val verification = PasswordHasher.verify(USERNAME, SECRET, legacy) + + assertThat(verification).isEqualTo(HashVerification.Mismatch) + } + + @Test + fun reject_a_legacy_md5_hash_that_belongs_to_a_different_username() { + val legacy = UserHelper.md5("another-user", SECRET) + + val verification = PasswordHasher.verify(USERNAME, SECRET, legacy) + + assertThat(verification).isEqualTo(HashVerification.Mismatch) + } + + @Test + fun ask_for_an_upgrade_when_the_stored_iteration_count_is_outdated() { + val outdated = pbkdf2Hash(SECRET, iterations = 10_000) + + val verification = PasswordHasher.verify(USERNAME, SECRET, outdated) + + assertThat(verification).isEqualTo(HashVerification.Match(needsUpgrade = true)) + } + + @Test + fun reject_a_wrong_secret_against_an_outdated_hash() { + val outdated = pbkdf2Hash(SECRET, iterations = 10_000) + + val verification = PasswordHasher.verify(USERNAME, "wrong", outdated) + + assertThat(verification).isEqualTo(HashVerification.Mismatch) + } + + @Test + fun reject_malformed_stored_values() { + val malformed = listOf( + "\$pbkdf2-sha256\$i=210000\$only-three-segments", + "\$pbkdf2-sha256\$210000\$c2FsdA==\$aGFzaA==", + "\$pbkdf2-sha256\$i=zero\$c2FsdA==\$aGFzaA==", + "\$pbkdf2-sha256\$i=0\$c2FsdA==\$aGFzaA==", + "\$unknown-algorithm\$i=210000\$c2FsdA==\$aGFzaA==", + "\$pbkdf2-sha256\$i=210000\$not base64!\$aGFzaA==", + ) + + malformed.forEach { + assertThat(PasswordHasher.verify(USERNAME, SECRET, it)).isEqualTo(HashVerification.Mismatch) + } + } + + private fun pbkdf2Hash(secret: String, iterations: Int): String { + val salt = ByteArray(SALT_LENGTH) { it.toByte() } + val spec = PBEKeySpec(secret.toCharArray(), salt, iterations, KEY_LENGTH_BITS) + val derived = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256").generateSecret(spec).encoded + return "\$pbkdf2-sha256\$i=$iterations\$${salt.toByteString().base64()}\$${derived.toByteString().base64()}" + } + + companion object { + private const val USERNAME = "username" + private const val SECRET = "s3cr3t" + private const val SALT_LENGTH = 16 + private const val KEY_LENGTH_BITS = 256 + } +} diff --git a/core/src/test/java/org/hisp/dhis/android/core/user/internal/LogInCallUnitShould.kt b/core/src/test/java/org/hisp/dhis/android/core/user/internal/LogInCallUnitShould.kt index acd22bfaa84..9a81c94eb60 100644 --- a/core/src/test/java/org/hisp/dhis/android/core/user/internal/LogInCallUnitShould.kt +++ b/core/src/test/java/org/hisp/dhis/android/core/user/internal/LogInCallUnitShould.kt @@ -35,6 +35,8 @@ import org.hisp.dhis.android.core.arch.api.executors.internal.CoroutineAPICallEx import org.hisp.dhis.android.core.arch.helpers.UserHelper import org.hisp.dhis.android.core.arch.storage.internal.Credentials import org.hisp.dhis.android.core.arch.storage.internal.CredentialsSecureStore +import org.hisp.dhis.android.core.arch.storage.internal.HashVerification +import org.hisp.dhis.android.core.arch.storage.internal.PasswordHasher import org.hisp.dhis.android.core.arch.storage.internal.UserIdInMemoryStore import org.hisp.dhis.android.core.common.AuthorizationType import org.hisp.dhis.android.core.common.BaseCallShould @@ -102,7 +104,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(credentials.username).thenReturn(USERNAME) whenever(credentials.password).thenReturn(PASSWORD) whenever(authenticatedUser.user()).thenReturn(UID) - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PASSWORD)) + whenever(authenticatedUser.hash()).thenReturn(PASSWORD_HASH) whenever(systemInfoFromAPI.contextPath()).thenReturn(BASE_URL) whenever(systemInfoFromDb.contextPath()).thenReturn(BASE_URL) systemInfoCall.stub { @@ -269,15 +271,58 @@ class LogInCallUnitShould : BaseCallShould() { assertD2Error(D2ErrorCode.BAD_CREDENTIALS) { login() } } + @Test + fun succeed_for_login_offline_when_the_stored_hash_is_a_legacy_md5_one() = runTest { + whenAPICall { throw d2Error } + givenExistingDatabase() + whenever(authenticatedUserStore.selectFirst()).thenReturn(legacyAuthenticatedUser()) + + login() + + verifySuccessOffline() + } + + @Test + fun replace_a_legacy_md5_hash_after_a_successful_offline_login() = runTest { + whenAPICall { throw d2Error } + givenExistingDatabase() + whenever(authenticatedUserStore.selectFirst()).thenReturn(legacyAuthenticatedUser()) + + login() + + val captor = argumentCaptor() + verifyBlocking(authenticatedUserStore) { updateOrInsertWhere(captor.capture()) } + assertThat(captor.firstValue.user()).isEqualTo(UID) + assertHashMatchesPassword(captor.firstValue.hash()) + } + + private fun legacyAuthenticatedUser() = + AuthenticatedUser.builder().user(UID).hash(LEGACY_PASSWORD_HASH).build() + + @Test + fun not_rewrite_the_stored_hash_after_an_offline_login_with_a_current_hash() = runTest { + whenAPICall { throw d2Error } + givenExistingDatabase() + whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) + + login() + + verify(authenticatedUserStore, never()).updateOrInsertWhere(any()) + } + private fun verifySuccess() = runTest { - val authenticatedUserModel = AuthenticatedUser.builder() - .user(UID) - .hash(UserHelper.md5(USERNAME, PASSWORD)) - .build() - verify(authenticatedUserStore).updateOrInsertWhere(authenticatedUserModel) + val captor = argumentCaptor() + verify(authenticatedUserStore).updateOrInsertWhere(captor.capture()) + assertThat(captor.firstValue.user()).isEqualTo(UID) + assertHashMatchesPassword(captor.firstValue.hash()) verify(userHandler).handle(eq(apiUser)) } + private fun assertHashMatchesPassword(hash: String?) { + assertThat(PasswordHasher.verify(USERNAME, PASSWORD, hash!!)) + .isEqualTo(HashVerification.Match(needsUpgrade = false)) + } + private fun verifySuccessOffline() { verify(credentialsSecureStore).set(Credentials(USERNAME, SERVER_URL, PASSWORD, null, null)) verify(userIdStore).set("test_uid") @@ -292,7 +337,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(oauth2StateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(null) whenever(databasesConfigurationStore.get()).thenReturn(oauth2Configuration()) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) val user = instantiateCall(USERNAME, PIN, SERVER_URL) @@ -306,7 +351,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(oauth2StateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(null) whenever(databasesConfigurationStore.get()).thenReturn(oauth2Configuration()) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) instantiateCall(USERNAME, PIN, SERVER_URL) @@ -323,7 +368,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(oauth2StateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(null) whenever(databasesConfigurationStore.get()).thenReturn(oauth2Configuration()) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, "correct")) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) assertD2Error(D2ErrorCode.BAD_CREDENTIALS_OFFLINE_CODE) { @@ -368,7 +413,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(databasesConfigurationStore.get()) .thenReturn(configurationWith(AuthorizationType.OPEN_ID_CONNECT)) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) val user = instantiateCall(USERNAME, PIN, SERVER_URL) @@ -384,7 +429,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(openIDConnectStateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(openIdAuthState) whenever(databasesConfigurationStore.get()).thenReturn(configurationWith(null)) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) instantiateCall(USERNAME, PIN, SERVER_URL) @@ -445,7 +490,7 @@ class LogInCallUnitShould : BaseCallShould() { val state = oauth2State(accessToken = ACCESS_TOKEN) whenever(oauth2StateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(state) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) instantiateCall(USERNAME, PIN, SERVER_URL) @@ -461,7 +506,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(oauth2StateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(state) givenExistingDatabase() // Stored hash corresponds to a different PIN. - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, "correct")) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) assertD2Error(D2ErrorCode.BAD_CREDENTIALS_OFFLINE_CODE) { @@ -508,7 +553,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(openIDConnectStateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(openIdAuthState) whenever(databasesConfigurationStore.get()).thenReturn(configurationWith(null)) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) instantiateCall(USERNAME, PIN, SERVER_URL) @@ -522,7 +567,7 @@ class LogInCallUnitShould : BaseCallShould() { fun log_in_offline_without_contacting_server_when_openid_state_exists_for_account() = runTest { whenever(openIDConnectStateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(openIdAuthState) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) val user = instantiateCall(USERNAME, PIN, SERVER_URL) @@ -535,7 +580,7 @@ class LogInCallUnitShould : BaseCallShould() { fun persist_credentials_with_openid_state_and_pin_after_openid_login() = runTest { whenever(openIDConnectStateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(openIdAuthState) givenExistingDatabase() - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) instantiateCall(USERNAME, PIN, SERVER_URL) @@ -558,7 +603,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(openIDConnectStateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(openIdAuthState) givenExistingDatabase() // Stored hash corresponds to a different PIN. - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, "correct")) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) // The PIN is the offline code for both token-based types, so both report the same error. @@ -603,7 +648,7 @@ class LogInCallUnitShould : BaseCallShould() { fun succeed_on_oauth2_online_relogin_when_the_account_has_a_pin() = runTest { whenever(apiUser.username()).thenReturn(USERNAME) // The account configured a PIN after the first login, so the stored hash is not null. - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) val user = reLogInWithOAuth2Token(oauth2State(accessToken = ACCESS_TOKEN)) @@ -616,7 +661,7 @@ class LogInCallUnitShould : BaseCallShould() { @Test fun preserve_the_stored_pin_hash_on_oauth2_online_relogin() = runTest { whenever(apiUser.username()).thenReturn(USERNAME) - val storedHash = UserHelper.md5(USERNAME, PIN) + val storedHash = PIN_HASH whenever(authenticatedUser.hash()).thenReturn(storedHash) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) @@ -636,7 +681,7 @@ class LogInCallUnitShould : BaseCallShould() { .copy(accessToken = null, refreshToken = null) whenever(credentialsSecureStore.get()) .thenReturn(Credentials(USERNAME, SERVER_URL, null, PIN, null, discardedState)) - whenever(authenticatedUser.hash()).thenReturn(UserHelper.md5(USERNAME, PIN)) + whenever(authenticatedUser.hash()).thenReturn(PIN_HASH) whenever(authenticatedUserStore.selectFirst()).thenReturn(authenticatedUser) val freshState = oauth2State(accessToken = "access-token-2") @@ -702,5 +747,12 @@ class LogInCallUnitShould : BaseCallShould() { private const val ACCESS_TOKEN = "access-token-1" private const val ID_TOKEN = "id-token-1" private const val FRESH_ID_TOKEN = "fresh-id-token-1" + + // Deriving a PBKDF2 hash is deliberately expensive, so it is done once for the whole class. + private val PASSWORD_HASH: String by lazy { PasswordHasher.hash(PASSWORD) } + private val PIN_HASH: String by lazy { PasswordHasher.hash(PIN) } + + @Suppress("DEPRECATION") + private val LEGACY_PASSWORD_HASH: String by lazy { UserHelper.md5(USERNAME, PASSWORD) } } } diff --git a/core/src/test/java/org/hisp/dhis/android/core/user/oauth2/internal/OAuth2HandlerImplShould.kt b/core/src/test/java/org/hisp/dhis/android/core/user/oauth2/internal/OAuth2HandlerImplShould.kt index 3609b233aa2..bc967465231 100644 --- a/core/src/test/java/org/hisp/dhis/android/core/user/oauth2/internal/OAuth2HandlerImplShould.kt +++ b/core/src/test/java/org/hisp/dhis/android/core/user/oauth2/internal/OAuth2HandlerImplShould.kt @@ -31,11 +31,12 @@ import com.google.common.truth.Truth.assertThat import kotlinx.serialization.json.jsonObject import kotlinx.serialization.json.jsonPrimitive import org.hisp.dhis.android.core.arch.helpers.Result -import org.hisp.dhis.android.core.arch.helpers.UserHelper import org.hisp.dhis.android.core.arch.json.internal.KotlinxJsonParser import org.hisp.dhis.android.core.arch.storage.internal.Credentials import org.hisp.dhis.android.core.arch.storage.internal.CredentialsSecureStore +import org.hisp.dhis.android.core.arch.storage.internal.HashVerification import org.hisp.dhis.android.core.arch.storage.internal.InMemorySecureStore +import org.hisp.dhis.android.core.arch.storage.internal.PasswordHasher import org.hisp.dhis.android.core.maintenance.D2Error import org.hisp.dhis.android.core.maintenance.D2ErrorCode import org.hisp.dhis.android.core.maintenance.D2ErrorComponent @@ -514,7 +515,8 @@ class OAuth2HandlerImplShould { assertThat(credentialsCaptor.firstValue.oauth2State).isNotNull() val userCaptor = argumentCaptor() verifyBlocking(authenticatedUserStore) { updateOrInsertWhere(userCaptor.capture()) } - assertThat(userCaptor.firstValue.hash()).isEqualTo(UserHelper.md5(USERNAME, PIN)) + assertThat(PasswordHasher.verify(USERNAME, PIN, userCaptor.firstValue.hash()!!)) + .isEqualTo(HashVerification.Match(needsUpgrade = false)) } @Test @@ -553,7 +555,7 @@ class OAuth2HandlerImplShould { fun changePin_replaces_pin_when_current_matches() { val current = credentialsWithOAuth2(sampleOAuth2State()).copy(pin = PIN) whenever(credentialsSecureStore.get()).thenReturn(current) - val existing = AuthenticatedUser.builder().user("uid").hash(current.getHash()).build() + val existing = AuthenticatedUser.builder().user("uid").hash(current.newPasswordHash()).build() authenticatedUserStore.stub { onBlocking { selectFirst() }.doReturn(existing) } diff --git a/core/src/test/java/org/hisp/dhis/android/core/user/openid/OpenIDConnectHandlerImplShould.kt b/core/src/test/java/org/hisp/dhis/android/core/user/openid/OpenIDConnectHandlerImplShould.kt index f95de262ebd..4f28a247174 100644 --- a/core/src/test/java/org/hisp/dhis/android/core/user/openid/OpenIDConnectHandlerImplShould.kt +++ b/core/src/test/java/org/hisp/dhis/android/core/user/openid/OpenIDConnectHandlerImplShould.kt @@ -31,9 +31,10 @@ import android.content.Context import com.google.common.truth.Truth.assertThat import net.openid.appauth.AuthState import org.hisp.dhis.android.core.arch.helpers.Result -import org.hisp.dhis.android.core.arch.helpers.UserHelper import org.hisp.dhis.android.core.arch.storage.internal.Credentials import org.hisp.dhis.android.core.arch.storage.internal.CredentialsSecureStore +import org.hisp.dhis.android.core.arch.storage.internal.HashVerification +import org.hisp.dhis.android.core.arch.storage.internal.PasswordHasher import org.hisp.dhis.android.core.maintenance.D2Error import org.hisp.dhis.android.core.maintenance.D2ErrorCode import org.hisp.dhis.android.core.maintenance.D2ErrorComponent @@ -104,7 +105,8 @@ class OpenIDConnectHandlerImplShould { assertThat(credentialsCaptor.firstValue.openIDConnectState).isNotNull() val userCaptor = argumentCaptor() verifyBlocking(authenticatedUserStore) { updateOrInsertWhere(userCaptor.capture()) } - assertThat(userCaptor.firstValue.hash()).isEqualTo(UserHelper.md5(USERNAME, PIN)) + assertThat(PasswordHasher.verify(USERNAME, PIN, userCaptor.firstValue.hash()!!)) + .isEqualTo(HashVerification.Match(needsUpgrade = false)) } @Test @@ -143,7 +145,7 @@ class OpenIDConnectHandlerImplShould { fun changePin_replaces_pin_when_current_matches() { val current = credentialsWithOpenId(authState).copy(pin = PIN) whenever(credentialsSecureStore.get()).thenReturn(current) - val existing = AuthenticatedUser.builder().user("uid").hash(current.getHash()).build() + val existing = AuthenticatedUser.builder().user("uid").hash(current.newPasswordHash()).build() authenticatedUserStore.stub { onBlocking { selectFirst() }.doReturn(existing) }