mirror of
https://github.com/rustdesk/rustdesk.git
synced 2026-10-20 00:17:55 +00:00
fix(windows): move installer scripts into protected temp directories (#16554)
* fix(windows): move installer scripts into protected temp directories Create a private directory under Windows Temp with its final DACL in CreateDirectoryW. Grant Administrators and SYSTEM full access and use OWNER RIGHTS to suppress the ordinary creator's implicit WRITE_DAC. Keep the existing elevated system-cmd copy and SHA-256 verification flow. Keep auxiliary files in the same directory, report cleanup failures, and check command length before creating the directory. This avoids PowerShell, helper executables, and application startup or DLL linking changes. The Windows Temp parent must retain its standard protection against deletion by unprivileged users. UAC cancellation can leave an empty protected directory; the error reports its path. Signed-off-by: fufesou <linlong1266@gmail.com> * fix(windows): preserve installer results when cleanup fails Keep the original installer exit code and report remaining temporary directories or inspection errors as warnings with their paths. Include the directory path in creation errors. Restore the existing handoff assertions in a separate test module, add a locked-directory regression for successful and failed installers, and remove the unrelated ERRORLEVEL environment change. Validation: scoped formatting and whitespace checks passed. Native cmd probes reproduced the failure before the fix and preserved both exit codes afterwards. No project build or Rust test execution was performed. Signed-off-by: fufesou <linlong1266@gmail.com> * fix(windows): clarify installer cleanup warnings Describe protected directories as possibly remaining when cleanup cannot be verified. Remove the cleanup-only test added during review while retaining existing handoff coverage. Signed-off-by: fufesou <linlong1266@gmail.com> * test(windows): keep installer handoff tests inline Restore the inline test module and original helper order to reduce the diff against master. Keep the directory cleanup assertions and original bootstrap checks. Signed-off-by: fufesou <linlong1266@gmail.com> * fix(windows): include paths in installer directory check errors Signed-off-by: fufesou <linlong1266@gmail.com> * fix(windows): clear inherited ERRORLEVEL before installation Signed-off-by: fufesou <linlong1266@gmail.com> * fix(windows): avoid duplicate directory error messages Signed-off-by: fufesou <linlong1266@gmail.com> * test(windows): verify installer directory permissions Signed-off-by: fufesou <linlong1266@gmail.com> --------- Signed-off-by: fufesou <linlong1266@gmail.com>
This commit is contained in:
fufesou
@@ -1,12 +1,16 @@
|
||||
mod directory;
|
||||
|
||||
use super::{
|
||||
installer_shell::{
|
||||
get_system_executable, path_for_cmd_assignment, path_for_cmd_environment,
|
||||
run_elevated_and_wait, trusted_install_environment,
|
||||
BATCH_SHORTCUT_DECODE_FAILURE_EXIT_CODE, CMD_RELATIVE_PATH,
|
||||
WIN7_SHELL_EXECUTE_MAX_PARAMETER_CHARS,
|
||||
},
|
||||
validate_install_app_name, ResultType,
|
||||
};
|
||||
use hbb_common::{
|
||||
anyhow::anyhow,
|
||||
bail, log,
|
||||
sha2::{Digest, Sha256},
|
||||
};
|
||||
@@ -99,10 +103,8 @@ fn verified_install_bootstrap(
|
||||
runner_directory: &Path,
|
||||
) -> ResultType<String> {
|
||||
let source = path_for_cmd_assignment(&script.path)?;
|
||||
let runner = runner_directory.join(format!(
|
||||
"rustdesk_install_{}.bat",
|
||||
uuid::Uuid::new_v4().simple()
|
||||
));
|
||||
let directory = path_for_cmd_assignment(runner_directory)?;
|
||||
let runner = runner_directory.join("install.bat");
|
||||
let runner = path_for_cmd_assignment(&runner)?;
|
||||
let cmd_path = get_system_executable(CMD_RELATIVE_PATH)?;
|
||||
let certutil_path = get_system_executable(CERTUTIL_RELATIVE_PATH)?;
|
||||
@@ -113,13 +115,13 @@ fn verified_install_bootstrap(
|
||||
// Short names preserve headroom under the Windows 7 ShellExecuteExW 2,048
|
||||
// UTF-16-character parameter limit. Paths are stored with delayed expansion
|
||||
// disabled, then expanded indirectly so literal `!` survives: S=source,
|
||||
// R=runner, Q=cmd.exe, H=certutil.exe, F=findstr.exe, C=created flag, E=exit code.
|
||||
// R=runner, Q=cmd.exe, H=certutil.exe, F=findstr.exe, D=directory, E=exit code.
|
||||
Ok(format!(
|
||||
"setlocal DisableDelayedExpansion & set \"S={source}\" & set \"R={runner}\" & \
|
||||
set \"Q={cmd}\" & set \"H={certutil}\" & set \"F={findstr}\" & \
|
||||
set \"C=0\" & set \"E=0\" & setlocal EnableDelayedExpansion & \
|
||||
set \"ERRORLEVEL=\" & set \"D={directory}\" & set \"E=0\" & setlocal EnableDelayedExpansion & \
|
||||
if exist \"!R!\" (set \"E={INSTALL_HANDOFF_RUNNER_EXISTS_EXIT_CODE}\") else (\
|
||||
set \"C=1\" & copy /Y \"!S!\" \"!R!\" > nul || \
|
||||
copy /Y \"!S!\" \"!R!\" > nul || \
|
||||
(set \"E={INSTALL_HANDOFF_COPY_FAILURE_EXIT_CODE}\") & \
|
||||
if \"!E!\"==\"0\" (\"!H!\" -hashfile \"!R!\" SHA256 > \"!R!.hash\" || \
|
||||
set \"E={INSTALL_HANDOFF_HASH_FAILURE_EXIT_CODE}\") & \
|
||||
@@ -127,26 +129,51 @@ fn verified_install_bootstrap(
|
||||
set \"E={INSTALL_HANDOFF_HASH_MISMATCH_EXIT_CODE}\") & \
|
||||
if \"!E!\"==\"0\" (\"!Q!\" /D /E:ON /V:OFF /C \"\"!R!\"\" & \
|
||||
set \"E=!errorlevel!\")) & \
|
||||
if \"!C!\"==\"1\" (rd /s /q \"!R!.dir\" > nul 2>&1 & \
|
||||
del /f /q \"!R!\" \"!R!.*\" > nul 2>&1) & exit /b !E!",
|
||||
rd /s /q \"!D!\" > nul 2>&1 & exit /b !E!",
|
||||
install_hash_pattern(&script.expected_hash),
|
||||
))
|
||||
}
|
||||
|
||||
fn verified_install_parameters(script: &InstallCommandScript) -> ResultType<String> {
|
||||
let system_directory = get_system_executable("")?;
|
||||
Ok(format!(
|
||||
fn verified_install_parameters(
|
||||
script: &InstallCommandScript,
|
||||
directory: &Path,
|
||||
) -> ResultType<String> {
|
||||
let parameters = format!(
|
||||
"/D /E:ON /V:ON /C {}",
|
||||
verified_install_bootstrap(script, &system_directory)?
|
||||
))
|
||||
verified_install_bootstrap(script, directory)?
|
||||
);
|
||||
let length = parameters.encode_utf16().count();
|
||||
if length >= WIN7_SHELL_EXECUTE_MAX_PARAMETER_CHARS {
|
||||
bail!("Elevated command is too long: {length} UTF-16 characters");
|
||||
}
|
||||
Ok(parameters)
|
||||
}
|
||||
|
||||
pub(super) fn run_cmds(cmds: String, show: bool, tip: &str) -> ResultType<()> {
|
||||
validate_install_app_name(&crate::get_app_name())?;
|
||||
let script = write_install_script(cmds)?;
|
||||
let cmd_path = get_system_executable(CMD_RELATIVE_PATH)?;
|
||||
let parameters = verified_install_parameters(&script)?;
|
||||
let exit_code = run_elevated_and_wait(&cmd_path, ¶meters, show)?;
|
||||
let directory = directory::path()?;
|
||||
let parameters = verified_install_parameters(&script, &directory)?;
|
||||
directory::create(&directory)?;
|
||||
let exit_code = run_elevated_and_wait(&cmd_path, ¶meters, show).map_err(|err| {
|
||||
anyhow!(
|
||||
"Installer handoff failed; protected directory may remain at {}: {err}",
|
||||
directory.display()
|
||||
)
|
||||
})?;
|
||||
// Cleanup must not change the result of an installation that already ran.
|
||||
match fs::symlink_metadata(&directory) {
|
||||
Ok(_) => log::warn!(
|
||||
"Protected installer directory may remain at {}",
|
||||
directory.display()
|
||||
),
|
||||
Err(err) if err.kind() == io::ErrorKind::NotFound => {}
|
||||
Err(err) => log::warn!(
|
||||
"Protected installer directory may remain at {}: {err}",
|
||||
directory.display()
|
||||
),
|
||||
}
|
||||
if exit_code != 0 {
|
||||
bail!(
|
||||
"{tip} failed with elevated exit code {exit_code}: {}",
|
||||
@@ -208,7 +235,7 @@ mod tests {
|
||||
.expect("install script should be created");
|
||||
let bootstrap = verified_install_bootstrap(&script, &runner_dir)
|
||||
.expect("native verifier bootstrap should be generated");
|
||||
assert_native_handoff_structure(&script, &shortcut_commands, &bootstrap);
|
||||
assert_native_handoff_structure(&script, &shortcut_commands, &runner_dir);
|
||||
|
||||
let output = run_install_bootstrap_for_test(&bootstrap);
|
||||
assert!(
|
||||
@@ -218,15 +245,25 @@ mod tests {
|
||||
);
|
||||
assert!(marker.exists(), "verified install script must execute");
|
||||
std::fs::remove_file(&marker).expect("test marker should be removed");
|
||||
assert!(
|
||||
!runner_dir.exists(),
|
||||
"protected runner directory must be removed"
|
||||
);
|
||||
std::fs::create_dir(&runner_dir).expect("runner directory should be created");
|
||||
assert_replaced_install_script_is_rejected(&script, &runner_dir, &marker);
|
||||
std::fs::remove_dir(runner_dir).expect("runner directory should be empty");
|
||||
assert!(
|
||||
!runner_dir.exists(),
|
||||
"failed handoff directory must be removed"
|
||||
);
|
||||
}
|
||||
|
||||
fn assert_native_handoff_structure(
|
||||
script: &InstallCommandScript,
|
||||
shortcut_commands: &str,
|
||||
bootstrap: &str,
|
||||
runner_dir: &Path,
|
||||
) {
|
||||
let bootstrap = verified_install_bootstrap(script, runner_dir)
|
||||
.expect("native verifier bootstrap should be generated");
|
||||
assert!(shortcut_commands.contains("certutil"));
|
||||
assert!(shortcut_commands.contains("-decode"));
|
||||
assert!(!shortcut_commands.to_ascii_lowercase().contains("cscript"));
|
||||
@@ -240,8 +277,8 @@ mod tests {
|
||||
.collect::<Vec<_>>()
|
||||
.join(" *");
|
||||
assert!(bootstrap.contains(&format!("/R /I /X /C:\"{win7_hash_pattern}\"")));
|
||||
let parameters =
|
||||
verified_install_parameters(script).expect("elevated parameters should be generated");
|
||||
let parameters = verified_install_parameters(script, runner_dir)
|
||||
.expect("elevated parameters should be generated");
|
||||
assert!(bootstrap.contains("certutil.exe"));
|
||||
assert!(bootstrap.contains("findstr.exe"));
|
||||
assert!(!bootstrap.to_ascii_lowercase().contains("powershell"));
|
||||
|
||||
@@ -0,0 +1,185 @@
|
||||
use super::super::{
|
||||
installer_shell::{get_system_executable, path_for_cmd_environment},
|
||||
wide_string,
|
||||
};
|
||||
use hbb_common::{anyhow::anyhow, bail, ResultType};
|
||||
use std::{
|
||||
fs, mem,
|
||||
os::windows::fs::MetadataExt,
|
||||
path::{Path, PathBuf},
|
||||
};
|
||||
use windows::{
|
||||
core::{w, PCWSTR},
|
||||
Win32::{
|
||||
Foundation::{LocalFree, HLOCAL},
|
||||
Security::{
|
||||
Authorization::{
|
||||
ConvertStringSecurityDescriptorToSecurityDescriptorW, SDDL_REVISION_1,
|
||||
},
|
||||
PSECURITY_DESCRIPTOR, SECURITY_ATTRIBUTES,
|
||||
},
|
||||
Storage::FileSystem::{CreateDirectoryW, FILE_ATTRIBUTE_REPARSE_POINT},
|
||||
},
|
||||
};
|
||||
|
||||
pub(super) fn path() -> ResultType<PathBuf> {
|
||||
let system = get_system_executable("")?;
|
||||
let windows = system
|
||||
.parent()
|
||||
.ok_or_else(|| anyhow!("System directory has no parent"))?;
|
||||
let temporary = windows.join("Temp");
|
||||
for parent in temporary.ancestors() {
|
||||
let metadata = fs::symlink_metadata(parent).map_err(|err| {
|
||||
anyhow!(
|
||||
"Failed to inspect installer temporary parent {}: {err}",
|
||||
parent.display()
|
||||
)
|
||||
})?;
|
||||
if !metadata.is_dir() || metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT.0 != 0 {
|
||||
bail!(
|
||||
"Installer temporary parent is not a regular directory: {}",
|
||||
parent.display()
|
||||
);
|
||||
}
|
||||
}
|
||||
Ok(temporary.join(format!(
|
||||
"rustdesk-install-{}",
|
||||
uuid::Uuid::new_v4().simple()
|
||||
)))
|
||||
}
|
||||
|
||||
pub(super) fn create(path: &Path) -> ResultType<()> {
|
||||
let wide_path = wide_string(path_for_cmd_environment(path)?);
|
||||
let mut descriptor = PSECURITY_DESCRIPTOR::default();
|
||||
// OWNER RIGHTS removes the ordinary creator's implicit WRITE_DAC permission.
|
||||
// The owner can inspect the ACL, but only Administrators/SYSTEM can modify contents.
|
||||
// https://learn.microsoft.com/windows-server/identity/ad-ds/manage/understand-special-identities-groups#owner-rights
|
||||
unsafe {
|
||||
ConvertStringSecurityDescriptorToSecurityDescriptorW(
|
||||
w!("D:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;RC;;;OW)"),
|
||||
SDDL_REVISION_1,
|
||||
&mut descriptor,
|
||||
None,
|
||||
)?;
|
||||
}
|
||||
let attributes = SECURITY_ATTRIBUTES {
|
||||
nLength: mem::size_of::<SECURITY_ATTRIBUTES>() as u32,
|
||||
lpSecurityDescriptor: descriptor.0,
|
||||
bInheritHandle: false.into(),
|
||||
};
|
||||
let result = unsafe { CreateDirectoryW(PCWSTR(wide_path.as_ptr()), Some(&attributes)) };
|
||||
unsafe { LocalFree(Some(HLOCAL(descriptor.0))) };
|
||||
result.map_err(|err| {
|
||||
anyhow!(
|
||||
"Failed to create protected installer directory {}: {err}",
|
||||
path.display()
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use winapi::um::winnt::MAXIMUM_ALLOWED;
|
||||
use windows::{
|
||||
core::{Owned, BOOL},
|
||||
Win32::{
|
||||
Foundation::HANDLE,
|
||||
Security::{
|
||||
AccessCheck,
|
||||
Authorization::{GetNamedSecurityInfoW, SE_FILE_OBJECT},
|
||||
DuplicateToken, SecurityImpersonation, DACL_SECURITY_INFORMATION, GENERIC_MAPPING,
|
||||
GROUP_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, PRIVILEGE_SET,
|
||||
TOKEN_DUPLICATE, TOKEN_QUERY,
|
||||
},
|
||||
Storage::FileSystem::{
|
||||
FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, FILE_GENERIC_WRITE,
|
||||
READ_CONTROL,
|
||||
},
|
||||
System::Threading::{GetCurrentProcess, OpenProcessToken},
|
||||
},
|
||||
};
|
||||
|
||||
#[test]
|
||||
fn protected_directory_enforces_creator_permissions() {
|
||||
let elevated =
|
||||
super::super::super::is_elevated(None).expect("creator elevation should be available");
|
||||
// The user-owned parent allows cleanup through DELETE_CHILD. AccessCheck
|
||||
// below checks the child's DACL, not that separate parent permission.
|
||||
let directory = std::env::temp_dir().join(format!(
|
||||
"rustdesk_installer_acl_{}",
|
||||
uuid::Uuid::new_v4().simple()
|
||||
));
|
||||
create(&directory).expect("production directory creation should succeed");
|
||||
let access = creator_directory_access(&directory);
|
||||
fs::remove_dir(&directory).expect("test directory should be removed");
|
||||
let expected = if elevated {
|
||||
FILE_ALL_ACCESS
|
||||
} else {
|
||||
READ_CONTROL
|
||||
};
|
||||
assert_eq!(
|
||||
access.expect("the created directory's access should be checked"),
|
||||
(true, expected.0),
|
||||
"only elevated administrators may modify the protected directory"
|
||||
);
|
||||
}
|
||||
|
||||
fn creator_directory_access(path: &Path) -> ResultType<(bool, u32)> {
|
||||
let wide_path = wide_string(path_for_cmd_environment(path)?);
|
||||
let mut descriptor = PSECURITY_DESCRIPTOR::default();
|
||||
unsafe {
|
||||
GetNamedSecurityInfoW(
|
||||
PCWSTR(wide_path.as_ptr()),
|
||||
SE_FILE_OBJECT,
|
||||
OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION,
|
||||
None,
|
||||
None,
|
||||
None,
|
||||
None,
|
||||
&mut descriptor,
|
||||
)
|
||||
.ok()?;
|
||||
}
|
||||
let _descriptor = unsafe { Owned::new(HLOCAL(descriptor.0)) };
|
||||
let token = creator_impersonation_token()?;
|
||||
let mapping = GENERIC_MAPPING {
|
||||
GenericRead: FILE_GENERIC_READ.0,
|
||||
GenericWrite: FILE_GENERIC_WRITE.0,
|
||||
GenericExecute: FILE_GENERIC_EXECUTE.0,
|
||||
GenericAll: FILE_ALL_ACCESS.0,
|
||||
};
|
||||
let mut privileges = PRIVILEGE_SET::default();
|
||||
let mut privilege_size = mem::size_of::<PRIVILEGE_SET>() as u32;
|
||||
let mut granted = 0;
|
||||
let mut allowed = BOOL::default();
|
||||
unsafe {
|
||||
AccessCheck(
|
||||
descriptor,
|
||||
*token,
|
||||
MAXIMUM_ALLOWED,
|
||||
&mapping,
|
||||
Some(&mut privileges),
|
||||
&mut privilege_size,
|
||||
&mut granted,
|
||||
&mut allowed,
|
||||
)?;
|
||||
}
|
||||
Ok((allowed.as_bool(), granted))
|
||||
}
|
||||
|
||||
fn creator_impersonation_token() -> ResultType<Owned<HANDLE>> {
|
||||
let mut token = HANDLE::default();
|
||||
unsafe {
|
||||
OpenProcessToken(
|
||||
GetCurrentProcess(),
|
||||
TOKEN_QUERY | TOKEN_DUPLICATE,
|
||||
&mut token,
|
||||
)?
|
||||
};
|
||||
let token = unsafe { Owned::new(token) };
|
||||
let mut impersonation = HANDLE::default();
|
||||
unsafe { DuplicateToken(*token, SecurityImpersonation, &mut impersonation)? };
|
||||
Ok(unsafe { Owned::new(impersonation) })
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user