Repository navigation
fix(security): digest small hosts files on POSIX too (#21051) - #21865
Conversation
UX Review (Opus 5.5) — ⏭️ skippedRevision |
Design Review (Opus 5.5) — ✅ PASSDesign-level review of Design-Verdict: PASS Check that the issue-tracker reproduction (176/200 stale answers in a tight loop) now gives zero on a real coarse-ctime filesystem. [DESIGN-REVIEWED] fdc2332 Description read[DESCRIPTION-READ] 4cbb51eea4bb9254f2fda30219f844170cd5615485c21a788ce788d33fc463da That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The fix is sound for the hosts table, but the same coarse-ctime stat key still guards the governance-profile cache, and "Not generalizable" never mentions it. Not justified as shipped
What this change shipsInventory (6 items) — 5 justifiedIntent: stop the ssh self-target floor on Linux from serving a stale hosts table after a fast same-size rewrite. FIX.
[FIRST-PRINCIPLES-REVIEWED] fdc2332 Description read[DESCRIPTION-READ] c7b2e371bd6d46dd6fb2b9ffcf572f5e951643e1fbc724786665be0349495e93 That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 6.1 Review — ✅ no blocking findingsGPT 6.1 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Security Scope Review -- ✅ PASSWhich legitimate operations does Verdict basis: adjudicated, zero confirmed regressions. Security scope review -- adjudicated candidates
No candidate flipped from allowed to refused. Scope-Verdict: PASS TIGHTENING: The cache key for the ssh-self floor's hosts-file table now includes a content digest on POSIX as well as Windows. Any hosts file up to one read chunk (64 KiB) is re-read and hashed on every gate call. Before, POSIX keyed on stat alone. A failed or oversize digest read raises ADJUDICATED: 16 candidates, 0 confirmed newly-refused. The harness handed me no confirmed rows; the per-leg verdicts come from the deterministic fold. PLATFORM COVERAGE:
[SCOPE-REVIEWED] fdc2332 |
|
🤖 Kiro Crew Auto-Pipeline [operator: buluoray#ce473fca]: Sensitive change review
|
|
🤖 Kiro Crew Auto-Pipeline [operator: buluoray#ce473fca]: Disposition of the Security Scope Review CONCERNS on 542e780, plus the CI red, both on head 8b74a3c. CI red (self-caused, fixed). Backend Tests shard 2 on Linux and Windows failed Unadjudicated item, case 1: an unreadable hosts file (e.g. 0600 root). Not new. On main a POSIX key reads nothing, but a cache miss parses the file. That read fails, Case 2: the file grows past 64 KiB between the stat and the read. Transient on both trees. On main the in-call parse is bounded by the same chunk and raises |
542e780 to
8b74a3c
Compare
|
🤖 Kiro Crew Auto-Pipeline [operator: buluoray#ce473fca]: Sensitive change review
|
|
🤖 Kiro Crew Auto-Pipeline [operator: buluoray#ce473fca]: Disposition of the Security Scope Review CONCERNS on 8b74a3c. No code change. Both states were already pending on main. (a) hosts file can be stat'ed but not opened. On main a cache miss parses the file. The parse read raises (b) a file of 64 KiB or less grows past 64 KiB between the stat and the read. This PR adds at most one pending answer, inside a window main answers pending right after. In that call the digest read finds more than a chunk and answers pending. On main the same call could still serve the table cached under the pre-growth stat. From the next call on, both trees stat the larger size. On an event loop both schedule the warm and answer pending until it lands. The suggested fallback to a stat-only key would not avoid that. With a small size in the key, the on-loop parse is bounded by the same chunk, raises |
8b74a3c to
0431f30
Compare
|
🤖 Kiro Crew Auto-Pipeline [operator: buluoray#ce473fca]: Sensitive change review
|
The ssh self-target floor caches its parsed hosts table under a key built from os.stat plus the own-address state. On POSIX the key relied on st_ctime to see a same-size rewrite that restores mtime, but Linux stamps ctime from a coarse clock, so two writes inside one tick left the key unchanged and the gate served a table parsed from the old content. Take the bounded blake2b digest the Windows key already carries for any file no larger than one read chunk on every platform. The rule that a file over one chunk is unverifiable and never served stays Windows-only, where ctime is creation time; the remaining same-tick window for a POSIX file over one chunk is written down in the key's docstring. The platform predicate is renamed to say what it tests. Closes #21051
0431f30 to
fdc2332
Compare
|
🤖 Kiro Crew Auto-Pipeline [operator: buluoray#ce473fca]: Sensitive change review
|
|
HOLD on fdc2332 (crew-pr-reviewer, read-only). This PR changes the ssh self-target floor in argv_floor_hosts.py#L262: on POSIX the hosts-table cache key now carries a content digest, and a failed digest read answers pending. That changes what the sandbox floor refuses, so it is reserved for a human maintainer even though the code looks sound. Auto-merge is armed on this PR, so the hold needs a human action to take effect. One AI concern is unanswered: the First Principles lane (CONCERNS) points out that governance_profiles.py _dir_fingerprint has the same same-size, one-tick blind spot. Please post a disposition, and either open an issue for it and link it in the description, or show the path is unreachable. The change itself is small and fails closed. The Security Scope Review found 0 newly refused commands on macOS, Ubuntu and Windows, and CI is green on this head. — crew-pr-reviewer (r111) |
bolichen97
left a comment
There was a problem hiding this comment.
Deep review passed. The POSIX hosts-file digest closes a stale-cache path and fails closed on read errors.
Sensitive change review
- Rule changed: small POSIX hosts files now join the cache key with a bounded content digest.
- Before -> after: a same-size rewrite inside one timestamp tick could reuse an old host map; changed bytes now force a parse.
- Worst case if wrong: a local alias could be mistaken for a remote SSH target, or valid dotless targets could be refused.
- Evidence checked: the parsed buffer must match the keyed digest; the key is checked again after parsing; reads are bounded; focused tests and mutation checks cover POSIX rewrites and read failures.
- No regression found: failures remain fail closed, Windows behavior is unchanged, and all required checks pass.
- Undo: revert this pull request.
Follow-up: add an A-to-B-to-A race test for the digest comparison, and track the similar config-directory fingerprint gap separately.
Problem / Motivation
Goal: On Linux, rewriting the hosts file to the same size within one coarse ctime tick always makes the ssh/curl self-host floor re-read it, so a name the new content maps to this machine is never answered from the table parsed from the old content.
The self-target floor caches its parsed hosts table under
_hosts_file_key:(st_mtime, st_ctime, st_size, digest, published, own). The content digest was taken only on Windows, so on POSIX the key relied onst_ctimeto see a same-size rewrite that restores mtime. Linux stamps ctime from a coarse clock (about 10 ms on xfs in the report), so two writes inside one tick leave(mtime, ctime, size)unchanged and the gate keeps serving the old table. The report reproduced a stale remote answer in 176 of 200 tight-loop iterations.Why it matters
The floor exists to refuse an ssh/scp/sftp/rsync target that is really this machine. A hosts alias rewritten to a local address inside one tick was judged remote until something else moved the key. Writing the hosts file needs privilege, so this is a robustness gap in the cache key rather than a privilege boundary, but the floor should not depend on clock granularity.
Not a goal
_hosts_file_keydocstring instead.st_ino(the report's option 1): it misses an in-place write, which is the reported case.What changed (motivation → approach → change)
Symptom: a same-size rewrite inside one ctime tick is served from the stale table. Root cause: on POSIX the key carried no content digest, and ctime alone cannot separate two writes inside one tick. Change (the report's option 3, reusing machinery the module already has):
_hosts_file_keynow takes the bounded blake2b digest for any file no larger than_HOSTS_FILE_READ_CHUNK(64 KiB) on every platform, not only on Windows. The read is bounded by the chunk size and its failure raises_HostsFileUnreadable, exactly as on Windows today._parse_and_cache_for_keyalready hashes and parses from one buffer whenever the key carries a digest, so on POSIX the table is now also built from exactly the content its key names. No new refusal: a small file whose read fails was already pending through the parse path._hosts_content_digest_enabledto_ctime_is_creation_time, since it no longer gates the digest and only says what the platform's ctime means.docs/system-specs/modules/security.md, ssh self-target floor) now states the key shape.Cost: one bounded read and hash per gate call for a typical hosts file, the same as Windows pays today.
Backwards compatibility
Compatible: the key only gains a field value on POSIX, so a rewrite that changed the key before still changes it; every refusal on main still refuses.
Reader: src/kiro_crew/security/argv_floor.py:_unverifiable_hosts_file -- entry: chat|cron|subagent -- calls the renamed
_ctime_is_creation_time, same Windows-only answer; test_windows_large_file_never_serves_a_remote_or_absent_verdictReader: test/test_denied_commands_security.py:TestHostsFileWarmUp fixture and
_windows-- entry: chat -- patch the renamed seam; the whole class passesTests
In
test/test_denied_commands_security.py,TestHostsFileWarmUp:test_posix_same_size_rewrite_inside_one_ctime_tick_re_parses: own set holds10.4.4.4, the hosts file holds10.9.9.9 farbox, and anos.statseam returns the first stat for every call, so mtime, ctime and size never move (no real clock involved). The first verdict is remote (False); after an in-place same-size rewrite to10.4.4.4 farboxthe next verdict is local (True) and the cached table is the new one.test_posix_large_file_keeps_a_stat_key_and_is_served: a small POSIX file's key carries a digest; a file over one chunk keeps digestNoneand is still served after a warm (POSIX is not routed into the Windows unverifiable rule).test_small_file_read_failure_on_the_digest_is_pending[windows|posix](was the Windows-onlytest_windows_small_file_read_failure_on_the_digest_is_pending): a digest read that fails answers pending even with a table cached. This is new on POSIX, a fail-closed answer while the file cannot be read, so it is now pinned there too.test_posix_key_does_no_content_readis removed: it asserted the old behaviour this PR changes.Mutation: restoring the Windows-only condition (
if _ctime_is_creation_time() and stat.st_size <= _HOSTS_FILE_READ_CHUNK:) reddens the three POSIX tests (..._re_parsesand..._is_pending[posix]withassert False is True,..._is_servedwithassert None is not None). Local runs: the whole oftest/test_denied_commands_security.pypasses with-n0(1372 passed, 1 skipped), plustest/test_flake_pattern_ratchet.py.Manual verification
N/A — unit coverage sufficient: the stat seam reproduces the coarse-tick case exactly (unchanged mtime, ctime and size). Not reproduced on a real coarse-tick filesystem here, and the Windows path is unchanged and not run on a Windows host.
Related Issues
Closes #21051
Pattern harvest
Not generalizable: a cache key that trusts a filesystem timestamp to separate writes is specific to this cache; the module's other stamp checks (the native-loader digest in the same spec) already pair the stamp with a content hash.
Decision for the owner
The decision is whether the POSIX hosts-file key should pay one bounded 64 KiB read and hash per gate call to close the coarse-ctime gap. This draft chose yes, reusing the existing Windows digest path, over keying on
st_ino, which would miss the in-place rewrite the issue reports.Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
Contributions are accepted under the project's Apache-2.0 license.