From 243ca350447c39cb5356ebdaa1f8185c428b448b Mon Sep 17 00:00:00 2001 From: Florian Franzen Date: Sat, 3 Oct 2026 15:16:57 +0200 Subject: [PATCH 1/2] Count a malformed PIN of the right length as a wrong PIN --- CHANGELOG.md | 2 ++ src/commands.rs | 9 +++++---- tests/command_response.ron | 11 +++++++++++ 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e1a0096..9c5e1e53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ default (5) - Fix the inverted parsing of the touch requirement in SET MANAGEMENT KEY's P2 (0xFF means no touch, 0xFE means touch required) +- Let a malformed PIN of the right length burn a retry instead of rejecting it as a + format error ## [v0.7.0][] (2026-08-17) diff --git a/src/commands.rs b/src/commands.rs index 88cf4209..b3ff3571 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -166,10 +166,11 @@ impl TryFrom> for Verify { } Ok(match (logout.0, data.len()) { (false, 0) => Verify::Status(key_reference), - (false, 8) => Verify::Login(VerifyLogin::PivPin( - data.try_into() - .map_err(|_| Status::IncorrectDataParameter)?, - )), + // A malformed PIN of the right length can never match the stored + // one, so it burns a retry like a wrong PIN + (false, 8) => Verify::Login(VerifyLogin::PivPin(Pin(data + .try_into() + .map_err(|_| Status::IncorrectDataParameter)?))), (false, _) => return Err(Status::IncorrectDataParameter), (true, 0) => Verify::Logout(key_reference), (true, _) => return Err(Status::IncorrectDataParameter), diff --git a/tests/command_response.ron b/tests/command_response.ron index 7bc7ecea..d62cbdbc 100644 --- a/tests/command_response.ron +++ b/tests/command_response.ron @@ -13,6 +13,17 @@ Select ] ), + IoTest( + name: "Malformed pin burns a retry", + cmd_resp: [ + // eight 0xFF bytes can never match, but count like a wrong pin + VerifyApplicationPin(pin: "FFFFFFFFFFFFFFFF", expected_status: RemainingRetries(2)), + VerifyApplicationPin(pin: "FFFFFFFFFFFFFFFF", expected_status: RemainingRetries(1)), + // the correct pin still verifies and resets the counter + VerifyApplicationPin(), + VerifyApplicationPin(pin: "FFFFFFFFFFFFFFFF", expected_status: RemainingRetries(2)), + ] + ), IoTest( name: "Bad management key", cmd_resp: [ From 2a267d6aad0be1d3a14a83ad996c370d47d4856b Mon Sep 17 00:00:00 2001 From: Florian Franzen Date: Thu, 8 Oct 2026 18:42:07 +0200 Subject: [PATCH 2/2] Validate the new PIN in RESET RETRY COUNTER --- CHANGELOG.md | 1 + src/lib.rs | 9 ++++----- src/state.rs | 9 +++++++-- tests/command_response.ron | 4 ++++ 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c5e1e53..649881be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ (0xFF means no touch, 0xFE means touch required) - Let a malformed PIN of the right length burn a retry instead of rejecting it as a format error +- Validate the new PIN in RESET RETRY COUNTER ## [v0.7.0][] (2026-08-17) diff --git a/src/lib.rs b/src/lib.rs index 52aefebf..3972816c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1016,11 +1016,10 @@ impl LoadedAuthenticator<'_, T> { } fn reset_retry_counter(&mut self, data: ResetRetryCounter) -> Result { - let res = self.state.persistent.reset_retry_counter( - &Puk(data.puk), - &Pin(data.pin), - self.trussed, - )?; + let res = + self.state + .persistent + .reset_retry_counter(&Puk(data.puk), &data.pin, self.trussed)?; if !res { return Err(Status::RemainingRetries( self.state.persistent.remaining_puk_retries(self.trussed), diff --git a/src/state.rs b/src/state.rs index 864ce5f8..1f911e7f 100644 --- a/src/state.rs +++ b/src/state.rs @@ -648,12 +648,17 @@ impl Persistent { pub fn reset_retry_counter( &mut self, puk: &Puk, - new_pin: &Pin, + new_pin: &[u8; 8], client: &mut T, ) -> Result { let Some(puk_key) = self.get_puk_key(puk, client)? else { return Ok(false); }; + // The PUK is compared first, like VERIFY would; a stored PIN has to be well formed + let Ok(new_pin) = Pin::try_from(new_pin.as_slice()) else { + syscall!(client.delete(puk_key)); + return Err(Status::IncorrectDataParameter); + }; let Some(pin_key) = syscall!(client.unwrap_key_from_file( Mechanism::Chacha8Poly1305, @@ -669,7 +674,7 @@ impl Persistent { return Err(Status::UnspecifiedNonpersistentExecutionError); }; - self.reset_pin(*new_pin, pin_key, client)?; + self.reset_pin(new_pin, pin_key, client)?; syscall!(client.delete(pin_key)); syscall!(client.delete(puk_key)); Ok(true) diff --git a/tests/command_response.ron b/tests/command_response.ron index d62cbdbc..abf20a7a 100644 --- a/tests/command_response.ron +++ b/tests/command_response.ron @@ -400,6 +400,10 @@ VerifyApplicationPin(pin: "313233343536FFFF", expected_status: RemainingRetries(2)), ResetRetryCounter(puk: "FFFFFFFFFFFFFFFF", new_pin: "3132333435363738", expected_status: RemainingRetries(4)), VerifyApplicationPin(pin: "3132333435363738", expected_status: RemainingRetries(1)), + // a wrong puk burns a retry whatever the new pin looks like (how ykman blocks the puk) + ResetRetryCounter(puk: "FFFFFFFFFFFFFFFF", new_pin: "FFFFFFFFFFFFFFFF", expected_status: RemainingRetries(3)), + // a malformed new pin is rejected after the puk matched, before anything is stored + ResetRetryCounter(puk: "0102030405060708", new_pin: "FFFFFFFFFFFFFFFF", expected_status: IncorrectDataParameter), ResetRetryCounter(puk: "0102030405060708", new_pin: "3132333435363738"), VerifyApplicationPin(pin: "3132333435363738"), ChangePuk(