fix(module): accept Windows creator-owner module cache ACL - #25
Conversation
Tiny Sweeper reviewThis pull request adds support for the CREATOR OWNER well-known SID and the current process user SID in the Windows ACL check. Review across 6 lanes found 3 active actionable findings: the CREATOR OWNER SID is trusted unconditionally, which may grant write access to untrusted accounts; and the new user-SID trust path lacks a regression test. The change is not safe to merge as-is. State: Changes requested Review snapshot
Completeness: Complete What changedModifies `windows_directory_grants_untrusted_write` to treat the CREATOR OWNER well-known SID and the current process token user SID as trusted when checking ACLs, with two added Windows tests. This prevents false refusals for directories with inheritable creator-owner permissions, but introduces an unconditional trust for CREATOR OWNER that may accept directories writable by an untrusted account. Features
TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Resolved this pass
Before merge
How this fits togetherflowchart LR
n0["attach_raw"]:::impacted
n1["ModuleInfo"]:::impacted
n2["load_dir"]:::impacted
n3["register_lazy"]:::impacted
n0 -->|uses| n1
n2 -->|uses| n1
n2 -->|calls| n3
n3 -->|uses| n1
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0027 · 112,564 in / 9,178 out · 20,876 cached (19%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 285 embedded
critique: $0.0010 · 35,132 in / 1,658 out · 2,112 cached (6%) · gpt-5.6-luna
security: $0.0009 · 34,644 in / 1,140 out · 1,868 cached (5%) · gpt-5.6-luna
tests: $0.0005 · 32,860 in / 4,102 out · 15,360 cached (47%) · deepseek-v4-flash
description: $0.0001 · 7,173 in / 589 out · 1,536 cached (21%) · deepseek-v4-flash
tinysweeper
Bot
added
the
priority: p3
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0049 · 179,613 in / 17,074 out · 9,432 cached (5%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 380 embedded
critique: $0.0022 · 81,105 in / 4,763 out · 2,029 cached (3%) · gpt-5.6-luna, deepseek-v4-flash
security: $0.0019 · 68,771 in / 2,756 out · 5,355 cached (8%) · gpt-5.6-luna
tests: $0.0004 · 16,972 in / 3,056 out · 1,024 cached (6%) · deepseek-v4-flash
description: $0.0003 · 8,520 in / 5,059 out · 1,024 cached (12%) · deepseek-v4-flash
There was a problem hiding this comment.
Requesting changes: 3 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0038 · 117,532 in / 23,201 out · 14,552 cached (12%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 380 embedded
critique: $0.0014 · 39,214 in / 6,148 out · 4,160 cached (11%) · gpt-5.6-luna, deepseek-v4-flash
security: $0.0015 · 49,658 in / 2,760 out · 3,736 cached (8%) · gpt-5.6-luna
tests: $0.0005 · 14,242 in / 8,030 out · 2,048 cached (14%) · deepseek-v4-flash
description: $0.0002 · 5,752 in / 4,402 out · 1,024 cached (18%) · deepseek-v4-flash
|
|
||
| #[cfg(windows)] | ||
| #[test] | ||
| fn a_creator_owner_cache_directory_is_accepted() { |
There was a problem hiding this comment.
Test acceptance of an explicit current-user write ACE
The implementation now trusts the process token's user SID, but the tests only cover CREATOR OWNER and Everyone. They do not establish the new contract that a directory writable by the current user is accepted, nor distinguish that case from a directory writable by another account. Add a Windows test that grants write access to the current token's user SID and verifies acceptance, plus a distinct non-current-user principal case.
[RULE] missing-regression-test ·
| || unsafe { EqualSid(sid, admin_sid.as_ptr().cast()) } != 0 | ||
| || unsafe { EqualSid(sid, system_sid.as_ptr().cast()) } != 0; | ||
| || unsafe { EqualSid(sid, system_sid.as_ptr().cast()) } != 0 | ||
| // CREATOR OWNER is an inheritable placeholder for the owner |
There was a problem hiding this comment.
Do not trust CREATOR OWNER as an unconditional principal
This accepts any write-capable ACE whose SID is CREATOR OWNER without checking that it is inherit-only or otherwise limited to the intended inheritance semantics. A directory with this ACE can grant the creator of a child module ownership and write access, allowing another account to replace a module that this check admits. Only accept CREATOR OWNER when the ACE flags prove it cannot grant write access to an untrusted account for the directory or its module children, or reject it by default.
Additional critique observation
Do not trust CREATOR OWNER unconditionally
[RULE] unconditional-principal-trust
This treats every allowed ACE whose SID is CREATOR OWNER as trusted, without checking whether the ACE is inherit-only or whether its mask grants any write permission. A directory ACL containing a CREATOR OWNER ACE with write access that applies to the directory can therefore be classified as safe even though the effective creator/owner principal is not necessarily one of the explicitly trusted principals. Model the ACE inheritance/effective-principal semantics, or only accept CREATOR OWNER when the ACE cannot grant write access to the checked directory itself.
[RULE] unsafe-acl-principal ·
| if user_read == 0 { | ||
| return true; | ||
| } | ||
| let user_sid = unsafe { (*user.as_ptr().cast::<SidAndAttributes>()).sid }; |
There was a problem hiding this comment.
Test the new user-SID trust path
The new process-token SID is now accepted as trusted, but the Windows tests only cover CREATOR OWNER and Everyone. Add a test that grants write access specifically to the current user's SID and verifies admission, plus a contrasting test for a different user SID, so this security-sensitive exception is exercised rather than only compiled.
[RULE] missing-security-test ·
| let trusted = unsafe { EqualSid(sid, owner) } != 0 | ||
| // A directory's owner may be Administrators even when this | ||
| // process has its own explicit full-control ACE. | ||
| || unsafe { EqualSid(sid, user_sid) } != 0 |
There was a problem hiding this comment.
Test the new user-SID trust path
The new code trusts the current process user SID when checking directory ACLs, but there is no test that verifies this path. Add a test that creates a directory with an explicit ACE granting full control to the current user (e.g., via icacls /grant "*S-1-5-21-...:F") and asserts that windows_directory_grants_untrusted_write returns false.
[RULE] untested-behaviour ·
1 participant
Summary
CREATOR OWNERplaceholder when admitting a native module directory.Everyone, and require a trusted file owner.CREATOR OWNER, the process SID, and broad write grants on directories and files.Why
OpenHuman 0.64.0 reports Sentry issue TAURI-RUST-113D: its TinyMemory module is refused on Windows with
module directory is writable by another user. The ACL scan classified both an inheritedCREATOR OWNERentry and an explicit write ACE for the running account as unrelated writers when the directory owner SID differs from the process SID. The Windows CI runner reproduced that ACL shape.Validation
cargo fmt --all -- --checkcargo test --locked -p tinybus --features modules module::host::tests --lib -- --nocapture(33 passed, 10 fixture-dependent ignored on macOS)The OpenHuman gitlink update will follow this upstream fix.