From 76e03fb890b35d17dfb54793eca59fcbc10bcc69 Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Mon, 7 Jul 2025 11:03:59 +0200 Subject: [PATCH 1/4] fix(otp): Handle case when last slot is deleted Fixes #182 --- .../YubiKey/Pipelines/OtpErrorTransform.cs | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs index 604b705e0..d25cc9358 100644 --- a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs +++ b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs @@ -16,6 +16,7 @@ using Microsoft.Extensions.Logging; using Yubico.Core.Iso7816; using Yubico.Core.Logging; +using Yubico.YubiKey.Otp; using Yubico.YubiKey.Otp.Commands; namespace Yubico.YubiKey.Pipelines @@ -57,25 +58,36 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT try { var responseApdu = _nextTransform.Invoke(command, commandType, responseType); - int afterSequence = new ReadStatusResponse(responseApdu).GetData().SequenceNumber; - int expectedSequence = (beforeSequence + 1) % 0x100; + var readStatusResponse = new ReadStatusResponse(responseApdu); + var otpStatus = readStatusResponse.GetData(); + byte nextSequence = otpStatus.SequenceNumber; // If we see the sequence number change, we can assume that the configuration was applied successfully. Otherwise // we just invent an error in the response. - return afterSequence != expectedSequence - ? new ResponseApdu(responseApdu.Data.ToArray(), SWConstants.WarningNvmUnchanged) - : responseApdu; + return IsValidSequenceProgression(beforeSequence, nextSequence, otpStatus) + ? responseApdu + : FailedApdu(responseApdu.Data.ToArray()); } catch (KeyboardConnectionException e) { _logger.LogWarning(e, "Handling keyboard connection exception. Translating to APDU response."); - return new ResponseApdu(Array.Empty(), SWConstants.WarningNvmUnchanged); + return FailedApdu([]); } + + static ResponseApdu FailedApdu(byte[] data) => new(data, SWConstants.WarningNvmUnchanged); } public void Setup() => _nextTransform.Setup(); public void Cleanup() => _nextTransform.Cleanup(); + + private static bool IsValidSequenceProgression(int beforeSequence, int nextSequence, OtpStatus afterStatus) + { + const int configStatusMask = 0x1F; + + return nextSequence == beforeSequence + 1 || // Normal increment + (beforeSequence > 0 && nextSequence == 0 && (afterStatus.TouchLevel & configStatusMask) == 0); // When deleting the "last" slot + } } } From 4f77bb3b33df2fde1a11f1b0d3388254e5376ef1 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Mon, 7 Jul 2025 17:15:09 +0200 Subject: [PATCH 2/4] test(otp): Add unit tests for IsValidSequenceProgression method --- .../YubiKey/Pipelines/OtpErrorTransform.cs | 4 +- .../Pipelines/OtpErrorTransformTests.cs | 74 +++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs diff --git a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs index d25cc9358..eebc984d2 100644 --- a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs +++ b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs @@ -60,7 +60,7 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT var responseApdu = _nextTransform.Invoke(command, commandType, responseType); var readStatusResponse = new ReadStatusResponse(responseApdu); var otpStatus = readStatusResponse.GetData(); - byte nextSequence = otpStatus.SequenceNumber; + int nextSequence = otpStatus.SequenceNumber; // If we see the sequence number change, we can assume that the configuration was applied successfully. Otherwise // we just invent an error in the response. @@ -82,7 +82,7 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT public void Cleanup() => _nextTransform.Cleanup(); - private static bool IsValidSequenceProgression(int beforeSequence, int nextSequence, OtpStatus afterStatus) + internal static bool IsValidSequenceProgression(int beforeSequence, int nextSequence, OtpStatus afterStatus) { const int configStatusMask = 0x1F; diff --git a/Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs b/Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs new file mode 100644 index 000000000..a35e580fb --- /dev/null +++ b/Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs @@ -0,0 +1,74 @@ +using Xunit; +using Yubico.YubiKey.Otp; + +namespace Yubico.YubiKey.Pipelines; + +public class OtpErrorTransformTests +{ + [Theory] + [InlineData(0, 1)] // First increment + [InlineData(5, 6)] // Normal increment + [InlineData(254, 255)] // Near byte boundary + [InlineData(257, 258)] // Near byte boundary + + public void IsValidSequenceProgression_NormalIncrement_ReturnsTrue(int before, int after) + { + var status = CreateOtpStatus(touchLevel: 0xFF); // TouchLevel irrelevant + + bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, after, status); + + Assert.True(isValid); + } + + [Theory] + [InlineData(0, 2)] // Skip increment + [InlineData(5, 7)] // Jump by 2 + [InlineData(10, 8)] // Backwards + public void IsValidSequenceProgression_InvalidIncrement_ReturnsFalse(int before, int after) + { + var status = CreateOtpStatus(touchLevel: 0x00); // TouchLevel irrelevant + + bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, after, status); + + Assert.False(isValid); + } + + [Theory] + [InlineData(1, 0x00)] // Config bits clear + [InlineData(5, 0x20)] // Upper bits set, config clear + [InlineData(255, 0xE0)] // Multiple upper bits, config clear + public void IsValidSequenceProgression_ValidReset_ReturnsTrue(int before, byte touchLevel) + { + var status = CreateOtpStatus(touchLevel); + + bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, 0, status); + + Assert.True(isValid); + } + + [Theory] + [InlineData(0, 0x00)] // Already zero (invalid before state) + [InlineData(1, 0x01)] // Config bit set + [InlineData(5, 0x1F)] // All config bits set + [InlineData(5, 0x21)] // Config + upper bits set + public void IsValidSequenceProgression_InvalidReset_ReturnsFalse(int before, byte touchLevel) + { + var status = CreateOtpStatus(touchLevel); + + bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, 0, status); + + Assert.False(isValid); + } + + private static OtpStatus CreateOtpStatus(byte touchLevel) => new() + { + FirmwareVersion = new FirmwareVersion(), + SequenceNumber = 0, + TouchLevel = touchLevel, + ShortPressConfigured = false, + LongPressConfigured = false, + ShortPressRequiresTouch = false, + LongPressRequiresTouch = false, + LedBehaviorInverted = false + }; +} From 8f24ae97c7b2576d2cdb96cb023a3ea6040b6a68 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 8 Jul 2025 11:59:48 +0200 Subject: [PATCH 3/4] refactor: Refactored OtpErrorTransform and tests for readability and simplicity --- .../YubiKey/Pipelines/OtpErrorTransform.cs | 83 +++++++++++------ .../Pipelines/OtpErrorTransformTests.cs | 91 +++++++++++-------- 2 files changed, 107 insertions(+), 67 deletions(-) diff --git a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs index eebc984d2..abcfddb0f 100644 --- a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs +++ b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs @@ -38,56 +38,85 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT { // If this is just a regular ReadStatusCommand, or it's a command that doesn't ask for a status response // in return, invoke the pipeline as usual. - if (commandType == typeof(ReadStatusCommand) - || responseType != typeof(ReadStatusResponse)) + if (commandType == typeof(ReadStatusCommand) || + responseType != typeof(ReadStatusResponse)) { - return _nextTransform.Invoke(command, commandType, responseType); + return InvokeCommand(command, commandType, responseType); } // Otherwise we assume this to be a command that applies a config (and therefore looks for a status response). // In order to detect failures, we grab the status structure before applying said command so that we have a // sequence number to compare to. - int beforeSequence = new ReadStatusResponse( - _nextTransform.Invoke( - new ReadStatusCommand().CreateCommandApdu(), - typeof(ReadStatusCommand), - typeof(ReadStatusResponse))) - .GetData() - .SequenceNumber; + var beforeStatus = GetCurrentStatus(); + var responseApdu = InvokeCommand(command, commandType, responseType, out var afterStatus); try { - var responseApdu = _nextTransform.Invoke(command, commandType, responseType); - var readStatusResponse = new ReadStatusResponse(responseApdu); - var otpStatus = readStatusResponse.GetData(); - int nextSequence = otpStatus.SequenceNumber; - - // If we see the sequence number change, we can assume that the configuration was applied successfully. Otherwise - // we just invent an error in the response. - return IsValidSequenceProgression(beforeSequence, nextSequence, otpStatus) + // If we see the sequence number change, we can assume that the configuration was applied successfully. + // Otherwise we just invent an error in the response. + return IsValidSequenceProgression(beforeStatus, afterStatus) ? responseApdu - : FailedApdu(responseApdu.Data.ToArray()); + : CreateFailedApdu(responseApdu.Data.ToArray()); } catch (KeyboardConnectionException e) { _logger.LogWarning(e, "Handling keyboard connection exception. Translating to APDU response."); - return FailedApdu([]); + return CreateFailedApdu([]); } - - static ResponseApdu FailedApdu(byte[] data) => new(data, SWConstants.WarningNvmUnchanged); } public void Setup() => _nextTransform.Setup(); public void Cleanup() => _nextTransform.Cleanup(); - - internal static bool IsValidSequenceProgression(int beforeSequence, int nextSequence, OtpStatus afterStatus) + + // Internal for testing + internal static bool IsValidSequenceProgression(OtpStatus beforeStatus, OtpStatus afterStatus) { - const int configStatusMask = 0x1F; + byte before = beforeStatus.SequenceNumber; + byte after = afterStatus.SequenceNumber; + + bool normalIncrement = after == before + 1; + bool validReset = before > 0 && after == 0 && + afterStatus is { LongPressConfigured: false, ShortPressConfigured: false }; + + return normalIncrement || validReset; + } - return nextSequence == beforeSequence + 1 || // Normal increment - (beforeSequence > 0 && nextSequence == 0 && (afterStatus.TouchLevel & configStatusMask) == 0); // When deleting the "last" slot + private ResponseApdu InvokeCommand( + CommandApdu commandApdu, + Type commandType, + Type responseType, + out OtpStatus afterStatus) + { + var responseApdu = _nextTransform.Invoke(commandApdu, commandType, responseType); + afterStatus = ReadStatus(responseApdu); + return responseApdu; } + + private ResponseApdu InvokeCommand(CommandApdu commandApdu, Type commandType, Type responseType) => + _nextTransform.Invoke(commandApdu, commandType, responseType); + + private OtpStatus GetCurrentStatus() + { + var command = new ReadStatusCommand(); + var responseApdu = _nextTransform.Invoke( + command.CreateCommandApdu(), + typeof(ReadStatusCommand), + typeof(ReadStatusResponse)); + + return command + .CreateResponseForApdu(responseApdu) + .GetData(); + } + + private static OtpStatus ReadStatus(ResponseApdu responseApdu) + { + var readStatusResponse = new ReadStatusResponse(responseApdu); + var afterStatus = readStatusResponse.GetData(); + return afterStatus; + } + + private static ResponseApdu CreateFailedApdu(byte[] data) => new(data, SWConstants.WarningNvmUnchanged); } } diff --git a/Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs b/Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs index a35e580fb..eb3cfa8b6 100644 --- a/Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs +++ b/Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs @@ -6,67 +6,78 @@ namespace Yubico.YubiKey.Pipelines; public class OtpErrorTransformTests { [Theory] - [InlineData(0, 1)] // First increment - [InlineData(5, 6)] // Normal increment - [InlineData(254, 255)] // Near byte boundary - [InlineData(257, 258)] // Near byte boundary - - public void IsValidSequenceProgression_NormalIncrement_ReturnsTrue(int before, int after) + [InlineData(0, 1)] // First increment + [InlineData(5, 6)] // Normal increment + [InlineData(254, 255)] // Near byte boundary + public void IsValidSequenceProgression_NormalIncrement_ReturnsTrue( + byte before, + byte after) { - var status = CreateOtpStatus(touchLevel: 0xFF); // TouchLevel irrelevant - - bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, after, status); - + var beforeStatus = CreateOtpStatus(before, hasConfigs: true); + var afterStatus = CreateOtpStatus(after, hasConfigs: true); + + var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus); + Assert.True(isValid); } [Theory] - [InlineData(0, 2)] // Skip increment - [InlineData(5, 7)] // Jump by 2 - [InlineData(10, 8)] // Backwards - public void IsValidSequenceProgression_InvalidIncrement_ReturnsFalse(int before, int after) + [InlineData(0, 2)] // Skip increment + [InlineData(5, 7)] // Jump by 2 + [InlineData(10, 8)] // Backwards + [InlineData(255, 0)] // Byte overflow (should be rejected as normal increment) + public void IsValidSequenceProgression_InvalidIncrement_ReturnsFalse( + byte before, + byte after) { - var status = CreateOtpStatus(touchLevel: 0x00); // TouchLevel irrelevant - - bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, after, status); - + var beforeStatus = CreateOtpStatus(before, hasConfigs: true); + var afterStatus = CreateOtpStatus(after, hasConfigs: true); // Still has configs + + var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus); + Assert.False(isValid); } [Theory] - [InlineData(1, 0x00)] // Config bits clear - [InlineData(5, 0x20)] // Upper bits set, config clear - [InlineData(255, 0xE0)] // Multiple upper bits, config clear - public void IsValidSequenceProgression_ValidReset_ReturnsTrue(int before, byte touchLevel) + [InlineData(1)] // Delete from sequence 1 + [InlineData(5)] // Delete from sequence 5 + [InlineData(255)] // Delete from sequence 255 + public void IsValidSequenceProgression_ValidReset_ReturnsTrue( + byte before) { - var status = CreateOtpStatus(touchLevel); - - bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, 0, status); - + var beforeStatus = CreateOtpStatus(before, hasConfigs: true); + var afterStatus = CreateOtpStatus(0, hasConfigs: false); // No configs remain + + var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus); + Assert.True(isValid); } [Theory] - [InlineData(0, 0x00)] // Already zero (invalid before state) - [InlineData(1, 0x01)] // Config bit set - [InlineData(5, 0x1F)] // All config bits set - [InlineData(5, 0x21)] // Config + upper bits set - public void IsValidSequenceProgression_InvalidReset_ReturnsFalse(int before, byte touchLevel) + [InlineData(0, false)] // Already zero + [InlineData(1, true)] // Reset but configs still exist + [InlineData(5, true)] // Reset but configs still exist + public void IsValidSequenceProgression_InvalidReset_ReturnsFalse( + byte before, + bool afterHasConfigs) { - var status = CreateOtpStatus(touchLevel); - - bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, 0, status); - + var beforeStatus = CreateOtpStatus(before, hasConfigs: true); + var afterStatus = CreateOtpStatus(0, hasConfigs: afterHasConfigs); + + var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus); + Assert.False(isValid); } - private static OtpStatus CreateOtpStatus(byte touchLevel) => new() + private static OtpStatus CreateOtpStatus( + byte sequenceNumber, + bool hasConfigs) => new() { FirmwareVersion = new FirmwareVersion(), - SequenceNumber = 0, - TouchLevel = touchLevel, - ShortPressConfigured = false, - LongPressConfigured = false, + SequenceNumber = sequenceNumber, + TouchLevel = 0x00, + ShortPressConfigured = hasConfigs, + LongPressConfigured = false, // Only test one slot for simplicity ShortPressRequiresTouch = false, LongPressRequiresTouch = false, LedBehaviorInverted = false From 93d8d7b4e99d73d4c7301668711bc43e2453895b Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Thu, 31 Jul 2025 11:35:24 +0200 Subject: [PATCH 4/4] refactor(otp): Simplify command invocation and error handling in OtpErrorTransform --- .../YubiKey/Pipelines/OtpErrorTransform.cs | 27 +++++-------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs index abcfddb0f..8b416324a 100644 --- a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs +++ b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs @@ -41,14 +41,15 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT if (commandType == typeof(ReadStatusCommand) || responseType != typeof(ReadStatusResponse)) { - return InvokeCommand(command, commandType, responseType); + return _nextTransform.Invoke(command, commandType, responseType); } // Otherwise we assume this to be a command that applies a config (and therefore looks for a status response). // In order to detect failures, we grab the status structure before applying said command so that we have a // sequence number to compare to. var beforeStatus = GetCurrentStatus(); - var responseApdu = InvokeCommand(command, commandType, responseType, out var afterStatus); + var responseApdu = _nextTransform.Invoke(command, commandType, responseType); + var afterStatus = ReadStatus(responseApdu); try { @@ -62,7 +63,7 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT { _logger.LogWarning(e, "Handling keyboard connection exception. Translating to APDU response."); - return CreateFailedApdu([]); + return CreateFailedApdu(); } } @@ -83,20 +84,6 @@ internal static bool IsValidSequenceProgression(OtpStatus beforeStatus, OtpStatu return normalIncrement || validReset; } - private ResponseApdu InvokeCommand( - CommandApdu commandApdu, - Type commandType, - Type responseType, - out OtpStatus afterStatus) - { - var responseApdu = _nextTransform.Invoke(commandApdu, commandType, responseType); - afterStatus = ReadStatus(responseApdu); - return responseApdu; - } - - private ResponseApdu InvokeCommand(CommandApdu commandApdu, Type commandType, Type responseType) => - _nextTransform.Invoke(commandApdu, commandType, responseType); - private OtpStatus GetCurrentStatus() { var command = new ReadStatusCommand(); @@ -105,9 +92,7 @@ private OtpStatus GetCurrentStatus() typeof(ReadStatusCommand), typeof(ReadStatusResponse)); - return command - .CreateResponseForApdu(responseApdu) - .GetData(); + return ReadStatus(responseApdu); } private static OtpStatus ReadStatus(ResponseApdu responseApdu) @@ -117,6 +102,6 @@ private static OtpStatus ReadStatus(ResponseApdu responseApdu) return afterStatus; } - private static ResponseApdu CreateFailedApdu(byte[] data) => new(data, SWConstants.WarningNvmUnchanged); + private static ResponseApdu CreateFailedApdu(byte[]? data = null) => new(data ?? [], SWConstants.WarningNvmUnchanged); } }