From 07b3299dc36c897181336a2a2eb5ffb47c7ef1a5 Mon Sep 17 00:00:00 2001 From: danielTari Date: Fri, 21 Aug 2026 09:19:58 +0200 Subject: [PATCH 1/2] upgrade credentials encryption from md5 to PBKDF2 --- .../android/core/arch/helpers/UserHelper.kt | 5 + .../core/arch/storage/internal/Credentials.kt | 24 ++- .../arch/storage/internal/HashVerification.kt | 47 ++++ .../arch/storage/internal/PasswordHasher.kt | 204 ++++++++++++++++++ .../android/core/user/internal/LogInCall.kt | 35 ++- .../user/oauth2/internal/OAuth2HandlerImpl.kt | 2 +- .../user/openid/OpenIDConnectHandlerImpl.kt | 2 +- .../core/arch/helpers/UserHelperShould.kt | 1 + .../storage/internal/CredentialsShould.kt | 54 ++++- .../storage/internal/PasswordHasherShould.kt | 151 +++++++++++++ .../core/user/internal/LogInCallUnitShould.kt | 72 ++++++- .../internal/OAuth2HandlerImplShould.kt | 8 +- .../openid/OpenIDConnectHandlerImplShould.kt | 8 +- 13 files changed, 581 insertions(+), 32 deletions(-) create mode 100644 core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/HashVerification.kt create mode 100644 core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasher.kt create mode 100644 core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasherShould.kt 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 e395352e365..3f5515d6c9f 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 @@ -50,8 +49,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..8f2bafba8b4 --- /dev/null +++ b/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/HashVerification.kt @@ -0,0 +1,47 @@ +/* + * Copyright (c) 2004-2025, 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 { + + /** + * The secret matches the stored hash. [needsUpgrade] is true when the stored hash was produced + * with an algorithm or a set of parameters that are no longer the current ones, so the caller + * should rewrite it with [PasswordHasher.hash] while it still holds the plaintext secret. + */ + data class Match(val needsUpgrade: Boolean) : HashVerification + + /** + * The secret does not match the stored hash, or the stored hash cannot be verified on this + * device. + */ + 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..c1899a95628 --- /dev/null +++ b/core/src/main/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasher.kt @@ -0,0 +1,204 @@ +/* + * Copyright (c) 2004-2025, 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. It is embedded in every stored value, so it can + * be raised in a future release without invalidating the hashes already written: they will + * simply report [HashVerification.Match.needsUpgrade] the next time they are verified. + */ + 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 41ebaa16a24..a1c6f88b661 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.ServerUrlParser @@ -188,7 +189,7 @@ internal class LogInCall( } val authenticatedUser = AuthenticatedUser.builder() .user(user.uid()) - .hash(credentials.getHash()) + .hash(credentials.newPasswordHash()) .build() authenticatedUserStore.updateOrInsertWhere(authenticatedUser) @@ -206,7 +207,8 @@ internal class LogInCall( private suspend fun verifyPinAgainstStoredHash(credentials: Credentials) { val existing = authenticatedUserStore.selectFirst() ?: return - if (existing.hash() != credentials.getHash()) { + // No rehash is needed here: the caller overwrites the hash right after this check. + if (credentials.matches(existing.hash()) is HashVerification.Mismatch) { throw exceptions.badCredentialsError() } } @@ -221,18 +223,35 @@ internal class LogInCall( } val existingUser = authenticatedUserStore.selectFirst() ?: throw exceptions.noUserOfflineError() - if (credentials.getHash() != existingUser.hash()) { - throw when (credentials.authorizationType) { - AuthorizationType.BASIC -> exceptions.badCredentialsError() - AuthorizationType.OPEN_ID_CONNECT -> exceptions.badCredentialsError() - AuthorizationType.OAUTH2 -> exceptions.incorrectOfflineCodeError() - } + when (val verification = credentials.matches(existingUser.hash())) { + is HashVerification.Mismatch -> + throw when (credentials.authorizationType) { + AuthorizationType.BASIC -> exceptions.badCredentialsError() + AuthorizationType.OPEN_ID_CONNECT -> exceptions.badCredentialsError() + AuthorizationType.OAUTH2 -> exceptions.incorrectOfflineCodeError() + } + + 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 bea0b2bcffd..78d70bcdfe0 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 @@ -252,7 +252,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..e479a92d251 --- /dev/null +++ b/core/src/test/java/org/hisp/dhis/android/core/arch/storage/internal/PasswordHasherShould.kt @@ -0,0 +1,151 @@ +/* + * Copyright (c) 2004-2025, 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 1f770344ad4..bdc47beb86f 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.BaseCallShould import org.hisp.dhis.android.core.configuration.internal.MultiUserDatabaseManager @@ -96,7 +98,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 { @@ -251,15 +253,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 } + whenever(multiUserDatabaseManager.loadExistingKeepingEncryption(SERVER_URL, USERNAME)).thenReturn(true) + whenever(authenticatedUserStore.selectFirst()).thenReturn(legacyAuthenticatedUser()) + + login() + + verifySuccessOffline() + } + + @Test + fun replace_a_legacy_md5_hash_after_a_successful_offline_login() = runTest { + whenAPICall { throw d2Error } + whenever(multiUserDatabaseManager.loadExistingKeepingEncryption(SERVER_URL, USERNAME)).thenReturn(true) + 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 } + whenever(multiUserDatabaseManager.loadExistingKeepingEncryption(SERVER_URL, USERNAME)).thenReturn(true) + 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") @@ -306,7 +351,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(oauth2StateSecureStore.get(SERVER_URL, USERNAME)).thenReturn(state) whenever(multiUserDatabaseManager.loadExistingKeepingEncryption(SERVER_URL, USERNAME)) .thenReturn(true) - 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(multiUserDatabaseManager.loadExistingKeepingEncryption(SERVER_URL, USERNAME)) .thenReturn(true) // 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) { @@ -419,7 +464,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(openIDConnectTokenRefresher.blockingGetFreshTokenOrNull(openIdAuthState)) .thenReturn(FRESH_ID_TOKEN) // 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) { @@ -456,7 +501,7 @@ class LogInCallUnitShould : BaseCallShould() { whenever(multiUserDatabaseManager.loadExistingKeepingEncryption(SERVER_URL, USERNAME)) .thenReturn(true) // 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) // OpenID accounts keep the generic bad-credentials error; only OAuth2 reports an offline-code error. @@ -487,5 +532,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 d4e181b9b30..9ee6920dd0c 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 @@ -452,7 +453,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 @@ -491,7 +493,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) } From 744304cadc34c23029f21f197cc32037f51164a6 Mon Sep 17 00:00:00 2001 From: danielTari Date: Mon, 24 Aug 2026 16:19:53 +0200 Subject: [PATCH 2/2] clean comments --- .../core/arch/storage/internal/HashVerification.kt | 12 +----------- .../core/arch/storage/internal/PasswordHasher.kt | 6 ++---- .../arch/storage/internal/PasswordHasherShould.kt | 2 +- 3 files changed, 4 insertions(+), 16 deletions(-) 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 index 8f2bafba8b4..72b0e9d1eb6 100644 --- 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 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-2025, University of Oslo + * Copyright (c) 2004-2026, University of Oslo * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -32,16 +32,6 @@ package org.hisp.dhis.android.core.arch.storage.internal */ internal sealed interface HashVerification { - /** - * The secret matches the stored hash. [needsUpgrade] is true when the stored hash was produced - * with an algorithm or a set of parameters that are no longer the current ones, so the caller - * should rewrite it with [PasswordHasher.hash] while it still holds the plaintext secret. - */ data class Match(val needsUpgrade: Boolean) : HashVerification - - /** - * The secret does not match the stored hash, or the stored hash cannot be verified on this - * device. - */ 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 index c1899a95628..9ddf2d64f70 100644 --- 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 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-2025, University of Oslo + * Copyright (c) 2004-2026, University of Oslo * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -61,9 +61,7 @@ internal object PasswordHasher { private const val KEY_LENGTH_BITS = 256 /** - * Cost factor applied to newly derived hashes. It is embedded in every stored value, so it can - * be raised in a future release without invalidating the hashes already written: they will - * simply report [HashVerification.Match.needsUpgrade] the next time they are verified. + * Cost factor applied to newly derived hashes. */ private const val ITERATIONS = 210_000 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 index e479a92d251..5700c7857b0 100644 --- 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 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-2025, University of Oslo + * Copyright (c) 2004-2026, University of Oslo * All rights reserved. * * Redistribution and use in source and binary forms, with or without