fix: code audit fixes across all YubiKey applet modules#455
Open
DennisDyallo wants to merge 34 commits intoyubikit-appletsfrom
Open
fix: code audit fixes across all YubiKey applet modules#455DennisDyallo wants to merge 34 commits intoyubikit-appletsfrom
DennisDyallo wants to merge 34 commits intoyubikit-appletsfrom
Conversation
- Fix TLV length encoding for 128-255 byte certificates (missing 0x81 case) - Zero inputBuffer before ArrayPool return in DecryptBlock/EncryptBlock - Add bounds check in ParseVersionResponse for truncated responses - Zero ModPow intermediate bytes (BigInteger.ToByteArray result) - Fix connection leak in CreatePivSessionAsync on failure - Remove empty CheckAlgorithmSupport method - Delete placeholder tests Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Replace legacy SHA256.Create().ComputeHash() with static SHA256.HashData() - Zero expected auth tag after FixedTimeEquals in PinUvAuthProtocol V1/V2 - Remove pointless async/await Task.CompletedTask in DisposeAsync - Remove overly broad catch(Exception) in LargeBlobEntry.TryDecrypt - Remove dead MutableTemplateInfo and TemplateInfoExtensions file classes - Modernize null checks to ArgumentNullException.ThrowIfNull() - Delete placeholder tests Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Zero data buffer containing secret in PutCredentialAsync finally block
- Replace FixedTimeEquals with SequenceEqual for non-secret credential IDs
- Eliminate string allocations in CompareTo via StringComparison.OrdinalIgnoreCase
- Convert TotpIdPattern to [GeneratedRegex] for AOT-friendly source generation
- Add CultureInfo.InvariantCulture to all int.Parse calls
- Use StringComparison.Ordinal for Contains(':') check
- Delete placeholder tests
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Fix connection leak in CreateYubiOtpSessionAsync on failure - Throw ArgumentException on NDEF content exceeding 54-byte data area - Add ArgumentNullException.ThrowIfNull for connection parameter - Extract static readonly FirmwareVersion constants - Remove unused System.Buffers import - Delete placeholder test Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Fix DER SEQUENCE length encoding for P-521 signatures (seqLength >= 128) - Zero private key material in PrivateKeyTemplate.ToBytes() intermediates - Add upper bound validation (<=255) in SetPinAttemptsAsync to prevent truncation - Fix XML doc for VerifyPinAsync extended parameter (was backwards) - Remove unused DigestInfo headers (Sha224, Sha512_224, Sha512_256) - Delete unused CardholderRelatedData class - Replace FixedTimeEquals with SequenceEqual for non-secret OID comparison - Remove redundant (Pw) casts in VerifyPwAsync - Delete placeholder tests Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ead code - Fix swapped ValidateCheckSum arguments in PutKeyAsync(ECPrivateKey) - Fix hardcoded "ExceptionMessages.ChecksumError" string literal - Zero private key material (parameters.D) after encryption - Add using var for DisposableTlvList in GetCertificatesAsync/GetCaIdentifiersAsync - Add CancellationToken to ClearAllowListAsync - Remove dead code: ParseCertificateBundle, ParseCaIdentifiers, EncodeControlReference, EncodeKeyReference - Rename StoreAllowlistAsync → StoreAllowListAsync for consistent PascalCase - Modernize to collection expressions and stackalloc Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…e salt - Fix PublicKeyLength from 64 to 65 for EcP256YubicoAuthentication - Fix ChangeCredentialPasswordAsync to use ThrowOnCredentialPasswordFailure - Change Pbkdf2Salt from mutable byte[] to ReadOnlySpan<byte> property - Add public key length validation in GetPublicKeyAsync - Remove unreachable MinLabelLength check - Remove stale TODO comment - Delete placeholder tests Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Extract EnsureProtocol() with [MemberNotNull] replacing 27 inline null checks - Merge DecryptBlock/EncryptBlock into single CryptoBlock method - Merge ParseWitnessResponse/ParseChallengeResponse into ParseAuthResponse - Extract EncodeObjectId helper from GetObjectAsync/PutObjectAsync Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…edup parsers - Fix ExtensionOutput.Decode to delegate to DecodeWithRawData (was silently discarding all extension data) - Add mutual exclusion guard for largeBlob/largeBlobAssertion in ExtensionBuilder - Extract shared CoseKeyWriter from ClientPin and HmacSecretInput - Replace duplicate CBOR parsers in CredentialManagementModels with canonical Parse methods from PublicKeyCredentialDescriptor/UserEntity Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…erWriter - Change Credential.Id from byte[] to ReadOnlyMemory<byte> with defensive copy - Extract ConcatTlvs helpers replacing 5 manual TLV concatenation sites - Change CalculateAsync return type to Task<ReadOnlyMemory<byte>> - Replace MemoryStream+ToArray with ArrayBufferWriter in CollectResponseData Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…n, response check - Extract ProcessHmacKey into SlotConfiguration base class (was duplicated in HmacSha1SlotConfiguration and HotpSlotConfiguration) - Extract InitializeKeys into KeyboardSlotConfiguration (was duplicated in YubiOtpSlotConfiguration and StaticTicketSlotConfiguration) - Add response length validation in OtpHidBackend.WriteUpdateAsync Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…static dict - Fix EcAttributes.Create to always use ECDH algorithm ID for X25519 - Consolidate 5 KeyRef switch dispatchers into single GetKeyRefData tuple method - Extract static AlgoAttributeToKeyRef dictionary (was rebuilt per-call) - Replace .AsMemory().ToArray() with .AsSpan().ToArray() in 4 files Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…rNotNull - Expand ISecurityDomainSession from 6 to 15 methods (all public methods now on the interface for DI/mocking) - Extract shared PutEcKeyAsync helper from ECPublicKey/ECPrivateKey overloads - Add [MemberNotNull] to EnsureInitializedProtocol, remove null-forgiving operators Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
… test data - Extract ThrowOnRetryFailure shared by management key and credential password handlers - Extract TransmitWithRetryCheckAsync applied to 9 call sites - Change test DefaultManagementKey from mutable byte[] to ReadOnlyMemory<byte> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Add BerLength.Write/EncodingSize for correct BER-TLV length encoding (eliminates error-prone hand-rolled encoding across modules) - Add SWConstants.ExtractRetryCount/IsVerifyFailWithRetries for 0x63Cx retry count extraction (duplicated in PIV, YubiHsm, OpenPgp) - Add EnsureReady() to ApplicationSession base class combining ThrowIfDisposed + IsInitialized checks Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Fix connection resource leak when session creation throws after ConnectAsync succeeds. Affects: Fido2, Oath, OpenPgp, SecurityDomain, YubiHsm, Management. PIV and YubiOtp were already fixed in stage 1. Each module now wraps CreateAsync in try/catch that disposes the connection on failure, matching the PIV/YubiOtp pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- PIV: Replace 8 hand-rolled BER-TLV length encoding sites with BerLength.Write/EncodingSize (Certificates, Crypto, DataObjects) - YubiHsm: Delegate ExtractRetries to SWConstants.ExtractRetryCount - OpenPgp: Replace inline retry extraction with SWConstants.ExtractRetryCount (also fixes subtle bug: old mask 0xFF00 matched 0x6300-0x63FF, new mask 0xFFF0 correctly matches only 0x63C0-0x63CF) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
EnsureReady() was added but never adopted by any module — each session has its own [MemberNotNull]-annotated protocol guard that the compiler requires for nullable analysis. Removing to avoid dead code. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Zero inputArr in DesBlockOperation finally block (security) - Replace single-byte length casts in BuildAuthResponse with BerLength.Write - Add TODO for GetBioMetadataAsync positional parsing (needs TLV verification) - Replace 5 inline retry count extractions with SWConstants.ExtractRetryCount - Delegate PivPinUtilities.GetRetriesFromStatusWord to SWConstants - Replace List<byte> with ArrayBufferWriter in StoreCertificateAsync/PutObjectAsync Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Fix double SkipValue in ParseDecryptedBlob (corrupted parse on non-empty byte string keys — conditional skip based on whether key was consumed) - Extract shared ECDH P-256 key agreement into PinUvAuthHelpers, eliminating ~40 lines of duplicated code between PinUvAuthProtocolV1/V2.Encapsulate Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…params
- Make CredentialData implement IDisposable, zeroing Secret on Dispose
- Zero intermediate HmacShortenKey buffer when different from input
- Update 4 caller sites to use 'using' (CLI commands, examples)
- Change ValidateAsync/SetKeyAsync params from byte[] to ReadOnlyMemory<byte>
- Fix path.Contains(':') missing StringComparison.Ordinal
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ilter - Replace silent access code truncation with strict size validation - Add exception filter to prevent swallowing OperationCanceledException Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…NTEGER - Zero FormatEcSignPayload hash buffer after sign/authenticate APDU - Fix KdfIterSaltedS2k.Dispose() to call base.Dispose() - Replace single-byte length in EncodeAsn1Integer with BerLength.Write - Fix KdfNone.ToBytes() .AsMemory().ToArray() → .AsSpan().ToArray() Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…n, DI param - Fix GetCaIdentifiersAsync bounds check (Length >= 2 instead of !IsEmpty) - Dispose Tlv objects in 5 methods (StoreAllowList, StoreCaIssuer, StoreCertificates, GetCertificates, and nested constructions) - Remove ZeroMemory(parameters.D) that mutated caller's ECPrivateKey - Add firmwareVersion parameter to SecurityDomainSessionFactory delegate Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Encodes a collection of TLV objects then disposes each in a finally block. Prevents sensitive data from lingering in undisposed Tlv buffers when Tlvs are created inline for EncodeList calls. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Replace TlvHelper.EncodeList with EncodeAndDisposeList at all call sites where Tlvs are created inline. Prevents sensitive data (management keys, credential passwords, private keys) from lingering in undisposed buffers. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add using to Tlv objects in ParseAuthResponse, ParseCryptoResponse, UnwrapDataObjectResponse, and ParsePublicKey. Copies data out before disposal to avoid use-after-dispose. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…into yubikey-codeaudit
…shared core Restore clear encrypt/decrypt semantics at call sites while keeping shared buffer management in CryptoBlockCore. Parameters renamed to plaintext/ciphertext for self-documenting data flow. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Plans/todo-backlog-workplan.md: 19 Jira issues (YESDK-1559–1577) prioritized - Plans/foamy-swimming-summit.md: Stage 4 implementation plan with deferred analysis - Plans/handoff.md: Updated with full 5-stage session summary Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…into yubikey-codeaudit
ModPow returned the raw BigInteger byte array directly when its length matched the output size, but the finally block then zeroed that same array via CryptographicOperations.ZeroMemory. Since arrays are reference types, callers received a zeroed buffer, breaking all RSA decrypt ops. Clone with .AsSpan().ToArray() before returning so the finally block safely zeros only the BigInteger's internal allocation. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
… format
Replace 36 old-format [Trait("RequiresUserPresence", "true")] with
[Trait(TestCategories.Category, TestCategories.RequiresUserPresence)]
across 17 files. The old format was not caught by the --smoke filter
(Category!=RequiresUserPresence), causing 39 false failures.
Add missing trait to 8 test methods that require user presence but
had no trait at all: ToggleAlwaysUv, SetMinPinLength,
GetFingerprintSensorInfo, GetCredentialsMetadata, and 4
EncryptedMetadata tests.
Remove duplicate trait on FidoPinManagementTests.ChangePin.
Update comment in FidoSessionSimpleTests to reference new format.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
HidYubiKey.DeviceId used VendorId:ProductId:Usage which is identical across multiple YubiKeys, causing one key's HID interfaces to overwrite the other in the device cache. Include ReaderName (unique device path) in DeviceId, matching how PcscYubiKey already uses ReaderName. ClientPin.GetPinUvAuthTokenUsingPinAsync always sent CTAP2.1 subcommand 0x09 (getPinUvAuthTokenUsingPinWithPermissions), failing on CTAP2.0 devices. Check pinUvAuthToken option from GetInfo and fall back to legacy getPinToken (subcommand 0x05) when unsupported, matching python-fido2's ClientPin.get_pin_token() logic. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive 5-stage code audit and remediation across all 7 YubiKey applet modules + Core. 27 commits, 87 files changed, net -244 lines.
Stage 1: Bug Fixes & Security (7 commits)
Targeted fixes for correctness bugs, security gaps, and dead code removal.
Stage 2: DRY Refactoring & Interface Expansion (7 commits)
Stage 3: Cross-Module Core Extraction (4 commits)
BerLength.Write/EncodingSize,SWConstants.ExtractRetryCount/IsVerifyFailWithRetriesEnsureReady()from ApplicationSessionStage 4: Remaining Findings + All Deferred Items (6 commits)
Stage 5: Cross-Module Tlv Disposal (3 commits)
TlvHelper.EncodeAndDisposeList— encodes then disposes each Tlv in finallyEncodeListsites withEncodeAndDisposeList(sensitive data in buffers)Items Explicitly Skipped (with rationale)
Audit Statistics
Test plan
🤖 Generated with Claude Code