fix: scope Keychain items by an explicit owner attribute - #73
Merged
Conversation
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 <noreply@anthropic.com>
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.
Fixes #55.
The ambiguity
Scoping was a dot-prefix match on the service string:
The service is
{app}.{provider}andValidateProviderNamepermits.in provider names, socom.acme.cli.pro.Adobeis what appcom.acme.cliwrites for providerpro.Adobeand what appcom.acme.cli.prowrites for providerAdobe. No parsing distinguishes them — the format does not encode where the app id ends. Reverse-DNS identifiers nest by convention, so the collision is plausible, and the neighbour could list, export and delete credentials it did not own.The fix
Items record their owning app in
kSecAttrComment;IsOwnedByThisAppmatches it exactly.kSecAttrComment, notkSecAttrGeneric— deliberately.kSecAttrGenericparticipates 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.kSecAttrCommentis plain metadata and carries no such risk. I started onkSecAttrGenericand moved off it for exactly this reason.Every query filters, including the exact-service one.
QueryItemspins a specific service, which for the same ambiguity is still not proof of ownership.QuerySingleItem,QueryItemsandConfirmItemVisiblebecome instance methods so no call site can bypass the check by being static.What this does not fix, stated plainly
Legacy items carry no owner and fall back to the old prefix test — without which every already-stored credential would vanish. So legacy items remain ambiguous until rewritten; only items written from this release on are scoped exactly. Closing that fully would need a migration that rewrites items on first access, which is a larger piece of work and a separate decision.
Verification
Build clean, 412 tests (354 passed, 58 skipped) locally.
The two new tests are macOS-only and skipped here — CI's macOS leg is the only thing that exercises them, and it is the only verification this change gets. They cover both halves of the bug: a dot-prefix neighbour sees nothing through
ListCredentialsAsync,ExportCredentialsAsyncorGetProviderNamesAsync, and cannot delete the other app's credential. Both assert the owner still sees its own item, so an over-tight filter fails too.I flagged on the issue that I did not want to make this change unverified; you asked for it, so it is here with the risk stated rather than smoothed over. Worth watching the macOS leg specifically.
🤖 Generated with Claude Code