From 8b48b6ed275dc7cf670d520eadd4da6c865ee665 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Tue, 25 Aug 2026 12:07:39 +0800 Subject: [PATCH] fix: scope Keychain items by an explicit owner attribute (#55) App scoping was i.Service.StartsWith(_appIdentifier + ".", Ordinal). The service is "{app}.{provider}" and ValidateProviderName permits dots in provider names, so the format is genuinely ambiguous: "com.acme.cli.pro.Adobe" is what app "com.acme.cli" writes for provider "pro.Adobe" and equally what app "com.acme.cli.pro" writes for provider "Adobe". Reverse-DNS identifiers nest by convention, so a neighbour could list, export and delete credentials it did not own. Items now record their owning app identifier in kSecAttrComment, and IsOwnedByThisApp matches it exactly. QueryItems, QuerySingleItem and QueryAllItemsForApp all filter through it -- including QueryItems, which pins an exact service and was still not proof of ownership for the same reason. QuerySingleItem, QueryItems and ConfirmItemVisible become instance methods so no call site can bypass the check. kSecAttrComment rather than kSecAttrGeneric deliberately: kSecAttrGeneric participates in the primary key for generic-password items on some macOS versions, so adding it to items that previously lacked it could change uniqueness semantics and let a legacy and a new item coexist for one service+account. kSecAttrComment is plain metadata. Legacy items carry no owner and fall back to the old prefix test, so nothing already stored disappears. That fallback is a deliberate, documented partial fix: legacy items stay ambiguous until rewritten, and only items written from here on are scoped exactly. Two macOS tests cover both halves -- a dot-prefix neighbour sees nothing through list, export or provider enumeration, and cannot delete the other app's credential. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 11 +++ .../Keychain/KeychainCredentialManager.cs | 70 ++++++++++++++++--- .../Persistence/Keychain/KeychainInterop.cs | 12 ++++ .../KeychainCredentialManagerTests.cs | 56 +++++++++++++++ 4 files changed, 141 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59df1af..cac36f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The Keychain backend could see another CLI's credentials** (#55). App scoping was a + dot-prefix match on the service string, and since the service is `{app}.{provider}` and + provider names may contain dots, app `com.acme.cli` could not distinguish its own + `pro.Adobe` item from app `com.acme.cli.pro`'s `Adobe` item. The neighbour could list, + export **and delete** credentials it did not own; reverse-DNS identifiers nest by + convention, so the collision is plausible rather than contrived. Items now record their + owning app in `kSecAttrComment` and every query filters on it exactly. Items written before + this release carry no owner and fall back to the old prefix test, so nothing already stored + becomes invisible — meaning **legacy items stay ambiguous until rewritten**, and only items + written from this release on are scoped exactly. + - **`RestoreCredentialAsync_Preserves…` flaked on the OS-native backends** (#61). The test retried until one credential was visible and then asserted `IsSelected`, which comes from a *separate* store item written moments earlier — so the credential could become visible diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs index ae82064..20d1981 100644 --- a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs @@ -71,6 +71,7 @@ public Task AddCredentialAsync(string providerName, string accountName, Label = accountName, Description = environment, Data = Encoding.UTF8.GetBytes(credentialData), + OwnerAppIdentifier = _appIdentifier, }; AddItem(attrs); @@ -91,7 +92,7 @@ public Task AddCredentialAsync(string providerName, string accountName, /// Best-effort: returns after a bounded wait even if the item never /// appears, leaving any genuine failure to the caller's own lookup. /// - private static void ConfirmItemVisible(string service, string account) + private void ConfirmItemVisible(string service, string account) { const int maxAttempts = 20; for (var attempt = 1; attempt <= maxAttempts; attempt++) @@ -344,6 +345,7 @@ public Task RestoreCredentialAsync(CredentialExport credential) Label = credential.AccountName, Description = credential.Environment, Data = Encoding.UTF8.GetBytes(credential.CredentialData), + OwnerAppIdentifier = _appIdentifier, }); if (credential.IsSelected) @@ -391,6 +393,33 @@ private string ResolveStoredProviderName(string providerName) return providerName; } + /// + /// Whether belongs to this manager's app. + /// + /// + /// Items written by this version carry the owning app identifier in + /// kSecAttrGeneric and are matched on it exactly. Items written before that + /// attribute existed carry nothing, so they fall back to the old service-prefix + /// check — without which every previously stored credential would vanish. + /// + /// The fallback is not a complete fix and is not presented as one: a legacy item + /// belonging to an app whose identifier is a dot-prefix of another's stays + /// ambiguous until it is rewritten. Everything written from now on is scoped + /// exactly (#55). + /// + /// + private bool IsOwnedByThisApp(KeychainItem item) + { + if (!string.IsNullOrEmpty(item.OwnerAppIdentifier)) + { + return string.Equals(item.OwnerAppIdentifier, _appIdentifier, StringComparison.Ordinal); + } + + // Legacy item: no owner recorded, so fall back to the ambiguous prefix test. + return item.Service.StartsWith(_appIdentifier + ".", StringComparison.Ordinal) + || string.Equals(item.Service, SelectionsService, StringComparison.Ordinal); + } + private string ServiceFor(string providerName) => $"{_appIdentifier}.{providerName}"; private string SelectionsService => $"{_appIdentifier}{SelectionsServiceSuffix}"; @@ -431,6 +460,7 @@ private void WriteSelection(string providerName, string accountId) Label = $"{_appIdentifier} active credential ({providerName})", Description = string.Empty, Data = bytes, + OwnerAppIdentifier = _appIdentifier, }); } else @@ -497,6 +527,12 @@ private sealed class KeychainItem public string? Description { get; init; } public DateTime? CreatedAt { get; init; } public byte[]? Data { get; init; } + + /// + /// Owning app identifier, stored in kSecAttrComment. Null for items + /// written before this attribute existed — see . + /// + public string? OwnerAppIdentifier { get; init; } } private static void AddItem(KeychainItem item) @@ -510,6 +546,13 @@ private static void AddItem(KeychainItem item) var descCf = Track(handles, NewCfString(item.Description ?? string.Empty)); var dataCf = Track(handles, NewCfData(item.Data ?? [])); + // kSecAttrComment records which app owns the item, so scoping does not have + // to be inferred from the service string. That inference was ambiguous: + // the service is "{app}.{provider}" and provider names may contain dots, so + // app "com.acme.cli" could not tell its own "pro.Adobe" item from app + // "com.acme.cli.pro"'s "Adobe" item (#55). + var ownerCf = Track(handles, NewCfString(item.OwnerAppIdentifier ?? string.Empty)); + var pairs = new List<(IntPtr, IntPtr)> { (Constants.KSecClass, Constants.KSecClassGenericPassword), @@ -517,6 +560,7 @@ private static void AddItem(KeychainItem item) (Constants.KSecAttrAccount, accountCf), (Constants.KSecAttrLabel, labelCf), (Constants.KSecAttrDescription, descCf), + (Constants.KSecAttrComment, ownerCf), (Constants.KSecValueData, dataCf), }; @@ -588,7 +632,12 @@ private static void DeleteItem(string service, string account) } } - private static KeychainItem? QuerySingleItem(string service, string account, bool includeData) + /// + /// Fetches one item by service + account, or null if it does not exist or is not + /// ours. See — an exact service query is not on its + /// own proof of ownership, because the service string is ambiguous (#55). + /// + private KeychainItem? QuerySingleItem(string service, string account, bool includeData) { var handles = new List(); try @@ -615,7 +664,11 @@ private static void DeleteItem(string service, string account) try { - return DecodeItem(result); + var decoded = DecodeItem(result); + + // Present but owned by another app: treat as not found rather than + // handing back someone else's credential (#55). + return decoded is not null && IsOwnedByThisApp(decoded) ? decoded : null; } finally { @@ -631,7 +684,7 @@ private static void DeleteItem(string service, string account) } } - private static List QueryItems(string service, bool includeData) + private List QueryItems(string service, bool includeData) { // Bulk query: request attributes only. Combining kSecReturnAttributes // + kSecReturnData + kSecMatchLimitAll in a single SecItemCopyMatching @@ -661,7 +714,9 @@ private static List QueryItems(string service, bool includeData) try { - stubs = DecodeArray(result); + // The service string alone does not establish ownership, so filter even + // though this query pinned an exact service (#55). + stubs = [.. DecodeArray(result).Where(IsOwnedByThisApp)]; } finally { @@ -732,9 +787,7 @@ private List QueryAllItemsForApp(bool includeData) try { var items = DecodeArray(result); - stubs = [.. items - .Where(i => i.Service.StartsWith(_appIdentifier + ".", StringComparison.Ordinal) - || string.Equals(i.Service, SelectionsService, StringComparison.Ordinal))]; + stubs = [.. items.Where(IsOwnedByThisApp)]; } finally { @@ -832,6 +885,7 @@ private static List DecodeArray(IntPtr arrayOrDict) Description = ReadCfStringAt(dict, Constants.KSecAttrDescription), CreatedAt = ReadCfDateAt(dict, Constants.KSecAttrCreationDate), Data = ReadCfDataAt(dict, Constants.KSecValueData), + OwnerAppIdentifier = ReadCfStringAt(dict, Constants.KSecAttrComment), }; } diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs index 9c2c4e9..d2838ed 100644 --- a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs @@ -153,6 +153,18 @@ internal static class Constants internal static readonly IntPtr KSecAttrLabel = LoadSymbol(Security, "kSecAttrLabel"); internal static readonly IntPtr KSecAttrDescription = LoadSymbol(Security, "kSecAttrDescription"); internal static readonly IntPtr KSecAttrGeneric = LoadSymbol(Security, "kSecAttrGeneric"); + + /// + /// kSecAttrComment. Used to record which app owns an item. + /// + /// + /// Deliberately not kSecAttrGeneric, which participates in the primary + /// key for generic-password items on some macOS versions — adding it to items + /// that previously lacked it would change uniqueness semantics and could let a + /// legacy item and a new one coexist for the same service + account. + /// kSecAttrComment is plain metadata and carries no such risk. + /// + internal static readonly IntPtr KSecAttrComment = LoadSymbol(Security, "kSecAttrComment"); internal static readonly IntPtr KSecAttrCreationDate = LoadSymbol(Security, "kSecAttrCreationDate"); internal static readonly IntPtr KSecValueData = LoadSymbol(Security, "kSecValueData"); diff --git a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs index d3ba450..ed4083a 100644 --- a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs +++ b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs @@ -455,6 +455,62 @@ public async Task SelectCredentialAsync_ReplacingAnExistingSelection_LeavesExact Assert.False(listed.Single(c => c.AccountId == prod).IsSelected); } + [Fact] + [SupportedOSPlatform("macos")] + public async Task Items_AreInvisibleToAnAppWhoseIdentifierIsADotPrefix() + { + Assert.SkipWhen(_skip, SkipReason); + + // "…cli" is a dot-prefix of "…cli.pro", and the service string is + // "{app}.{provider}" — so the neighbour's prefix test matched pro's item and it + // could list, export and delete another CLI's credentials (#55). + var neighbourId = $"test.nextiteration.sca.{Guid.NewGuid():N}"; + var neighbour = new KeychainCredentialManager(neighbourId); + var pro = new KeychainCredentialManager(neighbourId + ".pro"); + + var id = await pro.AddCredentialAsync("Adobe", "prod", "Production", "{\"apiKey\":\"PRO\"}"); + try + { + // pro's service is "{neighbourId}.pro.Adobe": it still starts with + // "{neighbourId}." so the old check matched, but the owner attribute does not. + Assert.Empty(await neighbour.GetProviderNamesAsync()); + Assert.Empty(await neighbour.ExportCredentialsAsync()); + Assert.Empty(await neighbour.ListCredentialsAsync("pro.Adobe")); + + // And the owner still sees its own item. + Assert.Single(await RetryHelper.UntilAsync(() => pro.ExportCredentialsAsync(), r => r.Count == 1)); + } + finally + { + _ = await pro.DeleteCredentialAsync(id); + } + } + + [Fact] + [SupportedOSPlatform("macos")] + public async Task NeighbourCannotDeleteAnotherAppsCredential() + { + Assert.SkipWhen(_skip, SkipReason); + + var neighbourId = $"test.nextiteration.sca.{Guid.NewGuid():N}"; + var neighbour = new KeychainCredentialManager(neighbourId); + var pro = new KeychainCredentialManager(neighbourId + ".pro"); + + var id = await pro.AddCredentialAsync("Adobe", "prod", "Production", "{\"apiKey\":\"PRO\"}"); + try + { + // The destructive half of the same bug: delete resolves the item through the + // same app-scoped lookup, so an unfixed prefix test let a neighbour remove + // credentials it does not own. + Assert.False(await neighbour.DeleteCredentialAsync(id)); + Assert.Single(await RetryHelper.UntilAsync(() => pro.ExportCredentialsAsync(), r => r.Count == 1)); + } + finally + { + _ = await pro.DeleteCredentialAsync(id); + } + } + private sealed class FakeAdobeSummaryProvider : ICredentialSummaryProvider { public string ProviderName => "Adobe";