Skip to content

syscall: use O_PATH for held directory traversal - #1065

Open
FusionPow wants to merge 1 commit into
RsyncProject:masterfrom
FusionPow:issue-1064-search-only-dirfds
Open

syscall: use O_PATH for held directory traversal#1065
FusionPow wants to merge 1 commit into
RsyncProject:masterfrom
FusionPow:issue-1064-search-only-dirfds

Conversation

@FusionPow

Copy link
Copy Markdown

Use O_PATH on Linux for directory descriptors used only for path traversal, fchdir(), or as *at() parents. Reopen final directory endpoints with the caller-requested access mode and retain the existing O_RDONLY fallback elsewhere.

Add coverage for exact sources and readable descendants beneath search-only directories, known-file creation beneath write/search-only destinations, and retained refusal to enumerate unreadable directories.

Fixes #1064.

Use O_PATH on Linux for directory descriptors used only for path
traversal, fchdir(), or as *at() parents. Reopen final directory
endpoints with the caller-requested access mode and retain the existing
O_RDONLY fallback elsewhere.

Add coverage for exact sources and readable descendants beneath
search-only directories, known-file creation beneath write/search-only
destinations, and retained refusal to enumerate unreadable directories.

Fixes RsyncProject#1064.
@FusionPow
FusionPow force-pushed the issue-1064-search-only-dirfds branch from 05c00c3 to c512980 Compare August 20, 2026 20:56
@seks99x

seks99x commented Aug 22, 2026

Copy link
Copy Markdown
Member

This looks good Thanks!

I think there is still some cases like this not covered in the source which I still didn't confirm, which if confirmed, we should step back and find a broader solution that covers all edge cases cleanly ( like fixing ds_take() itself or something ) rather than stacking patches on top of one another. I'll come back to this again.

@steadytao

Copy link
Copy Markdown
Member

It does LGTM. I'll leave this for a bit while @seks99x looks for some possible untouched cases but overall good to merge in about 2-3 days if unconfirmed.

@seks99x

seks99x commented Aug 25, 2026

Copy link
Copy Markdown
Member

Sorry for the delay. I'll confirm this tomorrow

@tridge

tridge commented Aug 25, 2026

Copy link
Copy Markdown
Member

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report (with the other AIReview PR): https://uav.tridgell.net/RsyncReviews/index.html#pr1065

Reviewed at head c512980a46 — verdict: COMMENT. Nicely constructed. The split into secure_relative_open() (readable endpoint) vs the new secure_relative_dirfd() (O_PATH endpoint) is clean, and I traced every consumer of the now-O_PATH held fd (generator.c, sender.c, delete.c, rsync.c, util1.c, the do_*_atfd shims, the ACL /proc/self/fd path): each uses it only for *at(), fchdir(), or as pathname authority — all legal on an O_PATH dir fd. The one forbidden op, fdopendir(), stays on a readable fd (secure_opendir() requests O_RDONLY|O_DIRECTORY; delete enumeration goes via get_dirlist(), not del_dirfd). The fallback is real (O_RDONLY|O_DIRECTORY, not #define O_PATH 0), the security property is unchanged (pinned fds + per-component O_NOFOLLOW), and the endpoint-reopen refactor is fd-clean. No blocking defect. Three points:

  • ISSUE (portability) — fchdir() on an O_PATH fd regresses on Linux 2.6.39–3.4. O_PATH arrived in 2.6.39, but O_PATH descriptors only became usable with fchdir() in 3.5. The held dirfds are now O_PATH (syscall.c:82) and the confinement path fchdir()s them (util1.c:1258/1351/1369), so a binary built against current headers and run on a 2.6.39–3.4 kernel gets EBADF — daemon-module / receiver-destination setup fails even for ordinary readable dirs (pre-2.6.39, the O_PATH opens themselves fail). FreeBSD is unaffected (its O_PATH permits fchdir()). Those kernels are long EOL so impact is low, but it's a silent regression on a security-relevant path; a cheap EBADF fallback (reopen "." as O_RDONLY|O_DIRECTORY relative to the pinned fd and fchdir() that) or a documented minimum kernel would close it. (Surfaced by an independent cold pass; verified against source + kernel history.)
  • NOTE — SECURITY.md over-states the non-Linux fallback: it says other platforms keep the O_RDONLY fallback, but the gate is feature-based (#if defined O_PATH), so FreeBSD ≥13 also uses O_PATH fds (and supports them for fchdir()). Not a regression there — but the BSD path is untested (the new test self-skips off Linux), so a FreeBSD smoke test is worth it and the doc should be corrected.
  • NOTE — SECURITY.md O_NOFOLLOW wording: it says anchors use O_PATH|O_DIRECTORY|O_NOFOLLOW, but anchors are opened without O_NOFOLLOW (open_anchor_dirfd); only traversal components carry it.

(seks99x/steadytao have already LGTM'd; none of the above blocks that.)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rsync still requires read permission for search-only directories

4 participants