Skip to content

fix(file): let a created file inherit its directory DACL - #571

Closed
tietoumiao wants to merge 1 commit into
T-Auto:mainfrom
tietoumiao:fix/420-write-file-inherited-dacl
Closed

tietoumiao wants to merge 1 commit into
T-Auto:mainfrom
tietoumiao:fix/420-write-file-inherited-dacl

Conversation

@tietoumiao

@tietoumiao tietoumiao commented Nov 1, 2026 •

Copy link
Copy Markdown

Closes #420

What was wrong

A file created by ops_write_file kept only the caller's explicit ACE:
native/windows-metadata/src/lib.rs built a protected descriptor D:P(A;;FA;;<sid>), so the
parent directory's inheritable ACEs were dropped. The symptom in the issue is immediate: the
freshly created file cannot be deleted in the same session, and other tools/users lose access to it.

The change

  • native/windows-metadata/src/lib.rs — after the bytes exist, ask the OS to recompute the staged
    DACL with inheritance (SetSecurityInfo(… DACL_SECURITY_INFORMATION | UNPROTECTED_DACL_SECURITY_INFORMATION …)),
    so the child carries the directory's inheritable ACEs as inherited entries plus one explicit
    full-control ACE for the caller. verify_adopted still fails closed before publication: every
    object-inheritable directory entry must be present, the caller's explicit ACE must be there, and
    the DACL must not be protected. Both sides are reduced through MapGenericMask (file mapping)
    before the exact match, because Windows maps GA/SDGXGWGR/GXGR to specific rights while it
    propagates them — the earlier code compared the stored generic spelling against the propagated
    mask and read a correctly inherited GA as missing.
  • native/crates/ops-files/src/{file_publication.rs,file_single.rs} — plumb the adoption into the
    create path; staging stays private until the bytes exist.
  • test/file-inherited-dacl.test.mjs (new) + a windows-metadata unit test, docs/CHANGELOG.md,
    docs/windows-security-regressions.md + docs/Windows安全回归.md (the W1 row, including "existing
    files do not self-heal").

Deliberately not done: overwrite, copy/move and cache objects keep their current descriptors; no
retroactive repair of files created by older versions.

Validation

$ cargo test --locked -p dsh-ops-windows-metadata --manifest-path native/Cargo.toml
test tests::a_created_file_adopts_directory_grants_and_keeps_the_caller_ace ... ok
test result: ok. 3 passed; 0 failed

$ node bin/dsh-ops.mjs build-ptc        # rebuilt + staged worker from this branch
$ node test/run.mjs file-inherited-dacl
  ok   a created file inherits its directory grants and stays deletable in the same session
dsh-ops file-inherited-dacl: all 1 checks passed

# revert-red 1 (adoption removed)
FAILED … "Adopted DACL is still protected; the created file cannot inherit"   (2 passed; 1 failed)
# revert-red 2 (generic→specific mapping removed)
FAILED … "Adopted DACL is missing a directory grant the file should inherit"  (0 passed; 1 failed)
  • 撤掉本次实现后变红的检查:上面两条
  • docs/CHANGELOG.md ## [Unreleased]:已加(EN+ZH)
  • 面向模型的描述 / 参数:无变化(只改创建时的安全描述符)
  • CI:待维护者批准后由 pull_request 事件触发,届时回填 CI required 的 run URL

Environment

  • OS / 架构:Windows 11 x64
  • Node:24.16.0
  • Rust:1.97.1(cargo build --release --locked)
  • 基于上游 main 的 commit:065cd7f
  • 安装通道:源码 tarball + npm ci --ignore-scripts

未验证 / 风险披露

  • 方向性:采用"让 OS 重算 DACL"后,父目录上的 deny ACE 也会作用到新建文件(旧行为被保护
    DACL 挡住)。这是 issue 想要的方向,但若目录上有针对创建者的 deny,行为会变化。
    verify_adopted 仍要求调用者显式 ACE 且失败即中止,不发布半成品。
  • 已有文件不会自愈(已写进 W1i 行与 CHANGELOG)。
  • 本机 cargo test --workspace 的 ops-results 4 项失败是预存的(受限令牌 + %TEMP%),在
    pristine main 上复现,与本片无关。

CI

$ gh run view 41338748305 --json url,event,conclusion --jq '"(.url) (.event) (.conclusion)"'
https://github.com/T-Auto/dsh-ops/actions/runs/41338748305 pull_request action_required

A file created by ops_write_file kept only the caller's explicit ACE. Staging builds a protected D:P(A;;FA;;;<sid>) descriptor before any byte exists and publication hard-links that inode, so the directory's inheritable grants - including the sandbox write capability the host granted the workspace - never reached the published file. The same session could not delete what it had just created, and other tools or accounts the directory grants lost access (T-Auto#420).

The staged object stays private until its bytes are complete; ops_write_file then asks the operating system to recompute that DACL from the directory with UNPROTECTED semantics, which keeps the caller's explicit full-control entry, adds every object-inheritable directory entry as an inherited ACE, and leaves directory deny entries in force because the kernel merges them instead of this crate rebuilding the set. Only creation adopts: an overwritten file keeps its own descriptor, a copy keeps its source descriptor, and cache objects stay private.

The fail-closed verification compares generic rights as the specific rights Windows propagates: a directory entry spelled GA reaches a file as FA, SDGXGWGR as 0x1301bf and GXGR as 0x1200a9, so MapGenericMask reduces both sides before the exact match. Without that reduction a correctly inherited generic entry reads as missing and every workspace whose ACL carries inherit-only generic entries refuses the create.

Asserted twice: native/windows-metadata creates a child under a directory with a distinctive inheritable grant plus an inherit-only generic grant and requires both plus the caller's explicit ACE, and test/file-inherited-dacl drives the real worker to create a file, compares its SDDL against the directory's inheritable entries with the same mapping, and deletes it in the same session. Removing the DACL write turns the native test red with 'Adopted DACL is still protected'; dropping the mask mapping turns it red with 'missing a directory grant'.

Existing files do not self-heal: a file published by an older version keeps its descriptor until it is recreated. Documented in the Windows security regression map.
@Haoming9527

Copy link
Copy Markdown
Collaborator

This overlaps #547's create-only DACL work. The two implementations should be reviewed as alternatives, with ordinary CreateFile inheritance as the comparison. Two concrete gaps remain in head 93ead1b:

  1. verify_adopted() compares parent and child entries by unchanged SID after mapping only generic rights. A parent (A;OIIO;FA;;;CO) entry is a valid counterexample: Windows substitutes CREATOR OWNER with the child's owner SID, so present.contains(&grant) rejects a correctly inherited file. Windows ACE inheritance rules cover both generic rights and generic SID substitution. Please include creator-owner, inherited deny and token-default controls against an ordinarily created sibling, rather than requiring every parent SID to survive literally.
  2. The staging handle still uses FILE_SHARE_READ|WRITE|DELETE (7). After adopt_parent_dacl() expands permissions and before the hard-link publication, another writer admitted by the inherited DACL can open and alter the staged bytes. The subsequent sync_all() does not revalidate their hash, so the published bytes can disagree with the operation's receipt. fix(file): inherit parent permissions for new Windows files #547 keeps FILE_SHARE_WRITE excluded for the full publication lifetime and has a competing-writer regression after DACL expansion; this branch needs an equivalent invariant.

Also keep the desired semantics explicit: preserving an extra caller full-control ACE differs from the ordinary file's inherited permission set, especially with inherited denies. This source review does not claim to have run this branch; the original work remains available for comparison.

@Nicholas023 Nicholas023 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

审查结论:REQUEST_CHANGES — 建议关闭

方向(新建后让 OS 重算 DACL)是对的,但本 PR 引入了一个 0.5.0 基线没有的崩溃路径。

1. NULL DACL 父目录会让 worker panic(阻断,新缺陷)

父目录为 NULL DACL 时,本 PR 的 worker:

text exit: 101 stderr: thread 'main' panicked at windows-permissions-0.2.4/.../get_security_descriptor_dacl_sacl.rs:53:1: $f indicated success but returned NULL dir now: [".c.txt.dsh-ops-31cf596-…"] ← staging 泄漏

控制变量对照(唯一变量 = worker):

text worker NULL DACL 父目录 基线 8934fd33 exit 0,completed,文件正常创建 本 PR fb4421a4 exit 101,panic,staging 泄漏

let Some(dacl) = sd.dacl() else … 的防御是死代码 —— 该库对 present-but-NULL DACL 直接 panic。这违反「工具失败必须是返回值,不得 panic」。

2. 新 e2e 在非 Windows 静默通过(阻断)

test/file-inherited-dacl.test.mjs 的 report() / process.exit() 都在 if (process.platform === 'win32') 内部。模拟非 Windows 后 runner 报 1 suite(s) passed —— 零检查算通过,与「skip 不算 pass」相反,且 verify-gate 的 POSIX allow-skip 白名单里也没有它。

3. 与 #547 重叠且更弱

verify_adopted() 按「不变 SID」逐条比对,会被 Windows 的 CREATOR OWNER/CREATOR GROUP 替换规则误拒(父目录 (A;OIIO;FA;;;CO) 传播后变成 owner SID);staging 句柄仍用 FILE_SHARE_READ|WRITE|DELETE,adopt 扩权后到硬链接发布前存在写共享窗口。#547 用 CreatePrivateObjectSecurityEx + held parent descriptor,并在整个发布生命周期排除 FILE_SHARE_WRITE。

4. e2e 对环境假设脆弱

默认环境下直接红:ACCESS_DENIED(受限令牌写 %TEMP%),且 whoami 解析到 Git coreutils 报 extra operand '/user'。

已核实为真的部分

  • 0.5.0 基线上真 worker 创建的文件 DACL 只有 1 条非继承 ACE;同目录普通创建有 5 条继承 ACE —— 问题本身真实存在
  • Rust 单测 3 passed(需正确设置 TEMP/TMPDIR)
  • revert-red(去掉 UnprotectedDacl)→ Adopted DACL is still protected 变红,还原后 3 passed

处置建议

关闭。 采纳 #547。若要救本 PR,需要:与普通创建的文件逐字比较 DACL、NULL DACL 不 panic、补非 Windows skip 与 gate 白名单、修 whoami 绝对路径、排除 FILE_SHARE_WRITE —— 做完基本就收敛成 #547 了。

@T-Auto T-Auto closed this Nov 2, 2026
@tietoumiao

Copy link
Copy Markdown
Author

Closing — #547 owns the DACL adoption and is in main (#557), with CreatePrivateObjectSecurityEx, a held parent descriptor and no FILE_SHARE_WRITE in the publication window. Both blocking findings are accepted: the NULL-DACL parent made the worker panic (a failure must be a returned value, not a panic) and the new e2e counted zero checks as a pass on non-Windows. The problem statement itself was verified true, which is why #547 is the right home.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants