commit 1b685ec15207c5e138a71c2001dd2595f56af4b4 parent 7aaa18caacf0d10fa287d75e03e4fb322e128437 Author: Antoine A <> Date: Mon, 1 Jul 2024 17:58:34 +0200 bank: setup password hashing upgrade and clean code Diffstat:
16 files changed, 181 insertions(+), 85 deletions(-)
diff --git a/bank/src/main/kotlin/tech/libeufin/bank/auth/auth.kt b/bank/src/main/kotlin/tech/libeufin/bank/auth/auth.kt @@ -26,6 +26,7 @@ import io.ktor.server.routing.* import io.ktor.util.* import io.ktor.util.pipeline.* import tech.libeufin.bank.* +import tech.libeufin.bank.db.AccountDAO.* import tech.libeufin.bank.db.Database import tech.libeufin.common.* import tech.libeufin.common.api.* @@ -143,9 +144,11 @@ private suspend fun doBasicAuth(db: Database, encoded: String): String { "Malformed Basic auth credentials found in the Authorization header", TalerErrorCode.GENERIC_HTTP_HEADERS_MALFORMED ) - val hash = db.account.passwordHash(login) ?: throw unauthorized("Unknown account") - if (!PwCrypto.checkpw(plainPassword, hash)) throw unauthorized("Bad password") - return login + return when (db.account.checkPassword(login, plainPassword)) { + CheckPasswordResult.UnknownAccount -> throw unauthorized("Unknown account") + CheckPasswordResult.PasswordMismatch -> throw unauthorized("Bad password") + CheckPasswordResult.Success -> login + } } fun validScope(required: TokenScope, scope: TokenScope): Boolean { diff --git a/bank/src/main/kotlin/tech/libeufin/bank/db/AccountDAO.kt b/bank/src/main/kotlin/tech/libeufin/bank/db/AccountDAO.kt @@ -83,7 +83,7 @@ class AccountDAO(private val db: Database) { setString(10, login) oneOrNull { Pair( - PwCrypto.checkpw(password, it.getString(1)) && it.getBoolean(2), + PwCrypto.checkpw(password, it.getString(1)).match && it.getBoolean(2), it.getBankPayto("internal_payto_uri", "name", ctx) ) } @@ -413,39 +413,71 @@ class AccountDAO(private val db: Database) { oldPw: String?, is2fa: Boolean ): AccountPatchAuthResult = db.serializableTransaction { conn -> - val (currentPwh, tanRequired) = conn.withStatement(""" - SELECT password_hash, (NOT ? AND tan_channel IS NOT NULL) + val (customerId, currentPwh, tanRequired) = conn.withStatement(""" + SELECT customer_id, password_hash, (NOT ? AND tan_channel IS NOT NULL) FROM customers WHERE login=? AND deleted_at IS NULL """) { setBoolean(1, is2fa) setString(2, login) oneOrNull { - Pair(it.getString(1), it.getBoolean(2)) + Triple(it.getLong(1), it.getString(2), it.getBoolean(3)) } ?: return@serializableTransaction AccountPatchAuthResult.UnknownAccount } if (tanRequired) { AccountPatchAuthResult.TanRequired - } else if (oldPw != null && !PwCrypto.checkpw(oldPw, currentPwh)) { + } else if (oldPw != null && !PwCrypto.checkpw(oldPw, currentPwh).match) { AccountPatchAuthResult.OldPasswordMismatch } else { - conn.withStatement("UPDATE customers SET password_hash=? where login=?") { - setString(1, PwCrypto.hashpw(newPw)) - setString(2, login) - executeUpdateCheck() + val newPwh = PwCrypto.hashpw(newPw) + conn.withStatement("UPDATE customers SET password_hash=? WHERE customer_id=?") { + setString(1, newPwh) + setLong(2, customerId) + executeUpdate() } AccountPatchAuthResult.Success } } - /** Get password hash of account [login] */ - suspend fun passwordHash(login: String): String? = db.serializable( - "SELECT password_hash FROM customers WHERE login=? AND deleted_at IS NULL" - ) { - setString(1, login) - oneOrNull { - it.getString(1) + /** Result status of customer account password check */ + enum class CheckPasswordResult { + UnknownAccount, + PasswordMismatch, + Success + } + + /** Check password of account [login] against [pw], rehashing it if outdated */ + suspend fun checkPassword(login: String, pw: String): CheckPasswordResult { + // Get user current password hash + val info = db.serializable( + "SELECT customer_id, password_hash FROM customers WHERE login=? AND deleted_at IS NULL" + ) { + setString(1, login) + oneOrNull { + Pair(it.getLong(1), it.getString(2)) + } } + if (info == null) return CheckPasswordResult.UnknownAccount + val (customerId, currentPwh) = info + + // Check password + val check = PwCrypto.checkpw(pw, currentPwh) + if (!check.match) return CheckPasswordResult.PasswordMismatch + + // Reshah if outdated + if (check.outdated) { + val newPwh = PwCrypto.hashpw(pw) + db.serializable( + "UPDATE customers SET password_hash=? where customer_id=? AND password_hash=?" + ) { + setString(1, newPwh) + setLong(2, customerId) + setString(3, currentPwh) + executeUpdate() + } + } + + return CheckPasswordResult.Success } /** Get bank info of account [login] */ diff --git a/bank/src/main/kotlin/tech/libeufin/bank/helpers.kt b/bank/src/main/kotlin/tech/libeufin/bank/helpers.kt @@ -104,8 +104,7 @@ fun ApplicationRequest.withdrawConfirmUrl(id: UUID) = url { suspend fun createAdminAccount(db: Database, cfg: BankConfig, pw: String? = null): AccountCreationResult { var pwStr = pw if (pwStr == null) { - val pwBuf = ByteArray(32) - Random().nextBytes(pwBuf) + val pwBuf = ByteArray(32).rand() pwStr = String(pwBuf, Charsets.UTF_8) } diff --git a/bank/src/test/kotlin/CoreBankApiTest.kt b/bank/src/test/kotlin/CoreBankApiTest.kt @@ -27,6 +27,7 @@ import tech.libeufin.bank.* import tech.libeufin.bank.auth.* import tech.libeufin.common.* import tech.libeufin.common.db.* +import tech.libeufin.common.crypto.* import java.time.Duration import java.time.Instant import java.util.* @@ -34,6 +35,42 @@ import kotlin.test.assertEquals import kotlin.test.assertNotNull import kotlin.test.assertNull +class CoreBankSecurityTest { + @Test + fun passwordUpdate() = bankSetup { db -> + suspend fun currentHash(): String { + return db.serializable( + "SELECT password_hash FROM customers WHERE login='customer'" + ) { + one { + it.getString(1) + } + } + } + + // Set outdated hash + val password = "customer-password" + val pwh = CryptoUtil.hashStringSHA256(password).encodeBase64() + val hash = "sha256\$$pwh" + db.serializable( + "UPDATE customers SET password_hash=? WHERE login='customer'" + ) { + setString(1, hash) + executeUpdate() + } + assertEquals(hash, currentHash()) + + // Check hash is updated + client.getA("/accounts/customer").assertOk() + val newHash = currentHash() + assert(hash != newHash) + + // Check hash stay the same + client.getA("/accounts/customer").assertOk() + assertEquals(newHash, currentHash()) + } +} + class CoreBankConfigTest { // GET /config @Test diff --git a/bank/src/test/kotlin/DatabaseTest.kt b/bank/src/test/kotlin/DatabaseTest.kt @@ -95,7 +95,7 @@ class DatabaseTest { // GC logic launch { try { - db.gc.collect(Instant.now(), Duration.ofMillis(20), Duration.ofMillis(20), Duration.ofMillis(20)) + db.gc.collect(Instant.now(), Duration.ofMinutes(1), Duration.ofMinutes(1), Duration.ofMinutes(1)) } catch (e: Exception) { // Check only serialization exception } diff --git a/bank/src/test/kotlin/bench.kt b/bank/src/test/kotlin/bench.kt @@ -74,8 +74,7 @@ class Bench { } gen("bearer_tokens(content, creation_time, expiration_time, scope, is_refreshable, bank_customer, description, last_access)") { val account = if (it > mid) customerAccount else it+4 - Random.nextBytes(token32) - val hex = token32.encodeHex() + val hex = token32.rand().encodeHex() "\\\\x$hex\t0\t0\treadonly\tfalse\t$account\t\\N\t0\n" } gen("bank_account_transactions(creditor_payto_uri, creditor_name, debtor_payto_uri, debtor_name, subject, amount, transaction_date, direction, bank_account_id)") { @@ -84,8 +83,7 @@ class Bench { "creditor_payto\tcreditor_name\tdebtor_payto\tdebtor_name\tsubject\t(42,0)\t0\tdebit\t$account\n" } gen("bank_transaction_operations") { - Random.nextBytes(token32) - val hex = token32.encodeHex() + val hex = token32.rand().encodeHex() "\\\\x$hex\t$it\n" } gen("tan_challenges(body, op, code, creation_date, expiration_date, retry_counter, customer)") { @@ -94,21 +92,17 @@ class Bench { } gen("taler_withdrawal_operations(withdrawal_uuid, wallet_bank_account, reserve_pub, creation_date)") { val account = if (it > mid) customerAccount else it+4 - Random.nextBytes(token32) - val hex = token32.encodeHex() + val hex = token32.rand().encodeHex() val uuid = UUID.randomUUID() "$uuid\t$account\t\\\\x$hex\t0\n" } gen("taler_exchange_outgoing(wtid, request_uid, exchange_base_url, bank_transaction, creditor_account_id)") { - Random.nextBytes(token32) - val hex32 = token32.encodeHex() - Random.nextBytes(token64) - val hex64 = token64.encodeHex() + val hex32 = token32.rand().encodeHex() + val hex64 = token64.rand().encodeHex() "\\\\x$hex32\t\\\\x$hex64\turl\t${it*2-1}\t$it\n" } gen("taler_exchange_incoming(reserve_pub, bank_transaction)") { - Random.nextBytes(token32) - val hex = token32.encodeHex() + val hex = token32.rand().encodeHex() "\\\\x$hex\t${it*2}\n" } gen("bank_stats(timeframe, start_time)") { @@ -118,8 +112,7 @@ class Bench { } gen("cashout_operations(request_uid,amount_debit,amount_credit,subject,creation_time,bank_account,local_transaction)") { val account = if (it > mid) customerAccount else it+4 - Random.nextBytes(token32) - val hex = token32.encodeHex() + val hex = token32.rand().encodeHex() "\\\\x$hex\t(0,0)\t(0,0)\tsubject\t0\t$account\t$it\n" } diff --git a/common/src/main/kotlin/AnsiColor.kt b/common/src/main/kotlin/AnsiColor.kt @@ -37,7 +37,7 @@ object ANSI { fun CharSequence.displayLength(): Int = splitToSequence(ANSI_PATTERN).sumOf { it.length } - /** Format a [msg] using optionals [fg] and [bg] colors and optionaly make the text [bold] */ + /** Format a [msg] using optionals [fg] and [bg] colors and optionally make the text [bold] */ fun fmt(msg: String, fg: Color? = null, bg: Color? = null, bold: Boolean = false): String { if (fg == null && bg == null && !bold) return msg return buildString { diff --git a/common/src/main/kotlin/crypto/CryptoUtil.kt b/common/src/main/kotlin/crypto/CryptoUtil.kt @@ -25,6 +25,8 @@ import org.bouncycastle.asn1.x509.Extension import org.bouncycastle.cert.jcajce.* import org.bouncycastle.jce.provider.BouncyCastleProvider import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder +import org.bouncycastle.crypto.generators.Argon2BytesGenerator; +import org.bouncycastle.crypto.params.Argon2Parameters; import tech.libeufin.common.* import java.io.ByteArrayOutputStream import java.io.InputStream @@ -259,4 +261,20 @@ object CryptoUtil { fun hashStringSHA256(input: String): ByteArray = MessageDigest.getInstance("SHA-256").digest(input.toByteArray(Charsets.UTF_8)) + + fun hashArgon2id(password: String, salt: ByteArray): ByteArray { + // OSWAP recommended config https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id + val builder = Argon2Parameters.Builder(Argon2Parameters.ARGON2_id) + .withIterations(1) + .withMemoryAsKB(47104) + .withParallelism(1) + .withSalt(salt) + + val gen = Argon2BytesGenerator() + gen.init(builder.build()) + + val result = ByteArray(32) + gen.generateBytes(password.toCharArray(), result, 0, result.size); + return result + } } \ No newline at end of file diff --git a/common/src/main/kotlin/crypto/PwCrypto.kt b/common/src/main/kotlin/crypto/PwCrypto.kt @@ -19,41 +19,55 @@ package tech.libeufin.common.crypto -import tech.libeufin.common.encodeBase64 +import tech.libeufin.common.* import java.security.SecureRandom +data class PasswordHashCheck( + val match: Boolean, + val outdated: Boolean +) + /** Cryptographic operations for secure password storage and verification */ object PwCrypto { - // TODO Use a real password hashing method to store passwords - private val SECURE_RNG = SecureRandom() /** Hash [pw] using the strongest supported hashing method */ fun hashpw(pw: String): String { - val saltBytes = ByteArray(8) - SECURE_RNG.nextBytes(saltBytes) - val salt = saltBytes.encodeBase64() - val pwh = CryptoUtil.hashStringSHA256("$salt|$pw").encodeBase64() - return "sha256-salted\$$salt\$$pwh" + val salt = ByteArray(16).rand(SECURE_RNG) + /* TODO Argon2id + val pwh = CryptoUtil.hashArgon2id(pw, salt) + return "argon2id\$${salt.encodeBase64()}\$${pwh.encodeBase64()}" + */ + val saltEncoded = salt.encodeBase64() + val pwh = CryptoUtil.hashStringSHA256("$saltEncoded|$pw").encodeBase64() + return "sha256-salted\$$saltEncoded\$$pwh" } - /** Check whether [pw] match hashed [storedPwHash] */ - fun checkpw(pw: String, storedPwHash: String): Boolean { - val components = storedPwHash.split('$') - when (val algo = components[0]) { - "sha256" -> { // Support legacy unsalted passwords - if (components.size != 2) throw Exception("bad password hash") + /** Check whether [pw] match hashed [storedPwHash] and if it should be rehashed */ + fun checkpw(pw: String, storedPwHash: String): PasswordHashCheck { + val components = storedPwHash.split('$', limit = 4) + return when (val algo = components[0]) { + "sha256" -> { + require(components.size == 2) { "bad password hash format" } val hash = components[1] val pwh = CryptoUtil.hashStringSHA256(pw).encodeBase64() - return pwh == hash + PasswordHashCheck(pwh == hash, true) } "sha256-salted" -> { - if (components.size != 3) throw Exception("bad password hash") + require(components.size == 3) { "bad password hash format" } val salt = components[1] val hash = components[2] val pwh = CryptoUtil.hashStringSHA256("$salt|$pw").encodeBase64() - return pwh == hash + PasswordHashCheck(pwh == hash, false) } + /* TODO Argon2id + "argon2id" -> { + require(components.size == 3) { "bad password hash format" } + val salt = components[1].decodeBase64() + val hash = components[2] + val pwh = CryptoUtil.hashArgon2id(pw, salt).encodeBase64() + PasswordHashCheck(pwh == hash, false) + } */ else -> throw Exception("unsupported hash algo: '$algo'") } } diff --git a/common/src/main/kotlin/helpers.kt b/common/src/main/kotlin/helpers.kt @@ -23,6 +23,7 @@ import java.io.ByteArrayOutputStream import java.io.FilterInputStream import java.io.InputStream import java.math.BigInteger +import java.security.SecureRandom import java.util.* import java.util.zip.DeflaterInputStream import java.util.zip.InflaterInputStream @@ -62,6 +63,10 @@ fun ByteArray.rand(): ByteArray { Random.nextBytes(this) return this } +fun ByteArray.rand(rng: SecureRandom): ByteArray { + rng.nextBytes(this) + return this +} fun ByteArray.encodeHex(): String = HexFormat.of().formatHex(this) fun ByteArray.encodeUpHex(): String = HexFormat.of().withUpperCase().formatHex(this) fun ByteArray.encodeBase64(): String = Base64.getEncoder().encodeToString(this) diff --git a/common/src/test/kotlin/CryptoUtilTest.kt b/common/src/test/kotlin/CryptoUtilTest.kt @@ -19,12 +19,10 @@ import org.junit.Ignore import org.junit.Test -import tech.libeufin.common.Base32Crockford import tech.libeufin.common.crypto.CryptoUtil import tech.libeufin.common.crypto.PwCrypto -import tech.libeufin.common.decodeUpHex -import tech.libeufin.common.encodeHex -import tech.libeufin.common.encodeUpHex +import tech.libeufin.common.crypto.PasswordHashCheck +import tech.libeufin.common.* import java.util.* import kotlin.io.path.Path import kotlin.io.path.readBytes @@ -114,7 +112,17 @@ class CryptoUtilTest { @Test fun passwordHashing() { - val x = PwCrypto.hashpw("myinsecurepw") - assertTrue(PwCrypto.checkpw("myinsecurepw", x)) + val password = "myinsecurepw" + + // Check roundtrip + val hash = PwCrypto.hashpw(password) + assertEquals(PwCrypto.checkpw(password, hash), PasswordHashCheck(true, false)) + assertEquals(PwCrypto.checkpw("other", hash), PasswordHashCheck(false, false)) + + // Check outdated + val pwh = CryptoUtil.hashStringSHA256(password).encodeBase64() + val outdatedHash = "sha256\$$pwh" + assertEquals(PwCrypto.checkpw(password, outdatedHash), PasswordHashCheck(true, true)) + assertEquals(PwCrypto.checkpw("other", outdatedHash), PasswordHashCheck(false, true)) } } diff --git a/common/src/test/kotlin/EncodingTest.kt b/common/src/test/kotlin/EncodingTest.kt @@ -19,12 +19,9 @@ import org.junit.Ignore import org.junit.Test -import tech.libeufin.common.Base32Crockford import tech.libeufin.common.crypto.CryptoUtil import tech.libeufin.common.crypto.PwCrypto -import tech.libeufin.common.decodeUpHex -import tech.libeufin.common.encodeHex -import tech.libeufin.common.encodeUpHex +import tech.libeufin.common.* import java.util.* import kotlin.io.path.Path import kotlin.io.path.readBytes @@ -48,9 +45,7 @@ class EncodingTest { // Many size for (size in 0..100) { - val blob = ByteArray(size) - Random().nextBytes(blob) - roundTripBytes(blob) + roundTripBytes(ByteArray(size).rand()) } val ORIGINAL = "00111VVBASE32TESTXRST7M8J4H2VY3E0N561BAFWDCPKQG9ZNTG" @@ -89,8 +84,7 @@ class EncodingTest { if (gnunetInstalled) { for (size in 0..100) { // Generate random blob - val blob = ByteArray(size) - Random().nextBytes(blob) + val blob = ByteArray(size).rand() // Encode with kotlin val encoded = Base32Crockford.encode(blob) diff --git a/nexus/src/main/kotlin/tech/libeufin/nexus/Main.kt b/nexus/src/main/kotlin/tech/libeufin/nexus/Main.kt @@ -131,8 +131,7 @@ class InitiatePayment: CliktCommand("Initiate an outgoing payment") { throw Exception("Wrong currency: expected $currency got ${amount.currency}") val requestUid = requestUid ?: run { - val bytes = ByteArray(16) - kotlin.random.Random.nextBytes(bytes) + val bytes = ByteArray(16).rand() Base32Crockford.encode(bytes) } @@ -214,8 +213,7 @@ class FakeIncoming: CliktCommand("Genere a fake incoming payment") { throw Exception("Wrong currency: expected ${cfg.currency} got ${amount.currency}") val bankId = run { - val bytes = ByteArray(16) - kotlin.random.Random.nextBytes(bytes) + val bytes = ByteArray(16).rand() Base32Crockford.encode(bytes) } diff --git a/nexus/src/main/kotlin/tech/libeufin/nexus/api/WireGatewayApi.kt b/nexus/src/main/kotlin/tech/libeufin/nexus/api/WireGatewayApi.kt @@ -47,8 +47,7 @@ fun Routing.wireGatewayApi(db: Database, cfg: NexusConfig) = authApi(cfg.wireGat cfg.checkCurrency(req.amount) req.credit_account.expectRequestIban() val bankId = run { - val bytes = ByteArray(16) - kotlin.random.Random.nextBytes(bytes) + val bytes = ByteArray(16).rand() Base32Crockford.encode(bytes) } val res = db.exchange.transfer( @@ -93,8 +92,7 @@ fun Routing.wireGatewayApi(db: Database, cfg: NexusConfig) = authApi(cfg.wireGat req.debit_account.expectRequestIban() val timestamp = Instant.now() val bankId = run { - val bytes = ByteArray(16) - kotlin.random.Random.nextBytes(bytes) + val bytes = ByteArray(16).rand() Base32Crockford.encode(bytes) } val res = db.payment.registerTalerableIncoming(IncomingPayment( diff --git a/nexus/src/main/kotlin/tech/libeufin/nexus/ebics/EbicsCommon.kt b/nexus/src/main/kotlin/tech/libeufin/nexus/ebics/EbicsCommon.kt @@ -359,14 +359,13 @@ suspend fun doEbicsUpload( orderId } +private val SECURE_RNG = SecureRandom() + /** * @param size in bits */ fun getNonce(size: Int): ByteArray { - val sr = SecureRandom() - val ret = ByteArray(size / 8) - sr.nextBytes(ret) - return ret + return ByteArray(size / 8).rand(SECURE_RNG) } class PreparedUploadData( diff --git a/nexus/src/test/kotlin/helpers.kt b/nexus/src/test/kotlin/helpers.kt @@ -99,8 +99,7 @@ fun genInitPay( // Generates an incoming payment, given its subject. fun genInPay(subject: String, amount: String = "KUDOS:44"): IncomingPayment { val bankId = run { - val bytes = ByteArray(16) - kotlin.random.Random.nextBytes(bytes) + val bytes = ByteArray(16).rand() Base32Crockford.encode(bytes) } return IncomingPayment( @@ -115,8 +114,7 @@ fun genInPay(subject: String, amount: String = "KUDOS:44"): IncomingPayment { // Generates an outgoing payment, given its subject and messageId fun genOutPay(subject: String, messageId: String? = null): OutgoingPayment { val id = messageId ?: run { - val bytes = ByteArray(16) - kotlin.random.Random.nextBytes(bytes) + val bytes = ByteArray(16).rand() Base32Crockford.encode(bytes) } return OutgoingPayment(