diff --git a/AGENTS.md b/AGENTS.md index 7ccdac324..1619cdbbe 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,7 @@ - Async byte data crossing awaits should use `Memory`, `ReadOnlyMemory`, or `IMemoryOwner`, not `Span`. - Avoid `.ToArray()` and LINQ on byte data unless data must escape the current scope. - Always zero PINs, PUKs, keys, SCP material, and secret-derived buffers with `CryptographicOperations.ZeroMemory()`. +- Classify YubiKey-returned data by semantics: device metadata/status does not need special zeroing; authentication material, token material, decrypted plaintext, and secret-derived output does. - Use `CryptographicOperations.FixedTimeEquals` for secret-derived comparisons; do not use `SequenceEqual` on secrets. - Never store a privately cloned sensitive `byte[]` in a struct; struct copies keep references you cannot reliably zero. Use a sealed disposable class for owned sensitive buffers. - Logging is static via `YubiKitLogging.CreateLogger()`; do not add SDK constructors that require `ILogger`/`ILoggerFactory`. diff --git a/CLAUDE.md b/CLAUDE.md index 1b8de5374..2e047e7e4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -56,6 +56,7 @@ These are the always-loaded mandates. Each section ends with a JIT pointer to de **Security:** - ✅ ALWAYS zero sensitive data: `CryptographicOperations.ZeroMemory()` +- ✅ Classify byte data by semantic meaning, not by direction. YubiKey-returned device metadata/status (device info TLVs, firmware version, capabilities, form factor, public status) does not require special zeroing beyond normal ownership/disposal. YubiKey-returned authentication material, token material, decrypted plaintext, or secret-derived output does. - ✅ ALWAYS dispose crypto objects: `using var aes = Aes.Create()` - ✅ ALWAYS use `CryptographicOperations.FixedTimeEquals` for secret-derived comparisons - ❌ NEVER log PINs, keys, or sensitive payloads diff --git a/src/Core/src/Devices/DeviceInfoReader.cs b/src/Core/src/Devices/DeviceInfoReader.cs index 6cefde6f0..2893c04d7 100644 --- a/src/Core/src/Devices/DeviceInfoReader.cs +++ b/src/Core/src/Devices/DeviceInfoReader.cs @@ -61,22 +61,47 @@ public static async Task ReadAsync( byte page = 0; var allPagesTlvs = new List(); - var hasMoreData = true; - while (hasMoreData) + var remainingPages = 1; + while (remainingPages > 0) { + --remainingPages; var pageTlvs = await ReadPageAsync(protocol, page, cancellationToken).ConfigureAwait(false); - allPagesTlvs.AddRange(pageTlvs); - var moreData = pageTlvs.SingleOrDefault(t => t.Tag == TagMoreDeviceInfo); - if (moreData is null) - break; + Tlv? moreData = null; + foreach (var tlv in pageTlvs) + { + if (tlv.Tag != TagMoreDeviceInfo) + { + continue; + } + + if (moreData is not null) + { + throw new BadResponseException($"Duplicate more-data tags in device info page {page}."); + } + + moreData = tlv; + } + + if (moreData is not null && moreData.Length > 0) + { + remainingPages = moreData.Value.Span[^1]; + } + + foreach (var tlv in pageTlvs) + { + if (tlv.Tag == TagMoreDeviceInfo) + { + continue; + } + + allPagesTlvs.Add(tlv); + } - hasMoreData = moreData.Length == 1 && moreData.Value.Span[0] == 1; ++page; } - using var allTlvs = new DisposableTlvList(allPagesTlvs); - return DeviceInfo.CreateFromTlvs([.. allTlvs], defaultVersion); + return DeviceInfo.CreateFromTlvs(allPagesTlvs, defaultVersion); } private static async Task ReadPageAsync( diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Devices/DeviceInfoReaderTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Devices/DeviceInfoReaderTests.cs index fa645b888..3250c3fc1 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Devices/DeviceInfoReaderTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Devices/DeviceInfoReaderTests.cs @@ -20,6 +20,7 @@ public async Task ReadAsync_SmartCardSinglePage_ParsesDeviceInfo() Assert.Equal(0x01020304, info.SerialNumber); Assert.Equal(new FirmwareVersion(5, 7, 2), info.FirmwareVersion); + Assert.Equal([0x2A], info.ChallengeResponseTimeout.ToArray()); Assert.Equal([0], protocol.RequestedPages); } @@ -37,6 +38,50 @@ public async Task ReadAsync_SmartCardMoreData_ReadsNextPage() Assert.Equal(new FirmwareVersion(5, 7, 2), info.FirmwareVersion); } + [Fact] + public async Task ReadAsync_SmartCardMoreDataCount_ReadsRemainingPages() + { + var protocol = new FakeSmartCardProtocol( + BuildPage(new Tlv(0x10, [0x02])), + BuildPage(new Tlv(0x10, [0x01])), + BuildPage(CreateRequiredDeviceInfoTlvs())); + + var info = await DeviceInfoReader.ReadAsync(protocol, null, TestContext.Current.CancellationToken); + + Assert.Equal([0, 1, 2], protocol.RequestedPages); + Assert.Equal(0x01020304, info.SerialNumber); + Assert.Equal(new FirmwareVersion(5, 7, 2), info.FirmwareVersion); + } + + [Fact] + public async Task ReadAsync_SmartCardMoreDataCountOnFirstPage_ReadsRemainingPagesWithoutRepeatedMoreDataTag() + { + var protocol = new FakeSmartCardProtocol( + BuildPage(new Tlv(0x10, [0x02])), + BuildPage(new Tlv(0x1F, [0x00])), + BuildPage(CreateRequiredDeviceInfoTlvs())); + + var info = await DeviceInfoReader.ReadAsync(protocol, null, TestContext.Current.CancellationToken); + + Assert.Equal([0, 1, 2], protocol.RequestedPages); + Assert.Equal(0x01020304, info.SerialNumber); + Assert.Equal(new FirmwareVersion(5, 7, 2), info.FirmwareVersion); + } + + [Fact] + public async Task ReadAsync_SmartCardMoreDataCountZero_StopsAfterCurrentPage() + { + var protocol = new FakeSmartCardProtocol( + BuildPage([.. CreateRequiredDeviceInfoTlvs(), new Tlv(0x10, [0x00])]), + BuildPage(new Tlv(0x0A, [0x00]))); + + var info = await DeviceInfoReader.ReadAsync(protocol, null, TestContext.Current.CancellationToken); + + Assert.Equal([0], protocol.RequestedPages); + Assert.Equal(0x01020304, info.SerialNumber); + Assert.Equal(new FirmwareVersion(5, 7, 2), info.FirmwareVersion); + } + [Fact] public async Task ReadAsync_SmartCardInvalidPageLength_ThrowsPageAwareBadResponse() { @@ -50,6 +95,33 @@ public async Task ReadAsync_SmartCardInvalidPageLength_ThrowsPageAwareBadRespons Assert.Contains("actual 1", ex.Message, StringComparison.OrdinalIgnoreCase); } + [Fact] + public async Task ReadAsync_SmartCardDuplicateMoreDataTags_ThrowsPageAwareBadResponse() + { + var protocol = new FakeSmartCardProtocol(BuildPage(new Tlv(0x10, [0x01]), new Tlv(0x10, [0x01]))); + + var ex = await Assert.ThrowsAsync( + () => DeviceInfoReader.ReadAsync(protocol, null, TestContext.Current.CancellationToken)); + + Assert.Contains("Duplicate", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("page 0", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public async Task ReadAsync_SmartCardDuplicateMoreDataTagsAfterAccumulatingTlvs_ThrowsPageAwareBadResponse() + { + var protocol = new FakeSmartCardProtocol( + BuildPage(new Tlv(0x1F, [0x00]), new Tlv(0x10, [0x01])), + BuildPage(new Tlv(0x10, [0x01]), new Tlv(0x10, [0x01]))); + + var ex = await Assert.ThrowsAsync( + () => DeviceInfoReader.ReadAsync(protocol, null, TestContext.Current.CancellationToken)); + + Assert.Equal([0, 1], protocol.RequestedPages); + Assert.Contains("Duplicate", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("page 1", ex.Message, StringComparison.OrdinalIgnoreCase); + } + [Fact] public async Task ReadAsync_FidoSinglePage_ParsesDeviceInfo() { @@ -62,6 +134,21 @@ public async Task ReadAsync_FidoSinglePage_ParsesDeviceInfo() Assert.Equal([0], protocol.RequestedPages); } + [Fact] + public async Task ReadAsync_FidoMoreDataCount_ReadsRemainingPages() + { + var protocol = new FakeFidoHidProtocol( + BuildPage(new Tlv(0x10, [0x02])), + BuildPage(new Tlv(0x1F, [0x00])), + BuildPage(CreateRequiredDeviceInfoTlvs())); + + var info = await DeviceInfoReader.ReadAsync(protocol, null, TestContext.Current.CancellationToken); + + Assert.Equal([0, 1, 2], protocol.RequestedPages); + Assert.Equal(0x01020304, info.SerialNumber); + Assert.Equal(new FirmwareVersion(5, 7, 2), info.FirmwareVersion); + } + [Fact] public async Task ReadAsync_OtpSinglePage_ParsesDeviceInfo() { @@ -72,6 +159,22 @@ public async Task ReadAsync_OtpSinglePage_ParsesDeviceInfo() Assert.Equal(0x01020304, info.SerialNumber); Assert.Equal(new FirmwareVersion(5, 7, 2), info.FirmwareVersion); + Assert.Equal([0], protocol.RequestedPages); + } + + [Fact] + public async Task ReadAsync_OtpMoreDataCount_ReadsRemainingPages() + { + var protocol = new FakeOtpHidProtocol( + BuildOtpFrame(BuildPage(new Tlv(0x10, [0x02]))), + BuildOtpFrame(BuildPage(new Tlv(0x1F, [0x00]))), + BuildOtpFrame(BuildPage(CreateRequiredDeviceInfoTlvs()))); + + var info = await DeviceInfoReader.ReadAsync(protocol, null, TestContext.Current.CancellationToken); + + Assert.Equal([0, 1, 2], protocol.RequestedPages); + Assert.Equal(0x01020304, info.SerialNumber); + Assert.Equal(new FirmwareVersion(5, 7, 2), info.FirmwareVersion); } [Fact] @@ -147,7 +250,7 @@ private static Tlv[] CreateRequiredDeviceInfoTlvs() => new(0x14, [0x00]), new(0x15, [0x00]), new(0x06, [0x00, 0x00]), - new(0x07, [0x00]), + new(0x07, [0x2A]), new(0x08, [0x00]), new(0x05, [0x05, 0x07, 0x02]), new(0x02, [0x01, 0x02, 0x03, 0x04]) @@ -217,13 +320,18 @@ private sealed class FakeOtpHidProtocol(params byte[][] frames) : IOtpHidProtoco { private readonly Queue _frames = new(frames); + public List RequestedPages { get; } = []; + public FirmwareVersion? FirmwareVersion => null; public Task> SendAndReceiveAsync( byte slot, ReadOnlyMemory data, - CancellationToken cancellationToken = default) => - Task.FromResult>(_frames.Dequeue()); + CancellationToken cancellationToken = default) + { + RequestedPages.Add(data.Length == 0 ? (byte)0 : data.Span[0]); + return Task.FromResult>(_frames.Dequeue()); + } public Task> ReadStatusAsync(CancellationToken cancellationToken = default) => throw new NotSupportedException();