From 59686730b371ef965b10fc9175f71f311237d2c9 Mon Sep 17 00:00:00 2001 From: slvnlrt Date: Sat, 27 Dec 2025 23:17:36 +0100 Subject: [PATCH] fix(security): prevent path traversal in P2P file transfers and deletions Add path validation to FileTransferProtocolHandler and FileDeleteProtocolHandler to prevent unauthorized file access during P2P operations. - Add is_path_allowed() validation against registered Locations - Add dynamic location lookup via CoreContext integration - Add security regression tests for both protocols Prevents malicious paired devices from reading/writing/deleting files outside registered library locations. --- core/src/lib.rs | 7 +- .../service/network/protocol/file_delete.rs | 205 +++++++++++-- .../service/network/protocol/file_transfer.rs | 275 +++++++++++++++++- 3 files changed, 461 insertions(+), 26 deletions(-) diff --git a/core/src/lib.rs b/core/src/lib.rs index 72c8cdece..7449fbf8a 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -614,7 +614,7 @@ async fn register_default_protocol_handlers( networking.device_registry(), logger.clone(), command_sender, - data_dir, + data_dir.clone(), networking.endpoint().cloned(), networking.active_connections(), ), @@ -653,6 +653,11 @@ async fn register_default_protocol_handlers( // Inject device registry into file transfer handler for encryption file_transfer_handler.set_device_registry(networking.device_registry()); + // Inject context for dynamic location-based path validation + // The handler will query all libraries for registered locations at runtime. + // This ensures file transfers can only target directories that are managed by Spacedrive. + file_transfer_handler.set_context(context.clone()); + // Get device ID for job activity handler let device_id = context .device_manager diff --git a/core/src/service/network/protocol/file_delete.rs b/core/src/service/network/protocol/file_delete.rs index 2d86b7f77..65cb6485b 100644 --- a/core/src/service/network/protocol/file_delete.rs +++ b/core/src/service/network/protocol/file_delete.rs @@ -16,18 +16,24 @@ use uuid::Uuid; pub struct FileDeleteProtocolHandler { /// Optional context for accessing job system context: Option>, + /// Allowed paths for file deletions (for testing and static config) + allowed_paths: std::sync::Arc>>, } impl FileDeleteProtocolHandler { /// Create a new file delete protocol handler pub fn new() -> Self { - Self { context: None } + Self { + context: None, + allowed_paths: Default::default(), + } } /// Create handler with context pub fn with_context(context: Arc) -> Self { Self { context: Some(context), + allowed_paths: Default::default(), } } @@ -36,6 +42,12 @@ impl FileDeleteProtocolHandler { self.context = Some(context); } + /// Set the allowed paths for file deletions (for testing). + pub fn set_allowed_paths(&self, paths: Vec) { + let mut allowed = self.allowed_paths.write().unwrap(); + *allowed = paths; + } + /// Handle delete request from remote device async fn handle_delete_request( &self, @@ -59,24 +71,26 @@ impl FileDeleteProtocolHandler { // Execute deletion using the strategy // Note: We're creating a minimal job context for this operation // In a full implementation, this might integrate with the job system - let results = - match Self::execute_deletion_with_strategy(&strategy, &paths, mode.clone()).await { - Ok(results) => results, - Err(e) => { - return Ok(FileDeleteMessage::Response { - request_id, - results: paths - .iter() - .map(|path| crate::ops::files::delete::strategy::DeleteResult { - path: path.clone(), - success: false, - bytes_freed: 0, - error: Some(format!("Strategy execution failed: {}", e)), - }) - .collect(), - }); - } - }; + let results = match self + .execute_deletion_with_strategy(&strategy, &paths, mode.clone()) + .await + { + Ok(results) => results, + Err(e) => { + return Ok(FileDeleteMessage::Response { + request_id, + results: paths + .iter() + .map(|path| crate::ops::files::delete::strategy::DeleteResult { + path: path.clone(), + success: false, + bytes_freed: 0, + error: Some(format!("Strategy execution failed: {}", e)), + }) + .collect(), + }); + } + }; Ok(FileDeleteMessage::Response { request_id, @@ -91,14 +105,11 @@ impl FileDeleteProtocolHandler { /// Execute deletion with strategy (simplified without full job context) async fn execute_deletion_with_strategy( + &self, strategy: &LocalDeleteStrategy, paths: &[crate::domain::addressing::SdPath], mode: DeleteMode, ) -> anyhow::Result> { - // Create a minimal execution context - // Note: In production, this should properly integrate with the job system - // For now, we're executing directly - let mut results = Vec::new(); for path in paths { @@ -106,6 +117,21 @@ impl FileDeleteProtocolHandler { .as_local_path() .ok_or_else(|| anyhow::anyhow!("Path is not local"))?; + // Validate path is within allowed locations + if !self.is_path_allowed(local_path) { + tracing::warn!( + path = %local_path.display(), + "Delete request rejected: path outside allowed locations" + ); + results.push(crate::ops::files::delete::strategy::DeleteResult { + path: path.clone(), + success: false, + bytes_freed: 0, + error: Some("Path not within allowed locations".to_string()), + }); + continue; + } + // Get file size before deletion let size = tokio::fs::metadata(local_path) .await @@ -135,6 +161,68 @@ impl FileDeleteProtocolHandler { Ok(results) } + + /// Check if a path is within allowed locations (registered Locations from all libraries). + /// Uses canonicalization to prevent traversal attacks. + fn is_path_allowed(&self, path: &std::path::Path) -> bool { + // Canonicalize the target path to resolve symlinks and `..` + let canonical_path = match path.canonicalize() { + Ok(p) => p, + Err(_) => return false, // Path doesn't exist - can't delete anyway + }; + + // Get allowed paths from all libraries via CoreContext + let allowed_paths = self.get_all_allowed_paths(); + + if allowed_paths.is_empty() { + tracing::warn!("No allowed paths configured for file delete - denying access"); + return false; + } + + // Check if canonicalized path starts with any allowed path + for allowed in allowed_paths { + if let Ok(canonical_allowed) = allowed.canonicalize() { + if canonical_path.starts_with(&canonical_allowed) { + return true; + } + } + } + + false + } + + /// Get all allowed paths by combining static allowed_paths with dynamic locations. + fn get_all_allowed_paths(&self) -> Vec { + let mut paths = Vec::new(); + + // Add statically configured allowed paths + { + let allowed = self.allowed_paths.read().unwrap(); + paths.extend(allowed.clone()); + } + + // Add dynamic location paths from all libraries via CoreContext + if let Some(ctx) = &self.context { + let library_manager_guard = ctx.library_manager.blocking_read(); + if let Some(library_manager) = library_manager_guard.as_ref() { + let library_list: Vec> = + tokio::runtime::Handle::current().block_on(library_manager.list()); + for library in library_list { + let location_manager = + crate::location::LocationManager::new((*ctx.events).clone()); + if let Ok(locations) = tokio::runtime::Handle::current() + .block_on(location_manager.list_locations(&library)) + { + for loc in locations { + paths.push(loc.path.clone()); + } + } + } + } + } + + paths + } } impl Default for FileDeleteProtocolHandler { @@ -245,3 +333,74 @@ impl super::ProtocolHandler for FileDeleteProtocolHandler { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + #[test] + fn test_is_path_allowed_rejects_paths_outside_allowed_locations() { + let handler = FileDeleteProtocolHandler::new(); + + // Without context, no paths are allowed (fail-safe) + let outside_path = std::path::Path::new("/etc/passwd"); + assert!( + !handler.is_path_allowed(outside_path), + "Paths outside allowed locations must be rejected" + ); + + #[cfg(windows)] + { + let system_path = std::path::Path::new("C:\\Windows\\System32\\config\\SAM"); + assert!( + !handler.is_path_allowed(system_path), + "System paths must be rejected" + ); + } + } + + #[test] + fn test_is_path_allowed_denies_all_when_no_context() { + let handler = FileDeleteProtocolHandler::new(); + + // Without context, all paths should be denied (fail-safe) + let any_path = std::env::temp_dir().join("some_file.txt"); + assert!( + !handler.is_path_allowed(&any_path), + "When no context is configured, all access should be denied" + ); + } + + #[test] + fn test_get_all_allowed_paths_returns_empty_without_context() { + let handler = FileDeleteProtocolHandler::new(); + let paths = handler.get_all_allowed_paths(); + assert!( + paths.is_empty(), + "Without context, allowed paths should be empty" + ); + } + + #[test] + fn test_is_path_allowed_accepts_paths_inside_allowed_locations() { + let handler = FileDeleteProtocolHandler::new(); + + // Create a temp directory as the allowed location + let temp_dir = std::env::temp_dir().join("spacedrive_delete_test_allowed"); + let inner_path = temp_dir.join("subdir").join("file.txt"); + std::fs::create_dir_all(inner_path.parent().unwrap()).ok(); + std::fs::write(&inner_path, "test content").ok(); + + handler.set_allowed_paths(vec![temp_dir.clone()]); + + // Test: Path inside allowed location should be ACCEPTED + assert!( + handler.is_path_allowed(&inner_path), + "Paths inside allowed locations should be accepted" + ); + + // Clean up + std::fs::remove_dir_all(&temp_dir).ok(); + } +} diff --git a/core/src/service/network/protocol/file_transfer.rs b/core/src/service/network/protocol/file_transfer.rs index 9bfd91885..7e1fc455d 100644 --- a/core/src/service/network/protocol/file_transfer.rs +++ b/core/src/service/network/protocol/file_transfer.rs @@ -40,6 +40,11 @@ pub struct FileTransferProtocolHandler { Option>>, /// Logger for protocol operations logger: Arc, + /// Allowed paths for file transfers (indexed locations). + /// File writes are restricted to these directories for security. + allowed_paths: Arc>>, + /// Core context for dynamic location lookup (if available). + core_context: Option>, } /// Configuration for file transfers @@ -228,6 +233,8 @@ impl FileTransferProtocolHandler { config, device_registry: None, logger, + allowed_paths: Arc::new(RwLock::new(Vec::new())), + core_context: None, } } @@ -335,6 +342,113 @@ impl FileTransferProtocolHandler { self.device_registry = Some(device_registry); } + /// Set the allowed paths for file transfers. + /// Only paths under these directories will be accepted for read/write operations. + /// This is a critical security measure to prevent arbitrary file access. + pub fn set_allowed_paths(&self, paths: Vec) { + let mut allowed = self.allowed_paths.write().unwrap(); + *allowed = paths; + } + + /// Add a single allowed path. + pub fn add_allowed_path(&self, path: PathBuf) { + let mut allowed = self.allowed_paths.write().unwrap(); + if !allowed.iter().any(|p| p == &path) { + allowed.push(path); + } + } + + /// Set the core context for dynamic location lookup. + /// This enables the handler to query registered locations from all libraries. + pub fn set_context(&mut self, context: std::sync::Arc) { + self.core_context = Some(context); + } + + /// Get all allowed paths by combining static allowed_paths with dynamic locations. + /// This queries all libraries for their registered locations. + fn get_all_allowed_paths(&self) -> Vec { + let mut paths = Vec::new(); + + // Add statically configured allowed paths + { + let allowed = self.allowed_paths.read().unwrap(); + paths.extend(allowed.clone()); + } + + // Add dynamic location paths from all libraries via CoreContext + if let Some(ctx) = &self.core_context { + let library_manager_guard = ctx.library_manager.blocking_read(); + if let Some(library_manager) = library_manager_guard.as_ref() { + // Get all active libraries using tokio's block_on (safe in async context) + let library_list: Vec> = + tokio::runtime::Handle::current().block_on(library_manager.list()); + for library in library_list { + // Get locations for this library using LocationManager + let location_manager = + crate::location::LocationManager::new((*ctx.events).clone()); + if let Ok(locations) = tokio::runtime::Handle::current() + .block_on(location_manager.list_locations(&library)) + { + for loc in locations { + paths.push(loc.path.clone()); + } + } + } + } + } + + paths + } + + /// Check if a path is within one of the allowed paths. + /// Uses canonicalization to prevent traversal attacks. + fn is_path_allowed(&self, path: &std::path::Path) -> bool { + // Canonicalize the target path to resolve symlinks and `..` + let canonical_path = match path.canonicalize() { + Ok(p) => p, + Err(_) => { + // If the path doesn't exist yet (for writes), check the parent + if let Some(parent) = path.parent() { + match parent.canonicalize() { + Ok(p) => p, + Err(_) => return false, // Parent doesn't exist + } + } else { + return false; // No parent (root path) + } + } + }; + + // Get all allowed paths (static + dynamic from locations) + let allowed_paths = self.get_all_allowed_paths(); + + // If no allowed paths are configured and no context, deny all (fail-safe) + if allowed_paths.is_empty() { + tracing::warn!("No allowed paths configured for file transfer - denying access"); + return false; + } + + for allowed_root in allowed_paths.iter() { + // Canonicalize the allowed root for comparison + let canonical_root = match allowed_root.canonicalize() { + Ok(p) => p, + Err(_) => continue, // Skip non-existent allowed paths + }; + + // Check if the target path starts with the allowed root + if canonical_path.starts_with(&canonical_root) { + return true; + } + } + + tracing::warn!( + "Path {:?} is not within any allowed location. Allowed: {:?}", + path, + allowed_paths.iter().take(5).collect::>() // Log first 5 for brevity + ); + false + } + /// Derive chunk encryption key from session keys fn derive_chunk_key( &self, @@ -552,6 +666,22 @@ impl FileTransferProtocolHandler { .. } = request { + // SECURITY: Validate destination path is within allowed locations + let dest_path = std::path::Path::new(&destination_path); + if !self.is_path_allowed(dest_path) { + tracing::warn!( + path = %destination_path, + from_device = %from_device, + "Transfer request rejected: destination path outside allowed locations" + ); + return Ok(FileTransferMessage::TransferResponse { + transfer_id, + accepted: false, + reason: Some("Destination path not within allowed locations".to_string()), + supported_resume: false, + }); + } + // For trusted devices, auto-accept transfers let accepted = match transfer_mode { TransferMode::TrustedCopy => true, @@ -851,6 +981,22 @@ impl FileTransferProtocolHandler { )) .await; + // Validate destination path is within allowed locations + // This prevents arbitrary file write attacks from malicious peers. + let dest_path_buf = PathBuf::from(&destination_path); + if !self.is_path_allowed(&dest_path_buf) { + self.logger + .warn(&format!( + "Transfer {} rejected: destination path {:?} is not within allowed locations", + transfer_id, destination_path + )) + .await; + return Err(NetworkingError::Protocol(format!( + "Destination path not allowed: {}", + destination_path + ))); + } + // Create new transfer session let session = TransferSession { id: transfer_id, @@ -1004,6 +1150,7 @@ impl FileTransferProtocolHandler { /// Validate that a path is safe to access for PULL requests. /// Prevents directory traversal attacks and enforces access boundaries. + /// SECURITY: Only allows access to files within registered locations. fn validate_path_access(&self, path: &std::path::Path, _requested_by: Uuid) -> bool { // Normalize path to prevent directory traversal. // canonicalize() resolves all symlinks and `..` components. @@ -1017,8 +1164,15 @@ impl FileTransferProtocolHandler { return false; } - // Note: Device pairing already establishes trust. Additional library-level - // access control (restricting to indexed locations) can be added later. + // Validate path is within allowed locations + // This prevents arbitrary file read attacks from malicious peers. + if !self.is_path_allowed(&normalized) { + tracing::warn!( + "Path access denied: {:?} is not within allowed locations", + path + ); + return false; + } true } @@ -1723,4 +1877,121 @@ mod tests { .update_session_state(&transfer_id, TransferState::Active) .is_err()); } + + // Path validation security tests + + #[test] + fn test_is_path_allowed_rejects_paths_outside_allowed_locations() { + let logger = Arc::new(SilentLogger); + let handler = FileTransferProtocolHandler::new_default(logger); + + // Create a temp directory as the only allowed location + let temp_dir = std::env::temp_dir().join("spacedrive_test_allowed"); + std::fs::create_dir_all(&temp_dir).ok(); + handler.set_allowed_paths(vec![temp_dir.clone()]); + + // Test: Path outside allowed location should be REJECTED + let outside_path = std::path::Path::new("/etc/passwd"); + assert!( + !handler.is_path_allowed(outside_path), + "Paths outside allowed locations must be rejected" + ); + + // Test: Windows system path should be REJECTED + #[cfg(windows)] + { + let system_path = std::path::Path::new("C:\\Windows\\System32\\config\\SAM"); + assert!( + !handler.is_path_allowed(system_path), + "System paths must be rejected" + ); + } + + // Clean up + std::fs::remove_dir_all(&temp_dir).ok(); + } + + #[test] + fn test_is_path_allowed_accepts_paths_inside_allowed_locations() { + let logger = Arc::new(SilentLogger); + let handler = FileTransferProtocolHandler::new_default(logger); + + // Create a temp directory as the allowed location + let temp_dir = std::env::temp_dir().join("spacedrive_test_allowed_inner"); + let inner_path = temp_dir.join("subdir").join("file.txt"); + std::fs::create_dir_all(inner_path.parent().unwrap()).ok(); + std::fs::write(&inner_path, "test content").ok(); + + handler.set_allowed_paths(vec![temp_dir.clone()]); + + // Test: Path inside allowed location should be ACCEPTED + assert!( + handler.is_path_allowed(&inner_path), + "Paths inside allowed locations should be accepted" + ); + + // Clean up + std::fs::remove_dir_all(&temp_dir).ok(); + } + + #[test] + fn test_is_path_allowed_rejects_traversal_attempts() { + let logger = Arc::new(SilentLogger); + let handler = FileTransferProtocolHandler::new_default(logger); + + // Create a temp directory as the only allowed location + let temp_dir = std::env::temp_dir().join("spacedrive_test_traversal"); + std::fs::create_dir_all(&temp_dir).ok(); + handler.set_allowed_paths(vec![temp_dir.clone()]); + + // Test: Path traversal attempt should be REJECTED + // Note: canonicalize() will resolve this, but if it resolves outside, it's rejected + let traversal_path = temp_dir.join("..").join("..").join("etc").join("passwd"); + assert!( + !handler.is_path_allowed(&traversal_path), + "Path traversal attempts must be rejected" + ); + + // Clean up + std::fs::remove_dir_all(&temp_dir).ok(); + } + + #[test] + fn test_is_path_allowed_denies_all_when_no_paths_configured() { + let logger = Arc::new(SilentLogger); + let handler = FileTransferProtocolHandler::new_default(logger); + + // Don't configure any allowed paths - this should deny ALL access (fail-safe) + let any_path = std::env::temp_dir().join("some_file.txt"); + assert!( + !handler.is_path_allowed(&any_path), + "When no allowed paths configured, all access should be denied" + ); + } + + #[test] + fn test_add_allowed_path_works() { + let logger = Arc::new(SilentLogger); + let handler = FileTransferProtocolHandler::new_default(logger); + + let temp_dir = std::env::temp_dir().join("spacedrive_test_add_path"); + std::fs::create_dir_all(&temp_dir).ok(); + let file_path = temp_dir.join("test_file.txt"); + std::fs::write(&file_path, "content").ok(); + + // Initially denied + assert!(!handler.is_path_allowed(&file_path)); + + // Add the path + handler.add_allowed_path(temp_dir.clone()); + + // Now allowed + assert!( + handler.is_path_allowed(&file_path), + "add_allowed_path should permit access to paths within the added directory" + ); + + // Clean up + std::fs::remove_dir_all(&temp_dir).ok(); + } }