fix(fingerprint): prevent .kosli_ignore from excluding itself - #1156
Conversation
|
Claude finished @pbeckham's task in 5m 53s —— View job Review
Static analysis
I could not run the suite — Since the last round
Findings1.
2. The runtime messages name Same mismatch, reachable at runtime: 3. A dangling
What is good hereThe walk-level protection is the right design and it is unfalsifiable by construction, which is the part worth keeping: The test discipline is unusually good for a security fix. Differential assertions against a clean baseline are the right shape for a bug about digests colliding; the annotations distinguishing legs that pin a property from legs that merely document it (bare The scope discipline is also right. Deferring the Correcting stale rationales in |
1059d3b to
84c9528
Compare
…tising --exclude Review of #1156 found the check that stops .kosli_ignore excluding itself could still be defeated, and that the new help text recommended the one configuration that undoes the fix. isIgnoreFile now matches the file name case-insensitively before consulting os.SameFile. Every error path in os.SameFile returns false, which here meant "not the ignore file, exclude it", handing the tree back control over its own measurement: on Windows os.SameFile resolves the file index by opening the path (types_windows.go loadFileId) and reports false when that loses to a locked or shared file. Leading with the name also drops the stat per excluded path, which an ignore file globbing node_modules paid on every one of them. Excluding the ignore file with --exclude keeps it out of the digest but still applies the entries it carries, so an added file listed there is still hidden. That is the pre-fix behaviour in full. The help text now says so and points at the migration that does not reinstate it: move the entries to --exclude and delete the file, which reproduces the old fingerprint exactly. DirSha256 warns when a flag excludes the ignore file, so the weakened mode shows up in the CI log. The test case covering it is renamed to say it pins the migration path rather than a safety property. TestDirSha256IgnoreFileHardLinkIsNotExcluded gives the os.SameFile branch CI coverage on a case-sensitive filesystem, where the case-variant tests skip. It fails when os.SameFile is stubbed out. Refs kosli-dev/server#6785 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DirSha256 reads .kosli_ignore from inside the directory it is measuring, and that file was allowed to list itself. A tree an attacker can write into therefore controlled its own measurement: add a file, add a .kosli_ignore naming both the added file and itself, and the fingerprint is unchanged. So files could be added to a deployed tree indefinitely while Kosli kept reporting it as byte-identical to the approved artifact. Modifying or deleting a file was always caught; the primitive was add-only. The walk is now told which path to keep, and never skips it. filepath.WalkDir emits the ignore file's path exactly as the tree stores it, and that same string is what the exclusion check compares, so identification and comparison are one string and nothing has to model how an exclusion pattern expands, normalises or folds. Filtering the resolved patterns instead was the first approach and proved consistently wrong in ways that were invisible from the fingerprint: matching on base name silently un-excluded nested ignore files, filepathx emits "dir//.kosli_ignore" for a ** pattern, and a case-insensitive filesystem stores whichever case was written. ignoreFilePathInTree reads the name from the directory listing rather than building it from ignoreFileName, since a case-insensitive filesystem opens the file under any case while storing one. Name folding finds it, with an os.SameFile pass as a fallback so a folding rule Go does not implement cannot leave the file unprotected. That matters where the operator's filesystem is not the deployment target's: snapshot s3 and snapshot azure download and unzip the tree locally. An operator flag can still exclude the file. That is the migration path off the old behaviour and it reproduces the old fingerprint, but it keeps applying the entries the tree carries, so a file added to the directory and listed there stays hidden. The help text says so and points at the migration that does not: move the entries to --exclude and delete the file, which reproduces the old fingerprint exactly. DirSha256 warns when a flag excludes the file, gated on the path the walk will actually emit so it cannot claim a weakening that did not happen. A tree whose in-tree .kosli_ignore excludes itself fingerprints differently after this change. None of the .kosli_ignore files in kosli-dev repos does that, and every pre-existing golden hash is unchanged, so it is a no-op for every other tree. Tested against 23 self-exclusion spellings, including ./, //, **, **/**, character classes, ? wildcards, brace and trailing-slash forms, an unnormalised app/../ traversal, a self-symlink and three letter cases. Hard links and symlinks to the ignore file stay excludable, since excluding a distinct walk entry never removes the file itself, and a nested .kosli_ignore stays an ordinary file. Removing the protection fails 8 tests; building the stored name instead of reading it fails 7. Refs kosli-dev/server#6785 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cb593a3 to
76c528e
Compare
Review found the walk-level protection could pick the wrong file, restoring kosli-dev/server#6785 in full on a case-sensitive filesystem, which is what CI and most deployment targets run. ignoreFilePathInTree matched the directory entry by folded name. os.ReadDir returns entries sorted and ".KOSLI_IGNORE" sorts before ".kosli_ignore" (K=0x4B < k=0x6B), so a tree holding both as two distinct files protected the upper-case one. excludePathsFromFile reads the rules from ignoreFileName byte-exact, so the protected file was a decoy that lists nothing while the file whose entries are applied was left free to exclude itself again. Reproduced on a case-sensitive APFS volume: an approved build carrying an empty ".KOSLI_IGNORE" alongside a self-excluding ".kosli_ignore" fingerprinted 1175efba, and so did the same tree with app/backdoor.js added and listed. A decoy like that is not contrived. Committing ".KOSLI_IGNORE" from a case-insensitive machine and ".kosli_ignore" from a case-sensitive one leaves both in the repository. An exact name match now wins over a folded one, so the file protected is always the file the rules came from. Other spellings are ordinary files on a case-sensitive filesystem and stay excludable, like a nested ignore file. The os.SameFile fallback is dropped. ignoreFileName is pure ASCII, so simple case folding is the only transformation in play and strings.EqualFold already covers it, leaving the pass unreachable. Where it did fire it took the first same-inode entry from a sorted listing, which could be a hard link sorting earlier than the ignore file, protecting the alias and leaving the real file exposed. It could only mislead. TestIgnoreFilePathInTreeWithBothSpellings and TestDirSha256IgnoreFileCannotHideItselfBesideACaseVariant cover both, and skip where the two spellings are one file. Verified against a case-sensitive volume: both pass, and both fail when the fold is restored ahead of the exact match. Refs kosli-dev/server#6785 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g to what holds Three review findings, all about claiming more than the fix delivers. The help text said a directory "cannot hide files that have been added to it". That is false, and it was operator-facing guidance about a security property. What the fix pins is the exclusion list: its name and content are in the digest, so the tree cannot change the rules without changing the fingerprint. The list is one of patterns, resolved against the deployed tree at fingerprint time, so a path the approved list already matches stays hidden whatever is later put there. Verified: an untouched ".kosli_ignore" of "*.log" and an added "evil.log" both fingerprint 5d1800fa. The text now says the list cannot be changed, and that entries should be kept as narrow as possible. The warning fired whenever a flag excluded the ignore file, including when that file was empty or held only comments, announcing that "the paths it lists are still applied" for a file that lists nothing. It now sits inside the branch that already knows the file carries rules. The flag still takes effect either way, since dropping an empty file from the digest is a legitimate instruction; only the claim about it was wrong. Nothing held the warning itself, which is the only signal an operator gets that they are in the weakened mode, and which had been wrong twice. Its silent legs matter as much as the loud one, so TestDirSha256WarnsOnlyWhenAFlagExclusionWeakensTheFingerprint pins both: rules present warns, while empty, comment-only, a ** spelling the walk never matches, an unrelated exclusion and no flag at all stay silent. Loosening the comparison to a folded one fails 2 of them; removing the rules gate fails 3. Refs kosli-dev/server#6785 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Locating the ignore file and reading its rules are two reads of the same path, so they can disagree in either direction. b086128 closed one: the file resolves but the listing does not show it. This is the mirror, and it was still open. os.Lstat answers ENOENT, "" is returned, and by the time excludePathsFromFile runs the file is back, so its rules are applied with nothing protected and a self-excluding entry works again. This direction is worse than the one already fixed, because it lands on a clean baseline: the tree adds both a file and a .kosli_ignore listing itself and the added file, both drop out, and the digest equals that of a tree with neither - which is the approved fingerprint for an artifact that never had an ignore file. Race-only, and the window is two syscalls, but a rename loop only has to win once against a scheduled snapshot and a miss is silent. The invariant is stated where both facts are known: rules were read, so something must be protected. That covers both directions at once and subsumes the narrower refusal added in b086128, which is removed - it fired even when the vanished file carried no rules, and nothing could hide behind an empty exclusion list. Neither direction has a deterministic test: both need the two reads to disagree, which on a real filesystem means winning the race, and there is no seam to drive it from. Separately, protectedPath could name a directory. It means "the file whose rules are read" and a directory carries none, so directories are skipped when locating it, keeping such a tree excludable like any other. This is behaviour-neutral - verified the digests either side of the change are identical, since a flag exclusion already disengaged the protection before the walk saw it - so it tightens the definition rather than fixing a bypass. Refs kosli-dev/server#6785 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t was located The ignore file was reached by three routes: located by Lstat plus ReadDir, opened for its rules by a name rebuilt from ignoreFileName, and hashed by the walk. Locating and hashing were byte-identical by construction, both being filepath.Join(dirPath, entry.Name()), but reading was a separately resolved name, and 1b376a9, b086128 and b9ebedb were each after-the-fact detection of the two disagreeing. One disagreement survived all three, because both sides were non-empty. On a case-sensitive filesystem holding only .kosli_ignore: Lstat resolves, the tree renames it to .KOSLI_IGNORE, ReadDir finds no exact match and protects the folded name, the tree renames it back, and the rules are read from .kosli_ignore. The walk then emits a path that is not the protected one, so the ignore file and anything it lists are both skipped, landing on a clean baseline. The rules are now read from the located path, so the file protected and the file whose rules apply are the same string, and the walk emits that string by the same construction. The class is closed rather than a fourth case detected, and the refusal added in b9ebedb goes with it: it becomes unreachable, which is the same objection that retired os.SameFile and the earlier folded refusal. The IsDir skip also stops resting on excludePathsFromFile returning nothing for a directory, which is only true because bufio.Scanner fails with EISDIR and scanner.Err() is never checked - an ordinary cleanup would change that premise. One race is left and is noted rather than fixed: the rules are read before the file is hashed, so a tree rewriting it in between has old rules applied to newly hashed content. Closing it means hashing the bytes that were read. Behaviour-neutral off a race: an exact match still wins where both spellings exist, a case-insensitive tree still reads the stored spelling, a case-sensitive tree holding only .KOSLI_IGNORE still reads no rules, and a directory of that name stays excludable. All golden hashes, 23 spellings and the adversarial cases are unchanged. Refs kosli-dev/server#6785 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading the rules from the located path removed the mechanism four comments cited as their reason for existing, leaving them describing code that is no longer there. The exact-over-folded rule is no longer a security invariant. Since the caller reads the rules from whatever ignoreFilePathInTree returns, either answer protects the file it read, and a folded-first result would give ".kosli_ignore" no authority to exclude anything, itself included. What the rule actually preserves is which file means something: every release before this one read the rules from ignoreFileName byte-exact, so returning the folded spelling would hand authority to a previously inert ".KOSLI_IGNORE" and change the fingerprint with it. That is what a future reader must not simplify away, and the comment was telling them to check a claim that had become false. The error return is likewise not about two reads disagreeing, since there is only one read now. It is that "" is also the answer for a tree with no ignore file, so a failed read must not be indistinguishable from an absent one. TestDirSha256IgnoreFileCannotHideItselfBesideACaseVariant has gone insensitive as a result and now documents rather than pins: under a folded-first mutation the empty decoy is located, no rules are read, and the deployed tree differs from the baseline anyway. Verified on a case-sensitive APFS volume - it passes under that mutation where it used to fail, and only TestIgnoreFilePathInTreeWithBothSpellings still fails. Annotated, in the same way as the bare-** leg. Also corrects the previous commit's claim that the change was behaviour-neutral. Where ignoreFileName is a directory and another spelling is a file, the file's rules now apply where none were read before: verified on the same volume, c7dab185 against 3df4db05. Contrived, and the alternative is protecting a path that can carry no rules, but it is a difference and the comment says so. Refs kosli-dev/server#6785 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Applies the server repo's standard-comments to every comment this branch added. 143 comment lines become 50, and the cuts follow the standard's own tests rather than a length target. "No issue or PR numbers of ours" removed three references to the ticket. The commit body reaches it in one hop and can still be corrected; a copy in a comment cannot. "The fact, not the story" removed the history. Comments explaining what a rename "made" happen, what "every release before this one" read, what a case "pinned before that change", and what the pre-fix fingerprint was all read as diffs rather than as code, and none of it is true of the file someone opens fresh. "Information, not narration" emptied most of the test comments. Twelve restated the test name, an assertion, or a helper signature. Where a comment survived it is the one fact the name cannot carry: which legs of a table pin behaviour and which only document it, that filepathx resolves a ** pattern to a path the walk never emits, that 0300 lets Lstat resolve while ReadDir fails. "Cut clause by clause" took the rest. The reasoning about three routes to the same file, about which guard retired which, and about what was verified on a case-sensitive volume belongs in the commits that made those changes, where it is already recorded. No behaviour change: golden hashes, the 23 spellings and the adversarial cases are unchanged, and the suite passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reverses the gate added in e91d047. Gating the warning on the file carrying rules was wrong, and the reasoning I accepted for it confused the message's wording with the risk. What --exclude drops from the digest is the exclusion list's content, so its content at approval time does not constrain it later. An empty ignore file excluded by flag is weakened maximally: an approved tree of files plus an empty .kosli_ignore fingerprints 096fa174, and so does a deployed tree that adds evil.js plus a .kosli_ignore listing it. The gate also put the warning on the wrong side of the operator. Verified against that pair: the attest run, which an operator watches, was silent, while the snapshot run, which nobody watches, warned. Since --exclude lives in pinned CI config and both attest artifact and snapshot path read it, the silent run was the one that mattered. The warning is unconditional on content now, sits beside the flag handling, and says what is true for empty, comment-only and rule-carrying files alike: the rules are applied while the content is not fingerprinted, so the directory can change the list at any time. The two test legs for empty and comment-only files become wantWarning: true; the ** leg stays silent, since it never reaches the walk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fs.DirEntry.IsDir reports the dirent type, so a .kosli_ignore that is a symlink to a directory is not skipped. It becomes the protected path and the path opened for rules, where os.Open succeeds and the first Scan fails with EISDIR, leaving an empty rule set. Verified: such a tree fingerprints with no ignore file used and no error. Fail-safe, since an empty rule set hashes more rather than less, and no bypass follows. Left as a comment rather than code: enforcing it with !entry.Type().IsRegular() would also stop a .kosli_ignore symlinked to a shared rules file being read, changing the fingerprint for anyone doing that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
excludePathsFromFile never consulted scanner.Err(), so a stopped scan returned the entries read so far as though they were the file's rules. Reachable on an ordinary file: a line over bufio.MaxScanTokenSize, or an I/O error mid-read. Verified with a 70 KiB first line followed by two entries - no rules were read at all, nothing was logged, and the tree fingerprinted b17441c4 where its own rules give 8c07ad51. Not a bypass, since dropped rules hash more rather than less, but the digest is taken against a rule set the file does not hold and nothing reports it. Pre-existing, and 95ed5ac did not introduce it, but that commit made this function the only reader of the rules, so the swallowed error became the sole thing between a partial read and a fingerprint. Also turns the symlink-to-a-directory shape into a read error rather than an empty rule set, which is the better answer and the comment above now says so. Both shapes are pathological, so no realistic fingerprint changes: this converts silently wrong into loudly broken. Kept separate from the ignore-file work so it can be dropped from the squash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kosliIgnoreDesc is embedded in 21 commands and told all of them that excluding
the ignore file still applies its rules, and that the way to drop it safely is to
move its entries to --exclude. Neither holds for two of them.
snapshot azure-apps registers no --exclude, so both sentences describe a flag the
command does not have. snapshot s3 has one that means something else:
excludeBucketPathsFlag filters bucket paths by literal prefix at download time and
DirSha256 is called with []string{}, so --exclude .kosli_ignore stops the object
being downloaded and its rules are not applied at all - the opposite of what the
text said, with no warning, and no migration available for a glob entry. Those two
now carry a variant without the two sentences.
Also aligns the two shapes that are equally not a rules file. A directory of that
name was skipped and fingerprinted fine, while a symlink to one was located,
opened and failed the read once 2523383 checked scanner.Err, so kosli fingerprint
aborted on a tree carrying .kosli_ignore -> some/dir where it had produced a
digest. Nothing justified the harsher answer for the link. One os.Stat on the
candidate entry - at most two per tree, since the fold check now filters first -
treats them alike, and the tree fingerprints d71dd4a4 again. A symlink to a file
is untouched and still read.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
65af64e added the resolved-directory check with no test. Removing it makes DirSha256 abort with EISDIR on a tree carrying .kosli_ignore -> somedir, so the guard stands between an ordinary edit to that loop and a failing pipeline. Test only; no production change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Part of kosli-dev/server#6785. This reduces the vulnerability rather than closing it, so the ticket should stay open.
DirSha256read its.kosli_ignoreexclusion list from inside the directory it was measuring, and that file could list itself, so the tree controlled its own measurement. It no longer can: the list is pinned to the approved build's.Why
Anyone with write access to a deployed directory artifact could add files to it and keep Kosli reporting the tree as byte-identical to the approved artifact, indefinitely. Where a required-provenance policy sits over a directory fingerprint, that is a silent compliance bypass. It reached every directory fingerprint; modify and delete were always caught.
What changed
The walk is told which path to keep and never skips it, so the ignore file is always in the fingerprint.
filepath.WalkDiremits its path exactly as the tree stores it, and that is the string the exclusion check compares, so no exclusion pattern is inspected at all. The stored name is read from the directory listing rather than built, since a case-insensitive filesystem stores one spelling and opens any — which reaches Linux targets too, becausesnapshot s3andazureunzip the tree locally.None of the four
.kosli_ignorefiles in kosli-dev repos self-excludes, so no observable tree changes fingerprint. Where one does, moving its entries to--excludeand deleting the file recovers the old value; excluding the file itself recovers it too but stays vulnerable, whichDirSha256now warns about.What this does not close
The pinned list is one of patterns, resolved against the deployed tree, so a file added where the approved list already matches stays invisible. Verified against a kosli-dev demo's real ignore list (
logs/,*.log,*.html): addinglogs/shell.phporpayload.htmlkeeps the approved fingerprint exactly. Carrying the list in the attestation, the ticket's fuller fix, does not help — an approved list sayinglogsstill excludeslogs. Closing it needs a per-file manifest, which the artifact document does not store.The unconstrained add is gone; what remains is the operator's declared blind spot. For a tree with no
.kosli_ignore, the vulnerability is absent rather than reduced.Verification
TestDirSha256IgnoreFileCannotHideItselfand...CaseInsensitiveFSpin the self-exclusion shapes against a clean baseline, whichdigest_test.gohad no test for; five further tests pin the scope of the refusal and the operator-facing messages. Removing the protection fails 7 of them, and building the stored name rather than reading it fails 5.Beyond the suite, 23 self-exclusion spellings were checked by hand, and the cases that skip on macOS were run on a case-sensitive APFS volume.
test / Testruns the full integration suite and has passed on this branch. Every pre-existing golden hash inTestDirSha256is unchanged, so a green run shows no fingerprint change for anyone unaffected.