From fb68ae492f99a8b95c349408a5a545a051bf7aef Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:04:24 -0700 Subject: [PATCH 1/9] Add metadata and symlink_metadata functions to cargo_util::paths --- crates/cargo-util/src/paths.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 8ef782c018d..41cae158b96 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -211,18 +211,26 @@ pub fn open>(path: P) -> Result { File::open(path).with_context(|| format!("failed to open file `{}`", path.display())) } +/// Returns the metadata of the path (follows symlinks). +pub fn metadata(path: &Path) -> Result { + fs::metadata(path).with_context(|| format!("failed to stat `{}`", path.display())) +} + +/// Returns the metadata of the path (does not follow symlinks). +pub fn symlink_metadata(path: &Path) -> Result { + fs::symlink_metadata(path).with_context(|| format!("failed to lstat `{}`", path.display())) +} + /// Returns the last modification time of a file. pub fn mtime(path: &Path) -> Result { - let meta = - fs::metadata(path).with_context(|| format!("failed to stat `{}`", path.display()))?; + let meta = metadata(path)?; Ok(FileTime::from_last_modification_time(&meta)) } /// Returns the maximum mtime of the given path, recursing into /// subdirectories, and following symlinks. pub fn mtime_recursive(path: &Path) -> Result { - let meta = - fs::metadata(path).with_context(|| format!("failed to stat `{}`", path.display()))?; + let meta = metadata(path)?; if !meta.is_dir() { return Ok(FileTime::from_last_modification_time(&meta)); } From 218946206f86049aba28ab2b7644b8acbd98abea Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:05:50 -0700 Subject: [PATCH 2/9] Add ownership validation to cargo_util. --- crates/cargo-util/Cargo.toml | 4 +- crates/cargo-util/src/paths.rs | 3 + crates/cargo-util/src/paths/ownership.rs | 318 +++++++++++++++++++++++ 3 files changed, 323 insertions(+), 2 deletions(-) create mode 100644 crates/cargo-util/src/paths/ownership.rs diff --git a/crates/cargo-util/Cargo.toml b/crates/cargo-util/Cargo.toml index 86afbd0eeec..9b998b8e52f 100644 --- a/crates/cargo-util/Cargo.toml +++ b/crates/cargo-util/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cargo-util" -version = "0.1.3" +version = "0.1.4" edition = "2021" license = "MIT OR Apache-2.0" homepage = "https://github.com/rust-lang/cargo" @@ -25,4 +25,4 @@ core-foundation = { version = "0.9.0", features = ["mac_os_10_7_support"] } [target.'cfg(windows)'.dependencies] miow = "0.3.6" -winapi = { version = "0.3.9", features = ["consoleapi", "minwindef"] } +winapi = { version = "0.3.9", features = ["consoleapi", "minwindef", "sddl", "accctrl", "aclapi", "securitybaseapi"] } diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 41cae158b96..0d0fb0bbbac 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -1,5 +1,8 @@ //! Various utilities for working with files and paths. +mod ownership; + +pub use self::ownership::{validate_ownership, OwnershipError}; use anyhow::{Context, Result}; use filetime::FileTime; use std::env; diff --git a/crates/cargo-util/src/paths/ownership.rs b/crates/cargo-util/src/paths/ownership.rs new file mode 100644 index 00000000000..f0f13f11274 --- /dev/null +++ b/crates/cargo-util/src/paths/ownership.rs @@ -0,0 +1,318 @@ +use anyhow::Result; +use std::collections::HashSet; +use std::fmt; +use std::path::{Path, PathBuf}; + +/// Checks the ownership of the given path matches the current user. +/// +/// The `safe_directories` is a set of paths to allow, usually loaded from config. +pub fn validate_ownership(path: &Path, safe_directories: &HashSet) -> Result<()> { + if safe_directories.get(Path::new("*")).is_some() { + return Ok(()); + } + for safe_dir in safe_directories { + if path.starts_with(safe_dir) { + return Ok(()); + } + } + _validate_ownership(path) +} + +#[cfg(unix)] +fn _validate_ownership(path: &Path) -> Result<()> { + use super::symlink_metadata; + use std::env; + use std::os::unix::fs::MetadataExt; + let meta = symlink_metadata(path)?; + let current_user = unsafe { libc::geteuid() }; + fn get_username(uid: u32) -> String { + unsafe { + let amt = match libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX) { + n if n < 0 => 512 as usize, + n => n as usize, + }; + let mut buf = Vec::with_capacity(amt); + let mut passwd: libc::passwd = std::mem::zeroed(); + let mut result = std::ptr::null_mut(); + match libc::getpwuid_r( + uid, + &mut passwd, + buf.as_mut_ptr(), + buf.capacity(), + &mut result, + ) { + 0 if !result.is_null() => { + let ptr = passwd.pw_name as *const _; + let bytes = std::ffi::CStr::from_ptr(ptr).to_bytes().to_vec(); + String::from_utf8_lossy(&bytes).into_owned() + } + _ => String::from("Unknown"), + } + } + } + // This is used for testing to simulate a failure. + let simulate = match env::var_os("__CARGO_TEST_OWNERSHIP") { + Some(p) if path == p => true, + _ => false, + }; + if current_user != meta.uid() || simulate { + return Err(OwnershipError { + owner: get_username(meta.uid()), + current_user: get_username(current_user), + path: path.to_owned(), + } + .into()); + } + Ok(()) +} + +#[cfg(windows)] +fn _validate_ownership(path: &Path) -> Result<()> { + unsafe { windows::_validate_ownership(path) } +} + +/// An error representing a file that is owned by a different user. +#[allow(dead_code)] // Debug is required by std Error +#[derive(Debug)] +pub struct OwnershipError { + pub owner: String, + pub current_user: String, + pub path: PathBuf, +} + +impl std::error::Error for OwnershipError {} + +impl fmt::Display for OwnershipError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{self:?}") + } +} + +#[cfg(windows)] +mod windows { + use anyhow::{bail, Error, Result}; + use std::env; + use std::ffi::OsString; + use std::io; + use std::os::windows::ffi::{OsStrExt, OsStringExt}; + use std::path::Path; + use std::ptr::null_mut; + use winapi::shared::minwindef::{DWORD, FALSE, HLOCAL, TRUE}; + use winapi::shared::sddl::ConvertSidToStringSidW; + use winapi::shared::winerror::ERROR_INSUFFICIENT_BUFFER; + use winapi::um::accctrl::SE_FILE_OBJECT; + use winapi::um::aclapi::GetNamedSecurityInfoW; + use winapi::um::errhandlingapi::GetLastError; + use winapi::um::handleapi::CloseHandle; + use winapi::um::processthreadsapi::{GetCurrentProcess, OpenProcessToken}; + use winapi::um::securitybaseapi::{ + CheckTokenMembership, EqualSid, GetTokenInformation, IsValidSid, IsWellKnownSid, + }; + use winapi::um::winbase::{LocalFree, LookupAccountSidW}; + use winapi::um::winnt::{ + TokenUser, WinBuiltinAdministratorsSid, DACL_SECURITY_INFORMATION, HANDLE, + OWNER_SECURITY_INFORMATION, PSID, TOKEN_QUERY, TOKEN_USER, + }; + + pub(super) unsafe fn _validate_ownership(path: &Path) -> Result<()> { + let me = GetCurrentProcess(); + let mut token = null_mut(); + if OpenProcessToken(me, TOKEN_QUERY, &mut token) == 0 { + return Err( + Error::new(io::Error::last_os_error()).context("failed to get process token") + ); + } + let token = Handle { inner: token }; + let mut len: DWORD = 0; + // Get the size of the token buffer. + if GetTokenInformation(token.inner, TokenUser, null_mut(), 0, &mut len) != 0 + || GetLastError() != ERROR_INSUFFICIENT_BUFFER + { + return Err(Error::new(io::Error::last_os_error()) + .context("failed to get token information size")); + } + // Get the SID of the current user. + let mut token_info = Vec::::with_capacity(len as usize); + if GetTokenInformation( + token.inner, + TokenUser, + token_info.as_mut_ptr() as *mut _, + len, + &mut len, + ) == 0 + { + return Err( + Error::new(io::Error::last_os_error()).context("failed to get token information") + ); + } + let token_user = token_info.as_ptr() as *const TOKEN_USER; + let user_sid = (*token_user).User.Sid; + + // Get the SID of the owner of the path. + let path_w = wide_path(path); + let mut owner_sid = null_mut(); + let mut descriptor = LocalFreeWrapper { inner: null_mut() }; + let result = GetNamedSecurityInfoW( + path_w.as_ptr(), + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, + &mut owner_sid, + null_mut(), + null_mut(), + null_mut(), + &mut descriptor.inner, + ); + if result != 0 { + let io_err = io::Error::from_raw_os_error(result as i32); + return Err(Error::new(io_err).context(format!( + "failed to get security descriptor for path {}", + path.display() + ))); + } + if IsValidSid(owner_sid) == 0 { + bail!( + "unexpected invalid file owner sid for path {}", + path.display() + ); + } + let simulate = match env::var_os("__CARGO_TEST_OWNERSHIP") { + Some(p) if path == p => true, + _ => false, + }; + if !simulate && EqualSid(user_sid, owner_sid) != 0 { + return Ok(()); + } + // Allow paths that are owned by the Administrators Group if the user is + // also a member of the group. This is added for convenience. Files + // created when run with "Run as Administrator" are owned by the group. + if !simulate && IsWellKnownSid(owner_sid, WinBuiltinAdministratorsSid) == TRUE { + let mut is_member = FALSE; + if CheckTokenMembership(null_mut(), user_sid, &mut is_member) == 0 { + log::info!( + "failed to check if member of administrators: {}", + io::Error::last_os_error() + ); + // Fall through + } else { + if is_member == TRUE { + return Ok(()); + } + } + } + + let owner = sid_to_name(owner_sid).unwrap_or_else(|| sid_to_string(owner_sid)); + let current_user = sid_to_name(user_sid).unwrap_or_else(|| sid_to_string(user_sid)); + return Err(super::OwnershipError { + owner, + current_user, + path: path.to_owned(), + } + .into()); + } + + unsafe fn sid_to_string(sid: PSID) -> String { + let mut s_ptr = null_mut(); + if ConvertSidToStringSidW(sid, &mut s_ptr) == 0 { + log::info!( + "failed to convert sid to string: {}", + io::Error::last_os_error() + ); + return "Unknown".to_string(); + } + let len = (0..).take_while(|&i| *s_ptr.offset(i) != 0).count(); + let slice: &[u16] = std::slice::from_raw_parts(s_ptr, len); + let s = OsString::from_wide(slice); + LocalFree(s_ptr as *mut _); + s.into_string().unwrap_or_else(|_| "Unknown".to_string()) + } + + unsafe fn sid_to_name(sid: PSID) -> Option { + // Note: This operation may be very expensive and slow. + let mut name_size = 0; + let mut domain_size = 0; + let mut pe_use = 0; + // Get the length of the name. + if LookupAccountSidW( + null_mut(), // lpSystemName (where to search) + sid, + null_mut(), // Name + &mut name_size, + null_mut(), // ReferencedDomainName + &mut domain_size, + &mut pe_use, + ) != 0 + || GetLastError() != ERROR_INSUFFICIENT_BUFFER + { + log::debug!( + "failed to determine sid name length: {}", + io::Error::last_os_error() + ); + return None; + } + let mut name: Vec = vec![0; name_size as usize]; + let mut domain: Vec = vec![0; domain_size as usize]; + if LookupAccountSidW( + null_mut(), + sid, + name.as_mut_ptr(), + &mut name_size, + domain.as_mut_ptr(), + &mut domain_size, + &mut pe_use, + ) == 0 + { + log::debug!( + "failed to fetch name ({}): {}", + name_size, + io::Error::last_os_error() + ); + return None; + } + let name = str_from_wide(&name); + let domain = str_from_wide(&domain); + + return Some(format!("{domain}\\{name}")); + } + + struct Handle { + inner: HANDLE, + } + impl Drop for Handle { + fn drop(&mut self) { + unsafe { + CloseHandle(self.inner); + } + } + } + + struct LocalFreeWrapper { + inner: HLOCAL, + } + impl Drop for LocalFreeWrapper { + fn drop(&mut self) { + unsafe { + if !self.inner.is_null() { + LocalFree(self.inner); + self.inner = null_mut(); + } + } + } + } + + fn str_from_wide(wide: &[u16]) -> String { + let len = wide.iter().position(|i| *i == 0).unwrap_or(wide.len()); + let os_str = OsString::from_wide(&wide[..len]); + os_str + .into_string() + .unwrap_or_else(|_| "Invalid UTF-8".to_string()) + } + + fn wide_path(path: &Path) -> Vec { + let mut wide: Vec = path.as_os_str().encode_wide().collect(); + if wide.iter().any(|b| *b == 0) { + panic!("nul byte in wide string"); + } + wide.push(0); + wide + } +} From bbd41a4dca8e001c8a45979c563293ef0a5c0d77 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:12:01 -0700 Subject: [PATCH 3/9] Rework how workspace roots are discovered. This centralizes the workspace discovery code into one location `find_workspace_root_with_loader` so that it isn't duplicated. --- src/cargo/core/workspace.rs | 99 ++++++++++++++++++++++--------------- 1 file changed, 58 insertions(+), 41 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index cf5aeb5161e..e0e8802b966 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -133,6 +133,34 @@ impl WorkspaceConfig { WorkspaceConfig::Member { .. } => None, } } + + /// Returns the path of the workspace root based on this `[workspace]` configuration. + /// + /// Returns `None` if the root is not explicitly known. + /// + /// * `self_path` is the path of the manifest this `WorkspaceConfig` is located. + /// * `look_from` is the path where discovery started (usually the current + /// working directory), used for `workspace.exclude` checking. + fn get_ws_root(&self, self_path: &Path, look_from: &Path) -> Option { + match self { + WorkspaceConfig::Root(ances_root_config) => { + debug!("find_root - found a root checking exclusion"); + if !ances_root_config.is_excluded(look_from) { + debug!("find_root - found!"); + Some(self_path.to_owned()) + } else { + None + } + } + WorkspaceConfig::Member { + root: Some(path_to_root), + } => { + debug!("find_root - found pointer"); + Some(read_root_pointer(self_path, path_to_root)) + } + WorkspaceConfig::Member { .. } => None, + } + } } /// Intermediate configuration of a workspace root in a manifest. @@ -606,26 +634,13 @@ impl<'cfg> Workspace<'cfg> { } } - for ances_manifest_path in find_root_iter(manifest_path, self.config) { - debug!("find_root - trying {}", ances_manifest_path.display()); - match *self.packages.load(&ances_manifest_path)?.workspace_config() { - WorkspaceConfig::Root(ref ances_root_config) => { - debug!("find_root - found a root checking exclusion"); - if !ances_root_config.is_excluded(manifest_path) { - debug!("find_root - found!"); - return Ok(Some(ances_manifest_path)); - } - } - WorkspaceConfig::Member { - root: Some(ref path_to_root), - } => { - debug!("find_root - found pointer"); - return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root))); - } - WorkspaceConfig::Member { .. } => {} - } - } - Ok(None) + find_workspace_root_with_loader(manifest_path, self.config, |self_path| { + Ok(self + .packages + .load(self_path)? + .workspace_config() + .get_ws_root(self_path, manifest_path)) + }) } /// After the root of a workspace has been located, probes for all members @@ -1669,31 +1684,33 @@ pub fn resolve_relative_path( } } -fn parse_manifest(manifest_path: &Path, config: &Config) -> CargoResult { - let key = manifest_path.parent().unwrap(); - let source_id = SourceId::for_path(key)?; - let (manifest, _nested_paths) = read_manifest(manifest_path, source_id, config)?; - Ok(manifest) +/// Finds the path of the root of the workspace. +pub fn find_workspace_root(manifest_path: &Path, config: &Config) -> CargoResult> { + // FIXME(ehuss): Loading and parsing manifests just to find the root seems + // very inefficient. I think this should be reconsidered. + find_workspace_root_with_loader(manifest_path, config, |self_path| { + let key = self_path.parent().unwrap(); + let source_id = SourceId::for_path(key)?; + let (manifest, _nested_paths) = read_manifest(self_path, source_id, config)?; + Ok(manifest + .workspace_config() + .get_ws_root(self_path, manifest_path)) + }) } -pub fn find_workspace_root(manifest_path: &Path, config: &Config) -> CargoResult> { +/// Finds the path of the root of the workspace. +/// +/// This uses a callback to determine if the given path tells us what the +/// workspace root is. +fn find_workspace_root_with_loader( + manifest_path: &Path, + config: &Config, + mut loader: impl FnMut(&Path) -> CargoResult>, +) -> CargoResult> { for ances_manifest_path in find_root_iter(manifest_path, config) { debug!("find_root - trying {}", ances_manifest_path.display()); - match *parse_manifest(&ances_manifest_path, config)?.workspace_config() { - WorkspaceConfig::Root(ref ances_root_config) => { - debug!("find_root - found a root checking exclusion"); - if !ances_root_config.is_excluded(manifest_path) { - debug!("find_root - found!"); - return Ok(Some(ances_manifest_path)); - } - } - WorkspaceConfig::Member { - root: Some(ref path_to_root), - } => { - debug!("find_root - found pointer"); - return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root))); - } - WorkspaceConfig::Member { .. } => {} + if let Some(ws_root_path) = loader(&ances_manifest_path)? { + return Ok(Some(ws_root_path)); } } Ok(None) From b2d8a44a676cbc2152c126f38413bfa019a9ae07 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:21:28 -0700 Subject: [PATCH 4/9] Rework config walk_tree in preparation for safe_directories This reworks the walk_tree so that the home config can be loaded first in order to check safe_directories. --- src/cargo/util/config/mod.rs | 78 ++++++++++++++++++++---------------- tests/testsuite/config.rs | 5 +-- 2 files changed, 45 insertions(+), 38 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index e20004c0dcc..de2b45d768d 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1004,18 +1004,19 @@ impl Config { } pub(crate) fn load_values_unmerged(&self) -> CargoResult> { + self._load_values_unmerged() + .with_context(|| "could not load Cargo configuration") + } + + fn _load_values_unmerged(&self) -> CargoResult> { + let (cvs, mut seen) = self.walk_tree(&self.cwd, false)?; let mut result = Vec::new(); - let mut seen = HashSet::new(); - let home = self.home_path.clone().into_path_unlocked(); - self.walk_tree(&self.cwd, &home, |path| { - let mut cv = self._load_file(path, &mut seen, false)?; + for mut cv in cvs { if self.cli_unstable().config_include { self.load_unmerged_include(&mut cv, &mut seen, &mut result)?; } result.push(cv); - Ok(()) - }) - .with_context(|| "could not load Cargo configuration")?; + } Ok(result) } @@ -1037,20 +1038,19 @@ impl Config { } fn load_values_from(&self, path: &Path) -> CargoResult> { + let (cvs, _) = self + .walk_tree(path, true) + .with_context(|| "could not load Cargo configuration")?; + + // Merge all the files together. // This definition path is ignored, this is just a temporary container // representing the entire file. let mut cfg = CV::Table(HashMap::new(), Definition::Path(PathBuf::from("."))); - let home = self.home_path.clone().into_path_unlocked(); - - self.walk_tree(path, &home, |path| { - let value = self.load_file(path, true)?; - cfg.merge(value, false).with_context(|| { + for cv in cvs { + cfg.merge(cv, false).with_context(|| { format!("failed to merge configuration at `{}`", path.display()) })?; - Ok(()) - }) - .with_context(|| "could not load Cargo configuration")?; - + } match cfg { CV::Table(map, _) => Ok(map), _ => unreachable!(), @@ -1355,29 +1355,39 @@ impl Config { } } - fn walk_tree(&self, pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()> - where - F: FnMut(&Path) -> CargoResult<()>, - { - let mut stash: HashSet = HashSet::new(); + /// Walks from the given path upwards, loading `config.toml` files along the way. + /// + /// `includes` indicates whether or not `includes` directives should be loaded. + /// + /// Returns a `Vec` of each config file in the order they were loaded (home directory is last). + fn walk_tree(&self, pwd: &Path, includes: bool) -> CargoResult<(Vec, HashSet)> { + let mut seen: HashSet = HashSet::new(); + // Load "home" first so that safe.directories can be loaded. However, + // "home" will be last in the result. + let home = self.home_path.clone().into_path_unlocked(); + let home_cv = if let Some(path) = self.get_file_path(&home, "config", true)? { + Some(self._load_file(&path, &mut seen, includes)?) + } else { + None + }; + + let mut result = Vec::new(); for current in paths::ancestors(pwd, self.search_stop_path.as_deref()) { - if let Some(path) = self.get_file_path(¤t.join(".cargo"), "config", true)? { - walk(&path)?; - stash.insert(path); + let dot_cargo = current.join(".cargo"); + if dot_cargo == home { + // home is already loaded, don't load again. + continue; } - } - - // Once we're done, also be sure to walk the home directory even if it's not - // in our history to be sure we pick up that standard location for - // information. - if let Some(path) = self.get_file_path(home, "config", true)? { - if !stash.contains(&path) { - walk(&path)?; + if let Some(path) = self.get_file_path(&dot_cargo, "config", true)? { + let cv = self._load_file(&path, &mut seen, includes)?; + result.push(cv); } } - - Ok(()) + if let Some(home_cv) = home_cv { + result.push(home_cv); + } + Ok((result, seen)) } /// Gets the index for a registry. diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 4353ffcecd2..8f95f248040 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1181,10 +1181,7 @@ fn table_merge_failure() { assert_error( config.get::("table").unwrap_err(), "\ -could not load Cargo configuration - -Caused by: - failed to merge configuration at `[..]/.cargo/config` +failed to merge configuration at `[ROOT]/foo` Caused by: failed to merge key `table` between [..]/foo/.cargo/config and [..]/.cargo/config From fe72b8be470bda9657ce997b44130a85c1713080 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:26:41 -0700 Subject: [PATCH 5/9] Add OwnershipError converter. This adds a function that will convert an OwnershipError to a message explaining to the user what happened and how to fix it. This will be shared by both the config validation and the manifest validation. --- src/cargo/util/errors.rs | 61 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index 3668c92152f..05b0b9669a5 100644 --- a/src/cargo/util/errors.rs +++ b/src/cargo/util/errors.rs @@ -2,10 +2,12 @@ use crate::core::{TargetKind, Workspace}; use crate::ops::CompileOptions; -use anyhow::Error; +use crate::Config; +use anyhow::{format_err, Error}; use cargo_util::ProcessError; +use std::borrow::Cow; use std::fmt; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; pub type CargoResult = anyhow::Result; @@ -345,3 +347,58 @@ impl From for CliError { pub fn internal(error: S) -> anyhow::Error { InternalError::new(anyhow::format_err!("{}", error)).into() } + +/// Converts an `OwnershipError` into an `anyhow::Error` with a human-readable +/// explanation of what to do. +pub fn ownership_error( + e: &cargo_util::paths::OwnershipError, + what: &str, + to_add: &Path, + config: &Config, +) -> anyhow::Error { + let to_approve = if std::env::var_os("RUSTUP_TOOLCHAIN").is_some() { + // FIXME: shell_escape doesn't handle powershell (and possibly other shells) + let escaped = shell_escape::escape(Cow::Borrowed(to_add.to_str().expect("utf-8 path"))); + format!( + "To approve this directory, run:\n\ + \n \ + rustup set safe-directories add {escaped}\n\ + \n\ + See https://rust-lang.github.io/rustup/safe-directories.html for more information.", + ) + } else { + let home = config.home().clone().into_path_unlocked(); + let home_config = if home.join("config").exists() { + home.join("config") + } else { + home.join("config.toml") + }; + format!( + "To approve this directory, set the CARGO_SAFE_DIRECTORIES environment\n\ + variable to \"{}\" or edit\n\ + `{}` and add:\n\ + \n \ + [safe]\n \ + directories = [{}]\n\ + \n\ + See https://doc.rust-lang.org/nightly/cargo/reference/config.html#safedirectories\n\ + for more information.", + to_add.display(), + home_config.display(), + toml_edit::easy::Value::String(to_add.to_str().expect("utf-8 path").to_string()) + ) + }; + format_err!( + "`{}` is owned by a different user\n\ + For safety reasons, Cargo does not allow opening {what} by\n\ + a different user, unless explicitly approved.\n\ + \n\ + {to_approve}\n\ + \n\ + Current user: {}\n\ + Owner of file: {}", + e.path.display(), + e.current_user, + e.owner + ) +} From a31c16f6c6572908e69df47bd9a7560700d3ded7 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:30:56 -0700 Subject: [PATCH 6/9] Load safe.directories config and check it for config. This loads safe.directories from the home config, and then checks the ownership of all other directories. --- src/cargo/util/config/key.rs | 5 +- src/cargo/util/config/mod.rs | 149 ++++++++++++++++++++++++++++++++++- 2 files changed, 151 insertions(+), 3 deletions(-) diff --git a/src/cargo/util/config/key.rs b/src/cargo/util/config/key.rs index 181b7c56a9f..1870f2c676f 100644 --- a/src/cargo/util/config/key.rs +++ b/src/cargo/util/config/key.rs @@ -73,9 +73,10 @@ impl ConfigKey { /// Rewinds this `ConfigKey` back to the state it was at before the last /// `push` method being called. - pub fn pop(&mut self) { - let (_part, env) = self.parts.pop().unwrap(); + pub fn pop(&mut self) -> String { + let (part, env) = self.parts.pop().unwrap(); self.env.truncate(env); + part } /// Returns the corresponding environment variable key for this diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index de2b45d768d..7eda288633d 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -70,7 +70,7 @@ use crate::core::compiler::rustdoc::RustdocExternMap; use crate::core::shell::Verbosity; use crate::core::{features, CliUnstable, Shell, SourceId, Workspace}; use crate::ops; -use crate::util::errors::CargoResult; +use crate::util::errors::{ownership_error, CargoResult}; use crate::util::toml as cargo_toml; use crate::util::validate_package_name; use crate::util::{FileLock, Filesystem, IntoUrl, IntoUrlWithBase, Rustc}; @@ -186,6 +186,7 @@ pub struct Config { doc_extern_map: LazyCell, progress_config: ProgressConfig, env_config: LazyCell, + safe_directories: LazyCell>, /// This should be false if: /// - this is an artifact of the rustc distribution process for "stable" or for "beta" /// - this is an `#[test]` that does not opt in with `enable_nightly_features` @@ -284,6 +285,7 @@ impl Config { doc_extern_map: LazyCell::new(), progress_config: ProgressConfig::default(), env_config: LazyCell::new(), + safe_directories: LazyCell::new(), nightly_features_allowed: matches!(&*features::channel(), "nightly" | "dev"), } } @@ -1057,6 +1059,103 @@ impl Config { } } + /// Checks if the given path is owned by a different user. + fn check_safe_dir(&self, path: &Path, safe_directories: &HashSet) -> CargoResult<()> { + if !self.safe_directories_enabled() { + return Ok(()); + } + paths::validate_ownership(path, safe_directories).map_err(|e| { + match e.downcast_ref::() { + Some(e) => { + let mut to_add = e.path.parent().unwrap(); + if to_add.file_name().and_then(|f| f.to_str()) == Some(".cargo") { + to_add = to_add.parent().unwrap(); + } + ownership_error(e, "config files", to_add, self) + } + None => e, + } + }) + } + + /// Returns whether or not the nightly-only safe.directories behavior is enabled. + pub fn safe_directories_enabled(&self) -> bool { + // Because the config is loaded before the CLI options are available + // (primarily to handle aliases), this can't be gated on a `-Z` flag. + // So for now, this is only enabled via an environment variable. This + // has some downsides (such as not supporting the "allow" list), but + // should be fine for a one-off exception. + self.env + .get("CARGO_UNSTABLE_SAFE_DIRECTORIES") + .map(|s| s.as_str()) + == Some("true") + && self.nightly_features_allowed + } + + /// Returns the `safe.directories` config setting. + pub fn safe_directories(&self) -> CargoResult<&HashSet> { + self.safe_directories.try_borrow_with(|| { + if !self.safe_directories_enabled() { + return Ok(HashSet::new()); + } + let mut safe_directories: HashSet = self + .get_list("safe.directories")? + .map(|dirs| { + dirs.val + .iter() + .map(|(s, def)| def.root(self).join(s)) + .collect() + }) + .unwrap_or_default(); + self.extend_safe_directories_env(&mut safe_directories); + Ok(safe_directories) + }) + } + + fn extend_safe_directories_env(&self, safe_directories: &mut HashSet) { + // Note: Unlike other Cargo environment variables, this does not + // assume paths relative to the current directory (only absolute paths + // are supported). This is intended as an extra layer of safety. + if let Some(dirs) = self.env.get("CARGO_SAFE_DIRECTORIES") { + safe_directories.extend(env::split_paths(dirs)); + } + if let Some(dirs) = self.env.get("RUSTUP_SAFE_DIRECTORIES") { + safe_directories.extend(env::split_paths(dirs)); + } + } + + /// Loads the `safe.directories` setting directly from a `ConfigValue` + /// (which should be a Table of the root of the config file). + fn safe_directories_from_cv( + &self, + root: Option<&ConfigValue>, + ) -> CargoResult> { + if !self.safe_directories_enabled() { + return Ok(HashSet::new()); + } + let mut safe_directories: HashSet = root + .map(|root| root.get("safe.directories")) + .transpose()? + .flatten() + .map(|cv| cv.list("safe.directories")) + .transpose()? + .map(|dirs| { + dirs.iter() + .map(|(dir, def)| { + if dir != "*" { + def.root(self).join(dir) + } else { + PathBuf::from(dir) + } + }) + .collect() + }) + .unwrap_or_default(); + + self.extend_safe_directories_env(&mut safe_directories); + Ok(safe_directories) + } + fn load_file(&self, path: &Path, includes: bool) -> CargoResult { self._load_file(path, &mut HashSet::new(), includes) } @@ -1281,6 +1380,7 @@ impl Config { .merge(tmp_table, true) .with_context(|| format!("failed to merge --config argument `{arg}`"))?; } + reject_cli_unsupported(&loaded_args)?; Ok(loaded_args) } @@ -1370,6 +1470,7 @@ impl Config { } else { None }; + let safe_directories = self.safe_directories_from_cv(home_cv.as_ref())?; let mut result = Vec::new(); @@ -1380,7 +1481,17 @@ impl Config { continue; } if let Some(path) = self.get_file_path(&dot_cargo, "config", true)? { + self.check_safe_dir(&path, &safe_directories)?; let cv = self._load_file(&path, &mut seen, includes)?; + if let Some(safe) = cv.get("safe.directories")? { + bail!( + "safe.directories may only be configured from Cargo's home directory\n\ + Found `safe.directories` in {}\n\ + Cargo's home directory is {}\n", + safe.definition(), + home.display() + ); + } result.push(cv); } } @@ -1982,6 +2093,28 @@ impl ConfigValue { self.definition() ) } + + /// Retrieve a `ConfigValue` using a dotted key notation. + /// + /// This is similar to `Config::get`, but can be used directly on a root + /// `ConfigValue::Table`. This does *not* look at environment variables. + fn get(&self, key: &str) -> CargoResult> { + let mut key = ConfigKey::from_str(key); + let last = key.pop(); + let (mut table, _def) = self.table("")?; + let mut key_so_far = ConfigKey::new(); + for part in key.parts() { + key_so_far.push(part); + match table.get(part) { + Some(cv) => match cv { + CV::Table(t, _def) => table = t, + _ => cv.expected("table", &key_so_far.to_string())?, + }, + None => return Ok(None), + } + } + Ok(table.get(&last)) + } } pub fn homedir(cwd: &Path) -> Option { @@ -2473,3 +2606,17 @@ macro_rules! drop_eprint { $crate::__shell_print!($config, err, false, $($arg)*) ); } + +/// Rejects config entries set on the CLI that are not supported. +fn reject_cli_unsupported(root: &CV) -> CargoResult<()> { + let (root, _) = root.table("")?; + // safe.directories cannot be set on the CLI because the config is loaded + // before the CLI is parsed (primarily to handle aliases). + if let Some(cv) = root.get("safe") { + let (safe, _) = cv.table("safe")?; + if safe.contains_key("directories") { + bail!("safe.directories cannot be set via the CLI"); + } + } + Ok(()) +} From a9e85568287ce4c1f61fca7eefd95e86659cc5b2 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:34:07 -0700 Subject: [PATCH 7/9] Check ownership during Cargo.toml discovery. This adds a check for file ownership when locating Cargo.toml files. --- src/cargo/core/workspace.rs | 3 +++ src/cargo/ops/registry.rs | 4 ++-- src/cargo/util/command_prelude.rs | 2 +- src/cargo/util/important_paths.rs | 29 +++++++++++++++++++++++++++-- 4 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index e0e8802b966..6a192db243b 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -21,6 +21,7 @@ use crate::core::{EitherManifest, Package, SourceId, VirtualManifest}; use crate::ops; use crate::sources::{PathSource, CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::errors::{CargoResult, ManifestError}; +use crate::util::important_paths; use crate::util::interning::InternedString; use crate::util::lev_distance; use crate::util::toml::{read_manifest, InheritableFields, TomlDependency, TomlProfiles}; @@ -1707,7 +1708,9 @@ fn find_workspace_root_with_loader( config: &Config, mut loader: impl FnMut(&Path) -> CargoResult>, ) -> CargoResult> { + let safe_directories = config.safe_directories()?; for ances_manifest_path in find_root_iter(manifest_path, config) { + important_paths::check_safe_manifest_path(config, &safe_directories, &ances_manifest_path)?; debug!("find_root - trying {}", ances_manifest_path.display()); if let Some(ws_root_path) = loader(&ances_manifest_path)? { return Ok(Some(ws_root_path)); diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 0a3928e33f1..acb3fb8a9d1 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -830,7 +830,7 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> { let name = match opts.krate { Some(ref name) => name.clone(), None => { - let manifest_path = find_root_manifest_for_wd(config.cwd())?; + let manifest_path = find_root_manifest_for_wd(config)?; let ws = Workspace::new(&manifest_path, config)?; ws.current()?.package_id().name().to_string() } @@ -905,7 +905,7 @@ pub fn yank( let name = match krate { Some(name) => name, None => { - let manifest_path = find_root_manifest_for_wd(config.cwd())?; + let manifest_path = find_root_manifest_for_wd(config)?; let ws = Workspace::new(&manifest_path, config)?; ws.current()?.package_id().name().to_string() } diff --git a/src/cargo/util/command_prelude.rs b/src/cargo/util/command_prelude.rs index 325e765f29c..906e2e71cf4 100644 --- a/src/cargo/util/command_prelude.rs +++ b/src/cargo/util/command_prelude.rs @@ -344,7 +344,7 @@ pub trait ArgMatchesExt { } return Ok(path); } - find_root_manifest_for_wd(config.cwd()) + find_root_manifest_for_wd(config) } fn workspace<'a>(&self, config: &'a Config) -> CargoResult> { diff --git a/src/cargo/util/important_paths.rs b/src/cargo/util/important_paths.rs index 224c4ab8b86..5b0ce28b5ab 100644 --- a/src/cargo/util/important_paths.rs +++ b/src/cargo/util/important_paths.rs @@ -1,16 +1,21 @@ -use crate::util::errors::CargoResult; +use crate::util::errors::{self, CargoResult}; +use crate::util::Config; use cargo_util::paths; +use std::collections::HashSet; use std::path::{Path, PathBuf}; /// Finds the root `Cargo.toml`. -pub fn find_root_manifest_for_wd(cwd: &Path) -> CargoResult { +pub fn find_root_manifest_for_wd(config: &Config) -> CargoResult { let valid_cargo_toml_file_name = "Cargo.toml"; let invalid_cargo_toml_file_name = "cargo.toml"; let mut invalid_cargo_toml_path_exists = false; + let safe_directories = config.safe_directories()?; + let cwd = config.cwd(); for current in paths::ancestors(cwd, None) { let manifest = current.join(valid_cargo_toml_file_name); if manifest.exists() { + check_safe_manifest_path(config, &safe_directories, &manifest)?; return Ok(manifest); } if current.join(invalid_cargo_toml_file_name).exists() { @@ -43,3 +48,23 @@ pub fn find_project_manifest_exact(pwd: &Path, file: &str) -> CargoResult, + path: &Path, +) -> CargoResult<()> { + if !config.safe_directories_enabled() { + return Ok(()); + } + paths::validate_ownership(path, safe_directories).map_err(|e| { + match e.downcast_ref::() { + Some(e) => { + let to_add = e.path.parent().unwrap(); + errors::ownership_error(e, "manifests", to_add, config) + } + None => e, + } + }) +} From 25a21df3a709eb26ee158eab2ef8aa9798c53cf1 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:36:06 -0700 Subject: [PATCH 8/9] Add safe.directories testsuite. --- tests/testsuite/main.rs | 1 + tests/testsuite/safe_directories.rs | 536 ++++++++++++++++++++++++++++ 2 files changed, 537 insertions(+) create mode 100644 tests/testsuite/safe_directories.rs diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index cd817d744d3..72e0fb67975 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -115,6 +115,7 @@ mod rustdoc; mod rustdoc_extern_html; mod rustdocflags; mod rustflags; +mod safe_directories; mod search; mod shell_quoting; mod standard_lib; diff --git a/tests/testsuite/safe_directories.rs b/tests/testsuite/safe_directories.rs new file mode 100644 index 00000000000..b95822f3d90 --- /dev/null +++ b/tests/testsuite/safe_directories.rs @@ -0,0 +1,536 @@ +//! Tests for the `safe.directories` configuration option. + +use cargo_test_support::paths::{self, CargoPathExt}; +use cargo_test_support::{basic_manifest, cargo_process, project, Project}; +use std::fs; + +fn sample_config_project() -> Project { + project() + .file("src/lib.rs", "") + .file(".cargo/config.toml", "job.builds = 1") + .build() +} + +#[cargo_test] +fn gated() { + // Checks that it only works on nightly. + let p = sample_config_project(); + let cfg = p.root().join(".cargo/config.toml"); + p.cargo("check") + // This is ignored when no masquerade. + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); + eprintln!("safe path is {}", cfg.display()); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]`[ROOT]/foo/.cargo/config.toml` is owned[..]") + .with_status(101) + .run(); + let manifest = p.root().join("Cargo.toml"); + p.cargo("check") + // This is ignored when no masquerade. + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &manifest) + .with_stderr("[FINISHED] [..]") + .run(); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &manifest) + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]`[ROOT]/foo/Cargo.toml` is owned[..]") + .with_status(101) + .run(); +} + +#[cargo_test] +fn unsafe_config() { + // Checks that untrusted configs are rejected. + let p = sample_config_project(); + let cfg = p.root().join(".cargo/config.toml"); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .env("RUSTUP_TOOLCHAIN", "does-not-exist") + .masquerade_as_nightly_cargo() + .with_stderr(&format!( + "\ +error: could not load Cargo configuration + +Caused by: + `[ROOT]/foo/.cargo/config.toml` is owned by a different user + For safety reasons, Cargo does not allow opening config files by + a different user, unless explicitly approved. + + To approve this directory, run: + + rustup set safe-directories add [..][ROOT]/foo[..] + + See https://rust-lang.github.io/rustup/safe-directories.html for more information. + + Current user: [..] + Owner of file: [..] +" + )) + .with_status(101) + .run(); + + // Same check without rustup installed. + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .env_remove("RUSTUP_TOOLCHAIN") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +error: could not load Cargo configuration + +Caused by: + `[ROOT]/foo/.cargo/config.toml` is owned by a different user + For safety reasons, Cargo does not allow opening config files by + a different user, unless explicitly approved. + + To approve this directory, set the CARGO_SAFE_DIRECTORIES environment + variable to \"[ROOT]/foo\" or edit + `[ROOT]/home/.cargo/config.toml` and add: + + [safe] + directories = [[..][ROOT]/foo[..]] + + See https://doc.rust-lang.org/nightly/cargo/reference/config.html#safedirectories + for more information. + + Current user: [..] + Owner of file: [..] +", + ) + .with_status(101) + .run(); + + // Add the config in the home directory. + let cargo_home = paths::home().join(".cargo"); + cargo_home.mkdir_p(); + fs::write( + cargo_home.join("config.toml"), + &format!( + " + [safe] + directories = [{}] + ", + toml_edit::easy::Value::String(p.root().to_str().unwrap().to_string()) + ), + ) + .unwrap(); + + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] dev [..] +", + ) + .run(); +} + +#[cargo_test] +fn asterisk() { + // Checks that * allows all. + let p = sample_config_project(); + let cfg = p.root().join(".cargo/config.toml"); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]`[ROOT]/foo/.cargo/config.toml` is owned[..]") + .with_status(101) + .run(); + let cargo_home = paths::home().join(".cargo"); + cargo_home.mkdir_p(); + fs::write( + cargo_home.join("config.toml"), + " + [safe] + directories = ['*'] + ", + ) + .unwrap(); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] dev [..] +", + ) + .run(); +} + +#[cargo_test] +fn config_in_home_only() { + // Checks that safe.directories can only be set in the home directory. + let p = project() + .file("src/lib.rs", "") + .file(".cargo/config.toml", "safe.directories = ['*']") + .build(); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +error: could not load Cargo configuration + +Caused by: + safe.directories may only be configured from Cargo's home directory + Found `safe.directories` in [ROOT]/foo/.cargo/config.toml + Cargo's home directory is [ROOT]/home/.cargo +", + ) + .with_status(101) + .run(); +} + +#[cargo_test] +fn home_config_not_checked() { + // home config ownership doesn't matter. + let p = project().file("src/lib.rs", "").build(); + let cargo_home = paths::home().join(".cargo"); + cargo_home.mkdir_p(); + let home_config = cargo_home.join("config.toml"); + fs::write(&home_config, "build.jobs = 1").unwrap(); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &home_config) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] dev [..] +", + ) + .run(); +} + +#[cargo_test] +fn environment_variables() { + // Check that environment variables are supported. + let p = sample_config_project(); + let cfg = p.root().join(".cargo/config.toml"); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]`[ROOT]/foo/.cargo/config.toml` is owned[..]") + .with_status(101) + .run(); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .env("CARGO_SAFE_DIRECTORIES", p.root()) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] dev [..] +", + ) + .run(); + p.cargo("check") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .env("RUSTUP_SAFE_DIRECTORIES", p.root()) + .masquerade_as_nightly_cargo() + .with_stderr("[FINISHED] dev [..]") + .run(); +} + +#[cargo_test] +fn not_allowed_on_cli() { + // safe.directories is not supported on the CLI + let p = sample_config_project(); + p.cargo("check -Zunstable-options --config") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .arg("safe.directories=['foo']") + .masquerade_as_nightly_cargo() + .with_stderr("error: safe.directories cannot be set via the CLI") + .with_status(101) + .run(); +} + +#[cargo_test] +fn cli_path_is_allowed() { + // Allow loading config files owned by another user passed on the path. + // This is based on the idea that the user has explicitly listed the path, + // and that is regarded as intentional trust. + let p = project() + .file("src/lib.rs", "") + .file("myconfig.toml", "build.jobs = 1") + .build(); + p.cargo("check --config myconfig.toml -Zunstable-options") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", p.root().join("myconfig.toml")) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn included_path_is_allowed() { + // Allow loading configs from includes that are owned by another user. + // This is based on the idea that the a config owned by the user has + // explicitly listed the path, and that is regarded as intentional trust. + let p = project() + .file("src/lib.rs", "") + .file(".cargo/config.toml", "include = 'other.toml'") + .file(".cargo/other.toml", "build.jobs = 1") + .build(); + p.cargo("check -Z config-include") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", p.root().join(".cargo/other.toml")) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn aliases_are_handled() { + // Checks that aliases can trigger the check, and can be overridden. + // This is here because aliases are different and are handled very early. + let cargo_home = paths::home().join(".cargo"); + cargo_home.mkdir_p(); + let home_config = cargo_home.join("config.toml"); + fs::write(&home_config, "alias.x = 'new'").unwrap(); + + let p = sample_config_project(); + let cfg = p.root().join(".cargo/config.toml"); + p.cargo("x foo") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]`[ROOT]/foo/.cargo/config.toml` is owned[..]") + .with_status(101) + .run(); + + // Use * to allow + p.cargo("x foo") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .env("CARGO_SAFE_DIRECTORIES", "*") + .masquerade_as_nightly_cargo() + .with_stderr("[CREATED][..]") + .run(); + + // Try with alias defined within project. + p.change_file(".cargo/config.toml", "alias.y = 'new'"); + p.cargo("y foo") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]`[ROOT]/foo/.cargo/config.toml` is owned[..]") + .with_status(101) + .run(); + + // Use * to allow + p.cargo("y bar") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &cfg) + .env("CARGO_SAFE_DIRECTORIES", "*") + .masquerade_as_nightly_cargo() + .with_stderr("[CREATED][..]") + .run(); +} + +#[cargo_test] +fn unsafe_manifest() { + // "current" Cargo.toml owned by a different user + let p = project().file("src/lib.rs", "").build(); + let manifest = p.root().join("Cargo.toml"); + p.cargo("tree") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &manifest) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +error: `[ROOT]/foo/Cargo.toml` is owned by a different user +For safety reasons, Cargo does not allow opening manifests by +a different user, unless explicitly approved. + +To approve this directory, run: + + rustup set safe-directories add [ROOT]/foo + +See https://rust-lang.github.io/rustup/safe-directories.html for more information. + +Current user: [..] +Owner of file: [..] +", + ) + .with_status(101) + .run(); + p.cargo("tree") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &manifest) + .env("CARGO_SAFE_DIRECTORIES", &manifest) + .masquerade_as_nightly_cargo() + .with_stdout("foo [..]") + .with_stderr("") + .run(); +} + +#[cargo_test] +fn explicit_manifest_is_ok() { + // --manifest-path doesn't check, as it is an explicit path + let p = project().file("src/lib.rs", "").build(); + let manifest = p.root().join("Cargo.toml"); + cargo_process("check --manifest-path") + .arg(&manifest) + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &manifest) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn unsafe_workspace() { + // Workspace Cargo.toml owned by a different user. + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["member1"] + "#, + ) + .file("member1/Cargo.toml", &basic_manifest("member1", "1.0.0")) + .file("member1/src/lib.rs", "") + .build(); + let manifest = p.root().join("Cargo.toml"); + p.cargo("tree") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &manifest) + .masquerade_as_nightly_cargo() + // Make it search upwards. + .cwd(p.root().join("member1")) + .with_stderr_contains("error: `[ROOT]/foo/Cargo.toml` is owned by a different user") + .with_status(101) + .run(); +} + +#[cargo_test] +fn workspace_via_link() { + // package.workspace isn't a "search" and should be safe. + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["member1"] + "#, + ) + .file( + "member1/Cargo.toml", + r#" + [package] + workspace = ".." + name = "member1" + version = "1.0.0" + "#, + ) + .file("member1/src/lib.rs", "") + .build(); + let ws_manifest = p.root().join("Cargo.toml"); + let project_manifest = p.root().join("member1/Cargo.toml"); + cargo_process("check --manifest-path") + .arg(&project_manifest) + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &ws_manifest) + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] member1 [..] +[FINISHED] [..] +", + ) + .run(); + // Implied workspace root should fail. + cargo_process("check") + .cwd(p.root()) + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &ws_manifest) + .masquerade_as_nightly_cargo() + .with_stderr_contains("error: `[ROOT]/foo/Cargo.toml` is owned by a different user") + .with_status(101) + .run(); +} + +#[cargo_test] +fn path_dependency() { + // path dependencies should be ok + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { path="bar" } + "#, + ) + .file("src/lib.rs", "") + .file("bar/Cargo.toml", &basic_manifest("bar", "1.0.0")) + .file("bar/src/lib.rs", "") + .build(); + let bar_manifest = p.root().join("bar/Cargo.toml"); + p.cargo("tree") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &bar_manifest) + .masquerade_as_nightly_cargo() + .with_stderr("") + .with_stdout( + "\ +foo v0.1.0 [..] +└── bar v1.0.0 [..] +", + ) + .run(); + + // Discovery from the `bar` directory is not ok. + p.cargo("tree") + .cwd("bar") + .env("CARGO_UNSTABLE_SAFE_DIRECTORIES", "true") + .env("__CARGO_TEST_OWNERSHIP", &bar_manifest) + .masquerade_as_nightly_cargo() + .with_stderr_contains("error: `[ROOT]/foo/bar/Cargo.toml` is owned by a different user") + .with_status(101) + .run(); +} From e966aa707026c0b0107236d55fb0437e461f6142 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 Jun 2022 17:36:20 -0700 Subject: [PATCH 9/9] Add documentation for safe.directories. --- src/doc/src/reference/unstable.md | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 6bd68b8a24f..c0b83ae7416 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -103,6 +103,8 @@ Each new feature described below should explain how to use it. * [credential-process](#credential-process) — Adds support for fetching registry tokens from an external authentication program. * [`cargo logout`](#cargo-logout) — Adds the `logout` command to remove the currently saved registry token. * [http-registry](#http-registry) — Adds support for fetching from http registries (`sparse+`) +* Misc + * [safe-directories](#safe-directories) — Adds a security check for file discovery. ### allow-features @@ -1417,6 +1419,33 @@ dep-dev.workspace = true [specifying-dependencies]: specifying-dependencies.md [renaming-dependencies-in-cargotoml]: specifying-dependencies.md#renaming-dependencies-in-cargotoml +### safe-directories +* Tracking Issue: TODO +* RFC: [#3279](https://github.com/rust-lang/rfcs/pull/3279) + +The `CARGO_UNSTABLE_SAFE_DIRECTORIES=true` environment variable enables a mode where Cargo will check the ownership of `Cargo.toml` and `config.toml` files. +If the files are owned by a user different from the current user, +then Cargo will generate an error. +This is a security mechanism to ensure that a malicious user doesn't add one of those files in a parent directory of wherever you run `cargo`. +See the RFC for more details. + +The ownership check can be overridden with the `safe.directories` configuration setting. +This is an array of paths that you explicitly trust even if they are owned by another user. + +```toml +[safe] +directories = ["/path/to/project"] +``` + +This config setting may only be set in the [Cargo home directory](../guide/cargo-home.md). +Other file locations are not allowed. + +This config option can also be set with the `CARGO_SAFE_DIRECTORIES` or `RUSTUP_SAFE_DIRECTORIES` environment variables. +Multiple paths may be separated with `:` on Unix-like environments or `;` for Windows environments. + +An entry of an asterisk (such as `CARGO_SAFE_DIRECTORIES=*`) entirely disables the ownership check for all paths. + + ## Stabilized and removed features ### Compile progress @@ -1597,4 +1626,4 @@ See the [Features chapter](features.md#dependency-features) for more information The `-Ztimings` option has been stabilized as `--timings` in the 1.60 release. (`--timings=html` and the machine-readable `--timings=json` output remain -unstable and require `-Zunstable-options`.) \ No newline at end of file +unstable and require `-Zunstable-options`.)