diff --git a/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs b/Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs index 604b705e..8b416324 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 @@ -37,8 +38,8 @@ 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); } @@ -46,36 +47,61 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT // 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 = _nextTransform.Invoke(command, commandType, responseType); + var afterStatus = ReadStatus(responseApdu); try { - var responseApdu = _nextTransform.Invoke(command, commandType, responseType); - int afterSequence = new ReadStatusResponse(responseApdu).GetData().SequenceNumber; - int expectedSequence = (beforeSequence + 1) % 0x100; - - // 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; + // 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 + : CreateFailedApdu(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 CreateFailedApdu(); } } public void Setup() => _nextTransform.Setup(); public void Cleanup() => _nextTransform.Cleanup(); + + // Internal for testing + internal static bool IsValidSequenceProgression(OtpStatus beforeStatus, OtpStatus afterStatus) + { + 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; + } + + private OtpStatus GetCurrentStatus() + { + var command = new ReadStatusCommand(); + var responseApdu = _nextTransform.Invoke( + command.CreateCommandApdu(), + typeof(ReadStatusCommand), + typeof(ReadStatusResponse)); + + return ReadStatus(responseApdu); + } + + private static OtpStatus ReadStatus(ResponseApdu responseApdu) + { + var readStatusResponse = new ReadStatusResponse(responseApdu); + var afterStatus = readStatusResponse.GetData(); + return afterStatus; + } + + private static ResponseApdu CreateFailedApdu(byte[]? data = null) => 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 new file mode 100644 index 00000000..eb3cfa8b --- /dev/null +++ b/Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs @@ -0,0 +1,85 @@ +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 + public void IsValidSequenceProgression_NormalIncrement_ReturnsTrue( + byte before, + byte after) + { + 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 + [InlineData(255, 0)] // Byte overflow (should be rejected as normal increment) + public void IsValidSequenceProgression_InvalidIncrement_ReturnsFalse( + byte before, + byte after) + { + 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)] // Delete from sequence 1 + [InlineData(5)] // Delete from sequence 5 + [InlineData(255)] // Delete from sequence 255 + public void IsValidSequenceProgression_ValidReset_ReturnsTrue( + byte before) + { + 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, 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 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 sequenceNumber, + bool hasConfigs) => new() + { + FirmwareVersion = new FirmwareVersion(), + SequenceNumber = sequenceNumber, + TouchLevel = 0x00, + ShortPressConfigured = hasConfigs, + LongPressConfigured = false, // Only test one slot for simplicity + ShortPressRequiresTouch = false, + LongPressRequiresTouch = false, + LedBehaviorInverted = false + }; +}