From 07be33c1197a4b56edb966661a147784824aedfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=8E=AF=E5=87=9B?= <1348292515@qq.com> Date: Thu, 27 Mar 2025 21:23:41 +0800 Subject: [PATCH] fix(nu-command): support ACL, SELinux, e.g. in cd have_permission check (#15360) fixes #8095 # Description This approach is a bit straightforward, call access() check with the flag `X_OK`. Zsh[^1], Fish perform this check by the same approach. [^1]: https://github.com/zsh-users/zsh/blob/435cb1b748ce1f2f5c38edc1d64f4ee2424f9b3a/Src/exec.c#L6406 It could also avoid manual xattrs check on other *nix platforms. BTW, the execution bit for directories in *nix world means permission to access it's content, while the read bit means to list it's content. [^0] [^0]: https://superuser.com/a/169418 # User-Facing Changes Users could face less permission check bugs in their `cd` usage. # Tests + Formatting # After Submitting --------- Co-authored-by: Stefan Holderbach --- crates/nu-cli/src/repl.rs | 2 +- crates/nu-command/src/filesystem/cd.rs | 2 +- crates/nu-utils/src/filesystem.rs | 103 ++++++------------------- 3 files changed, 26 insertions(+), 81 deletions(-) diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 6641047ad9..ae384efd78 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -864,7 +864,7 @@ fn do_auto_cd( path.to_string_lossy().to_string() }; - if let PermissionResult::PermissionDenied(_) = have_permission(path.clone()) { + if let PermissionResult::PermissionDenied = have_permission(path.clone()) { report_shell_error( engine_state, &ShellError::Io(IoError::new_with_additional_context( diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs index 5ef3e4eb5e..be5381ae7d 100644 --- a/crates/nu-command/src/filesystem/cd.rs +++ b/crates/nu-command/src/filesystem/cd.rs @@ -132,7 +132,7 @@ impl Command for Cd { stack.set_cwd(path)?; Ok(PipelineData::empty()) } - PermissionResult::PermissionDenied(_) => { + PermissionResult::PermissionDenied => { Err(IoError::new(std::io::ErrorKind::PermissionDenied, call.head, path).into()) } } diff --git a/crates/nu-utils/src/filesystem.rs b/crates/nu-utils/src/filesystem.rs index 4ea89c4e05..4552d73ec3 100644 --- a/crates/nu-utils/src/filesystem.rs +++ b/crates/nu-utils/src/filesystem.rs @@ -1,29 +1,23 @@ +#[cfg(unix)] +use nix::unistd::{access, AccessFlags}; #[cfg(any(windows, unix))] use std::path::Path; -#[cfg(unix)] -use { - nix::{ - sys::stat::{mode_t, Mode}, - unistd::{Gid, Uid}, - }, - std::os::unix::fs::MetadataExt, -}; // The result of checking whether we have permission to cd to a directory #[derive(Debug)] -pub enum PermissionResult<'a> { +pub enum PermissionResult { PermissionOk, - PermissionDenied(&'a str), + PermissionDenied, } // TODO: Maybe we should use file_attributes() from https://doc.rust-lang.org/std/os/windows/fs/trait.MetadataExt.html // More on that here: https://learn.microsoft.com/en-us/windows/win32/fileio/file-attribute-constants #[cfg(windows)] -pub fn have_permission(dir: impl AsRef) -> PermissionResult<'static> { +pub fn have_permission(dir: impl AsRef) -> PermissionResult { match dir.as_ref().read_dir() { Err(e) => { if matches!(e.kind(), std::io::ErrorKind::PermissionDenied) { - PermissionResult::PermissionDenied("Folder is unable to be read") + PermissionResult::PermissionDenied } else { PermissionResult::PermissionOk } @@ -33,73 +27,15 @@ pub fn have_permission(dir: impl AsRef) -> PermissionResult<'static> { } #[cfg(unix)] -pub fn have_permission(dir: impl AsRef) -> PermissionResult<'static> { - match dir.as_ref().metadata() { - Ok(metadata) => { - let mode = Mode::from_bits_truncate(metadata.mode() as mode_t); - let current_user_uid = users::get_current_uid(); - if current_user_uid.is_root() { - return PermissionResult::PermissionOk; - } - let current_user_gid = users::get_current_gid(); - let owner_user = Uid::from_raw(metadata.uid()); - let owner_group = Gid::from_raw(metadata.gid()); - match ( - current_user_uid == owner_user, - current_user_gid == owner_group, - ) { - (true, _) => { - if mode.contains(Mode::S_IXUSR) { - PermissionResult::PermissionOk - } else { - PermissionResult::PermissionDenied( - "You are the owner but do not have execute permission", - ) - } - } - (false, true) => { - if mode.contains(Mode::S_IXGRP) { - PermissionResult::PermissionOk - } else { - PermissionResult::PermissionDenied( - "You are in the group but do not have execute permission", - ) - } - } - (false, false) => { - if mode.contains(Mode::S_IXOTH) - || (mode.contains(Mode::S_IXGRP) - && any_group(current_user_gid, owner_group)) - { - PermissionResult::PermissionOk - } else { - PermissionResult::PermissionDenied( - "You are neither the owner, in the group, nor the super user and do not have permission", - ) - } - } - } - } - Err(_) => PermissionResult::PermissionDenied("Could not retrieve file metadata"), - } -} - -#[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "android"))] -fn any_group(_current_user_gid: Gid, owner_group: Gid) -> bool { - users::current_user_groups() - .unwrap_or_default() - .contains(&owner_group) -} - -#[cfg(all( - unix, - not(any(target_os = "linux", target_os = "freebsd", target_os = "android")) -))] -fn any_group(current_user_gid: Gid, owner_group: Gid) -> bool { - users::get_current_username() - .and_then(|name| users::get_user_groups(&name, current_user_gid)) - .unwrap_or_default() - .contains(&owner_group) +/// Check that the process' user id has permissions to execute or +/// in the case of a directory traverse the particular directory +pub fn have_permission(dir: impl AsRef) -> PermissionResult { + // We check permissions for real user id, but that's fine, because in + // proper installations of nushell, effective UID (EUID) rarely differs + // from real UID (RUID). We strongly advise against setting the setuid bit + // on the nushell executable or shebang scripts starts with `#!/usr/bin/env nu` e.g. + // Most Unix systems ignore setuid on shebang by default anyway. + access(dir.as_ref(), AccessFlags::X_OK).into() } #[cfg(unix)] @@ -209,3 +145,12 @@ pub mod users { } } } + +impl From> for PermissionResult { + fn from(value: Result) -> Self { + match value { + Ok(_) => Self::PermissionOk, + Err(_) => Self::PermissionDenied, + } + } +}