fix: no longer sending chained data to key after receiving sw 0x67

Author: DennisDyallo

Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/CommandChainingTransform.cs

@@ -13,7 +13,10 @@
 // limitations under the License.
 
 using System;
+using System.Globalization;
+using Microsoft.Extensions.Logging;
 using Yubico.Core.Iso7816;
+using Yubico.Core.Logging;
 
 namespace Yubico.YubiKey.Pipelines
 {
@@ -23,7 +26,9 @@ namespace Yubico.YubiKey.Pipelines
     /// </summary>
     internal class CommandChainingTransform : IApduTransform
     {
-        public int MaxSize { get; internal set; } = 255;
+        private readonly ILogger _log = Log.GetLogger<CommandChainingTransform>();
+
+        public int MaxChunkSize { get; internal set; } = 255;
 
         readonly IApduTransform _pipeline;
 
@@ -41,33 +46,51 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT
                 throw new ArgumentNullException(nameof(command));
             }
 
-            if (command.Data.IsEmpty || command.Data.Length <= MaxSize)
+            // Send regular short APDU
+            int commandDataSize = command.Data.Length;
+            if (commandDataSize <= MaxChunkSize)
             {
+                _log.LogDebug("Sending short APDU");
                 return _pipeline.Invoke(command, commandType, responseType);
             }
 
+            // Send chained short APDU
             var sourceData = command.Data;
             ResponseApdu? responseApdu = null;
-
+            _log.LogDebug("APDU size exceeds size of short APDU, proceeding to send data in chunks instead");
             while (!sourceData.IsEmpty)
             {
-                int length = Math.Min(MaxSize, sourceData.Length);
-                var data = sourceData.Slice(0, length);
-                sourceData = sourceData.Slice(length);
+                int chunkLength = Math.Min(MaxChunkSize, sourceData.Length);
+                var dataChunk = sourceData[..chunkLength];
+                sourceData = sourceData[chunkLength..];
 
                 var partialApdu = new CommandApdu
                 {
-                    Cla = (byte)(command.Cla | (sourceData.IsEmpty ? 0 : 0x10)),
+                    Cla = (byte)(command.Cla | (sourceData.IsEmpty
+                        ? 0
+                        : 0x10)),
                     Ins = command.Ins,
                     P1 = command.P1,
                     P2 = command.P2,
-                    Data = data
+                    Data = dataChunk
                 };
 
                 responseApdu = _pipeline.Invoke(partialApdu, commandType, responseType);
+
+                // Stop sending data when the YubiKey response is 0x67 (when the chained data
+                // sent exceeds the max allowed length) and let caller handle the response
+                if (responseApdu.SW != SWConstants.WrongLength)
+                {
+                    continue;
+                }
+
+                _log.LogWarning("Sent data exceeds max allowed length by YubiKey. (SW: 0x{StatusWord})",
+                    responseApdu.SW.ToString("X4", CultureInfo.InvariantCulture));
+                
+                return responseApdu;
             }
 
-            return responseApdu!; // Covered by Debug.Assert above.
+            return responseApdu!;
         }
 
         public void Setup() => _pipeline.Setup();

Yubico.YubiKey/src/Yubico/YubiKey/SmartCardConnection.cs

@@ -34,8 +34,9 @@ internal class SmartCardConnection : IYubiKeyConnection
         private readonly byte[]? _applicationId;
         private readonly ISmartCardConnection _smartCardConnection;
         private IApduTransform _apduPipeline;
-        private bool _disposedValue;
 
+        private bool _disposedValue;
+        private bool IsOath => GetIsAuth();
         public ISelectApplicationData? SelectApplicationData { get; set; }
 
         /// <summary>
@@ -47,7 +48,7 @@ internal class SmartCardConnection : IYubiKeyConnection
         public SmartCardConnection(
             ISmartCardDevice smartCardDevice,
             YubiKeyApplication yubiKeyApplication)
