diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..44dbcd2 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,4 @@ +## 2024-05-24 - Pre-canonicalize Base Path to Prevent TOCTOU Bypass +**Vulnerability:** Path traversal bypass in `FileSystemContext`. A physical check against symlink escapes was entirely dependent on `self.base_path.canonicalize()` succeeding within `secure_path`. If canonicalization failed, the check was silently skipped. +**Learning:** Security checks that depend on file system operations like canonicalization must be performed at initialization, not lazily during use, to avoid Time-of-Check to Time-of-Use (TOCTOU) issues and silent bypasses. +**Prevention:** Canonicalize the base path during constructor initialization (`FileSystemContext::new`) and fail fast if it is invalid, returning a `Result`. Subsequent security checks can then rely on the pre-canonicalized path. diff --git a/crates/services/src/lib.rs b/crates/services/src/lib.rs index 4944b30..1177fb2 100644 --- a/crates/services/src/lib.rs +++ b/crates/services/src/lib.rs @@ -142,10 +142,13 @@ pub struct FileSystemContext { } impl FileSystemContext { - pub fn new>(base_path: P) -> Self { - Self { - base_path: base_path.as_ref().to_path_buf(), - } + pub fn new>(base_path: P) -> ServiceResult { + let canonical_base = base_path.as_ref().canonicalize().map_err(|e| { + ServiceError::execution_dynamic(format!("Failed to canonicalize base path: {}", e)) + })?; + Ok(Self { + base_path: canonical_base, + }) } /// Lexically validate path to prevent traversal attacks and symlink escapes @@ -196,24 +199,22 @@ impl FileSystemContext { // 2. Physical security check: verify symlinks don't escape base_path // We catch escapes by checking the longest existing prefix of the path. // This is robust even for new files (write_content). - if let Ok(canonical_base) = self.base_path.canonicalize() { - let mut current = final_path.as_path(); - while !current.exists() { - if let Some(parent) = current.parent() { - current = parent; - } else { - break; - } + let mut current = final_path.as_path(); + while !current.exists() { + if let Some(parent) = current.parent() { + current = parent; + } else { + break; } + } - if current.exists() - && let Ok(canonical_prefix) = current.canonicalize() - && !canonical_prefix.starts_with(&canonical_base) - { - return Err(ServiceError::execution_dynamic(format!( - "Path validation failed: {source} resolves outside base path via symlinks" - ))); - } + if current.exists() + && let Ok(canonical_prefix) = current.canonicalize() + && !canonical_prefix.starts_with(&self.base_path) + { + return Err(ServiceError::execution_dynamic(format!( + "Path validation failed: {source} resolves outside base path via symlinks" + ))); } Ok(final_path) @@ -310,7 +311,7 @@ mod tests { #[test] fn test_file_system_context_security() { let temp = std::env::temp_dir(); - let ctx = FileSystemContext::new(&temp); + let ctx = FileSystemContext::new(&temp).unwrap(); // Valid paths assert!(ctx.secure_path("test.txt").is_ok()); diff --git a/crates/thread/tests/integration.rs b/crates/thread/tests/integration.rs index 6648b57..48afbde 100644 --- a/crates/thread/tests/integration.rs +++ b/crates/thread/tests/integration.rs @@ -18,5 +18,5 @@ fn test_service_reexports_work() { use thread::services::FileSystemContext; // Just check if we can use the types - let _ctx = FileSystemContext::new("."); + let _ctx = FileSystemContext::new(".").unwrap(); }