fix(core): validate ctaphid keepalive framing

Author: DennisDyallo

src/Core/src/Protocols/Fido/Hid/FidoHidProtocol.cs

@@ -249,6 +249,7 @@ private async Task<ReadOnlyMemory<byte>> ReceiveResponse(
         var initPacket = await _connection.ReceiveAsync(cancellationToken).ConfigureAwait(false);
         while (IsKeepAlivePacket(initPacket.Span))
         {
+            ValidateInitPacket(initPacket.Span, channelId);
             _logger.LogTrace("Received keep-alive, waiting for response");
             initPacket = await _connection.ReceiveAsync(cancellationToken).ConfigureAwait(false);
         }
@@ -263,16 +264,11 @@ private async Task<ReadOnlyMemory<byte>> ReceiveResponse(
         var responseData = new byte[responseLength];
         var initDataLength = Math.Min(responseLength, CtapConstants.InitDataSize);
 
-        // Ensure we don't try to read more data than the packet contains
-        var availableDataInPacket = Math.Min(initDataLength, initPacket.Length - CtapConstants.InitHeaderSize);
-        if (availableDataInPacket < 0)
-            availableDataInPacket = 0;
-
-        initPacket.Span.Slice(CtapConstants.InitHeaderSize, availableDataInPacket)
+        initPacket.Span.Slice(CtapConstants.InitHeaderSize, initDataLength)
             .CopyTo(responseData);
 
         // Receive continuation packets if needed
-        var bytesReceived = availableDataInPacket;
+        var bytesReceived = initDataLength;
         byte expectedSequence = 0;
         while (bytesReceived < responseLength)
         {
@@ -282,15 +278,10 @@ private async Task<ReadOnlyMemory<byte>> ReceiveResponse(
                 responseLength - bytesReceived,
                 CtapConstants.ContinuationDataSize);
 
-            // Ensure we don't try to read more data than the packet contains
-            var availableContData = Math.Min(contDataLength, contPacket.Length - CtapConstants.ContinuationHeaderSize);
-            if (availableContData < 0)
-                availableContData = 0;
-
-            contPacket.Span.Slice(CtapConstants.ContinuationHeaderSize, availableContData)
+            contPacket.Span.Slice(CtapConstants.ContinuationHeaderSize, contDataLength)
                 .CopyTo(responseData.AsSpan(bytesReceived));
 
-            bytesReceived += availableContData;
+            bytesReceived += contDataLength;
             expectedSequence++;
         }
 
@@ -355,8 +346,8 @@ private static bool IsKeepAlivePacket(ReadOnlySpan<byte> packet) =>
 
     private static void ValidateInitPacket(ReadOnlySpan<byte> packet, uint channelId)
     {
-        if (packet.Length < CtapConstants.InitHeaderSize)
-            throw new InvalidOperationException("CTAP HID init packet is too short");
+        if (packet.Length != CtapConstants.PacketSize)
+            throw new InvalidOperationException("CTAP HID init packet must be exactly 64 bytes");
 
         var packetChannelId = BinaryPrimitives.ReadUInt32BigEndian(packet);
         if (packetChannelId != channelId)
@@ -368,8 +359,8 @@ private static void ValidateInitPacket(ReadOnlySpan<byte> packet, uint channelId
 
     private static void ValidateContinuationPacket(ReadOnlySpan<byte> packet, uint channelId, byte expectedSequence)
     {
-        if (packet.Length < CtapConstants.ContinuationHeaderSize)
-            throw new InvalidOperationException("CTAP HID continuation packet is too short");
+        if (packet.Length != CtapConstants.PacketSize)
+            throw new InvalidOperationException("CTAP HID continuation packet must be exactly 64 bytes");
 
         var packetChannelId = BinaryPrimitives.ReadUInt32BigEndian(packet);
         if (packetChannelId != channelId)
@@ -440,4 +431,4 @@ public void Dispose()
         _connection.Dispose();
         _disposed = true;
     }
-}
+}

src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Fido/Hid/FidoHidProtocolTests.cs

@@ -98,7 +98,58 @@ public async Task SendVendorCommandAsync_ResponseWithShortInitPacket_ThrowsInval
         var ex = await Assert.ThrowsAsync<InvalidOperationException>(() =>
             protocol.SendVendorCommandAsync(CtapConstants.CtapVendorFirst, ReadOnlyMemory<byte>.Empty, TestContext.Current.CancellationToken));
 
-        Assert.Contains("too short", ex.Message, StringComparison.OrdinalIgnoreCase);
+        Assert.Contains("exactly", ex.Message, StringComparison.OrdinalIgnoreCase);
+    }
+
+    [Fact]
+    public async Task SendVendorCommandAsync_ResponseWithPartialInitPacket_ThrowsInvalidOperationException()
+    {
+        var connection = new FakeFidoHidConnection();
+        var protocol = new FidoHidProtocol(connection);
+
+        var packet = CreateInitPacket(0x01020304, CtapConstants.CtapVendorFirst, [0xAA]);
+        connection.QueueResponsePackets(packet[..CtapConstants.InitHeaderSize]);
+
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() =>
+            protocol.SendVendorCommandAsync(CtapConstants.CtapVendorFirst, ReadOnlyMemory<byte>.Empty, TestContext.Current.CancellationToken));
+
+        Assert.Contains("exactly", ex.Message, StringComparison.OrdinalIgnoreCase);
+    }
+
+    [Fact]
+    public async Task SendVendorCommandAsync_ResponseWithPartialContinuationPacket_ThrowsInvalidOperationException()
+    {
+        var connection = new FakeFidoHidConnection();
+        var protocol = new FidoHidProtocol(connection);
+
+        var responsePayload = Enumerable.Range(0, CtapConstants.InitDataSize + 1)
+            .Select(i => (byte)i)
+            .ToArray();
+        var continuation = CreateContinuationPacket(0x01020304, sequence: 0, responsePayload.AsSpan(CtapConstants.InitDataSize));
+        connection.QueueResponsePackets(
+            CreateInitPacket(0x01020304, CtapConstants.CtapVendorFirst, responsePayload),
+            continuation[..CtapConstants.ContinuationHeaderSize]);
+
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() =>
+            protocol.SendVendorCommandAsync(CtapConstants.CtapVendorFirst, ReadOnlyMemory<byte>.Empty, TestContext.Current.CancellationToken));
+
+        Assert.Contains("exactly", ex.Message, StringComparison.OrdinalIgnoreCase);
+    }
+
+    [Fact]
+    public async Task SendVendorCommandAsync_ResponseWithWrongChannelKeepAlive_ThrowsInvalidOperationException()
+    {
+        var connection = new FakeFidoHidConnection();
+        var protocol = new FidoHidProtocol(connection);
+
+        connection.QueueResponsePackets(
+            CreateInitPacket(0x05060708, CtapConstants.CtapHidKeepAlive, [0x01]),
+            CreateInitPacket(0x01020304, CtapConstants.CtapVendorFirst, [0xAA]));
+
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() =>
+            protocol.SendVendorCommandAsync(CtapConstants.CtapVendorFirst, ReadOnlyMemory<byte>.Empty, TestContext.Current.CancellationToken));
+
+        Assert.Contains("channel", ex.Message, StringComparison.OrdinalIgnoreCase);
     }
 
     [Fact]
@@ -244,4 +295,4 @@ private byte[] CreateInitResponse()
             return CreateInitPacket(CtapConstants.BroadcastChannelId, CtapConstants.CtapHidInit, payload);
         }
     }
-}
+}