DG2608-10: [desktop] Unvalidated instance identifier enables privileged file operations

src-tauri/Cargo.toml

@@ -40,6 +40,7 @@ qrcode = { version = "0.14", features = ["image"] }
 tokio-tungstenite = { version = "0.30", features = ["native-tls"] }
 tokio-util = "0.7"
 url = "2"
+uuid = { version = "1", features = ["v4"] }
 vergen-git2 = { version = "10.0", features = ["build"] }
 wiremock = "0.6"
 x25519-dalek = { version = "3.0", features = [
@@ -134,7 +135,7 @@ time = { version = "0.3", features = ["formatting", "macros"] }
 tokio.workspace = true
 tokio-util = "0.7"
 tonic.workspace = true
-uuid = { version = "1", features = ["v4"] }
+uuid.workspace = true
 tonic-prost.workspace = true
 tower = "0.5"
 tracing.workspace = true

src-tauri/daemon/src/daemon.rs

@@ -25,7 +25,7 @@ use defguard_client_proto::defguard::{
 use defguard_client_service_locations::reconciler::{run_reconciler, ReconcileSignal};
 use defguard_client_service_locations::ServiceLocationError;
 #[cfg(any(windows, target_os = "linux"))]
-use defguard_client_service_locations::ServiceLocationManager;
+use defguard_client_service_locations::{validate_instance_id, ServiceLocationManager};
 #[cfg(not(target_os = "macos"))]
 use defguard_wireguard_rs::Kernel;
 #[cfg(target_os = "macos")]
@@ -222,7 +222,13 @@ impl DesktopDaemonService for DaemonService {
         request: tonic::Request<SaveServiceLocationsRequest>,
     ) -> Result<Response<()>, Status> {
         debug!("Received a request to save service locations");
-        let service_location = request.into_inner();
+        let mut service_location = request.into_inner();
+        service_location.instance_id = validate_instance_id(&service_location.instance_id)
+            .map_err(|err| {
+                let msg = format!("Failed to save service locations: {err}");
+                error!("{msg}");
+                Status::invalid_argument(msg)
+            })?;
 
         self.service_location_manager
             .write()
@@ -259,6 +265,11 @@ impl DesktopDaemonService for DaemonService {
     ) -> Result<Response<()>, Status> {
         debug!("Received a request to delete service locations");
         let instance_id = request.into_inner().instance_id;
+        let instance_id = validate_instance_id(&instance_id).map_err(|err| {
+            let msg = format!("Failed to delete service locations: {err}");
+            error!("{msg}");
+            Status::invalid_argument(msg)
+        })?;
 
         let mut manager = self.service_location_manager.write().unwrap();
         manager

src-tauri/enterprise/service-locations/Cargo.toml

@@ -22,6 +22,7 @@ serde.workspace = true
 serde_json.workspace = true
 thiserror.workspace = true
 tokio = { workspace = true, features = ["time"] }
+uuid.workspace = true
 
 [dev-dependencies]
 tempfile.workspace = true

src-tauri/enterprise/service-locations/src/lib.rs

@@ -1,6 +1,6 @@
-#[cfg(any(windows, target_os = "linux", test))]
-use std::ffi::OsStr;
 use std::{collections::HashMap, fmt, fs, path::Path, time::SystemTime};
+#[cfg(any(windows, target_os = "linux", test))]
+use std::{ffi::OsStr, path::PathBuf};
 
 use defguard_client_core::{
     database::models::{
@@ -17,6 +17,7 @@ use defguard_wireguard_rs::{error::WireguardInterfaceError, WGApi};
 use log::debug;
 use log::warn;
 use serde::{Deserialize, Serialize};
+use uuid::fmt::Hyphenated;
 
 #[cfg(target_os = "linux")]
 pub mod linux;
@@ -34,6 +35,8 @@ pub enum ServiceLocationError {
     InitError(String),
     #[error("Failed to load service location storage: {0}")]
     LoadError(String),
+    #[error("Invalid instance ID: {0}")]
+    InvalidInstanceId(String),
     #[error(transparent)]
     Io(#[from] std::io::Error),
     #[error(transparent)]
@@ -191,6 +194,21 @@ impl fmt::Debug for SingleServiceLocationData {
     }
 }
 
+pub fn validate_instance_id(instance_id: &str) -> Result<String, ServiceLocationError> {
+    instance_id
+        .parse::<Hyphenated>()
+        .map(|uuid| uuid.to_string())
+        .map_err(|_| ServiceLocationError::InvalidInstanceId(instance_id.to_string()))
+}
+
+#[cfg(any(windows, target_os = "linux", test))]
+fn instance_file_path(
+    directory: &Path,
+    instance_id: &str,
+) -> Result<PathBuf, ServiceLocationError> {
+    Ok(directory.join(format!("{}.json", validate_instance_id(instance_id)?)))
+}
+
 /// Whether the file at `path` already holds exactly `contents`. Makes a save idempotent thus
 /// allowing pushing service locations on every poll cycle.
 ///
@@ -284,6 +302,60 @@ pub fn to_service_location(location: &Location<Id>) -> Result<ServiceLocation, C
 mod tests {
     use super::*;
 
+    const VALID_INSTANCE_ID: &str = "0f8fad5b-d9cb-469f-a165-70867728950e";
+
+    #[test]
+    fn test_instance_id_is_normalized() {
+        assert_eq!(
+            validate_instance_id("0F8FAD5B-D9CB-469F-A165-70867728950E").unwrap(),
+            VALID_INSTANCE_ID
+        );
+    }
+
+    #[test]
+    fn test_non_uuid_instance_ids_are_rejected() {
+        let invalid = [
+            "",
+            "..",
+            "../../etc/defguard/evil",
+            "..\\..\\Windows\\Temp\\evil",
+            "C:\\Windows\\Temp\\evil",
+            "/etc/defguard/evil",
+            "0f8fad5b-d9cb-469f-a165-70867728950e/../evil",
+            "0f8fad5b-d9cb-469f-a165-70867728950",
+            "0f8fad5b-d9cb-469f-a165-70867728950eb",
+            "0f8fad5b-d9cb-469f-a165-7086772895ez",
+            "0f8fad5bd9cb469fa16570867728950e",
+            "{0f8fad5b-d9cb-469f-a165-70867728950e}",
+            "urn:uuid:0f8fad5b-d9cb-469f-a165-70867728950e",
+        ];
+
+        let dir = tempfile::tempdir().expect("failed to create temp dir");
+
+        for instance_id in invalid {
+            assert!(
+                validate_instance_id(instance_id).is_err(),
+                "instance ID {instance_id} should be rejected"
+            );
+            assert!(
+                instance_file_path(dir.path(), instance_id).is_err(),
+                "instance ID {instance_id} must not produce a file path"
+            );
+        }
+    }
+
+    #[test]
+    fn test_instance_file_path_stays_in_the_storage_directory() {
+        let dir = tempfile::tempdir().expect("failed to create temp dir");
+        let path = instance_file_path(dir.path(), VALID_INSTANCE_ID).unwrap();
+
+        assert_eq!(path.parent(), Some(dir.path()));
+        assert_eq!(
+            path.file_name().unwrap(),
+            OsStr::new(&format!("{VALID_INSTANCE_ID}.json"))
+        );
+    }
+
     /// A save is a no-op only if this comparison is exact: getting it wrong does not merely cost a
     /// disk write, it drops and rebuilds every tunnel on the box.
     #[test]

src-tauri/enterprise/service-locations/src/linux.rs

@@ -18,7 +18,8 @@ use defguard_wireguard_rs::{
 use log::{debug, error, info, warn};
 
 use crate::{
-    is_unchanged_on_disk, load_service_locations_from_directory, load_service_locations_from_file,
+    instance_file_path, is_unchanged_on_disk, load_service_locations_from_directory,
+    load_service_locations_from_file,
     reconciler::{
         reconcile_action, PostureAuthorizationRequest, PostureAuthorizations, ReconcileAction,
     },
@@ -35,8 +36,8 @@ fn get_shared_directory() -> PathBuf {
     PathBuf::from(DEFGUARD_DIR).join(SERVICE_LOCATIONS_SUBDIR)
 }
 
-fn get_instance_file_path(instance_id: &str) -> PathBuf {
-    get_shared_directory().join(format!("{instance_id}.json"))
+fn get_instance_file_path(instance_id: &str) -> Result<PathBuf, ServiceLocationError> {
+    instance_file_path(&get_shared_directory(), instance_id)
 }
 
 fn ensure_shared_directory() -> Result<PathBuf, ServiceLocationError> {
@@ -119,7 +120,7 @@ impl ServiceLocationManager {
             ServiceLocationData::from_save_request(request, service_locations.clone());
 
         ensure_shared_directory()?;
-        let instance_file_path = get_instance_file_path(instance_id);
+        let instance_file_path = get_instance_file_path(instance_id)?;
         let json = serde_json::to_string_pretty(&service_location_data)?;
 
         // Saving is pushed unconditionally on every poll cycle, so nothing having changed is the
@@ -653,7 +654,7 @@ impl ServiceLocationManager {
     ) -> Result<(), ServiceLocationError> {
         debug!("Deleting Linux service locations for instance {instance_id}");
 
-        let instance_file_path = get_instance_file_path(instance_id);
+        let instance_file_path = get_instance_file_path(instance_id)?;
         if instance_file_path.exists() {
             fs::remove_file(&instance_file_path)?;
             debug!("Deleted Linux service locations for instance {instance_id}");
@@ -676,7 +677,7 @@ impl ServiceLocationManager {
         &self,
         instance_id: &str,
     ) -> Result<Option<ServiceLocationData>, ServiceLocationError> {
-        let instance_file_path = get_instance_file_path(instance_id);
+        let instance_file_path = get_instance_file_path(instance_id)?;
         load_service_locations_from_file(&instance_file_path)
     }
 }

src-tauri/enterprise/service-locations/src/windows.rs

@@ -28,7 +28,8 @@ use windows_acl::acl::ACL;
 use windows_sys::Win32::NetworkManagement::IpHelper::NotifyAddrChange;
 
 use crate::{
-    is_unchanged_on_disk, load_service_locations_from_directory, load_service_locations_from_file,
+    instance_file_path, is_unchanged_on_disk, load_service_locations_from_directory,
+    load_service_locations_from_file,
     reconciler::{
         reconcile_action, PostureAuthorizationRequest, PostureAuthorizations, ReconcileAction,
         ReconcileSignal,
@@ -259,9 +260,7 @@ fn set_protected_acls(path: &str) -> Result<(), ServiceLocationError> {
 }
 
 fn get_instance_file_path(instance_id: &str) -> Result<PathBuf, ServiceLocationError> {
-    let mut path = get_shared_directory()?;
-    path.push(format!("{instance_id}.json"));
-    Ok(path)
+    instance_file_path(&get_shared_directory()?, instance_id)
 }
 
 pub(crate) fn is_user_logged_in() -> bool {