From 22aedd52f0aa4d300e11c90bb603af554d2d307d Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 11 Jun 2022 18:21:40 -0700 Subject: [PATCH 1/6] Import ownership check from Cargo. --- Cargo.toml | 3 + src/utils/mod.rs | 1 + src/utils/ownership.rs | 330 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 334 insertions(+) create mode 100644 src/utils/ownership.rs diff --git a/Cargo.toml b/Cargo.toml index 5ae776336b..934c736c7d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -82,6 +82,8 @@ winreg = "0.8" [target."cfg(windows)".dependencies.winapi] features = [ + "accctrl", + "aclapi", "combaseapi", "errhandlingapi", "fileapi", @@ -92,6 +94,7 @@ features = [ "minwindef", "processthreadsapi", "psapi", + "sddl", "shlobj", "shtypes", "synchapi", diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 627b01d844..af8d88a0f9 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,5 +1,6 @@ ///! Utility functions for Rustup pub(crate) mod notifications; +pub(crate) mod ownership; pub mod raw; pub(crate) mod toml_utils; pub(crate) mod tty; diff --git a/src/utils/ownership.rs b/src/utils/ownership.rs new file mode 100644 index 0000000000..18214f8947 --- /dev/null +++ b/src/utils/ownership.rs @@ -0,0 +1,330 @@ +//! This is temporary. +//! It will be replaced with `cargo_util` once it is published. + +use anyhow::Result; +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(crate) fn validate_ownership(path: &Path, safe_directories: &Vec) -> Result<()> { + for safe_dir in safe_directories { + if safe_dir == std::ffi::OsStr::new("*") { + return Ok(()); + } + if path.starts_with(safe_dir) { + return Ok(()); + } + } + _validate_ownership(path) +} + +#[cfg(unix)] +mod unix { + use super::OwnershipError; + use anyhow::{Context, Result}; + use std::fs; + use std::os::unix::fs::MetadataExt; + use std::path::Path; + + /// Returns the metadata of the path (does not follow symlinks). + fn symlink_metadata(path: &Path) -> Result { + fs::symlink_metadata(path).with_context(|| format!("failed to lstat `{}`", path.display())) + } + pub(super) fn _validate_ownership(path: &Path) -> Result<()> { + use std::env; + 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 = matches!(env::var_os("__SAFE_DIRECTORIES_TEST"), Some(p) if path == p); + 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(unix)] +use unix::_validate_ownership; + +#[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("__SAFE_DIRECTORIES_TEST") { + 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 10679a415869ae6651ee024dbdfa6c60ab365c9c Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 11 Jun 2022 18:22:48 -0700 Subject: [PATCH 2/6] Add safe_directories to settings.toml --- src/settings.rs | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/settings.rs b/src/settings.rs index af70d21396..daf17870f4 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -3,7 +3,7 @@ use std::collections::BTreeMap; use std::path::{Path, PathBuf}; use std::str::FromStr; -use anyhow::{Context, Result}; +use anyhow::{anyhow, Context, Result}; use crate::cli::self_update::SelfUpdateMode; use crate::dist::dist::Profile; @@ -81,6 +81,7 @@ pub struct Settings { pub overrides: BTreeMap, pub pgp_keys: Option, pub auto_self_update: Option, + pub safe_directories: Vec, } impl Default for Settings { @@ -93,6 +94,7 @@ impl Default for Settings { overrides: BTreeMap::new(), pgp_keys: None, auto_self_update: None, + safe_directories: Vec::new(), } } } @@ -155,6 +157,16 @@ impl Settings { .and_then(|mode| SelfUpdateMode::from_str(mode.as_str()).ok()); let profile = get_opt_string(&mut table, "profile", path)? .and_then(|p| Profile::from_str(p.as_str()).ok()); + let safe_directories = get_array(&mut table, "safe_directories", path)? + .into_iter() + .map(|p| match p { + toml::Value::String(s) => Ok(s), + _ => Err(anyhow!(format!( + "expected string in safe_directories in `{}`", + path + ))), + }) + .collect::>()?; Ok(Self { version, default_host_triple: get_opt_string(&mut table, "default_host_triple", path)?, @@ -163,6 +175,7 @@ impl Settings { overrides: Self::table_to_overrides(&mut table, path)?, pgp_keys: get_opt_string(&mut table, "pgp_keys", path)?, auto_self_update, + safe_directories, }) } pub(crate) fn into_toml(self) -> toml::value::Table { @@ -193,6 +206,15 @@ impl Settings { ); } + if !self.safe_directories.is_empty() { + let array = Vec::from_iter( + self.safe_directories + .iter() + .map(|p| toml::Value::String(p.to_string())), + ); + result.insert("safe_directories".to_owned(), toml::Value::Array(array)); + } + let overrides = Self::overrides_to_table(self.overrides); result.insert("overrides".to_owned(), toml::Value::Table(overrides)); From cefea1b590ffb182fabbdf48c345d9479cd7a79a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 11 Jun 2022 18:24:25 -0700 Subject: [PATCH 3/6] Check safe_directories when loading rust-toolchain files. --- Cargo.lock | 7 +++++++ Cargo.toml | 1 + src/config.rs | 41 +++++++++++++++++++++++++++++++++++++++++ src/errors.rs | 23 +++++++++++++++++++++++ 4 files changed, 72 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index e160a8dd18..fd93b29f40 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2033,6 +2033,7 @@ dependencies = [ "serde", "sha2 0.10.2", "sharded-slab", + "shell-escape", "strsim 0.10.0", "tar", "tempfile", @@ -2275,6 +2276,12 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "shell-escape" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "45bb67a18fa91266cc7807181f62f9178a6873bfad7dc788c42e6430db40184f" + [[package]] name = "signature" version = "1.3.2" diff --git a/Cargo.toml b/Cargo.toml index 934c736c7d..eb4049b14c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,6 +66,7 @@ url = "2.1" wait-timeout = "0.2" xz2 = "0.1.3" zstd = "0.11" +shell-escape = "0.1.5" [dependencies.retry] default-features = false diff --git a/src/config.rs b/src/config.rs index 7796849247..1ba47f0c45 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::env; use std::fmt::{self, Display}; use std::io; use std::path::{Path, PathBuf}; @@ -23,6 +24,7 @@ use crate::notifications::*; use crate::process; use crate::settings::{Settings, SettingsFile, DEFAULT_METADATA_VERSION}; use crate::toolchain::{DistributableToolchain, Toolchain, UpdateStatus}; +use crate::utils::ownership; use crate::utils::utils; #[derive(Debug, ThisError)] @@ -636,6 +638,7 @@ impl Cfg { ) -> Result> { let notify = self.notify_handler.as_ref(); let mut dir = Some(dir); + let safe_directories = self.safe_directories(settings); while let Some(d) = dir { // First check the override database @@ -673,6 +676,12 @@ impl Cfg { }; if let Ok(contents) = contents { + ownership::validate_ownership(&toolchain_file, &safe_directories).map_err( + |err| match err.downcast::() { + Ok(e) => RustupError::Ownership(e).into(), + Err(e) => e, + }, + )?; let override_file = Cfg::parse_override_file(contents, parse_mode)?; if let Some(toolchain_name) = &override_file.toolchain.channel { let all_toolchains = self.list_toolchains()?; @@ -1008,6 +1017,38 @@ impl Cfg { Ok(normalized_name.to_owned()) } } + + /// This adds the `RUSTUP_SAFE_DIRECTORIES` environment variable to the + /// given command so that the directory listing can be relayed to Cargo so + /// that it can share the same configuration. + pub(crate) fn rustup_safe_directories_env(&self, cmd: &mut Command) -> Result<()> { + self.settings_file.with(|s| { + let dirs = self.safe_directories(s); + if !dirs.is_empty() { + let dirs = env::join_paths(dirs)?; + cmd.env("RUSTUP_SAFE_DIRECTORIES", dirs); + } + Ok(()) + }) + } + + /// Returns the safe_directories setting, including from the environment. + fn safe_directories(&self, settings: &Settings) -> Vec { + let mut safe_directories: Vec = settings + .safe_directories + .iter() + .map(PathBuf::from) + .collect(); + if let Some(dirs) = process().var_os("RUSTUP_SAFE_DIRECTORIES") { + let from_env = env::split_paths(&dirs) + // Filter so that the environment doesn't grow if there are + // recursive invocations of a proxy. + .filter(|env_path| !safe_directories.iter().any(|d| d == env_path)) + .collect::>(); + safe_directories.extend(from_env); + } + safe_directories + } } /// Specifies how a `rust-toolchain`/`rust-toolchain.toml` configuration file should be parsed. diff --git a/src/errors.rs b/src/errors.rs index c2a972ac7d..372cd52e0d 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -1,5 +1,6 @@ #![allow(clippy::large_enum_variant)] +use std::borrow::Cow; use std::ffi::OsString; use std::fmt::Debug; use std::io::{self, Write}; @@ -10,6 +11,7 @@ use url::Url; use crate::currentprocess::process; use crate::dist::manifest::{Component, Manifest}; +use crate::utils::ownership::OwnershipError; const TOOLSTATE_MSG: &str = "If you require these components, please install and use the latest successful build version,\n\ @@ -115,6 +117,8 @@ pub enum RustupError { UnsupportedVersion(String), #[error("could not write {name} file: '{}'", .path.display())] WritingFile { name: &'static str, path: PathBuf }, + #[error("{}", ownership_error_msg(.0))] + Ownership(OwnershipError), } fn remove_component_msg(cs: &Component, manifest: &Manifest, toolchain: &str) -> String { @@ -197,3 +201,22 @@ fn component_unavailable_msg(cs: &[Component], manifest: &Manifest, toolchain: & String::from_utf8(buf).unwrap() } + +fn ownership_error_msg(err: &OwnershipError) -> String { + // rust-toolchain or rust-toolchain.toml + let what = err.path.file_name().unwrap().to_string_lossy(); + let to_add = err.path.parent().unwrap(); + let escaped = shell_escape::escape(Cow::Borrowed(to_add.to_str().expect("utf-8 path"))); + format!( + "`{}` is owned by a different user\n \ + For safety reasons, Rustup does not allow opening `{what}` files owned by\n \ + a different user, unless explicitly approved.\n\ + \n \ + 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.", + err.path.display(), + ) +} From 125255ba99656337785b64a5aa8b0165f72c7650 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 11 Jun 2022 18:25:15 -0700 Subject: [PATCH 4/6] Relay RUSTUP_SAFE_DIRECTORIES through proxies. --- src/cli/proxy_mode.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cli/proxy_mode.rs b/src/cli/proxy_mode.rs index 7b62934c31..dd71766690 100644 --- a/src/cli/proxy_mode.rs +++ b/src/cli/proxy_mode.rs @@ -47,9 +47,10 @@ fn direct_proxy( toolchain: Option<&str>, args: &[OsString], ) -> Result { - let cmd = match toolchain { + let mut cmd = match toolchain { None => cfg.create_command_for_dir(&utils::current_dir()?, arg0)?, Some(tc) => cfg.create_command_for_toolchain(tc, false, arg0)?, }; + cfg.rustup_safe_directories_env(&mut cmd)?; run_command_for_dir(cmd, arg0, args) } From eb4b07cee2e0a56dc021295c8d53ef7c4b1ea371 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 11 Jun 2022 18:25:52 -0700 Subject: [PATCH 5/6] Add `rustup set safe-directories` subcommand. --- src/cli/rustup_mode.rs | 86 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index cf300ff7d7..8ed027eefd 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -187,6 +187,13 @@ pub fn main() -> Result { ("default-host", Some(m)) => set_default_host_triple(cfg, m)?, ("profile", Some(m)) => set_profile(cfg, m)?, ("auto-self-update", Some(m)) => set_auto_self_update(cfg, m)?, + ("safe-directories", Some(c)) => match c.subcommand() { + ("add", Some(m)) => safe_directories_add(cfg, m.value_of("path").unwrap())?, + ("remove", Some(m)) => safe_directories_remove(cfg, m.value_of("path").unwrap())?, + ("list", Some(_)) => handle_epipe(safe_directories_list(cfg))?, + ("clear", Some(_)) => safe_directories_clear(cfg)?, + (_, _) => unreachable!(), + }, (_, _) => unreachable!(), }, ("completions", Some(c)) => { @@ -721,6 +728,30 @@ pub(crate) fn cli() -> App<'static, 'static> { .possible_values(SelfUpdateMode::modes()) .default_value(SelfUpdateMode::default_mode()), ), + ) + .subcommand( + SubCommand::with_name("safe-directories") + .about("Configure directories that allow toolchain override files") + .setting(AppSettings::VersionlessSubcommands) + .setting(AppSettings::DeriveDisplayOrder) + .setting(AppSettings::SubcommandRequiredElseHelp) + .subcommand( + SubCommand::with_name("add") + .about("Adds a path to the safe-directories list") + .arg(Arg::with_name("path").required(true)), + ) + .subcommand( + SubCommand::with_name("remove") + .about("Removes the given path from the safe directories list") + .arg(Arg::with_name("path").required(true)), + ) + .subcommand( + SubCommand::with_name("list") + .about("Lists the currently configured safe directories"), + ) + .subcommand( + SubCommand::with_name("clear").about("Removes all safe directories"), + ), ), ); @@ -1718,3 +1749,58 @@ fn output_completion_script(shell: Shell, command: CompletionCommand) -> Result< Ok(utils::ExitCode(0)) } + +pub(crate) fn safe_directories_add(cfg: &Cfg, path: &str) -> Result { + let p_buf = PathBuf::from(path); + if path != "*" && !p_buf.exists() { + warn!("path `{}` does not exist", path); + } + cfg.settings_file.with_mut(|s| { + if !s.safe_directories.iter().any(|p| Path::new(p) == p_buf) { + s.safe_directories.push(path.to_string()); + } + Ok(()) + })?; + info!("added `{}` to safe directories", path); + Ok(utils::ExitCode(0)) +} + +pub(crate) fn safe_directories_remove(cfg: &Cfg, path: &str) -> Result { + let p_buf = PathBuf::from(path); + let found = cfg.settings_file.with_mut(|s| { + Ok(s.safe_directories + .iter() + .position(|p| Path::new(p) == p_buf) + .map(|idx| s.safe_directories.remove(idx)) + .is_some()) + })?; + if found { + info!("removed `{}` from the safe directories list", path); + } else { + bail!("path `{}` was not found in the safe directory list", path); + } + Ok(utils::ExitCode(0)) +} + +pub(crate) fn safe_directories_list(cfg: &Cfg) -> Result { + cfg.settings_file.with(|s| { + if s.safe_directories.is_empty() { + info!("no safe directories configured"); + } else { + for path in &s.safe_directories { + writeln!(process().stdout(), "{}", path)?; + } + } + Ok(()) + })?; + Ok(utils::ExitCode(0)) +} + +pub(crate) fn safe_directories_clear(cfg: &Cfg) -> Result { + cfg.settings_file.with_mut(|s| { + s.safe_directories.clear(); + Ok(()) + })?; + info!("safe directories cleared"); + Ok(utils::ExitCode(0)) +} From 9a1b27e493938f0642125834d02d5e08025f56df Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 11 Jun 2022 18:26:07 -0700 Subject: [PATCH 6/6] Add tests for safe-directories. --- tests/cli-misc.rs | 96 +++++++++++++++++++++++++++++++++++++- tests/cli-rustup.rs | 95 +++++++++++++++++++++++++++++++++++++ tests/mock/clitools.rs | 1 + tests/mock/mock_bin_src.rs | 5 ++ 4 files changed, 196 insertions(+), 1 deletion(-) diff --git a/tests/cli-misc.rs b/tests/cli-misc.rs index bf199b451d..0a5ddaf3da 100644 --- a/tests/cli-misc.rs +++ b/tests/cli-misc.rs @@ -11,7 +11,7 @@ use rustup::test::this_host_triple; use rustup::utils::utils; use crate::mock::clitools::{ - self, expect_component_executable, expect_component_not_executable, expect_err, + self, expect_component_executable, expect_component_not_executable, expect_err, expect_err_ex, expect_not_stderr_ok, expect_ok, expect_ok_contains, expect_ok_eq, expect_ok_ex, expect_stderr_ok, expect_stdout_ok, run, set_current_dist_date, Config, Scenario, }; @@ -1097,3 +1097,97 @@ fn deprecated_interfaces() { ); }) } + +#[test] +fn safe_directories_add() { + setup(&|config| { + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "add", "foo"], + "", + "warning: path `foo` does not exist\n\ + info: added `foo` to safe directories\n", + ); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "add", "*"], + "", + "info: added `*` to safe directories\n", + ); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "list"], + "foo\n*\n", + "", + ); + }) +} + +#[test] +fn safe_directories_remove() { + setup(&|config| { + expect_ok(config, &["rustup", "set", "safe-directories", "add", "foo"]); + expect_ok(config, &["rustup", "set", "safe-directories", "add", "*"]); + expect_err_ex( + config, + &["rustup", "set", "safe-directories", "remove", "bar"], + "", + "error: path `bar` was not found in the safe directory list\n", + ); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "remove", "foo"], + "", + "info: removed `foo` from the safe directories list\n", + ); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "list"], + "*\n", + "", + ); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "remove", "*"], + "", + "info: removed `*` from the safe directories list\n", + ); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "list"], + "", + "info: no safe directories configured\n", + ); + }) +} + +#[test] +fn safe_directories_clear() { + setup(&|config| { + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "clear"], + "", + "info: safe directories cleared\n", + ); + expect_ok(config, &["rustup", "set", "safe-directories", "add", "foo"]); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "list"], + "foo\n", + "", + ); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "clear"], + "", + "info: safe directories cleared\n", + ); + expect_ok_ex( + config, + &["rustup", "set", "safe-directories", "list"], + "", + "info: no safe directories configured\n", + ); + }) +} diff --git a/tests/cli-rustup.rs b/tests/cli-rustup.rs index 4f15c7f259..3d17481a24 100644 --- a/tests/cli-rustup.rs +++ b/tests/cli-rustup.rs @@ -2263,3 +2263,98 @@ fn warn_on_duplicate_rust_toolchain_file() { ); }); } + +/// Checks safe directories are honored. +#[test] +fn safe_directories() { + setup(&|config| { + let cwd = config.current_dir(); + let toolchain_file = cwd.join("rust-toolchain.toml"); + let toolchain_str = toolchain_file.to_str().unwrap(); + let toolchain_parent_str = cwd.to_str().unwrap(); + raw::write_file(&toolchain_file, "[toolchain]\nchannel = \"nightly\"").unwrap(); + // It should err when owned by a different user. + let out = run( + config, + "rustc", + &["--version"], + &[("__SAFE_DIRECTORIES_TEST", toolchain_str)], + ); + assert!(!out.ok); + assert!(out.stderr.contains("is owned by a different user")); + + // Make sure env var works. + let out = run( + config, + "rustc", + &["--env", "RUSTUP_SAFE_DIRECTORIES"], + &[ + ("__SAFE_DIRECTORIES_TEST", toolchain_str), + ("RUSTUP_UNSTABLE_SAFE_DIRECTORIES", "true"), + ("RUSTUP_SAFE_DIRECTORIES", toolchain_parent_str), + ], + ); + assert!(out.ok); + assert_eq!(out.stdout, format!("{}\n", toolchain_parent_str)); + + // Add the dir to the allow list and try again, should succeed. + expect_ok( + config, + &[ + "rustup", + "set", + "safe-directories", + "add", + toolchain_parent_str, + ], + ); + let out = run( + config, + "rustc", + &["--env", "RUSTUP_SAFE_DIRECTORIES"], + &[ + ("__SAFE_DIRECTORIES_TEST", toolchain_str), + ("RUSTUP_UNSTABLE_SAFE_DIRECTORIES", "true"), + ], + ); + assert!(out.ok); + assert_eq!(out.stdout, format!("{}\n", toolchain_parent_str)); + + // Check that the env var will join and relay both settings and RUSTUP_SAFE_DIRECTORIES. + let out = run( + config, + "rustc", + &["--env", "RUSTUP_SAFE_DIRECTORIES"], + &[ + ("__SAFE_DIRECTORIES_TEST", toolchain_str), + ("RUSTUP_UNSTABLE_SAFE_DIRECTORIES", "true"), + ("RUSTUP_SAFE_DIRECTORIES", "foo"), + ], + ); + assert!(out.ok); + // assert_eq!(out.stdout, format!("{}\n", toolchain_parent_str)); + assert_eq!( + out.stdout.trim(), + std::env::join_paths([toolchain_parent_str, "foo"]) + .unwrap() + .to_str() + .unwrap() + ); + + // Make sure * works, too. + expect_ok(config, &["rustup", "set", "safe-directories", "clear"]); + expect_ok(config, &["rustup", "set", "safe-directories", "add", "*"]); + let out = run( + config, + "rustc", + &["--env", "RUSTUP_SAFE_DIRECTORIES"], + &[ + ("__SAFE_DIRECTORIES_TEST", toolchain_str), + ("RUSTUP_UNSTABLE_SAFE_DIRECTORIES", "true"), + ], + ); + assert!(out.ok); + assert_eq!(out.stdout, "*\n"); + // assert_eq!(out.stdout.trim(), std::env::join_paths([toolchain_parent_str, "*"]).unwrap().to_str().unwrap()); + }); +} diff --git a/tests/mock/clitools.rs b/tests/mock/clitools.rs index 25e2fe3d7b..6c80b74952 100644 --- a/tests/mock/clitools.rs +++ b/tests/mock/clitools.rs @@ -99,6 +99,7 @@ pub fn setup(s: Scenario, f: &dyn Fn(&mut Config)) { env::remove_var("RUSTUP_TOOLCHAIN"); env::remove_var("SHELL"); env::remove_var("ZDOTDIR"); + env::remove_var("RUSTUP_SAFE_DIRECTORIES"); // clap does it's own terminal colour probing, and that isn't // trait-controllable, but it does honour the terminal. To avoid testing // claps code, lie about whatever terminal this process was started under. diff --git a/tests/mock/mock_bin_src.rs b/tests/mock/mock_bin_src.rs index 7acafb67e1..20e1045b47 100644 --- a/tests/mock/mock_bin_src.rs +++ b/tests/mock/mock_bin_src.rs @@ -71,6 +71,11 @@ fn main() { let mut out = io::stderr(); writeln!(out, "{}", std::env::var("PATH").unwrap()).unwrap(); } + Some("--env") => { + let name = args.next().unwrap(); + let mut out = io::stdout(); + writeln!(out, "{}", std::env::var(name).unwrap()).unwrap(); + } _ => panic!("bad mock proxy commandline"), } }