diff --git a/devolutions-agent/src/broker/executor/windows/mod.rs b/devolutions-agent/src/broker/executor/windows/mod.rs index a143b06f5..e57440688 100644 --- a/devolutions-agent/src/broker/executor/windows/mod.rs +++ b/devolutions-agent/src/broker/executor/windows/mod.rs @@ -18,6 +18,7 @@ use win_api_wrappers::utils::WideString; use windows::Win32::Security::{TOKEN_ADJUST_PRIVILEGES, TOKEN_ALL_ACCESS, TOKEN_QUERY}; use super::{CommandExecutor, ExecutionContext, ExecutionOutput, ProcessStartedCallback}; +use crate::broker::policy_security; mod process; mod token; @@ -216,7 +217,7 @@ fn run_plan( "/IM".to_owned(), process_name.clone(), ]; - match create_process(token, &kill_cmd, session_id, false, None) { + match create_process(token, &kill_cmd, session_id, false, requires_elevation, None) { Ok(out) => info!(%process_name, exit_code = out.exit_code, "Kill-before-operation completed"), Err(error) => warn!(%process_name, %error, "Kill-before-operation failed (ignored)"), } @@ -226,8 +227,15 @@ fn run_plan( if let Some(pre) = &ctx.pre_command { info!("Running pre-operation command"); let command = prepare_shell_command(token, pre)?; - let out = create_process(token, command.args(), session_id, ctx.capture_output, None) - .context("failed to run pre-operation command")?; + let out = create_process( + token, + command.args(), + session_id, + ctx.capture_output, + requires_elevation, + None, + ) + .context("failed to run pre-operation command")?; if out.exit_code != 0 { bail!( "pre-operation command exited with code {}: {}", @@ -238,14 +246,21 @@ fn run_plan( } // 3. Main package-manager command. - let command = prepare_main_command(token, &ctx.command)?; - let output = create_process(token, command.args(), session_id, ctx.capture_output, process_started)?; + let command = prepare_main_command(token, &ctx.command, requires_elevation)?; + let output = create_process( + token, + command.args(), + session_id, + ctx.capture_output, + requires_elevation, + process_started, + )?; // 4. Post-operation command — runs after the main command; failures are logged only. if let Some(post) = &ctx.post_command { info!("Running post-operation command"); let command = prepare_shell_command(token, post)?; - match create_process(token, command.args(), session_id, false, None) { + match create_process(token, command.args(), session_id, false, requires_elevation, None) { Ok(out) if out.exit_code == 0 => {} Ok(out) => warn!(exit_code = out.exit_code, "Post-operation command exited non-zero"), Err(error) => warn!(%error, "Post-operation command failed"), @@ -255,27 +270,32 @@ fn run_plan( Ok(output) } -fn prepare_main_command(token: &Token, command: &[String]) -> anyhow::Result { +fn prepare_main_command( + token: &Token, + command: &[String], + requires_elevation: bool, +) -> anyhow::Result { let user_env = win_api_wrappers::utils::environment_block(Some(token), false) .context("failed to load user environment block")?; - prepare_main_command_in(command, None, Some(&user_env)) + prepare_main_command_in(command, None, Some(&user_env), requires_elevation) } fn prepare_main_command_in( command: &[String], temp_dir: Option<&Path>, user_env: Option<&HashMap>, + requires_elevation: bool, ) -> anyhow::Result { if let Some((script, command_arg_index)) = powershell_inline_script(command) { return prepare_powershell_script(command, command_arg_index, script, temp_dir); } if executable_is(command, "winget.exe") { - return prepare_winget_script(command, temp_dir, user_env); + return prepare_winget_script(command, temp_dir, user_env, requires_elevation); } if executable_is(command, "choco.exe") { - return prepare_chocolatey_script(command, temp_dir, user_env); + return prepare_chocolatey_script(command, temp_dir, user_env, requires_elevation); } if executable_is(command, "cargo.exe") { @@ -382,6 +402,7 @@ fn prepare_winget_script( command: &[String], temp_dir: Option<&Path>, user_env: Option<&HashMap>, + requires_elevation: bool, ) -> anyhow::Result { let mut script = String::new(); script.push_str("@echo off\r\n"); @@ -391,7 +412,7 @@ fn prepare_winget_script( let (executable, args) = command.split_first().context("empty WinGet command")?; let executable = user_env.map_or_else( || Ok(executable.clone()), - |env| resolve_winget_executable(env).map(|path| path.display().to_string()), + |env| resolve_winget_executable(env, requires_elevation).map(|path| path.display().to_string()), )?; append_batch_argument(&mut script, &executable)?; for arg in args { @@ -424,17 +445,25 @@ fn prepare_chocolatey_script( command: &[String], temp_dir: Option<&Path>, user_env: Option<&HashMap>, + requires_elevation: bool, ) -> anyhow::Result { - prepare_chocolatey_script_in(command, temp_dir, user_env) + prepare_chocolatey_script_in(command, temp_dir, user_env, requires_elevation) } fn prepare_chocolatey_script_in( command: &[String], temp_dir: Option<&Path>, user_env: Option<&HashMap>, + requires_elevation: bool, ) -> anyhow::Result { let default_install_root = default_chocolatey_install_dir()?; - prepare_chocolatey_script_in_with_default_install_root(command, temp_dir, user_env, &default_install_root) + prepare_chocolatey_script_in_with_default_install_root( + command, + temp_dir, + user_env, + &default_install_root, + requires_elevation, + ) } fn prepare_chocolatey_script_in_with_default_install_root( @@ -442,6 +471,7 @@ fn prepare_chocolatey_script_in_with_default_install_root( temp_dir: Option<&Path>, user_env: Option<&HashMap>, default_install_root: &Path, + requires_elevation: bool, ) -> anyhow::Result { let mut script = String::new(); script.push_str("@echo off\r\n"); @@ -449,7 +479,8 @@ fn prepare_chocolatey_script_in_with_default_install_root( script.push_str("\r\nset \"NO_COLOR=1\"\r\n"); let (_executable, args) = command.split_first().context("empty Chocolatey command")?; - let (executable, install_root) = resolve_trusted_chocolatey_executable(user_env, default_install_root)?; + let (executable, install_root) = + resolve_trusted_chocolatey_executable(user_env, default_install_root, requires_elevation)?; append_batch_set_value(&mut script, "ChocolateyInstall", &install_root.display().to_string())?; script.push_str("\r\n"); append_batch_argument(&mut script, &executable.display().to_string())?; @@ -841,6 +872,7 @@ fn default_chocolatey_install_dir() -> anyhow::Result { fn resolve_trusted_chocolatey_executable( user_env: Option<&HashMap>, default_install_root: &Path, + requires_elevation: bool, ) -> anyhow::Result<(PathBuf, PathBuf)> { let install_root = user_env .and_then(|env| env_value_ignore_case(env, "ChocolateyInstall")) @@ -862,6 +894,8 @@ fn resolve_trusted_chocolatey_executable( ); } + policy_security::verify_elevated_executable_security(&executable, requires_elevation)?; + Ok((executable, install_root)) } @@ -882,7 +916,7 @@ fn env_value_ignore_case<'a>(env: &'a HashMap, key: &str) -> Opt .map(|(_, value)| value.as_str()) } -fn resolve_winget_executable(env: &HashMap) -> anyhow::Result { +fn resolve_winget_executable(env: &HashMap, requires_elevation: bool) -> anyhow::Result { let path_var = env .iter() .find(|(key, _)| key.eq_ignore_ascii_case("PATH")) @@ -891,6 +925,7 @@ fn resolve_winget_executable(env: &HashMap) -> anyhow::Result ExplicitAccess { + ExplicitAccess { + access_permissions: permissions, + access_mode: GRANT_ACCESS, + inheritance: NO_INHERITANCE, + trustee: Trustee::Sid(sid), + } + } + + fn make_everyone_writable(path: &Path) { + let everyone = Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"); + let dacl = InheritableAcl { + kind: InheritableAclKind::Protected, + acl: Acl::new() + .and_then(|acl| acl.set_entries(&[grant(GENERIC_ALL.0, everyone)])) + .expect("build ACL with Everyone full-control entry"), + }; + let name = U16CString::from_os_str(path.as_os_str()).expect("no interior NUL in temp path"); + set_named_security_info(&name, SE_FILE_OBJECT, None, None, Some(&dacl), None) + .expect("set everyone-writable DACL"); + } + #[test] fn shell_command_uses_utf8_temp_batch_file() { let temp_dir = tempfile::tempdir().expect("create temp dir"); @@ -1147,7 +1213,7 @@ mod tests { "Write-Output 'héllo'".to_owned(), ]; let command = - prepare_main_command_in(&command, Some(temp_dir.path()), None).expect("prepare PowerShell command"); + prepare_main_command_in(&command, Some(temp_dir.path()), None, false).expect("prepare PowerShell command"); assert!(command.args()[0].ends_with(r"\System32\WindowsPowerShell\v1.0\powershell.exe")); assert_eq!(command.args()[1], "-NoProfile"); @@ -1173,7 +1239,7 @@ mod tests { "Write-Output 'héllo'".to_owned(), ]; let command = - prepare_main_command_in(&command, Some(temp_dir.path()), None).expect("prepare PowerShell command"); + prepare_main_command_in(&command, Some(temp_dir.path()), None, false).expect("prepare PowerShell command"); assert!(command.args()[0].ends_with(r"\System32\WindowsPowerShell\v1.0\powershell.exe")); assert_eq!(command.args()[1], "-NoProfile"); @@ -1193,7 +1259,7 @@ mod tests { "Write-Output 'héllo'".to_owned(), ]; let command = - prepare_main_command_in(&command, Some(temp_dir.path()), None).expect("prepare PowerShell command"); + prepare_main_command_in(&command, Some(temp_dir.path()), None, false).expect("prepare PowerShell command"); assert!(command.args()[0].ends_with(r"\PowerShell\7\pwsh.exe")); assert_eq!(command.args()[1], "-NoProfile"); @@ -1217,7 +1283,8 @@ mod tests { "Vendor.Package&Name".to_owned(), "100%".to_owned(), ]; - let command = prepare_main_command_in(&command, Some(temp_dir.path()), None).expect("prepare WinGet command"); + let command = + prepare_main_command_in(&command, Some(temp_dir.path()), None, false).expect("prepare WinGet command"); assert!(command.args()[0].ends_with(r"\System32\cmd.exe")); assert_eq!(command.args()[1], "/D"); @@ -1231,6 +1298,47 @@ mod tests { assert!(script.contains("exit /b %ERRORLEVEL%")); } + #[test] + fn winget_elevated_execution_rejects_everyone_writable_executable() { + let program_files = tempfile::tempdir().expect("create fake ProgramFiles"); + let windows_apps = program_files.path().join("WindowsApps"); + std::fs::create_dir_all(&windows_apps).expect("create fake WindowsApps dir"); + let winget_path = windows_apps.join("winget.exe"); + std::fs::write(&winget_path, b"").expect("write winget placeholder"); + make_everyone_writable(&winget_path); + + let env = HashMap::from([ + ("PATH".to_owned(), windows_apps.display().to_string()), + ("ProgramFiles".to_owned(), program_files.path().display().to_string()), + ]); + + let error = resolve_winget_executable(&env, true) + .expect_err("an everyone-writable winget.exe must be rejected for elevated execution"); + assert!( + error.to_string().contains("elevated package-manager executable"), + "unexpected error: {error}" + ); + } + + #[test] + fn winget_non_elevated_execution_allows_everyone_writable_executable() { + let program_files = tempfile::tempdir().expect("create fake ProgramFiles"); + let windows_apps = program_files.path().join("WindowsApps"); + std::fs::create_dir_all(&windows_apps).expect("create fake WindowsApps dir"); + let winget_path = windows_apps.join("winget.exe"); + std::fs::write(&winget_path, b"").expect("write winget placeholder"); + make_everyone_writable(&winget_path); + + let env = HashMap::from([ + ("PATH".to_owned(), windows_apps.display().to_string()), + ("ProgramFiles".to_owned(), program_files.path().display().to_string()), + ]); + + let resolved = + resolve_winget_executable(&env, false).expect("non-elevated resolution is not subject to the ACL check"); + assert_eq!(resolved, winget_path); + } + #[test] fn bun_cmd_command_uses_trusted_cmd_batch_wrapper() { let temp_dir = tempfile::tempdir().expect("create temp dir"); @@ -1245,8 +1353,8 @@ mod tests { "--global".to_owned(), ]; - let command = - prepare_main_command_in(&command, Some(temp_dir.path()), Some(&user_env)).expect("prepare Bun command"); + let command = prepare_main_command_in(&command, Some(temp_dir.path()), Some(&user_env), false) + .expect("prepare Bun command"); assert!(command.args()[0].ends_with(r"\System32\cmd.exe")); assert_eq!(command.args()[1], "/D"); @@ -1277,7 +1385,7 @@ mod tests { "--global".to_owned(), ]; - let command = prepare_main_command_in(&command, None, Some(&user_env)).expect("prepare Bun command"); + let command = prepare_main_command_in(&command, None, Some(&user_env), false).expect("prepare Bun command"); assert_eq!(command.args()[0], bun_path.display().to_string()); assert_eq!(command.args()[1], "add"); @@ -1346,7 +1454,7 @@ mod tests { } /// Reads back the DACL of `path` as an SDDL string (`D:...`). - fn read_file_dacl_sddl(path: &std::path::Path) -> String { + fn read_file_dacl_sddl(path: &Path) -> String { use win_api_wrappers::utils::WideString; use windows::Win32::Foundation::{ERROR_SUCCESS, HLOCAL, LocalFree}; use windows::Win32::Security::Authorization::{ @@ -1429,6 +1537,7 @@ mod tests { Some(temp_dir.path()), Some(&env), &install_root, + false, ) .expect("prepare Chocolatey command"); @@ -1465,6 +1574,7 @@ mod tests { Some(temp_dir.path()), Some(&env), &install_root, + false, ) { Ok(_) => panic!("missing trusted choco should fail"), Err(error) => error, @@ -1496,6 +1606,7 @@ mod tests { Some(temp_dir.path()), Some(&env), &trusted_install_root, + false, ) { Ok(_) => panic!("non-system ChocolateyInstall should fail"), Err(error) => error, @@ -1503,6 +1614,40 @@ mod tests { assert!(error.to_string().contains("trusted system Chocolatey folder")); } + #[test] + fn chocolatey_elevated_execution_rejects_everyone_writable_executable() { + let program_data = tempfile::tempdir().expect("create fake ProgramData"); + let install_root = program_data.path().join("chocolatey"); + let choco_bin = install_root.join("bin"); + std::fs::create_dir_all(&choco_bin).expect("create fake Chocolatey bin"); + let choco_path = choco_bin.join("choco.exe"); + std::fs::write(&choco_path, "").expect("create fake choco"); + make_everyone_writable(&choco_path); + + let error = resolve_trusted_chocolatey_executable(None, &install_root, true) + .expect_err("an everyone-writable choco.exe must be rejected for elevated execution"); + assert!( + error.to_string().contains("elevated package-manager executable"), + "unexpected error: {error}" + ); + } + + #[test] + fn chocolatey_non_elevated_execution_allows_everyone_writable_executable() { + let program_data = tempfile::tempdir().expect("create fake ProgramData"); + let install_root = program_data.path().join("chocolatey"); + let choco_bin = install_root.join("bin"); + std::fs::create_dir_all(&choco_bin).expect("create fake Chocolatey bin"); + let choco_path = choco_bin.join("choco.exe"); + std::fs::write(&choco_path, "").expect("create fake choco"); + make_everyone_writable(&choco_path); + + let (resolved, resolved_root) = resolve_trusted_chocolatey_executable(None, &install_root, false) + .expect("non-elevated resolution is not subject to the ACL check"); + assert_eq!(resolved, choco_path); + assert_eq!(resolved_root, install_root); + } + #[test] fn cargo_command_uses_batch_wrapper_for_utf8_output() { let temp_dir = tempfile::tempdir().expect("create temp dir"); @@ -1513,7 +1658,8 @@ mod tests { "--version".to_owned(), "15.1.0".to_owned(), ]; - let command = prepare_main_command_in(&command, Some(temp_dir.path()), None).expect("prepare Cargo command"); + let command = + prepare_main_command_in(&command, Some(temp_dir.path()), None, false).expect("prepare Cargo command"); assert!(command.args()[0].ends_with(r"\System32\cmd.exe")); assert_eq!(command.args()[1], "/D"); @@ -1539,7 +1685,7 @@ mod tests { let command = vec!["cargo.exe".to_owned(), "uninstall".to_owned(), "ripgrep".to_owned()]; let command = - prepare_main_command_in(&command, Some(temp_dir.path()), Some(&env)).expect("prepare Cargo command"); + prepare_main_command_in(&command, Some(temp_dir.path()), Some(&env), false).expect("prepare Cargo command"); let script = std::fs::read_to_string(&command.args()[5]).expect("read temp script"); assert!(script.contains(&format!( @@ -1559,7 +1705,7 @@ mod tests { "C:\\Tools\\\"& whoami &\"".to_owned(), ]; - let error = match prepare_main_command_in(&command, Some(temp_dir.path()), None) { + let error = match prepare_main_command_in(&command, Some(temp_dir.path()), None, false) { Ok(_) => panic!("quote should fail"), Err(error) => error, }; @@ -1580,8 +1726,8 @@ mod tests { "zlib:x64-windows".to_owned(), ]; let user_env = HashMap::from([("VCPKG_ROOT".to_owned(), vcpkg_root.display().to_string())]); - let command = - prepare_main_command_in(&command, Some(temp_dir.path()), Some(&user_env)).expect("prepare vcpkg command"); + let command = prepare_main_command_in(&command, Some(temp_dir.path()), Some(&user_env), false) + .expect("prepare vcpkg command"); assert!(command.args()[0].ends_with(r"\System32\cmd.exe")); assert_eq!(command.args()[1], "/D"); @@ -1608,8 +1754,7 @@ mod tests { ], post_command: None, effective_user: "DOMAIN\\user".to_owned(), - user_sid: Sid::from_well_known(windows::Win32::Security::WinWorldSid, None) - .expect("well-known Everyone SID"), + user_sid: Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"), elevation: Elevation::Elevated, scope: Some(Scope::User), capture_output: false, @@ -1639,8 +1784,8 @@ mod tests { "requests==2.31.0".to_owned(), "--no-input".to_owned(), ]; - let command = - prepare_main_command_in(&command, Some(temp_dir.path()), Some(&user_env)).expect("prepare Pip command"); + let command = prepare_main_command_in(&command, Some(temp_dir.path()), Some(&user_env), false) + .expect("prepare Pip command"); assert_eq!(command.args()[0], python_path.display().to_string()); assert_eq!(command.args()[1], "-m"); diff --git a/devolutions-agent/src/broker/executor/windows/process.rs b/devolutions-agent/src/broker/executor/windows/process.rs index e6f4816f1..230d38356 100644 --- a/devolutions-agent/src/broker/executor/windows/process.rs +++ b/devolutions-agent/src/broker/executor/windows/process.rs @@ -18,6 +18,7 @@ use windows::Win32::UI::WindowsAndMessaging::SW_HIDE; use crate::broker::executor::{ExecutionOutput, MAX_CAPTURED_OUTPUT_BYTES, ProcessStartedCallback, tail_utf8}; use crate::broker::operation_tracker::OperationTracker; +use crate::broker::policy_security; /// Create a process under the given token and wait for exit. /// @@ -34,6 +35,7 @@ pub(super) fn create_process( command: &[String], session_id: u32, capture: bool, + requires_elevation: bool, process_started: Option, ) -> anyhow::Result { let cmd_line = CommandLine::new(command.to_vec()); @@ -48,7 +50,7 @@ pub(super) fn create_process( let user_env = utils::environment_block(Some(token), false).context("failed to load user environment block")?; let exe_name = command.first().context("empty command")?; - let resolved_exe = resolve_executable(exe_name, &user_env)?; + let resolved_exe = resolve_executable(exe_name, &user_env, requires_elevation)?; info!( exe = %resolved_exe.display(), @@ -206,12 +208,24 @@ pub(super) fn create_process( /// /// Handles both absolute paths and bare names (e.g., `winget.exe`). /// Appends `.exe` if no extension is present and the file is not found as-is. -fn resolve_executable(exe_name: &str, env: &std::collections::HashMap) -> anyhow::Result { +/// +/// When `requires_elevation` is true (the command will run with an elevated or +/// machine-scope token), the resolved executable must additionally be writable only by +/// SYSTEM or the built-in Administrators group; otherwise a standard user could replace +/// the binary (e.g. via a weakly-ACL'd `%ProgramData%` install root) and have the broker +/// launch it with elevated privileges. This check is skipped for non-elevated executions, +/// since per-user tool installs (pip venvs, `~/.cargo`, etc.) are not admin-owned. +fn resolve_executable( + exe_name: &str, + env: &std::collections::HashMap, + requires_elevation: bool, +) -> anyhow::Result { let exe_path = Path::new(exe_name); // If already an absolute path, just verify it exists. if exe_path.is_absolute() { if exe_path.exists() { + policy_security::verify_elevated_executable_security(exe_path, requires_elevation)?; return Ok(exe_path.to_owned()); } bail!("executable not found at absolute path: {}", exe_path.display()); @@ -244,6 +258,7 @@ fn resolve_executable(exe_name: &str, env: &std::collections::HashMap ExplicitAccess { + ExplicitAccess { + access_permissions: permissions, + access_mode: GRANT_ACCESS, + inheritance: NO_INHERITANCE, + trustee: Trustee::Sid(sid), + } + } + + fn make_everyone_writable(path: &Path) { + let everyone = Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"); + let dacl = InheritableAcl { + kind: InheritableAclKind::Protected, + acl: Acl::new() + .and_then(|acl| acl.set_entries(&[grant(GENERIC_ALL.0, everyone)])) + .expect("build ACL with Everyone full-control entry"), + }; + let name = U16CString::from_os_str(path.as_os_str()).expect("no interior NUL in temp path"); + set_named_security_info(&name, SE_FILE_OBJECT, None, None, Some(&dacl), None) + .expect("set everyone-writable DACL"); + } + + #[test] + fn elevated_execution_rejects_everyone_writable_absolute_executable() { + let temp = tempfile::NamedTempFile::new().expect("create temp file"); + make_everyone_writable(temp.path()); + + let env = HashMap::new(); + let error = resolve_executable(&temp.path().display().to_string(), &env, true) + .expect_err("an everyone-writable executable must be rejected when it will run with an elevated token"); + assert!( + error.to_string().contains("elevated package-manager executable"), + "unexpected error: {error}" + ); + } + + #[test] + fn non_elevated_execution_allows_everyone_writable_absolute_executable() { + let temp = tempfile::NamedTempFile::new().expect("create temp file"); + make_everyone_writable(temp.path()); + + let env = HashMap::new(); + let resolved = resolve_executable(&temp.path().display().to_string(), &env, false) + .expect("non-elevated executables are not subject to the admin-only-writable check"); + assert_eq!(resolved, temp.path()); + } +} diff --git a/devolutions-agent/src/broker/mod.rs b/devolutions-agent/src/broker/mod.rs index 31a3c6003..d3ecf844f 100644 --- a/devolutions-agent/src/broker/mod.rs +++ b/devolutions-agent/src/broker/mod.rs @@ -10,7 +10,7 @@ pub mod executor; pub mod operation_tracker; pub mod pipe; pub mod policy_loader; -mod policy_security; +pub(crate) mod policy_security; pub mod policy_watcher; pub mod server; pub mod task; diff --git a/devolutions-agent/src/broker/policy_security.rs b/devolutions-agent/src/broker/policy_security.rs index 997d5b6bd..927bf4162 100644 --- a/devolutions-agent/src/broker/policy_security.rs +++ b/devolutions-agent/src/broker/policy_security.rs @@ -1,17 +1,23 @@ -//! Policy file security validation. +//! Admin-only-writable file security validation. //! -//! The policy file is the entire authorization control for the package broker. -//! `C:\ProgramData` subtrees are a known spot for over-permissive inherited ACEs: -//! if a standard user can write the policy file, they can self-authorize arbitrary -//! elevated installs. +//! Shared by two trust boundaries in the package broker: +//! - The policy file, which is the entire authorization control for the broker. +//! - Package-manager executables resolved for elevated/machine-scope execution +//! (e.g. `winget.exe`, `choco.exe`). //! -//! As defense-in-depth, before trusting a loaded policy, we verify that the file is -//! owned by SYSTEM or the built-in Administrators group and that its DACL does not -//! grant write access to any other principal. -//! The broker fails closed (pauses) when this check fails. +//! `C:\ProgramData` subtrees (and similar install roots) are a known spot for +//! over-permissive inherited ACEs: if a standard user can write the policy file, they +//! can self-authorize arbitrary elevated installs; if they can write (or replace) the +//! executable the broker launches with an elevated token, they can run arbitrary code +//! as SYSTEM/Administrator. +//! +//! As defense-in-depth, before trusting such a file, we verify that it is owned by +//! SYSTEM or the built-in Administrators group and that its DACL does not grant write +//! access to any other principal. Callers fail closed when this check fails. use std::fs::File; use std::os::windows::io::AsRawHandle as _; +use std::path::Path; use anyhow::{Context as _, bail}; use windows::Win32::Foundation::{ERROR_SUCCESS, GENERIC_ALL, GENERIC_WRITE, HANDLE, HLOCAL, LocalFree}; @@ -61,13 +67,27 @@ impl Drop for OwnedSecurityDescriptor { /// descriptor belongs to the very same file that is subsequently read (no TOCTOU window /// via file replacement). /// +/// See [`verify_admin_only_writable`] for the exact rules. +pub(crate) fn verify_policy_file_security(file: &File) -> anyhow::Result<()> { + verify_admin_only_writable(file, "policy file") +} + +/// Verify that `file` may only be written by SYSTEM or built-in Administrators. +/// +/// The check is performed on the already-opened file handle so the verified security +/// descriptor belongs to the very same file that is subsequently used (no TOCTOU window +/// via file replacement). +/// +/// `subject` is a short human-readable description of the file, used in error messages +/// (e.g. `"policy file"` or `"elevated package-manager executable"`). +/// /// Rules (fail-closed): /// - The owner must be SYSTEM or the built-in Administrators group. /// - A DACL must be present (a NULL DACL grants everyone full control). /// - Every access-allowed ACE granting write access must have SYSTEM or the built-in /// Administrators group as the trustee. /// - Unsupported (object/callback) access-allowed ACE types are rejected. -pub(crate) fn verify_policy_file_security(file: &File) -> anyhow::Result<()> { +pub(crate) fn verify_admin_only_writable(file: &File, subject: &str) -> anyhow::Result<()> { let handle = HANDLE(file.as_raw_handle()); let mut owner = PSID::default(); @@ -90,36 +110,63 @@ pub(crate) fn verify_policy_file_security(file: &File) -> anyhow::Result<()> { }; if ret != ERROR_SUCCESS { - bail!("failed to read policy file security information: error {}", ret.0); + bail!("failed to read {subject} security information: error {}", ret.0); } // SAFETY: On success, `owner` and `dacl` point into `descriptor` (or are null), which // outlives this call. - unsafe { verify_owner_and_dacl(owner, dacl) } + unsafe { verify_owner_and_dacl(subject, owner, dacl) } +} + +/// Opens `path` and verifies that it may only be written by SYSTEM or built-in Administrators. +/// +/// Convenience wrapper for callers that only have a path (not an already-open file handle), +/// such as the executor verifying a resolved package-manager executable before running it +/// with an elevated or machine-scope token. See [`verify_admin_only_writable`] for the exact +/// rules. Fails closed (including when the file cannot be opened at all). +pub(crate) fn verify_admin_only_writable_path(path: &Path, subject: &str) -> anyhow::Result<()> { + let file = File::open(path).with_context(|| format!("failed to open {subject} at {}", path.display()))?; + verify_admin_only_writable(&file, subject) +} + +/// Verify that a resolved package-manager executable which will be launched with an +/// elevated or machine-scope token is only writable by SYSTEM or built-in Administrators. +/// +/// No-op when `requires_elevation` is false, since non-elevated tool installs (pip venvs, +/// `~/.cargo`, `~/.bun`, etc.) are not expected to live under admin-only-writable paths. +/// Fails closed on any error (including when the file cannot be opened), so a resolution +/// or permission issue never silently allows an untrusted binary to run elevated. +pub(crate) fn verify_elevated_executable_security(path: &Path, requires_elevation: bool) -> anyhow::Result<()> { + if !requires_elevation { + return Ok(()); + } + + let subject = format!("elevated package-manager executable '{}'", path.display()); + verify_admin_only_writable_path(path, &subject) } /// Verify that `owner` is trusted and that `dacl` grants write access to trusted SIDs only. /// -/// See [`verify_policy_file_security`] for the exact rules. +/// See [`verify_admin_only_writable`] for the exact rules. /// /// # Safety /// /// - `owner` must be null or point to a valid SID. /// - `dacl` must be null or point to a valid, initialized ACL. -unsafe fn verify_owner_and_dacl(owner: PSID, dacl: *const ACL) -> anyhow::Result<()> { +unsafe fn verify_owner_and_dacl(subject: &str, owner: PSID, dacl: *const ACL) -> anyhow::Result<()> { if owner.0.is_null() { - bail!("policy file has no owner information"); + bail!("{subject} has no owner information"); } // SAFETY: Per function contract, `owner` points to a valid SID. if !unsafe { is_trusted_sid(owner) } { // SAFETY: Per function contract, `owner` points to a valid SID. let owner_string = unsafe { sid_to_string(owner) }; - bail!("policy file owner {owner_string} is not SYSTEM or built-in Administrators"); + bail!("{subject} owner {owner_string} is not SYSTEM or built-in Administrators"); } if dacl.is_null() { - bail!("policy file has a NULL DACL granting full control to everyone"); + bail!("{subject} has a NULL DACL granting full control to everyone"); } // SAFETY: Per function contract, `dacl` is a valid, non-null pointer to an initialized ACL. @@ -129,7 +176,7 @@ unsafe fn verify_owner_and_dacl(owner: PSID, dacl: *const ACL) -> anyhow::Result let mut ace_ptr: *mut core::ffi::c_void = std::ptr::null_mut(); // SAFETY: `dacl` is a valid ACL pointer and `idx` is within `AceCount`. - unsafe { GetAce(dacl, idx, &mut ace_ptr) }.context("failed to read policy file DACL entry")?; + unsafe { GetAce(dacl, idx, &mut ace_ptr) }.with_context(|| format!("failed to read {subject} DACL entry"))?; // SAFETY: GetAce succeeded, so `ace_ptr` points to an ACE starting with an ACE_HEADER. let header = unsafe { &*ace_ptr.cast::() }; @@ -152,12 +199,12 @@ unsafe fn verify_owner_and_dacl(owner: PSID, dacl: *const ACL) -> anyhow::Result // SAFETY: `trustee` points to a valid SID inside the ACE. let trustee_string = unsafe { sid_to_string(trustee) }; bail!( - "policy file DACL grants write access to {trustee_string}; only SYSTEM and built-in Administrators may be able to write the policy" + "{subject} DACL grants write access to {trustee_string}; only SYSTEM and built-in Administrators may be able to write it" ); } } // Fail closed on object/callback and other exotic allow ACE types. - other => bail!("policy file DACL contains unsupported ACE type {other}"), + other => bail!("{subject} DACL contains unsupported ACE type {other}"), } } @@ -270,7 +317,7 @@ mod tests { fn verify(&self) -> anyhow::Result<()> { // SAFETY: `owner` and `dacl` point into the owned security descriptor, which outlives // this call. - unsafe { verify_owner_and_dacl(self.owner, self.dacl) } + unsafe { verify_owner_and_dacl("test file", self.owner, self.dacl) } } } @@ -369,7 +416,7 @@ mod tests { } } - fn set_security(path: &std::path::Path, owner: Option<&Sid>, entries: &[ExplicitAccess]) -> anyhow::Result<()> { + fn set_security(path: &Path, owner: Option<&Sid>, entries: &[ExplicitAccess]) -> anyhow::Result<()> { let dacl = InheritableAcl { kind: InheritableAclKind::Protected, acl: Acl::new()?.set_entries(entries)?, @@ -422,4 +469,30 @@ mod tests { verify_policy_file_security(&file).expect("SYSTEM/Administrators-only policy file must be accepted"); } + + #[test] + fn everyone_writable_executable_path_is_rejected() { + let temp = tempfile::NamedTempFile::new().unwrap(); + + let everyone = Sid::from_well_known(WinWorldSid, None).unwrap(); + set_security(temp.path(), None, &[grant(GENERIC_ALL.0, everyone)]).unwrap(); + + let error = verify_admin_only_writable_path(temp.path(), "elevated package-manager executable").unwrap_err(); + assert!( + error.to_string().contains("elevated package-manager executable"), + "unexpected error: {error}" + ); + } + + #[test] + fn missing_executable_path_is_rejected() { + let temp = tempfile::tempdir().unwrap(); + let missing = temp.path().join("does-not-exist.exe"); + + let error = verify_admin_only_writable_path(&missing, "elevated package-manager executable").unwrap_err(); + assert!( + error.to_string().contains("failed to open"), + "unexpected error: {error}" + ); + } }