mirror/spacedrive
1
0
Fork 0
mirror of https://github.com/spacedriveapp/spacedrive.git synced 2026-05-19 13:55:40 -04:00
Code Issues Packages Projects Releases Wiki Activity

fix(security): prevent path traversal in P2P file transfers and deletions

Browse Source 
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.
This commit is contained in:
slvnlrt
2025-12-27 23:17:36 +01:00
parent 544f4ef063
commit 59686730b3
3 changed files with 461 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
core/src/lib.rs
View File

@@ -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

205
core/src/service/network/protocol/file_delete.rs
View File

@@ -16,18 +16,24 @@ use uuid::Uuid;
pub struct FileDeleteProtocolHandler {
/// Optional context for accessing job system
context: Option<Arc<crate::context::CoreContext>>,
/// Allowed paths for file deletions (for testing and static config)
allowed_paths: std::sync::Arc<std::sync::RwLock<Vec<std::path::PathBuf>>>,
}
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<crate::context::CoreContext>) -> 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<std::path::PathBuf>) {
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<Vec<crate::ops::files::delete::strategy::DeleteResult>> {
// 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<std::path::PathBuf> {
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<std::sync::Arc<crate::library::Library>> =
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();
}
}

275
core/src/service/network/protocol/file_transfer.rs
View File

@@ -40,6 +40,11 @@ pub struct FileTransferProtocolHandler {
Option<Arc<tokio::sync::RwLock<crate::service::network::device::DeviceRegistry>>>,
/// Logger for protocol operations
logger: Arc<dyn NetworkLogger>,
/// Allowed paths for file transfers (indexed locations).
/// File writes are restricted to these directories for security.
allowed_paths: Arc<RwLock<Vec<PathBuf>>>,
/// Core context for dynamic location lookup (if available).
core_context: Option<std::sync::Arc<crate::context::CoreContext>>,
}
/// 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<PathBuf>) {
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<crate::context::CoreContext>) {
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<PathBuf> {
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<std::sync::Arc<crate::library::Library>> =
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::<Vec<_>>() // 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();
}
}