Fix/windows installer directory guard (#16908)

* fix(windows): guard staged installer directories during handoff

Create restrictive staging directories and retain non-delete-sharing handles while CMD consumes packages, scripts and shortcuts. Adapt output paths and cleanup, and add a directory-replacement regression.

Validated with 11 focused Windows tests. Directory protection remains tied to the lifetime of the creating process.

Signed-off-by: fufesou <linlong1266@gmail.com>

* test(windows): streamline installer directory guard tests

Keep installer-file tests in their original inline module and remove the duplicate CMD-copy check from the directory replacement regression. Preserve directory protection and existing handoff coverage.

Validation: Windows library test compilation, four focused tests, rustfmt and git diff --check.
Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(windows): use plain rd for empty staging directories

Remove the inapplicable /q switch from non-recursive staging-directory cleanup. Keep recursive deletion confined to the runner directory.

Validated with Windows library test compilation, four focused tests and isolated CMD probes for owner termination, nonempty staging directories and exit-code preservation.

Signed-off-by: fufesou <linlong1266@gmail.com>

* test(windows): pin installer staging directory permissions

Check effective ordinary-owner directory rights independently of the sharing guard. Clarify guard lifetime and the Rust and CMD cleanup responsibilities.

Validation: Windows library test compilation, four focused tests, rustfmt and git diff --check.
Signed-off-by: fufesou <linlong1266@gmail.com>

---------

Signed-off-by: fufesou <linlong1266@gmail.com>

Cargo.toml

@@ -127,6 +127,8 @@ winapi = { version = "0.3", features = [
     "winspool",
 ] }
 windows = { version = "0.61", features = [
+    "Wdk_Foundation",
+    "Wdk_Storage_FileSystem",
     "Win32",
     "Win32_Foundation",
     "Win32_Security",
@@ -143,6 +145,7 @@ windows = { version = "0.61", features = [
     "Win32_System_Registry",
     "Win32_System_SystemInformation",
     "Win32_System_Threading",
+    "Win32_System_WindowsProgramming",
     "Win32_UI_Shell",
     "Win32_UI_WindowsAndMessaging",
 ] }

src/platform/windows/installer_files.rs

@@ -1,3 +1,5 @@
+mod staging;
+
 use super::{installer_handoff::directory, wide_string};
 use hbb_common::{anyhow::anyhow, log, ResultType};
 use std::{
@@ -29,6 +31,7 @@ use windows::{
 pub(super) struct ProtectedInstallFile {
     path: PathBuf,
     cleanup_handle: Option<Owned<HANDLE>>,
+    _directory: staging::StagingDirectory,
 }
 
 impl ProtectedInstallFile {
@@ -47,7 +50,12 @@ impl ProtectedInstallFile {
         source: &mut impl Read,
         expected_sha256: Option<&str>,
     ) -> ResultType<Self> {
-        let mut file = create_protected_file(path)?;
+        let filename = path
+            .file_name()
+            .ok_or_else(|| anyhow!("Protected installer path has no filename"))?;
+        let directory = staging::StagingDirectory::new(&path.with_extension("stage"))?;
+        let path = directory.path().join(filename);
+        let mut file = create_protected_file(&path)?;
         let result = (|| -> ResultType<Owned<HANDLE>> {
             io::copy(source, &mut file)?;
             file.sync_all()?;
@@ -72,8 +80,9 @@ impl ProtectedInstallFile {
             error
         })?;
         Ok(Self {
-            path: path.to_owned(),
+            path,
             cleanup_handle: Some(cleanup_handle),
+            _directory: directory,
         })
     }
 
@@ -81,6 +90,10 @@ impl ProtectedInstallFile {
         &self.path
     }
 
+    pub(super) fn directory(&self) -> &Path {
+        self._directory.path()
+    }
+
     pub(super) fn release(&mut self) {
         // cmd.exe cannot read/copy these files while a DELETE handle is open, so every
         // staged handle is closed before the elevated cmd starts.
@@ -183,7 +196,7 @@ fn delete_staged_file(handle: HANDLE) -> ResultType<()> {
 }
 
 #[cfg(test)]
-mod tests {
+pub(super) mod tests {
     use super::*;
     use windows::{
         core::BOOL,
@@ -199,7 +212,8 @@ mod tests {
                 TOKEN_QUERY,
             },
             Storage::FileSystem::{
-                FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, FILE_GENERIC_WRITE,
+                FILE_ADD_FILE, FILE_ADD_SUBDIRECTORY, FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE,
+                FILE_GENERIC_READ, FILE_GENERIC_WRITE,
             },
             System::Threading::{GetCurrentProcess, OpenProcessToken},
         },
@@ -212,6 +226,8 @@ mod tests {
         fs::create_dir(&root).unwrap();
         let path = root.join("package.tmp");
         let file = ProtectedInstallFile::new(&path, b"cancelled update").unwrap();
+        let staged_path = file.path().to_owned();
+        let staged_directory = file.directory().to_owned();
         // Omit DELETE_CHILD as in Windows Temp, but retain fixture ACL control.
         set_parent_acl(&root, w!("D:P(A;;0x1600af;;;OW)"));
         unsafe {
@@ -221,12 +237,17 @@ mod tests {
             RevertToSelf().unwrap();
         }
         set_parent_acl(&root, w!("D:P(A;;FA;;;OW)"));
-        let remaining = fs::symlink_metadata(&path);
+        let remaining = fs::symlink_metadata(&staged_path);
+        let directory_remaining = staged_directory.exists();
         fs::remove_dir_all(&root).unwrap();
         assert!(
             matches!(remaining, Err(error) if error.kind() == io::ErrorKind::NotFound),
             "Cancelled staging must remove the protected package"
         );
+        assert!(
+            !directory_remaining,
+            "Cancelled staging must remove its directory"
+        );
     }
 
     fn set_parent_acl(path: &Path, sddl: PCWSTR) {
@@ -264,6 +285,10 @@ mod tests {
             ordinary_owner_access(file.path()),
             (FILE_GENERIC_READ | FILE_GENERIC_EXECUTE).0
         );
+        assert_eq!(
+            ordinary_owner_access(file.directory()),
+            (FILE_GENERIC_READ | FILE_GENERIC_EXECUTE | FILE_ADD_FILE | FILE_ADD_SUBDIRECTORY).0
+        );
         assert_eq!(fs::read(file.path()).unwrap(), b"rustdesk");
         assert!(ProtectedInstallFile::new(&path, b"replacement").is_err());
         let rejected = root.join("rejected.tmp");
@@ -273,7 +298,7 @@ mod tests {
             Some(HASH)
         )
         .is_err());
-        assert!(!rejected.exists());
+        assert!(!rejected.with_extension("stage").exists());
         drop(file);
         fs::remove_dir(root).unwrap();
     }
@@ -370,4 +395,37 @@ mod tests {
         }
         unsafe { Owned::new(impersonation) }
     }
+
+    pub(in crate::platform::windows) fn release_copy_fixture(file: &mut ProtectedInstallFile) {
+        let bytes = fs::read(file.path()).unwrap();
+        delete_staged_file(**file.cleanup_handle.as_ref().unwrap()).unwrap();
+        file.release();
+        // Copy/cleanup tests use deletable children; file ACLs have their own regression.
+        fs::write(file.path(), bytes).unwrap();
+    }
+
+    #[test]
+    fn released_input_directory_rejects_replacement() {
+        use windows::Win32::Security::{ImpersonateLoggedOnUser, RevertToSelf};
+
+        let root = std::env::temp_dir().join(format!("rustdesk-copy-{}", uuid::Uuid::new_v4()));
+        fs::create_dir(&root).unwrap();
+        let requested = root.join("package 中文 !&^@().tmp");
+        let mut input = ProtectedInstallFile::new(&requested, b"verified installer").unwrap();
+        release_copy_fixture(&mut input);
+        let directory = input.path().parent().unwrap().to_owned();
+        let moved = root.with_extension("moved");
+        let token = ordinary_owner_token();
+        unsafe { ImpersonateLoggedOnUser(*token).unwrap() };
+        let renamed = fs::rename(&directory, &moved);
+        unsafe { RevertToSelf().unwrap() };
+        if renamed.is_ok() {
+            fs::rename(&moved, &directory).unwrap();
+        }
+        fs::remove_file(input.path()).unwrap();
+        drop(input);
+        fs::remove_dir(root).unwrap();
+
+        assert!(renamed.is_err(), "Released input directory was replaceable");
+    }
 }

src/platform/windows/installer_files/staging.rs

@@ -0,0 +1,133 @@
+use super::super::{
+    installer_files::delete_staged_file, installer_shell::path_for_cmd_environment, wide_string,
+};
+use hbb_common::{anyhow::anyhow, log, ResultType};
+use std::path::{Path, PathBuf};
+use windows::{
+    core::{w, Owned, PCWSTR},
+    Wdk::{
+        Foundation::OBJECT_ATTRIBUTES,
+        Storage::FileSystem::{
+            NtCreateFile, RtlDosPathNameToNtPathName_U_WithStatus, FILE_CREATE,
+            FILE_DIRECTORY_FILE, FILE_OPEN_REPARSE_POINT, FILE_SYNCHRONOUS_IO_NONALERT,
+        },
+    },
+    Win32::{
+        Foundation::{HANDLE, HLOCAL, OBJ_CASE_INSENSITIVE, UNICODE_STRING},
+        Security::{
+            Authorization::{
+                ConvertStringSecurityDescriptorToSecurityDescriptorW, SDDL_REVISION_1,
+            },
+            PSECURITY_DESCRIPTOR,
+        },
+        Storage::FileSystem::{
+            DELETE, FILE_ATTRIBUTE_DIRECTORY, FILE_GENERIC_READ, FILE_SHARE_READ,
+        },
+        System::{WindowsProgramming::RtlFreeUnicodeString, IO::IO_STATUS_BLOCK},
+    },
+};
+
+// The owner can read and create children, but cannot delete or change permissions.
+const STAGING_DIRECTORY_SDDL: PCWSTR = w!("D:P(A;;FA;;;SY)(A;;FA;;;BA)(A;;0x1200af;;;OW)");
+
+pub(super) struct StagingDirectory {
+    path: PathBuf,
+    guard: Owned<HANDLE>,
+}
+
+impl StagingDirectory {
+    pub(super) fn new(path: &Path) -> ResultType<Self> {
+        let descriptor = security_descriptor()?;
+        let native_path = NativePath::new(path)?;
+        let attributes = OBJECT_ATTRIBUTES {
+            Length: std::mem::size_of::<OBJECT_ATTRIBUTES>() as u32,
+            ObjectName: &native_path.0,
+            Attributes: OBJ_CASE_INSENSITIVE,
+            SecurityDescriptor: descriptor.0.cast(),
+            ..Default::default()
+        };
+        let mut handle = HANDLE::default();
+        let mut status = IO_STATUS_BLOCK::default();
+        // Acquire protection in the creation operation, before the parent can rename this entry.
+        unsafe {
+            NtCreateFile(
+                &mut handle,
+                FILE_GENERIC_READ | DELETE,
+                &attributes,
+                &mut status,
+                None,
+                FILE_ATTRIBUTE_DIRECTORY,
+                FILE_SHARE_READ,
+                FILE_CREATE,
+                FILE_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_REPARSE_POINT,
+                None,
+                0,
+            )
+            .ok()
+        }
+        .map_err(|error| {
+            anyhow!(
+                "Failed to create installer staging directory {}: {error}",
+                path.display()
+            )
+        })?;
+        Ok(Self {
+            path: path.to_owned(),
+            guard: unsafe { Owned::new(handle) },
+        })
+    }
+
+    pub(super) fn path(&self) -> &Path {
+        &self.path
+    }
+}
+
+impl Drop for StagingDirectory {
+    fn drop(&mut self) {
+        // Delete only the empty directory we created, without reopening a mutable path.
+        if let Err(error) = delete_staged_file(*self.guard) {
+            log::warn!(
+                "Protected installer staging directory may remain at {}: {error}",
+                self.path.display()
+            );
+        }
+    }
+}
+
+struct NativePath(UNICODE_STRING);
+
+impl NativePath {
+    fn new(path: &Path) -> ResultType<Self> {
+        let path_string = wide_string(path_for_cmd_environment(path)?);
+        let mut native_path = Self(UNICODE_STRING::default());
+        unsafe {
+            RtlDosPathNameToNtPathName_U_WithStatus(
+                PCWSTR(path_string.as_ptr()),
+                &mut native_path.0,
+                None,
+                None,
+            )
+            .ok()?;
+        }
+        Ok(native_path)
+    }
+}
+
+impl Drop for NativePath {
+    fn drop(&mut self) {
+        unsafe { RtlFreeUnicodeString(&mut self.0) };
+    }
+}
+
+fn security_descriptor() -> ResultType<Owned<HLOCAL>> {
+    let mut descriptor = PSECURITY_DESCRIPTOR::default();
+    unsafe {
+        ConvertStringSecurityDescriptorToSecurityDescriptorW(
+            STAGING_DIRECTORY_SDDL,
+            SDDL_REVISION_1,
+            &mut descriptor,
+            None,
+        )?;
+    }
+    Ok(unsafe { Owned::new(HLOCAL(descriptor.0)) })
+}

src/platform/windows/installer_handoff.rs

@@ -54,6 +54,17 @@ fn prepare_install_commands(cmds: &InstallCommands) -> ResultType<String> {
         .map(|file| path_for_cmd_environment(file.path()).map(|path| format!("\"{path}\"")))
         .collect::<ResultType<Vec<_>>>()?
         .join(" ");
+    // While the owner is alive, guards defer directory removal to Rust's Drop.
+    // CMD removes them if uninstall has already terminated the owner.
+    let directories = cmds
+        .files
+        .iter()
+        .map(|file| {
+            path_for_cmd_environment(file.directory())
+                .map(|path| format!(" & rd \"{path}\" > nul 2>&1"))
+        })
+        .collect::<ResultType<Vec<_>>>()?
+        .join("");
     // Call a fixed label so early EXIT /B still reaches cleanup, without reparsing a path.
     // Exit the dedicated cmd after self-deletion; EXIT /B would reopen the missing BAT.
     Ok(format!(
@@ -62,7 +73,7 @@ fn prepare_install_commands(cmds: &InstallCommands) -> ResultType<String> {
          call :rustdesk_install\r\n\
          set \"RUSTDESK_INSTALL_EXIT_CODE=%ERRORLEVEL%\"\r\n\
          rd /s /q \"%~f0.dir\" > nul 2>&1\r\n\
-         (del /q \"%~f0\" {files} > nul 2>&1 & exit %RUSTDESK_INSTALL_EXIT_CODE%)\r\n\
+         (del /q \"%~f0\" {files} > nul 2>&1{directories} & rd \"%~dp0\" > nul 2>&1 & exit %RUSTDESK_INSTALL_EXIT_CODE%)\r\n\
          :rustdesk_install\r\n\
          \"{chcp}\" {UTF8_CODE_PAGE} > nul || exit /b \
          {BATCH_CODE_PAGE_FAILURE_EXIT_CODE}\r\n\
@@ -93,16 +104,17 @@ fn protected_install_parameters(script: &InstallCommandScript) -> ResultType<Str
 pub(super) fn run_cmds(cmds: impl Into<InstallCommands>, show: bool, tip: &str) -> ResultType<()> {
     validate_install_app_name(&crate::get_app_name())?;
     let path = directory::path()?.with_extension("bat");
-    let directory = path.with_extension("bat.dir");
     let mut script = write_install_script(cmds.into(), &path)?;
+    let directory = script.file.path().with_extension("bat.dir");
     let cmd_path = get_system_executable(CMD_RELATIVE_PATH)?;
     let parameters = protected_install_parameters(&script)?;
     directory::create(&directory)?;
-    // cmd cannot open the BAT or copy its inputs while a DELETE-capable handle is open, so
-    // close them before launching it; a cancelled UAC prompt then leaves the files behind.
-    // Holding them until cmd read a release signal from a named pipe failed under F-Secure's
-    // behavior detection, which closed cmd's end first (#16840, #16842). The file DACLs and
-    // the parent directory's permissions still keep unelevated users from changing them.
+    // Close file DELETE handles before cmd opens the BAT or copies its inputs.
+    // Waiting on a named pipe failed under F-Secure (#16840, #16842).
+    // Directory guards protect paths only while their handles remain open.
+    // They survive this release, but cannot protect against an attacker who can
+    // control or terminate this process.
+    // A cancelled UAC prompt can leave the protected inputs behind.
     script.release_files();
     let process = launch_elevated(&cmd_path, &parameters, show).map_err(|err| {
         log::debug!("Failed to launch elevated installer: {err:#}");
@@ -153,9 +165,8 @@ mod tests {
         for exit_code in [0, COMMAND_FAILURE] {
             let root =
                 std::env::temp_dir().join(format!("rustdesk-handoff-{}", uuid::Uuid::new_v4()));
-            let source = root.join("source !&^@().bat");
-            let runner = source.with_extension("bat.dir");
-            fs::create_dir_all(&runner).unwrap();
+            let source = root.join("source 中文 !&^@().bat");
+            fs::create_dir(&root).unwrap();
             let bytes = shortcut_bytes(r"C:\RustDesk.exe", Some("--tray"), None).unwrap();
             let shortcut =
                 ProtectedInstallFile::new(&root.join("shortcut !&^@().tmp"), &bytes).unwrap();
@@ -169,16 +180,18 @@ mod tests {
                 files: vec![shortcut],
             };
             let mut script = write_install_script(commands, &source).unwrap();
-            // The DACL, not a held handle, keeps the unelevated owner from editing the BAT.
-            script.release_files();
-            if !super::super::is_elevated(None).unwrap() {
-                assert!(fs::write(script.file.path(), "exit /b 0\r\n").is_err());
+            let runner = script.file.path().with_extension("bat.dir");
+            fs::create_dir(&runner).unwrap();
+            for file in std::iter::once(&mut script.file).chain(&mut script._files) {
+                super::super::installer_files::tests::release_copy_fixture(file);
             }
             let output = run_script(&mut script);
             let copied = fs::read(&marker).unwrap();
             let cleaned = !runner.exists()
                 && !script.file.path().exists()
                 && script._files.iter().all(|file| !file.path().exists());
+            drop(script);
+            let directories_cleaned = fs::read_dir(&root).unwrap().count() == 1;
             fs::remove_dir_all(&root).unwrap();
             assert_eq!(
                 output.status.code().map(|code| code as u32),
@@ -186,7 +199,10 @@ mod tests {
                 "{output:?}"
             );
             assert_eq!(copied, bytes);
-            assert!(cleaned, "The runner and protected inputs must be removed");
+            assert!(
+                cleaned && directories_cleaned,
+                "The staged inputs must be removed"
+            );
         }
     }
 

src/platform/windows/installer_handoff/directory/tests/security.rs

@@ -43,9 +43,12 @@ fn junction_installer_parent_uses_program_data_for_staging() {
         assert_eq!(parent, get_known_folder(&FOLDERID_ProgramData).unwrap());
         let path = parent.join(format!("rustdesk-fallback-{}.tmp", uuid::Uuid::new_v4()));
         let staged = ProtectedInstallFile::new(&path, b"installer").unwrap();
+        let staged_path = staged.path().to_owned();
+        let staged_directory = staged.directory().to_owned();
         let contents = fs::read(staged.path()).unwrap();
         drop(staged);
         assert_eq!(contents, b"installer");
-        assert!(!path.exists());
+        assert!(!staged_path.exists());
+        assert!(!staged_directory.exists());
     }
 }