Repository navigation
fix(rdpdr): confine Unix drive paths to the share - #2221
Marc-André Moreau (mamoreau-devolutions) merged 8 commits into
Conversation
The Unix drive backend appended server-supplied paths to the shared directory as strings, so a `..` component let a malicious server create, read, truncate, rename, and delete files anywhere the client user can. Paths in create, rename, and query-directory requests are now split into components, and empty, `.`, `..`, and components containing `/` or NUL are rejected with STATUS_OBJECT_NAME_INVALID. Each path is opened one component at a time relative to a descriptor of the shared directory, with O_NOFOLLOW, so symbolic links inside the share are not followed either. Directory entries are stat'ed relative to the enumerated directory without following links. The backend also bounds what a server can consume: reads longer than 1 MiB fail with STATUS_INVALID_PARAMETER instead of allocating up to 4 GiB, and creates beyond a quarter of the soft descriptor limit (at most 1024 open files) fail with STATUS_INSUFFICIENT_RESOURCES. Side effects: a renamed handle now tracks its new path, so a later delete removes the renamed file; creating a directory no longer creates missing parents; and enumeration skips `.` and `..` consistently. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DpFKKTgMAgvuA7bM3LUXek
Opening a path component now refuses a mount point as well as a symbolic link. On Linux 5.6 and later, openat2 with RESOLVE_NO_XDEV refuses every mount, bind mounts included; elsewhere, a descriptor on a different filesystem than the shared directory is refused. Files are also opened with O_NONBLOCK, so opening a FIFO in the share no longer blocks the channel until a writer appears. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DpFKKTgMAgvuA7bM3LUXek
Rename now honors ReplaceIfExists and reports STATUS_OBJECT_NAME_COLLISION instead of overwriting an existing target. A disposition request with DeletePending cleared no longer deletes the file, and a directory handle is removed as a directory, reporting STATUS_DIRECTORY_NOT_EMPTY when it is not empty. An existing directory opened with FILE_OPEN_IF is opened read-only instead of failing, FILE_SUPERSEDE truncates instead of appending, and an initial directory query restarts enumeration even when it fails. The device comparison now runs only when openat2 is unavailable, since RESOLVE_NO_XDEV already refuses mounts and a Btrfs subvolume or overlay file reports another device without being one. End of file is set with the safe ftruncate wrapper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DpFKKTgMAgvuA7bM3LUXek
Create, rename and delete failures now report the status MS-FSA gives for the condition, such as STATUS_OBJECT_NAME_COLLISION, STATUS_OBJECT_PATH_NOT_FOUND or STATUS_FILE_IS_A_DIRECTORY, instead of STATUS_UNSUCCESSFUL. Errors map to the statuses the Windows drive backend reports, including STATUS_ACCESS_DENIED for a refused link or mount. A disposition request now marks the file for deletion when its handle closes, so clearing DeletePending before the close keeps the file, and a directory that is not empty is refused when it is marked. Renaming a file marked for deletion, a directory holding open files, or onto an existing directory fails with STATUS_ACCESS_DENIED. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge the descriptor-relative open into one function, give query directory one failure path, choose the create flags in a single match, and drop the read error enum and the cloning parent helper. Tests reuse one create helper, and new tests cover rename with ReplaceIfExists set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A pending delete was kept on the handle that set it and removed whatever held that handle's name at close, so after another handle renamed the file it could delete an unrelated file. Pending deletes are now kept per file, identified by device and inode: the file is removed when its last handle closes and only if the name still refers to it, every handle follows a rename, opening a file marked for deletion fails with STATUS_DELETE_PENDING, and FileStandardInformation reports the mark. A rename onto the source itself, such as a case-only rename on a case-insensitive filesystem, is no longer a collision, and a missing target directory reports STATUS_OBJECT_PATH_NOT_FOUND. A directory request with FILE_NON_DIRECTORY_FILE or a replacing disposition fails with STATUS_INVALID_PARAMETER. Directory listings return `.` and `..` first except for the shared directory, and an initial query restarts enumeration even when its path is rejected. Resetting the channel closes every open file, so handles the server forgot no longer count toward the open-file cap. Without openat2, a mount is refused before O_TRUNC can truncate the file behind it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Lines changed against the base:
These properties carry an
These tests run only on macOS, because they need a case- or normalization-insensitive filesystem: Note Human-tuned, LLM-assisted content. |
github-actions
Bot
added
kind/protocol
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR correctly fixes the path-traversal escape by resolving server paths component-by-component on descriptors with O_NOFOLLOW, refusing mounts (openat2 RESOLVE_NO_XDEV, st_dev fallback), and adding read/file-handle caps and O_NONBLOCK; the core confinement design is sound and well tested. Remaining issues are protocol-conformance gaps in the rewritten paths (DOS reserved names accepted, only a bare trailing '*' wildcard matched, SharedAccess ignored), two robustness gaps (openat2 EPERM silently downgrades confinement without a warning; a readdir error ends a listing as complete), and test/style cleanups (listing tests assert statuses only, is_inside strictness unpinned, duplicated inline-test setup and raw PDU offsets, debug! vs trace!, spec citations without titles).
- [protocol] Directory enumeration matches only a lone trailing '*' pattern, not the required wildcard matching — medium 🟠 — crates/ironrdp-rdpdr-native/src/nix/backend.rs
start_query enumerates only when the final component is exactly '*' and otherwise stats the pattern literally; RelativePath::parse accepts '*' and '?' inside components. MS-FSA 2.1.5.6.3, which the rewritten enumeration cites as its oracle, requires returning every entry matching the FileNamePattern via the MS-FSA 2.1.4.4 wildcard algorithm, so patterns such as *.* or *.txt (which can even exist literally on POSIX) produce STATUS_NO_SUCH_FILE while matching files exist. Pre-existing behavior and acknowledged out of scope by the author, but the enumeration was rewritten in this PR claiming that section, so the gap is fair to publish on the changed code. - [code-compressor] Oversized-read arm duplicates the DeviceReadResponse error construction — low 🟡 — crates/ironrdp-rdpdr-native/src/nix/backend.rs
The added MAX_READ_LENGTH check early-returns a DeviceReadResponse { read_data: Vec::new() } literal byte-for-byte the shape the Err arm below already builds, so the read closure has three response-construction sites (oversized, Ok, Err). Computing one result first - Err(NtStatus::INVALID_PARAMETER) for the oversized case, read_inner(...).map_err(...) otherwise, each with its existing warning - then building a single response from the (status, read_data) pair keeps identical statuses, warnings, and empty failure buffer while removing roughly ten lines and one duplicated struct literal, matching how set_information was flattened. - [rust-style] Routine per-request events logged at `debug!` instead of `trace!` — low 🟡 — crates/ironrdp-rdpdr-native/src/nix/backend.rs
STYLE.md 'Logging > Log levels' reserves `debug!` for significant one-off events and assigns anything that repeats to `trace!`. Line 761 (`debug!(?status, "Rename file failed")`) fires on every failed rename and line 1090 (`debug!(file_id, ?path, "Opened drive file")`) on every create - routine, server-driven protocol mechanics a consumer does not want at default verbosity. Change both to `trace!` with the same fields. - [rust-style] MS-FSA citations give section numbers but do not quote the section title — low 🟡 — crates/ironrdp-rdpdr-native/src/nix/backend.rs
STYLE.md 'Documentation' requires doc comments linking to reference documents to include verbatim the section name, using reference-style links. The linked doc comments at lines 45, 88, 816, and 840 cite sections only by number (e.g. 'as [MS-FSA] 2.1.5.5 describes' with a bare [MS-FSA] link), so the section title never appears. Extend each link text to include the section number and title, as in the STYLE.md good example.
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
github-actions
Bot
added
ai-reviewed/1
…e usable A failed directory read now reports its status instead of ending the listing, so a server no longer takes a partial listing for a complete one, and a directory that cannot be read is no longer marked for deletion as empty. Whether the process can use openat2 is probed once. After a successful probe, an EPERM from openat2 is returned rather than retried with openat, which would have let a bind mount through for that path. The rename check for open files below a directory runs only for rename requests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
triage/overlap
|
This pull request may overlap with #2222. Both pull requests change the Unix drive backend in ironrdp-rdpdr-native to honor NT file semantics on redirected drives, including delete-on-close marking, create dispositions, directory query matching, and protocol-visible NT status codes for the RDPDR channel. This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
github-actions
Bot
added
the
automation-failed
Several Unix drive operations could modify or delete a file other than the one the server meant, or reported a status Windows does not. Creates truncate an overwritten file only after every check passes, so a refused open leaves it intact. An invalid disposition fails with STATUS_INVALID_PARAMETER, names Windows cannot hold fail with STATUS_OBJECT_NAME_INVALID, and a path through any directory marked for deletion fails with STATUS_DELETE_PENDING. A rename moves only the file the handle holds, never whatever took its name since. Every handle opened through the renamed link follows it, under any spelling, while handles opened through other hard links keep theirs. Renaming onto another hard link of the same file removes the source link, but a name that only spells the source differently, as a case- or normalization-insensitive filesystem allows, never removes it. A rename refuses a target that is marked for deletion, open, or the share itself, and refuses moving into a directory marked for deletion. A directory holding open files is found by the identities of the directories each file was reached through, not by path. A delete removes the name it was marked through, and only while that name still refers to the marked file. It runs when the last handle closes, the channel resets or the backend drops. Marking a directory reads the open directory rather than its path, and the share itself cannot be marked. FileStandardInformation counts one link for a directory and leaves the marked link out. A directory query reports why it failed instead of an empty match, fails a listing on an entry it cannot inspect, and accepts the DOS wildcards in patterns. File IDs skip IDs still in use. Routine failures are logged at trace level. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XBnfMdFRrnCKLfmqueR2m
github-actions
Bot
added
kind/technical-debt
There was a problem hiding this comment.
The rewrite correctly confines server paths to the share via validated components and descriptor-relative opens with symlink/mount refusal, and its confinement core is sound. Remaining issues: non-'*' wildcard query patterns are accepted but matched literally (protocol-visible wrong result), the open path never enforces DesiredAccess/SharedAccess, a read-only file can be marked for deletion, '.'/'..' query patterns are wrongly rejected, and a failed delete-on-close permanently drops the mark. Minor items: status-code deviations, missing fuzz coverage for the untrusted path parser, untested reset-time pending deletes, and several small compression/style cleanups. Two test findings were discarded: the unmarked inline tests predate this PR. The bundling of the security fix with a broad MS-FSA rewrite is noted for reviewability.
- [protocol + skeptical] Wildcard query patterns other than a bare '*' are matched literally — medium 🟠 — crates/ironrdp-rdpdr-native/src/nix/backend.rs
start_query enumerates only when the pattern is exactly "*"; every other pattern parse_query accepts, including '*.txt' and 'a?b', is passed to fstatat as a literal name and fails with NO_SUCH_FILE when matching entries exist, while the Windows backend lets the native filesystem match. MS-FSA 2.1.5.6.3 requires pattern matching per 2.1.4.4, so a server enumerating with a filtered pattern sees a missing directory instead of matches. - [skeptical] Security confinement fix is bundled with a broad protocol-semantics rewrite — low 🟡 — crates/ironrdp-rdpdr-native/src/nix/backend.rs
The confinement fix forces create/rename/delete rework, but most of the diff is independent MS-FSA parity work (dot entries, NumberOfLinks, errno mapping, read cap, ID allocation, pattern acceptance) that changes server-visible behavior. Bundling it into one ~990-line production change widens the blast radius of the security fix and couples its rollback to unrelated changes; the semantics work would audit better as a follow-up. - [test] No test exercises pending deletes on channel reset — low 🟡 ❓ — crates/ironrdp-rdpdr-native/src/nix/backend.rs
The PR states a delete marked on a handle runs on close, channel reset, or drop. Drop is covered by dropping_the_backend_runs_pending_deletes, but the only reset test, reset_closes_the_files_the_server_forgot, asserts only that a new open succeeds after reset; no test marks a file for deletion, calls reset, and checks removal, so the close_all_files call in reset could drop the delete side effect without failing a test. - [test] Untrusted path parser has no fuzz target or malformed-input property — low 🟡 — crates/ironrdp-rdpdr-native/src/nix/path.rs
RelativePath::parse and parse_query validate arbitrary server-supplied strings and are this PR's security boundary, but they are covered only by enumerated example cases in the tests module. The repository's top-level fuzz/ crate is the natural home for a target; the PR comment documents the mount-point and openat2 gaps but says nothing about the parser not being fuzzed. - [code-compressor] close_all_files allocates a Vec of file IDs — low 🟡 — crates/ironrdp-rdpdr-native/src/nix/backend.rs
Collecting keys into a Vec is only needed because close_file mutates the map; a while-let loop over keys().next() terminates because close_file removes exactly that entry, closes every remaining file identically, and needs no temporary allocation.
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
| let is_valid_pattern = !matches!(pattern, "" | "." | "..") | ||
| && pattern.encode_utf16().count() <= MAX_NAME_CODE_UNITS | ||
| && !pattern | ||
| .chars() | ||
| .any(|character| character <= '\u{1f}' || matches!(character, ':' | '/' | '|')); | ||
| if !is_valid_pattern { | ||
| return Err(InvalidPath); | ||
| } |
There was a problem hiding this comment.
[protocol] Query patterns '.' and '..' are rejected as invalid names — low 🟡 — parse_query's is_valid_pattern rejects "." and ".." with InvalidPath, surfacing as STATUS_OBJECT_NAME_INVALID. MS-FSA 2.1.5.6.3 explicitly permits these patterns (they select the directory and its parent record), so rejecting them is a normative deviation.
| Err(Errno::ENOENT) if is_directory_request => { | ||
| if req.create_disposition == CreateDisposition::FILE_OPEN { | ||
| return Err(NtStatus::NO_SUCH_FILE); | ||
| } |
There was a problem hiding this comment.
[protocol] A missing name with FILE_OPEN/FILE_OVERWRITE reports NO_SUCH_FILE, not OBJECT_NAME_NOT_FOUND — low 🟡 — The directory arm returns NO_SUCH_FILE for FILE_OPEN of a missing directory and status_from_errno maps ENOENT to NO_SUCH_FILE for the file arm, where MS-FSA 2.1.5.1 requires STATUS_OBJECT_NAME_NOT_FOUND. The deviation is deliberate parity: the Windows backend itself remaps 0xC000000F to NO_SUCH_FILE for the server (windows/status.rs:37), so server-visible behavior matches; only the literal MS-FSA status differs.
| fn set_delete_pending( | ||
| open_file: &OpenFile, | ||
| pending_deletes: &mut HashMap<FileIdentity, RelativePath>, | ||
| delete_pending: bool, | ||
| ) -> nix::Result<()> { | ||
| if !delete_pending { | ||
| pending_deletes.remove(&open_file.identity); | ||
| return Ok(()); | ||
| } | ||
| // The shared directory cannot be removed through the share, and a mark on it would refuse every create in it until its handles close. | ||
| if open_file.path.file_name().is_none() { | ||
| return Err(Errno::EACCES); | ||
| } | ||
| if is_directory(&fstat(open_file.file.as_fd())?) { | ||
| // The open directory itself is read, not whatever its path names now. | ||
| // A duplicate of the handle would share its read position, so a second mark would read on from the end of the first. | ||
| let directory = nix::fcntl::openat( | ||
| open_file.file.as_fd(), | ||
| ".", | ||
| OFlag::O_RDONLY | OFlag::O_DIRECTORY | OFlag::O_CLOEXEC, | ||
| Mode::empty(), | ||
| )?; | ||
| let mut listing = DirectoryListing::open(directory, true)?; | ||
| if listing.next_entry()?.is_some() { | ||
| return Err(Errno::ENOTEMPTY); | ||
| } | ||
| } | ||
| pending_deletes.insert(open_file.identity, open_file.path.clone()); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[protocol] A read-only file can be marked for deletion — medium 🟠 — set_delete_pending checks only that the path has a name and that an open directory is empty before inserting into pending_deletes; it never checks read-only status. The backend reports Unix write-permission bits as FILE_ATTRIBUTE_READONLY, so a server can successfully mark and delete a file it sees as read-only, where MS-FSA 2.1.5.15.3 requires STATUS_CANNOT_DELETE.
| /// Opens or creates `path` beneath the shared directory as `req` asks, and returns it with its identity and the directories leading to it. | ||
| /// | ||
| /// A file marked for deletion is refused before it is overwritten, so a failed open leaves its contents intact. | ||
| fn open_file( | ||
| file_base: &str, | ||
| req: &DeviceCreateRequest, | ||
| path: &RelativePath, | ||
| pending_deletes: &HashMap<FileIdentity, RelativePath>, | ||
| ) -> Result<(std::fs::File, FileIdentity, Vec<FileIdentity>), NtStatus> { | ||
| let root = Root::open(file_base).map_err(|error| { | ||
| warn!(%error, "Open shared directory error"); | ||
| NtStatus::UNSUCCESSFUL | ||
| })?; | ||
| // [MS-SMB2] 2.2.13 SMB2 CREATE Request makes any disposition but the six it lists invalid. | ||
| if !matches!( | ||
| req.create_disposition, | ||
| CreateDisposition::FILE_SUPERSEDE | ||
| | CreateDisposition::FILE_OPEN | ||
| | CreateDisposition::FILE_CREATE | ||
| | CreateDisposition::FILE_OPEN_IF | ||
| | CreateDisposition::FILE_OVERWRITE | ||
| | CreateDisposition::FILE_OVERWRITE_IF | ||
| ) { | ||
| warn!(disposition = ?req.create_disposition, "Rejected an invalid create disposition"); | ||
| return Err(NtStatus::INVALID_PARAMETER); | ||
| } | ||
| let is_directory_request = req.create_options.contains(CreateOptions::FILE_DIRECTORY_FILE); | ||
| // It also makes a directory request invalid with FILE_NON_DIRECTORY_FILE or a replacing disposition. | ||
| if is_directory_request | ||
| && (req.create_options.contains(CreateOptions::FILE_NON_DIRECTORY_FILE) | ||
| || !matches!( | ||
| req.create_disposition, | ||
| CreateDisposition::FILE_CREATE | CreateDisposition::FILE_OPEN | CreateDisposition::FILE_OPEN_IF | ||
| )) | ||
| { | ||
| warn!(disposition = ?req.create_disposition, "Rejected an invalid directory create request"); | ||
| return Err(NtStatus::INVALID_PARAMETER); | ||
| } | ||
| let (parent, name, ancestors) = path.open_parent_with_ancestors(&root).map_err(|error| { | ||
| trace!(%error, ?path, "Open parent directory failed"); | ||
| status_from_parent_errno(error) | ||
| })?; | ||
| let ancestors = ancestors.iter().map(FileIdentity::of).collect::<Vec<_>>(); | ||
| // [MS-FSA] 2.1.5.1 Server Requests an Open of a File refuses a path through any directory marked for deletion, so nothing is created in it. | ||
| // A local process can still add entries to such a directory, and its delete then fails when its last handle closes. | ||
| if ancestors.iter().any(|ancestor| pending_deletes.contains_key(ancestor)) { | ||
| return Err(NtStatus::DELETE_PENDING); | ||
| } | ||
|
|
||
| // Statuses follow [MS-FSA] 2.1.5.1 Server Requests an Open of a File and 2.1.5.1.2 Open of an Existing File. | ||
| let flags = match fstatat(parent.as_fd(), name, AtFlags::AT_SYMLINK_NOFOLLOW) { | ||
| Ok(stat) if is_directory(&stat) => { | ||
| if req.create_options.contains(CreateOptions::FILE_NON_DIRECTORY_FILE) { | ||
| trace!("Refused to open a directory as a file"); | ||
| return Err(NtStatus::FILE_IS_A_DIRECTORY); | ||
| } | ||
| // An existing directory cannot be created, overwritten or superseded. | ||
| if !matches!( | ||
| req.create_disposition, | ||
| CreateDisposition::FILE_OPEN | CreateDisposition::FILE_OPEN_IF | ||
| ) { | ||
| trace!(disposition = ?req.create_disposition, "Refused to replace an existing directory"); | ||
| return Err(if path.file_name().is_none() { | ||
| NtStatus::ACCESS_DENIED | ||
| } else { | ||
| NtStatus::OBJECT_NAME_COLLISION | ||
| }); | ||
| } | ||
| OFlag::O_RDONLY | OFlag::O_DIRECTORY | ||
| } | ||
| Ok(_) if is_directory_request => { | ||
| trace!("Refused to open a file as a directory"); | ||
| return Err(if req.create_disposition == CreateDisposition::FILE_CREATE { | ||
| NtStatus::OBJECT_NAME_COLLISION | ||
| } else { | ||
| NtStatus::NOT_A_DIRECTORY | ||
| }); | ||
| } | ||
| Err(Errno::ENOENT) if is_directory_request => { | ||
| if req.create_disposition == CreateDisposition::FILE_OPEN { | ||
| return Err(NtStatus::NO_SUCH_FILE); | ||
| } | ||
| mkdirat(parent.as_fd(), name, Mode::from_bits_truncate(0o777)).map_err(|error| { | ||
| trace!(%error, ?path, "Create directory failed"); | ||
| status_from_errno(error) | ||
| })?; | ||
| OFlag::O_RDONLY | OFlag::O_DIRECTORY | ||
| } | ||
| Err(error) if error != Errno::ENOENT => { | ||
| trace!(%error, ?path, "Look up file failed"); | ||
| return Err(status_from_errno(error)); | ||
| } | ||
| _ => match req.create_disposition { | ||
| CreateDisposition::FILE_OPEN_IF => OFlag::O_CREAT | OFlag::O_RDWR, | ||
| CreateDisposition::FILE_CREATE => OFlag::O_CREAT | OFlag::O_EXCL | OFlag::O_RDWR, | ||
| CreateDisposition::FILE_SUPERSEDE => OFlag::O_CREAT | OFlag::O_RDWR, | ||
| CreateDisposition::FILE_OPEN => OFlag::O_RDONLY, | ||
| CreateDisposition::FILE_OVERWRITE => OFlag::O_RDWR, | ||
| CreateDisposition::FILE_OVERWRITE_IF => OFlag::O_CREAT | OFlag::O_RDWR, | ||
| // Refused before the lookup. | ||
| _ => return Err(NtStatus::INVALID_PARAMETER), | ||
| }, | ||
| }; | ||
| let file = root | ||
| .open_at(parent.as_fd(), name, flags, Mode::from_bits_truncate(0o666)) | ||
| .map_err(|error| { | ||
| trace!(%error, ?name, "Open file failed"); | ||
| status_from_errno(error) | ||
| })?; | ||
| let stat = fstat(file.as_fd()).map_err(status_from_errno)?; | ||
| let identity = FileIdentity::of(&stat); | ||
| // Windows refuses to open a file marked for deletion, where [MS-FSA] 2.1.5.1 Server Requests an Open of a File states this only for the directories on the path. | ||
| if pending_deletes.contains_key(&identity) { | ||
| return Err(NtStatus::DELETE_PENDING); | ||
| } | ||
| // Truncating only once the open can no longer fail keeps a refused file intact, where O_TRUNC would empty it first. | ||
| // Like O_TRUNC, this leaves anything but a regular file, such as a FIFO, unchanged. | ||
| let is_regular_file = SFlag::from_bits_truncate(stat.st_mode) & SFlag::S_IFMT == SFlag::S_IFREG; | ||
| if is_regular_file | ||
| && matches!( | ||
| req.create_disposition, | ||
| CreateDisposition::FILE_SUPERSEDE | ||
| | CreateDisposition::FILE_OVERWRITE | ||
| | CreateDisposition::FILE_OVERWRITE_IF | ||
| ) | ||
| { | ||
| ftruncate(file.as_fd(), 0).map_err(|error| { | ||
| warn!(%error, ?name, "Truncate file error"); | ||
| status_from_errno(error) | ||
| })?; | ||
| } | ||
| Ok((std::fs::File::from(file), identity, ancestors)) | ||
| } |
There was a problem hiding this comment.
[protocol] Open path never enforces DesiredAccess or SharedAccess — medium 🟠 — open_file ignores DeviceCreateRequest.DesiredAccess and SharedAccess entirely: two opens with FILE_SHARE_NONE both succeed, a delete mark succeeds without DELETE access, and FILE_OVERWRITE never requires write access. MS-FSA 2.1.5.1.2 requires access and sharing checks that MUST fail with STATUS_ACCESS_DENIED or STATUS_SHARING_VIOLATION, so the create path can never report them.
| /// Closes `file_id`, deleting the file when it is marked for deletion and no other handle holds it. | ||
| fn close_file(&mut self, file_id: u32) { | ||
| let Some(open_file) = self.open_files.remove(&file_id) else { | ||
| return; | ||
| }; | ||
| let still_open = self | ||
| .open_files | ||
| .values() | ||
| .any(|other| other.identity == open_file.identity); | ||
| if !still_open | ||
| && let Some(path) = self.pending_deletes.remove(&open_file.identity) | ||
| && let Err(error) = remove_file(&self.file_base, &path, open_file.identity) | ||
| { | ||
| warn!(%error, "Delete on close error"); | ||
| } | ||
| } |
There was a problem hiding this comment.
[skeptical] A failed delete-on-close permanently drops the delete mark — low 🟡 ❓ — close_file removes the entry from pending_deletes before attempting remove_file; if removal fails for any reason other than the name no longer referring to the file (ENOTEMPTY from an entry added after the emptiness check, EACCES, EIO), the mark is gone and no later close retries, silently losing a server-requested deletion. Whether Windows keeps the deletion pending and retries on a failed last-close removal is not established by the evidence.
| let is_valid_pattern = !matches!(pattern, "" | "." | "..") | ||
| && pattern.encode_utf16().count() <= MAX_NAME_CODE_UNITS | ||
| && !pattern | ||
| .chars() | ||
| .any(|character| character <= '\u{1f}' || matches!(character, ':' | '/' | '|')); | ||
| if !is_valid_pattern { | ||
| return Err(InvalidPath); | ||
| } |
There was a problem hiding this comment.
[code-compressor] parse_query re-implements most of is_valid_name inline — low 🟡 — The is_valid_pattern block duplicates is_valid_name (lines 212-219) minus the wildcard/quote characters and the trailing-space/dot rule. Extracting a shared helper for the common rules keeps the two rulesets from drifting when a forbidden character is added, with identical behavior.
| pub(crate) fn open_parent(&self, root: &Root) -> nix::Result<(OwnedFd, &str)> { | ||
| let Some((file_name, parents)) = self.components.split_last() else { | ||
| return Ok((root.directory.try_clone().map_err(io_to_errno)?, ".")); | ||
| }; | ||
|
|
||
| let parent = open_directories(root, parents, None)?; | ||
| Ok((parent, file_name)) | ||
| } | ||
|
|
||
| /// Opens this path as a directory. | ||
| pub(crate) fn open_directory(&self, root: &Root) -> nix::Result<OwnedFd> { | ||
| open_directories(root, &self.components, None) | ||
| } | ||
|
|
||
| /// Like [`Self::open_parent`], and also returns the status of each directory opened on the way, from the shared directory down to the one returned. | ||
| /// | ||
| /// The shared directory itself lies in no directory, so for it the list is empty. | ||
| pub(crate) fn open_parent_with_ancestors(&self, root: &Root) -> nix::Result<(OwnedFd, &str, Vec<FileStat>)> { | ||
| let Some((file_name, parents)) = self.components.split_last() else { | ||
| return Ok((root.directory.try_clone().map_err(io_to_errno)?, ".", Vec::new())); | ||
| }; | ||
|
|
||
| let mut ancestors = Vec::with_capacity(self.components.len()); | ||
| let parent = open_directories(root, parents, Some(&mut ancestors))?; | ||
| Ok((parent, file_name, ancestors)) | ||
| } | ||
| } | ||
|
|
||
| /// Whether Windows can create a file named `name`, so the server cannot reach a file it could not have created. | ||
| /// | ||
| /// [MS-FSCC] 2.1.5 Pathname forbids the characters and lengths checked here, and a trailing dot or space is refused because Win32 strips it, as the Windows backend does. | ||
| /// Unlike there, `:` never introduces a data stream, which Unix files lack, and DOS device names such as `CON` are ordinary names. | ||
| /// | ||
| /// [MS-FSCC]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/ffb795f3-027d-4a3c-997d-3085f2332f6f | ||
| fn is_valid_name(name: &str) -> bool { | ||
| !matches!(name, "" | "." | "..") | ||
| && name.encode_utf16().count() <= MAX_NAME_CODE_UNITS | ||
| && !name.ends_with([' ', '.']) | ||
| && !name.chars().any(|character| { | ||
| character <= '\u{1f}' || matches!(character, '<' | '>' | ':' | '"' | '/' | '\\' | '|' | '?' | '*') | ||
| }) | ||
| } | ||
|
|
||
| /// A path the backend refuses to resolve beneath the shared directory. | ||
| #[derive(Debug, PartialEq, Eq)] | ||
| pub(crate) struct InvalidPath; | ||
|
|
||
| fn open_directories( | ||
| root: &Root, | ||
| components: &[String], | ||
| mut ancestors: Option<&mut Vec<FileStat>>, | ||
| ) -> nix::Result<OwnedFd> { | ||
| let mut directory = root.directory.try_clone().map_err(io_to_errno)?; | ||
| if let Some(ancestors) = &mut ancestors { | ||
| ancestors.push(fstat(directory.as_fd())?); | ||
| } | ||
| for component in components { | ||
| directory = root.open_at(directory.as_fd(), component, DIRECTORY_FLAGS, Mode::empty())?; | ||
| if let Some(ancestors) = &mut ancestors { | ||
| ancestors.push(fstat(directory.as_fd())?); | ||
| } | ||
| } | ||
| Ok(directory) | ||
| } |
There was a problem hiding this comment.
[code-compressor] open_parent and open_parent_with_ancestors duplicate plumbing — low 🟡 — Both duplicate the split_last/root-clone logic and open_directories carries an Option<&mut Vec<FileStat>> parameter whose results both with-ancestors callers immediately remap to FileIdentity (backend.rs:1293). Collecting FileIdentity directly and delegating plain open_parent to the with-ancestors variant removes a function and the Option plumbing for one extra fstat per cold-path component walk.
| // [MS-FSA] 2.1.5.15.12 FileRenameInformation refuses to rename a file marked for deletion or a directory holding open files. | ||
| let rename_denied = matches!(req_inner.set_buffer, FileInformationClass::Rename(_)) | ||
| && backend.open_files.get(&file_id).is_some_and(|open_file| { | ||
| backend.pending_deletes.contains_key(&open_file.identity) | ||
| || backend | ||
| .open_files | ||
| .values() | ||
| .any(|other| other.ancestors.contains(&open_file.identity)) | ||
| }); | ||
| // A rename target that is open cannot be replaced, so the open files are collected before one of them is borrowed mutably. | ||
| let open_identities = if matches!(req_inner.set_buffer, FileInformationClass::Rename(_)) { | ||
| backend | ||
| .open_files | ||
| .values() | ||
| .map(|open_file| open_file.identity) | ||
| .collect() | ||
| } else { | ||
| HashSet::new() | ||
| }; |
There was a problem hiding this comment.
[code-compressor] Rename prelude matches Rename twice and scans open_files twice — low 🟡 — The prelude evaluates matches!(set_buffer, Rename(_)) twice and, for a rename, walks open_files once for rename_denied and again to collect identities that are unused when rename_denied short-circuits. One conditional rename block computes denial and identities in a single pass with the same statuses and the borrow-ordering constraint preserved.
| // Otherwise the rename goes ahead, which at most changes the spelling of the only link and never removes it. | ||
| let same_directory = | ||
| FileIdentity::of(&fstat(from_parent.as_fd()).map_err(status_from_errno)?) == to_parent_identity; | ||
| let is_another_link = source.st_nlink >= 2 |
There was a problem hiding this comment.
[rust-style] Use <= instead of >= in the hard-link count check — low 🟡 — STYLE.md strongly prefers < and <= so comparisons read small-to-big; replace source.st_nlink >= 2 with 2 <= source.st_nlink. The rest of the added code already follows this convention.
|
|
||
| /// A validated path relative to the shared directory. | ||
| /// | ||
| /// INVARIANT: every component is a name Windows can hold, as [`is_valid_name`] checks, so none is empty, `.` or `..`, or contains `/` or NUL. |
There was a problem hiding this comment.
[rust-style] Move the RelativePath invariant doc onto the components field — low 🟡 — STYLE.md places field invariants in the doc comment of the field they constrain and uses struct-level placement as the counter-example. The invariant constrains only components (line 118), so the INVARIANT doc belongs there; the open_files field in NixRdpdrBackend demonstrates the preferred placement.
github-actions
Bot
added
ai-reviewed/2
bbebd38
into
Devolutions:master
github-actions
Bot
removed
the
needs-author-action
The Unix RDPDR drive backend resolved server-supplied paths by joining strings, so
.., symbolic links and mount points could lead the server outside the shared directory, and its create, rename and delete handling did not follow NT semantics.Server paths are now parsed into components without touching the filesystem and opened one component at a time relative to the share, refusing symbolic links and mount points with
openat2where available and a device check otherwise.Names Windows cannot hold are refused with STATUS_OBJECT_NAME_INVALID, and opens never block, so a FIFO in the share cannot stall the client.
Create, rename, delete and directory queries follow MS-FSA and report the NT status the Windows backend reports for the same condition:
*, return.and..except in the share, and report errors instead of an early end or an empty match.File IDs skip IDs still in use, and the number of open files is capped at a quarter of the descriptor limit, at most 1024.