From a527c7f6f705a6dbb0944531432b0147674998b3 Mon Sep 17 00:00:00 2001 From: joshua Date: Wed, 3 Jun 2026 21:18:46 +0200 Subject: [PATCH] fix(pin): coalesce concurrent PIN check/confirm to stop lockout under-count and torn salt+hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concurrent VerifyPinCubit.checkPin() calls for one submission shared no serialization, so two parallel wrong attempts both read the same failed-attempts baseline and persisted the same incremented value — under-counting the lockout cascade and widening the brute-force window. SetupPinCubit._confirmPin() could likewise run twice (a confirm re-typed during the slow PBKDF2 hash), interleaving two salt+hash writes into a torn pair that no longer verifies and locks the user out of their own wallet. Both critical sections are now wrapped in a private in-flight Future that coalesces concurrent callers onto one round-trip and clears on completion, so the counter advances exactly once and the salt+hash pair is always written atomically from a single run. No public API added. Regression: test/screens/pin/verify_pin_cubit_test.dart:267 Regression: test/screens/pin/setup_pin_cubit_test.dart:163 Boundary: Tier 1 Architect-spec: fixer/fix-spec.md (loop fleet-20260603-190231-84831-2, iter 1) mandate-hash: b59fe886f6702d4c loop-id: fleet-20260603-190231-84831-2 iteration: 1 --- .../pin/bloc/setup_pin/setup_pin_cubit.dart | 10 ++++- .../pin/bloc/verify_pin/verify_pin_cubit.dart | 10 ++++- test/screens/pin/setup_pin_cubit_test.dart | 41 +++++++++++++++++++ test/screens/pin/verify_pin_cubit_test.dart | 25 +++++++++++ 4 files changed, 84 insertions(+), 2 deletions(-) diff --git a/lib/screens/pin/bloc/setup_pin/setup_pin_cubit.dart b/lib/screens/pin/bloc/setup_pin/setup_pin_cubit.dart index 11e77ac5..59a599ed 100644 --- a/lib/screens/pin/bloc/setup_pin/setup_pin_cubit.dart +++ b/lib/screens/pin/bloc/setup_pin/setup_pin_cubit.dart @@ -19,6 +19,8 @@ class SetupPinCubit extends Cubit { String? _createPin; + Future? _inflightConfirm; + void addDigit(int digit) { if (state.currentPin.length >= pinLength) return; @@ -57,7 +59,13 @@ class SetupPinCubit extends Cubit { } } - Future _confirmPin(String confirmPin) async { + // A confirm re-typed while the slow PBKDF2 hash is still in flight must not + // start a second salt+hash round-trip: coalesce onto the first so the + // persisted salt and hash always come from one run (never a torn pair). + Future _confirmPin(String confirmPin) => + _inflightConfirm ??= _runConfirmPin(confirmPin).whenComplete(() => _inflightConfirm = null); + + Future _runConfirmPin(String confirmPin) async { if (confirmPin == _createPin) { final salt = SecureStorage.generatePinSalt(); final hash = await SecureStorage.hashPinAsync(confirmPin, salt); diff --git a/lib/screens/pin/bloc/verify_pin/verify_pin_cubit.dart b/lib/screens/pin/bloc/verify_pin/verify_pin_cubit.dart index 8a43a36b..0973fccc 100644 --- a/lib/screens/pin/bloc/verify_pin/verify_pin_cubit.dart +++ b/lib/screens/pin/bloc/verify_pin/verify_pin_cubit.dart @@ -19,6 +19,8 @@ class VerifyPinCubit extends Cubit { final BiometricService _biometricService; final bool enableLockout; + Future? _inflightCheck; + void addDigit(int digit) { if (state is VerifyPinTemporarilyLocked || state is VerifyPinLocked) return; if (state.pin.length == pinLength) return; @@ -32,7 +34,13 @@ class VerifyPinCubit extends Cubit { emit(state.copyWith(pin: state.pin.substring(0, state.pin.length - 1))); } - Future checkPin() async { + // Concurrent checkPin() calls always verify the same submission, so coalesce + // them onto one in-flight round-trip: the read-modify-write lockout counter + // advances exactly once instead of under-counting on an interleaved race. + Future checkPin() => + _inflightCheck ??= _runCheckPin().whenComplete(() => _inflightCheck = null); + + Future _runCheckPin() async { final isCorrect = await _secureStorage.verifyPin(state.pin); if (isCorrect) { if (enableLockout) await _secureStorage.resetPinLockout(); diff --git a/test/screens/pin/setup_pin_cubit_test.dart b/test/screens/pin/setup_pin_cubit_test.dart index cc0256c6..67aa6179 100644 --- a/test/screens/pin/setup_pin_cubit_test.dart +++ b/test/screens/pin/setup_pin_cubit_test.dart @@ -158,5 +158,46 @@ void main() { expect(result, isTrue); verify(() => biometricService.enable()).called(1); }); + + group('concurrency (F-09: salt/hash setup race)', () { + test('confirm re-typed during the in-flight hash writes one atomic salt+hash', () async { + Uint8List? savedSalt; + String? savedHash; + when(() => secureStorage.setPinSalt(any())).thenAnswer((inv) async { + savedSalt = inv.positionalArguments.first as Uint8List; + }); + when(() => secureStorage.setPinHash(any())).thenAnswer((inv) async { + savedHash = inv.positionalArguments.first as String; + }); + final cubit = build(); + final completed = cubit.stream.firstWhere((s) => s.isComplete); + + // Create the pin -> confirm mode (with _createPin set). + for (final d in [1, 2, 3, 4, 5, 6]) { + cubit.addDigit(d); + } + // Confirm the pin -> fires _confirmPin #1 (in-flight on the slow PBKDF2 + // compute()). currentPin stays '123456', so the field still looks full. + for (final d in [1, 2, 3, 4, 5, 6]) { + cubit.addDigit(d); + } + // An impatient user backspaces and re-types the last digit while the + // hash is still computing -> fires _confirmPin #2 concurrently. + cubit.deleteDigit(); + cubit.addDigit(6); + + await completed.timeout(const Duration(seconds: 30)); + await pumpEventQueue(); + + // Single-effect + atomic: exactly one salt and one hash persisted, and + // the persisted hash must be PBKDF2(pin, persisted salt) — i.e. salt and + // hash provably come from the SAME run. At HEAD two interleaved runs + // write twice and can tear the pair (saltB + hashA) -> RED. + verify(() => secureStorage.setPinSalt(any())).called(1); + verify(() => secureStorage.setPinHash(any())).called(1); + expect(savedSalt, isNotNull); + expect(savedHash, SecureStorage.hashPin('123456', savedSalt!)); + }); + }); }); } diff --git a/test/screens/pin/verify_pin_cubit_test.dart b/test/screens/pin/verify_pin_cubit_test.dart index 3bf86183..015509bc 100644 --- a/test/screens/pin/verify_pin_cubit_test.dart +++ b/test/screens/pin/verify_pin_cubit_test.dart @@ -262,5 +262,30 @@ void main() { verifyNever(() => biometricService.authenticate()); }); }); + + group('concurrency (F-01: PIN-lockout under-count race)', () { + test('two parallel checkPin of one submission advance the counter once', () async { + when(() => secureStorage.verifyPin(any())).thenAnswer((_) async => false); + when(() => secureStorage.getPinFailedAttempts()).thenAnswer((_) async => 0); + final cubit = build(); + + // The PIN is fixed for the lifetime of a submission, so concurrent + // checkPin() calls always verify the SAME pin — one logical attempt + // double-fired. The 6th digit auto-fires checkPin() #1; a second + // checkPin() races it against the read-modify-write failure counter. + addPin(cubit, '99999'); + cubit.addDigit(9); // 6th digit -> auto checkPin() #1 (in-flight) + final second = cubit.checkPin(); // checkPin() #2, same state.pin + + await second.timeout(const Duration(seconds: 30)); + await pumpEventQueue(); + + // Single-effect: the counter must advance to exactly 1, written once. + // At HEAD both reads see 0 and both write 1 -> called twice (RED). + verify(() => secureStorage.setPinFailedAttempts(1)).called(1); + verifyNever(() => secureStorage.setPinFailedAttempts(2)); + expect(cubit.state.failedAttempts, 1); + }); + }); }); }