-            : this(smartCardDevice, yubiKeyApplication, null)
+            : this(smartCardDevice, yubiKeyApplication, (byte[]?)null)
         {
             if (yubiKeyApplication == YubiKeyApplication.Fido2)
             {
@@ -69,10 +70,7 @@ public SmartCardConnection(
         public SmartCardConnection(
             ISmartCardDevice smartCardDevice,
             byte[] applicationId)
-            : this(
-                smartCardDevice,
-                YubiKeyApplication.Unknown,
-                applicationId)
+            : this(smartCardDevice, YubiKeyApplication.Unknown, applicationId)
         {
             if (applicationId.SequenceEqual(YubiKeyApplication.Fido2.GetIso7816ApplicationId()))
             {
@@ -111,11 +109,36 @@ protected SmartCardConnection(
 
             _smartCardConnection = smartCardDevice.Connect();
 
-            _apduPipeline = new SmartCardTransform(_smartCardConnection);
+            // Set up the pipeline
+            _apduPipeline =new SmartCardTransform(_smartCardConnection);
             _apduPipeline = AddResponseChainingTransform(_apduPipeline);
             _apduPipeline = new CommandChainingTransform(_apduPipeline);
         }
 
+        public virtual TResponse SendCommand<TResponse>(IYubiKeyCommand<TResponse> yubiKeyCommand)
+            where TResponse : IYubiKeyResponse
+        {
+            using var _ = _smartCardConnection.BeginTransaction(out bool cardWasReset);
+            if (cardWasReset)
+            {
+                SelectApplication();
+            }
+
+            var responseApdu = _apduPipeline.Invoke(
+                yubiKeyCommand.CreateCommandApdu(),
+                yubiKeyCommand.GetType(),
+                typeof(TResponse));
+
+            return yubiKeyCommand.CreateResponseForApdu(responseApdu);
+        }
+        
+        public void Dispose()
+        {
+            // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
+            Dispose(disposing: true);
+            GC.SuppressFinalize(this);
+        }
+
         // Allow subclasses to build a different pipeline, which means they need
         // to get the current one.
         protected IApduTransform GetPipeline() => _apduPipeline;
@@ -133,12 +156,19 @@ protected void SetPipeline(IApduTransform apduPipeline)
             SelectApplication();
         }
 
-        // The application is set to Oath by enum or by application id
-        private bool IsOath =>
-            _yubiKeyApplication == YubiKeyApplication.Oath ||
-            (_applicationId != null &&
-            _applicationId.SequenceEqual(
-                YubiKeyApplication.Oath.GetIso7816ApplicationId()));
+        protected virtual void Dispose(bool disposing)
+        {
+            if (!_disposedValue)
+            {
+                if (disposing)
+                {
+                    _apduPipeline.Cleanup();
+                    _smartCardConnection.Dispose();
+                }
+
+                _disposedValue = true;
+            }
+        }
 
         private IApduTransform AddResponseChainingTransform(IApduTransform pipeline) =>
             IsOath
@@ -170,55 +200,20 @@ private void SelectApplication()
                         CultureInfo.CurrentCulture,
                         ExceptionMessages.SmartCardPipelineSetupFailed,
                         responseApdu.SW))
-                {
-                    SW = responseApdu.SW
-                };
+                    {
+                        SW = responseApdu.SW
+                    };
             }
 
             // Set the instance property SelectApplicationData
             var response = selectApplicationCommand.CreateResponseForApdu(responseApdu);
             SelectApplicationData = response.GetData();
         }
 
-        public virtual TResponse SendCommand<TResponse>(IYubiKeyCommand<TResponse> yubiKeyCommand)
-            where TResponse : IYubiKeyResponse
-        {
-            using (var _ = _smartCardConnection.BeginTransaction(out bool cardWasReset))
-            {
-                if (cardWasReset)
-                {
-                    SelectApplication();
-                }
-
-                var responseApdu = _apduPipeline.Invoke(
-                    yubiKeyCommand.CreateCommandApdu(),
-                    yubiKeyCommand.GetType(),
-                    typeof(TResponse)
-                    );
-
-                return yubiKeyCommand.CreateResponseForApdu(responseApdu);
-            }
-        }
-
-        protected virtual void Dispose(bool disposing)
-        {
-            if (!_disposedValue)
-            {
-                if (disposing)
-                {
-                    _apduPipeline.Cleanup();
-                    _smartCardConnection.Dispose();
-                }
-
-                _disposedValue = true;
-            }
-        }
-
-        public void Dispose()
-        {
-            // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
-            Dispose(disposing: true);
-            GC.SuppressFinalize(this);
-        }
+        private bool GetIsAuth() =>
+            _yubiKeyApplication == YubiKeyApplication.Oath ||
+            (_applicationId != null &&
+            _applicationId.SequenceEqual(
+                YubiKeyApplication.Oath.GetIso7816ApplicationId()));
     }
 }

Yubico.YubiKey/src/Yubico/YubiKey/SmartCardMaxApduSizes.cs

@@ -0,0 +1,38 @@
+// Copyright 2024 Yubico AB
+// 
+// Licensed under the Apache License, Version 2.0 (the "License").
+// You may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+// 
+//     http://www.apache.org/licenses/LICENSE-2.0
+// 
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+namespace Yubico.YubiKey;
+
+#pragma warning disable CA1707 // Allow underscore in constant
+/// <summary>
+/// This contains the maximum size (in bytes) of APDU commands for the various YubiKey models.
+/// </summary>
+public static class SmartCardMaxApduSizes
+{
+    /// <summary>
+    /// The max APDU command size for the YubiKey NEO
+    /// </summary>
+    public const int NEO = 1390;
+    
+    /// <summary>
+    /// The max APDU command size for the YubiKey 4 and greater
+    /// </summary>
+    public const int YK4 = 2038;
+
+    /// <summary>
+    /// The max APDU command size for the YubiKey 4.3 and greater
+    /// </summary>
+    public const int YK4_3 = 3062;
+}
+#pragma warning restore CA1707

Yubico.YubiKey/tests/integration/Yubico/YubiKey/Piv/Commands/PutDataCommandTests.cs

@@ -0,0 +1,85 @@
+// Copyright 2024 Yubico AB
+// 
+// Licensed under the Apache License, Version 2.0 (the "License").
+// You may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+// 
+//     http://www.apache.org/licenses/LICENSE-2.0
+// 
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using Xunit;
+using Yubico.Core.Iso7816;
+using Yubico.Core.Tlv;
+using Yubico.YubiKey.TestUtilities;
+
+namespace Yubico.YubiKey.Piv.Commands;
+
+public class PutDataCommandTests
+{
+    [Theory]
+    [InlineData(StandardTestDevice.Fw5)]
+    public void SendCommand_With_TooLargeApdu_ReturnsResultFailed(
+        StandardTestDevice testDeviceType)
+    {
+        using var pivSession = GetSession(testDeviceType);
+        
+        var tooLargeTlv = new TlvObject(0x53, new byte[10000]);
+        var tlvBytes = tooLargeTlv.GetBytes();
+        var command = new PutDataCommand(0x5F0000, tlvBytes);
+
+        var response = pivSession.Connection.SendCommand(command);
+
+        Assert.Equal(ResponseStatus.Failed, response.Status);
+        Assert.Equal(SWConstants.WrongLength, response.StatusWord);
+
+        // Cleanup
+        pivSession.ResetApplication();
+    }
+
+    [Theory]
+    [InlineData(StandardTestDevice.Fw5)]
+    public void SendCommand_with_ValidSizeApdu_ReturnsResultSuccess(
+        StandardTestDevice testDeviceType)
+    {
+        // Arrange
+        using var pivSession = GetSession(testDeviceType);
+
+        var validSizeTlv = new TlvObject(0x53, new byte[SmartCardMaxApduSizes.YK4_3]);
+        var tlvBytes = validSizeTlv.GetBytes();
+        var command = new PutDataCommand(0x5F0000, tlvBytes);
+        
+        // Act
+        var response = pivSession.Connection.SendCommand(command);
+        var actualSize = command.CreateCommandApdu().AsByteArray().Length;
+        Assert.Equal(3078, actualSize); // This is the current max APDU size of the YubiKey 5 series.
+        Assert.Equal(ResponseStatus.Success, response.Status);
+        Assert.Equal(SWConstants.Success, response.StatusWord);
+        
+        // Cleanup
+        pivSession.ResetApplication();
+    }
+
+    private static PivSession GetSession(StandardTestDevice testDeviceType)
+    {
+        PivSession? pivSession = null;
+        try
+        {
+            var testDevice = IntegrationTestDeviceEnumeration.GetTestDevice(testDeviceType);
+            pivSession = new PivSession(testDevice);
+            var collectorObj = new Simple39KeyCollector();
+            pivSession.KeyCollector = collectorObj.Simple39KeyCollectorDelegate;
+            pivSession.AuthenticateManagementKey();
+            return pivSession;
+        }
+        catch
+        {
+            pivSession?.Dispose();
+            throw;
+        }
+    }
+}

Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/CommandChainingTransformTests.cs

@@ -148,7 +148,7 @@ public void Invoke_CommandApduWithLargeDataBuffer_OrsHex10ToClaOnAllExceptLast()
 
             // Arrange
             var mockTransform = new Mock<IApduTransform>();
-            var transform = new CommandChainingTransform(mockTransform.Object) { MaxSize = 4 };
+            var transform = new CommandChainingTransform(mockTransform.Object) { MaxChunkSize = 4 };
             var commandApdu = new CommandApdu { Data = Enumerable.Repeat<byte>(0xFF, 16).ToArray() };
 
             _ = mockTransform
@@ -169,7 +169,7 @@ public void Invoke_CommandApduWithLargeDataBuffer_AllOtherApduPropertiesRemainUn
 
             // Arrange
             var mockTransform = new Mock<IApduTransform>();
-            var transform = new CommandChainingTransform(mockTransform.Object) { MaxSize = 4 };
+            var transform = new CommandChainingTransform(mockTransform.Object) { MaxChunkSize = 4 };
             var commandApdu = new CommandApdu
             {
                 Ins = 1,
@@ -201,7 +201,7 @@ public void Invoke_CommandApduWithLargeDataBuffer_SplitsDataAcrossInvokeCalls()
 
             // Arrange
             var mockTransform = new Mock<IApduTransform>();
-            var transform = new CommandChainingTransform(mockTransform.Object) { MaxSize = 4 };
+            var transform = new CommandChainingTransform(mockTransform.Object) { MaxChunkSize = 4 };
             var commandApdu = new CommandApdu
             {
                 Data = new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }