From 6b75adb7ebf6dfae54bce2d2dd6fb67ffe34a7f6 Mon Sep 17 00:00:00 2001 From: bitful-pannul Date: Thu, 29 Feb 2024 16:00:13 -0300 Subject: [PATCH 1/3] vfs: sanitize paths --- .../packages/app_store/app_store/src/lib.rs | 2 +- .../packages/app_store/app_store/src/types.rs | 2 +- kinode/src/vfs.rs | 54 +++++++++++++++---- 3 files changed, 46 insertions(+), 12 deletions(-) diff --git a/kinode/packages/app_store/app_store/src/lib.rs b/kinode/packages/app_store/app_store/src/lib.rs index 6965c1265..93837648c 100644 --- a/kinode/packages/app_store/app_store/src/lib.rs +++ b/kinode/packages/app_store/app_store/src/lib.rs @@ -574,7 +574,7 @@ pub fn handle_install( state: &mut State, package_id: &PackageId, ) -> anyhow::Result<()> { - let drive_path = format!("/{package_id}/pkg"); + let drive_path = format!("{package_id}/pkg"); let manifest = fetch_package_manifest(package_id)?; // always grant read/write to their drive, which we created for them let Some(read_cap) = get_capability( diff --git a/kinode/packages/app_store/app_store/src/types.rs b/kinode/packages/app_store/app_store/src/types.rs index 1d6adea24..c77f3d46a 100644 --- a/kinode/packages/app_store/app_store/src/types.rs +++ b/kinode/packages/app_store/app_store/src/types.rs @@ -319,7 +319,7 @@ impl State { } pub fn uninstall(&mut self, package_id: &PackageId) -> anyhow::Result<()> { - let drive_path = format!("/{package_id}/pkg"); + let drive_path = format!("{package_id}/pkg"); Request::new() .target(("our", "vfs", "distro", "sys")) .body(serde_json::to_vec(&vfs::VfsRequest { diff --git a/kinode/src/vfs.rs b/kinode/src/vfs.rs index 1c40ec533..6da5324b5 100644 --- a/kinode/src/vfs.rs +++ b/kinode/src/vfs.rs @@ -24,6 +24,8 @@ pub async fn vfs( panic!("failed creating vfs dir! {:?}", e); } + let vfs_path = fs::canonicalize(&vfs_path).await?; + let open_files: Arc>>> = Arc::new(DashMap::new()); let mut process_queues: HashMap>>> = @@ -91,7 +93,7 @@ async fn handle_request( send_to_loop: MessageSender, send_to_terminal: PrintSender, send_to_caps_oracle: CapMessageSender, - vfs_path: String, + vfs_path: PathBuf, ) -> Result<(), VfsError> { let KernelMessage { id, @@ -191,8 +193,8 @@ async fn handle_request( } // current prepend to filepaths needs to be: /package_id/drive/path - let (package_id, drive, rest) = parse_package_and_drive(&request.path).await?; - let drive = format!("/{}/{}", package_id, drive); + let (package_id, drive, rest) = parse_package_and_drive(&request.path, &vfs_path).await?; + let drive: String = format!("{}/{}", package_id, drive); let path = PathBuf::from(request.path.clone()); if km.source.process != *KERNEL_PROCESS_ID { @@ -209,7 +211,8 @@ async fn handle_request( .await?; } // real safe path that the vfs will use - let path = PathBuf::from(format!("{}{}/{}", vfs_path, drive, rest)); + let path = vfs_path.join(&drive).join(&rest); + let (body, bytes) = match request.action { VfsAction::CreateDrive => { // handled in check_caps. @@ -288,6 +291,7 @@ async fn handle_request( } VfsAction::Read => { let contents = fs::read(&path).await?; + ( serde_json::to_vec(&VfsResponse::Read).unwrap(), Some(contents), @@ -374,12 +378,12 @@ async fn handle_request( (serde_json::to_vec(&VfsResponse::Ok).unwrap(), None) } VfsAction::Rename { new_path } => { - let new_path = format!("{}/{}", vfs_path, new_path); + let new_path = vfs_path.join(new_path); fs::rename(path, new_path).await?; (serde_json::to_vec(&VfsResponse::Ok).unwrap(), None) } VfsAction::CopyFile { new_path } => { - let new_path = format!("{}/{}", vfs_path, new_path); + let new_path = vfs_path.join(new_path); fs::copy(path, new_path).await?; (serde_json::to_vec(&VfsResponse::Ok).unwrap(), None) } @@ -534,7 +538,35 @@ async fn handle_request( Ok(()) } -async fn parse_package_and_drive(path: &str) -> Result<(PackageId, String, String), VfsError> { +async fn parse_package_and_drive( + path: &str, + vfs_path: &PathBuf, +) -> Result<(PackageId, String, String), VfsError> { + // trim leading "/" for joining + let path = path.trim_start_matches('/'); + + // sanitize path. + let path = PathBuf::from(path); + + let full_path = vfs_path.join(&path); + + let canon_path = full_path.canonicalize()?; + + if !canon_path.starts_with(vfs_path) { + return Err(VfsError::BadRequest { + error: "input path tries to escape parent vfs directory.".into(), + })?; + } + + // extract original path. + let path = canon_path + .strip_prefix(vfs_path) + .map_err(|_| VfsError::BadRequest { + error: "input path tries to escape parent vfs directory".into(), + })? + .display() + .to_string(); + let mut parts: Vec<&str> = path.split('/').collect(); if parts[0].is_empty() { @@ -600,7 +632,7 @@ async fn check_caps( path: PathBuf, drive: String, package_id: PackageId, - vfs_dir_path: String, + vfs_dir: PathBuf, ) -> Result<(), VfsError> { let src_package_id = PackageId::new(source.process.package(), source.process.publisher()); @@ -721,7 +753,9 @@ async fn check_caps( return Ok(()); } - let (new_package_id, new_drive, _rest) = parse_package_and_drive(new_path).await?; + let (new_package_id, new_drive, _rest) = + parse_package_and_drive(new_path, &vfs_dir).await?; + let new_drive = format!("/{}/{}", new_package_id, new_drive); // if both new and old path are within the package_id path, ok if (src_package_id == package_id) && (src_package_id == new_package_id) { @@ -806,7 +840,7 @@ async fn check_caps( ) .await?; - let drive_path = format!("{}{}", vfs_dir_path, drive); + let drive_path = vfs_dir.join(&drive); fs::create_dir_all(drive_path).await?; Ok(()) } From 7eecda500348859319fdb8265507445d621e7db5 Mon Sep 17 00:00:00 2001 From: bitful-pannul Date: Thu, 29 Feb 2024 16:45:45 -0300 Subject: [PATCH 2/3] vfs: nit erroring --- kinode/src/vfs.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/kinode/src/vfs.rs b/kinode/src/vfs.rs index 6da5324b5..c103900aa 100644 --- a/kinode/src/vfs.rs +++ b/kinode/src/vfs.rs @@ -554,7 +554,11 @@ async fn parse_package_and_drive( if !canon_path.starts_with(vfs_path) { return Err(VfsError::BadRequest { - error: "input path tries to escape parent vfs directory.".into(), + error: format!( + "input path tries to escape parent vfs directory: {:?}", + path + ) + .into(), })?; } @@ -562,7 +566,11 @@ async fn parse_package_and_drive( let path = canon_path .strip_prefix(vfs_path) .map_err(|_| VfsError::BadRequest { - error: "input path tries to escape parent vfs directory".into(), + error: format!( + "input path tries to escape parent vfs directory: {:?}", + path + ) + .into(), })? .display() .to_string(); From 8290fe872f65f8d56ea73952c3c17f878381068b Mon Sep 17 00:00:00 2001 From: bitful-pannul Date: Thu, 29 Feb 2024 19:10:48 -0300 Subject: [PATCH 3/3] vfs: normalize instead of canonicalize --- .../packages/app_store/app_store/src/lib.rs | 2 +- .../packages/app_store/app_store/src/types.rs | 2 +- kinode/src/vfs.rs | 69 ++++++++++++++----- 3 files changed, 53 insertions(+), 20 deletions(-) diff --git a/kinode/packages/app_store/app_store/src/lib.rs b/kinode/packages/app_store/app_store/src/lib.rs index 93837648c..6965c1265 100644 --- a/kinode/packages/app_store/app_store/src/lib.rs +++ b/kinode/packages/app_store/app_store/src/lib.rs @@ -574,7 +574,7 @@ pub fn handle_install( state: &mut State, package_id: &PackageId, ) -> anyhow::Result<()> { - let drive_path = format!("{package_id}/pkg"); + let drive_path = format!("/{package_id}/pkg"); let manifest = fetch_package_manifest(package_id)?; // always grant read/write to their drive, which we created for them let Some(read_cap) = get_capability( diff --git a/kinode/packages/app_store/app_store/src/types.rs b/kinode/packages/app_store/app_store/src/types.rs index c77f3d46a..1d6adea24 100644 --- a/kinode/packages/app_store/app_store/src/types.rs +++ b/kinode/packages/app_store/app_store/src/types.rs @@ -319,7 +319,7 @@ impl State { } pub fn uninstall(&mut self, package_id: &PackageId) -> anyhow::Result<()> { - let drive_path = format!("{package_id}/pkg"); + let drive_path = format!("/{package_id}/pkg"); Request::new() .target(("our", "vfs", "distro", "sys")) .body(serde_json::to_vec(&vfs::VfsRequest { diff --git a/kinode/src/vfs.rs b/kinode/src/vfs.rs index c103900aa..6dba6cfb6 100644 --- a/kinode/src/vfs.rs +++ b/kinode/src/vfs.rs @@ -1,7 +1,7 @@ use dashmap::DashMap; use std::collections::{HashMap, VecDeque}; use std::io::prelude::*; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use std::sync::Arc; use tokio::fs; use tokio::fs::OpenOptions; @@ -23,7 +23,6 @@ pub async fn vfs( if let Err(e) = fs::create_dir_all(&vfs_path).await { panic!("failed creating vfs dir! {:?}", e); } - let vfs_path = fs::canonicalize(&vfs_path).await?; let open_files: Arc>>> = Arc::new(DashMap::new()); @@ -194,7 +193,7 @@ async fn handle_request( // current prepend to filepaths needs to be: /package_id/drive/path let (package_id, drive, rest) = parse_package_and_drive(&request.path, &vfs_path).await?; - let drive: String = format!("{}/{}", package_id, drive); + let drive = format!("/{}/{}", package_id, drive); let path = PathBuf::from(request.path.clone()); if km.source.process != *KERNEL_PROCESS_ID { @@ -211,7 +210,8 @@ async fn handle_request( .await?; } // real safe path that the vfs will use - let path = vfs_path.join(&drive).join(&rest); + let base_drive = join_paths_safely(&vfs_path, &drive); + let path = join_paths_safely(&base_drive, &rest); let (body, bytes) = match request.action { VfsAction::CreateDrive => { @@ -378,12 +378,12 @@ async fn handle_request( (serde_json::to_vec(&VfsResponse::Ok).unwrap(), None) } VfsAction::Rename { new_path } => { - let new_path = vfs_path.join(new_path); + let new_path = join_paths_safely(&vfs_path, &new_path); fs::rename(path, new_path).await?; (serde_json::to_vec(&VfsResponse::Ok).unwrap(), None) } VfsAction::CopyFile { new_path } => { - let new_path = vfs_path.join(new_path); + let new_path = join_paths_safely(&vfs_path, &new_path); fs::copy(path, new_path).await?; (serde_json::to_vec(&VfsResponse::Ok).unwrap(), None) } @@ -542,17 +542,11 @@ async fn parse_package_and_drive( path: &str, vfs_path: &PathBuf, ) -> Result<(PackageId, String, String), VfsError> { - // trim leading "/" for joining - let path = path.trim_start_matches('/'); - - // sanitize path. - let path = PathBuf::from(path); - - let full_path = vfs_path.join(&path); + let joined_path = join_paths_safely(&vfs_path, path); - let canon_path = full_path.canonicalize()?; - - if !canon_path.starts_with(vfs_path) { + // sanitize path.. + let normalized_path = normalize_path(&joined_path); + if !normalized_path.starts_with(vfs_path) { return Err(VfsError::BadRequest { error: format!( "input path tries to escape parent vfs directory: {:?}", @@ -563,7 +557,7 @@ async fn parse_package_and_drive( } // extract original path. - let path = canon_path + let path = normalized_path .strip_prefix(vfs_path) .map_err(|_| VfsError::BadRequest { error: format!( @@ -848,7 +842,7 @@ async fn check_caps( ) .await?; - let drive_path = vfs_dir.join(&drive); + let drive_path = join_paths_safely(&vfs_dir, &drive); fs::create_dir_all(drive_path).await?; Ok(()) } @@ -894,6 +888,45 @@ fn get_file_type(metadata: &std::fs::Metadata) -> FileType { } } +/// from rust/cargo/src/cargo/util/paths.rs +/// to avoid using std::fs::canonicalize, which fails on non-existent paths. +fn normalize_path(path: &Path) -> PathBuf { + let mut components = path.components().peekable(); + let mut ret = if let Some(c @ Component::Prefix(..)) = components.peek().cloned() { + components.next(); + PathBuf::from(c.as_os_str()) + } else { + PathBuf::new() + }; + + for component in components { + match component { + Component::Prefix(..) => unreachable!(), + Component::RootDir => { + ret.push(component.as_os_str()); + } + Component::CurDir => {} + Component::ParentDir => { + ret.pop(); + } + Component::Normal(c) => { + ret.push(c); + } + } + } + ret +} + +fn join_paths_safely(base: &PathBuf, extension: &str) -> PathBuf { + let extension_str = Path::new(extension) + .to_str() + .unwrap_or("") + .trim_start_matches('/'); + + let extension_path = Path::new(extension_str); + base.join(extension_path) +} + fn make_error_message( our_node: String, id: u64,