Merge pull request tonyantony300#113 from Retengart/security-fixes

fix(send): skip invalid path components instead of failing share

Author: tonyantony300

sendme/src/core/receive.rs

@@ -299,13 +299,86 @@ fn get_export_path(root: &Path, name: &str) -> anyhow::Result<PathBuf> {
 }
 
 fn validate_path_component(component: &str) -> anyhow::Result<()> {
-    anyhow::ensure!(
-        !component.contains('/'),
-        "path components must not contain the only correct path separator, /"
-    );
+    anyhow::ensure!(!component.is_empty(), "empty path component");
+    anyhow::ensure!(!component.contains('/'), "contains /");
+    anyhow::ensure!(!component.contains('\\'), "contains \\");
+    anyhow::ensure!(!component.contains(':'), "contains colon");
+    anyhow::ensure!(component != "..", "parent directory traversal");
+    anyhow::ensure!(component != ".", "current directory reference");
+    anyhow::ensure!(!component.contains('\0'), "contains null byte");
     Ok(())
 }
 
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn validate_rejects_empty() {
+        assert!(validate_path_component("").is_err());
+    }
+
+    #[test]
+    fn validate_rejects_slash() {
+        assert!(validate_path_component("a/b").is_err());
+    }
+
+    #[test]
+    fn validate_rejects_backslash() {
+        assert!(validate_path_component("a\\b").is_err());
+    }
+
+    #[test]
+    fn validate_rejects_parent_traversal() {
+        assert!(validate_path_component("..").is_err());
+    }
+
+    #[test]
+    fn validate_rejects_dot() {
+        assert!(validate_path_component(".").is_err());
+    }
+
+    #[test]
+    fn validate_rejects_null_byte() {
+        assert!(validate_path_component("a\0b").is_err());
+    }
+
+    #[test]
+    fn validate_rejects_colon() {
+        assert!(validate_path_component("C:foo").is_err());
+    }
+
+    #[test]
+    fn validate_accepts_normal() {
+        assert!(validate_path_component("file.txt").is_ok());
+        assert!(validate_path_component("my-file_v2.tar.gz").is_ok());
+    }
+
+    #[test]
+    fn get_export_path_blocks_drive_prefix() {
+        let root = Path::new("/tmp/test");
+        assert!(get_export_path(root, "C:foo").is_err());
+    }
+
+    #[test]
+    fn get_export_path_blocks_traversal() {
+        let root = Path::new("/tmp/test");
+        assert!(get_export_path(root, "../etc/passwd").is_err());
+        assert!(get_export_path(root, "subdir/../../etc/passwd").is_err());
+    }
+
+    #[test]
+    fn get_export_path_blocks_backslash() {
+        assert!(get_export_path(Path::new("/tmp/test"), "file\\name").is_err());
+    }
+
+    #[test]
+    fn get_export_path_allows_normal() {
+        let p = get_export_path(Path::new("/tmp/test"), "subdir/file.txt").unwrap();
+        assert_eq!(p, PathBuf::from("/tmp/test/subdir/file.txt"));
+    }
+}
+
 fn show_get_error(e: GetError) -> GetError {
     match &e {
         GetError::InitialNext { source, .. } => {

sendme/src/core/send.rs

@@ -196,18 +196,36 @@ async fn import(path: PathBuf, db: &Store) -> anyhow::Result<(TempTag, u64, Coll
     let root = path.parent().context("context get parent")?;
     let files = WalkDir::new(path.clone()).into_iter();
     let data_sources: Vec<(String, PathBuf)> = files
-        .map(|entry| {
-            let entry = entry?;
+        .filter_map(|entry| {
+            let entry = match entry {
+                Ok(e) => e,
+                Err(e) => {
+                    tracing::warn!("skipping inaccessible entry: {}", e);
+                    return None;
+                }
+            };
             if !entry.file_type().is_file() {
-                return Ok(None);
+                return None;
             }
             let path = entry.into_path();
-            let relative = path.strip_prefix(root)?;
-            let name = canonicalized_path_to_string(relative, true)?;
-            anyhow::Ok(Some((name, path)))
+            let relative = match path.strip_prefix(root) {
+                Ok(r) => r,
+                Err(e) => {
+                    tracing::warn!("skipping {}: {}", path.display(), e);
+                    return None;
+                }
+            };
+            match canonicalized_path_to_string(relative, true) {
+                Ok(name) => Some((name, path)),
+                Err(e) => {
+                    tracing::warn!("skipping {}: {}", path.display(), e);
+                    None
+                }
+            }
         })
-        .filter_map(Result::transpose)
-        .collect::<anyhow::Result<Vec<_>>>()?;
+        .collect();
+
+    anyhow::ensure!(!data_sources.is_empty(), "no valid files to share");
 
     let mut names_and_tags = n0_future::stream::iter(data_sources)
         .map(|(name, path)| {
@@ -298,6 +316,65 @@ pub fn canonicalized_path_to_string(
     Ok(path_str)
 }
 
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::path::Path;
+
+    #[cfg(unix)]
+    #[test]
+    fn canonicalized_path_rejects_backslash() {
+        let path = Path::new("system-systemd\\x2dcryptsetup.slice");
+        assert!(canonicalized_path_to_string(path, true).is_err());
+    }
+
+    #[test]
+    fn canonicalized_path_accepts_normal() {
+        let result = canonicalized_path_to_string(Path::new("subdir/file.txt"), true);
+        assert_eq!(result.unwrap(), "subdir/file.txt");
+    }
+
+    #[test]
+    fn canonicalized_path_rejects_parent_traversal() {
+        assert!(canonicalized_path_to_string(Path::new("../etc/passwd"), true).is_err());
+    }
+
+    #[test]
+    fn canonicalized_path_rejects_absolute_when_relative() {
+        assert!(canonicalized_path_to_string(Path::new("/etc/passwd"), true).is_err());
+    }
+
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn import_skips_invalid_files() {
+        use tempfile::TempDir;
+
+        let td = TempDir::new().unwrap();
+        let dir = td.path().join("testdir");
+        std::fs::create_dir_all(&dir).unwrap();
+        std::fs::write(dir.join("good.txt"), "hello").unwrap();
+        std::fs::write(dir.join(format!("bad{}file.txt", '\\')), "bad").unwrap();
+
+        let path = dir.canonicalize().unwrap();
+        let root = path.parent().unwrap();
+        let data_sources: Vec<(String, PathBuf)> = WalkDir::new(path.clone())
+            .into_iter()
+            .filter_map(|entry| {
+                let entry = entry.ok()?;
+                if !entry.file_type().is_file() {
+                    return None;
+                }
+                let path = entry.into_path();
+                let relative = path.strip_prefix(root).ok()?;
+                canonicalized_path_to_string(relative, true).ok().map(|name| (name, path))
+            })
+            .collect();
+
+        assert_eq!(data_sources.len(), 1, "should skip file with backslash");
+        assert!(data_sources[0].0.contains("good.txt"));
+    }
+}
+
 async fn show_provide_progress_with_logging(
     mut recv: mpsc::Receiver<iroh_blobs::provider::events::ProviderMessage>,
     app_handle: AppHandle,

sendme/src/main_reference.rs

@@ -295,10 +295,13 @@ fn get_or_create_secret(print: bool) -> anyhow::Result<SecretKey> {
 }
 
 fn validate_path_component(component: &str) -> anyhow::Result<()> {
-    anyhow::ensure!(
-        !component.contains('/'),
-        "path components must not contain the only correct path separator, /"
-    );
+    anyhow::ensure!(!component.is_empty(), "empty path component");
+    anyhow::ensure!(!component.contains('/'), "contains /");
+    anyhow::ensure!(!component.contains('\\'), "contains \\");
+    anyhow::ensure!(!component.contains(':'), "contains colon");
+    anyhow::ensure!(component != "..", "parent directory traversal");
+    anyhow::ensure!(component != ".", "current directory reference");
+    anyhow::ensure!(!component.contains('\0'), "contains null byte");
     Ok(())
 }
 

src-tauri/capabilities/default.json

@@ -14,9 +14,7 @@
 		"os:default",
 		"opener:default",
 		"dialog:default",
-		"shell:default",
 		"shell:allow-spawn",
-		"shell:allow-open",
 		"updater:default"
 	]
 }

src-tauri/capabilities/sendme.json

@@ -14,20 +14,14 @@
 		"dialog:default",
 		"dialog:allow-open",
 		"dialog:allow-save",
-		"shell:default",
 		"shell:allow-spawn",
-		"shell:allow-open",
 		{
 			"identifier": "http:default",
 			"allow": [
 				{
-					"url": "https://*:*"
-				},
-				{
-					"url": "http://*:*"
+					"url": "https://github.com/**"
 				}
-			],
-			"deny": []
+			]
 		}
 	]
 }