fix: Stop send short APDUs when exceeded max APDU size

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

@@ -1,4 +1,4 @@
-// Copyright 2021 Yubico AB
+// Copyright 2021 Yubico AB
 //
 // Licensed under the Apache License, Version 2.0 (the "License").
 // You may not use this file except in compliance with the License.
@@ -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;
 
@@ -33,6 +38,7 @@ public CommandChainingTransform(IApduTransform pipeline)
         }
 
         public void Cleanup() => _pipeline.Cleanup();
+        public void Setup() => _pipeline.Setup();
 
         public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseType)
         {
@@ -41,35 +47,59 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT
                 throw new ArgumentNullException(nameof(command));
             }
 
-            if (command.Data.IsEmpty || command.Data.Length <= MaxSize)
+            // Send single short APDU
+            int commandDataSize = command.Data.Length;
+            if (commandDataSize <= MaxChunkSize)
             {
+                _log.LogDebug("Sending short APDU");
                 return _pipeline.Invoke(command, commandType, responseType);
             }
 
-            var sourceData = command.Data;
-            ResponseApdu? responseApdu = null;
+            // Send a series of short APDU's
+            _log.LogDebug("APDU size exceeds size of short APDU, proceeding to send data in chunks instead");
+            return SendChainedApdu(command, commandType, responseType);
+        }
 
+        private ResponseApdu SendChainedApdu(CommandApdu command, Type commandType, Type responseType)
+        {
+            ResponseApdu? responseApdu = null;
+            var sourceData = command.Data;
             while (!sourceData.IsEmpty)
             {
-                int length = Math.Min(MaxSize, sourceData.Length);
-                var data = sourceData.Slice(0, length);
-                sourceData = sourceData.Slice(length);
-
-                var partialApdu = new CommandApdu
+                responseApdu = SendPartial(command, commandType, responseType, ref sourceData);
+                if (responseApdu.SW != SWConstants.Success)
                 {
-                    Cla = (byte)(command.Cla | (sourceData.IsEmpty ? 0 : 0x10)),
-                    Ins = command.Ins,
-                    P1 = command.P1,
-                    P2 = command.P2,
-                    Data = data
-                };
-
-                responseApdu = _pipeline.Invoke(partialApdu, commandType, responseType);
+                    _log.LogWarning("Received error response from YubiKey. (SW: 0x{StatusWord})", responseApdu.SW.ToString("X4", CultureInfo.CurrentCulture));
+                    return responseApdu;
+                }
             }
 
-            return responseApdu!; // Covered by Debug.Assert above.
+            return responseApdu!;
         }
 
-        public void Setup() => _pipeline.Setup();
+        private ResponseApdu SendPartial(
+            CommandApdu command,
+            Type commandType,
+            Type responseType,
+            ref ReadOnlyMemory<byte> sourceData)
+        {
+            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)),
+                Ins = command.Ins,
+                P1 = command.P1,
+                P2 = command.P2,
+                Data = dataChunk
+            };
+
+            var responseApdu = _pipeline.Invoke(partialApdu, commandType, responseType);
+            return responseApdu;
+        }
     }
 }

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

@@ -35,7 +35,7 @@ internal class SmartCardConnection : IYubiKeyConnection
         private readonly ISmartCardConnection _smartCardConnection;
         private IApduTransform _apduPipeline;
         private bool _disposedValue;
-
+        private bool IsOath => GetIsOauth();
         public ISelectApplicationData? SelectApplicationData { get; set; }
 
         /// <summary>
@@ -69,10 +69,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 +108,36 @@ protected SmartCardConnection(
 
             _smartCardConnection = smartCardDevice.Connect();
 
+            // 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 +155,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 +199,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 GetIsOauth() =>
+            _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;
+        }
+    }
+}