You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up from non-blocking review comments on #509 (fix(cli): reject and never write through symlinked cds get destinations), which hardened cli/getter.py against a symlink planted at the leaf destination path.
Remaining gaps
Symlinked parent directories are not guarded._write_actions/mkdir(parents=True, exist_ok=True) only checks the destination leaf path for a symlink. If an ancestor directory (e.g. profiles/ itself) is a symlink, mkdir/shutil.copy2 will still write through it. (cli/getter.py:643, review comment fix(cli): reject and never write through symlinked cds get destinations #509 (comment))
TOCTOU window between unlink() and copy2(). An attacker who replants a symlink in that narrow gap could still redirect the write. Full closure needs os.open(dest, O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW) plus a manual byte copy instead of shutil.copy2.
Test gap: existing symlink-guard tests in tests/test_getter.py only plant non-dangling symlinks (attack_target exists, then symlink_to). No test exercises a dangling symlink (target absent) even though the code comments claim "dangling or not" is handled. (review comment fix(cli): reject and never write through symlinked cds get destinations #509 (comment))
Suggested follow-up work
Guard ancestor directories of each destination for symlinks before mkdir(parents=True, ...).
Replace the unlink() + shutil.copy2() sequence with an atomic O_EXCL|O_NOFOLLOW open + manual byte copy to close the TOCTOU window.
Add a regression test that plants a dangling symlink at a destination path and asserts cds get rejects it the same way as a non-dangling one.
These were explicitly called out as non-blocking/out-of-scope for #509 (leaf-specific fix) and deferred to a follow-up.
Context
Follow-up from non-blocking review comments on #509 (fix(cli): reject and never write through symlinked cds get destinations), which hardened
cli/getter.pyagainst a symlink planted at the leaf destination path.Remaining gaps
_write_actions/mkdir(parents=True, exist_ok=True)only checks the destination leaf path for a symlink. If an ancestor directory (e.g.profiles/itself) is a symlink,mkdir/shutil.copy2will still write through it. (cli/getter.py:643, review comment fix(cli): reject and never write through symlinked cds get destinations #509 (comment))unlink()andcopy2(). An attacker who replants a symlink in that narrow gap could still redirect the write. Full closure needsos.open(dest, O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW)plus a manual byte copy instead ofshutil.copy2.tests/test_getter.pyonly plant non-dangling symlinks (attack_targetexists, thensymlink_to). No test exercises a dangling symlink (target absent) even though the code comments claim "dangling or not" is handled. (review comment fix(cli): reject and never write through symlinked cds get destinations #509 (comment))Suggested follow-up work
mkdir(parents=True, ...).unlink()+shutil.copy2()sequence with an atomicO_EXCL|O_NOFOLLOWopen + manual byte copy to close the TOCTOU window.cds getrejects it the same way as a non-dangling one.These were explicitly called out as non-blocking/out-of-scope for #509 (leaf-specific fix) and deferred to a follow-up.