From 8bcd837038566276be16e08bb7c7d6f47aa75e9f Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Mon, 3 Apr 2023 08:41:03 -0400 Subject: [PATCH] Fix file upload Add a flag for new files so we can skip the exists check when seeing if the new file is within the base directory. --- src/files.rs | 28 +++++++++++++++------------- src/main.rs | 9 +++++---- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/files.rs b/src/files.rs index 977d45f..4c6b76c 100644 --- a/src/files.rs +++ b/src/files.rs @@ -137,6 +137,7 @@ pub fn is_image_or_video(path: &Path) -> bool { pub fn is_valid_full_path + Debug + AsRef>( base: &P, path: &P, + new_file: bool, ) -> Option { debug!("Base: {:?}. Path: {:?}", base, path); @@ -150,7 +151,7 @@ pub fn is_valid_full_path + Debug + AsRef>( path }; - match is_path_above_base_dir(base, &mut path) { + match is_path_above_base_dir(base, &mut path, new_file) { Ok(path) => Some(path), Err(e) => { error!("{}", e); @@ -162,6 +163,7 @@ pub fn is_valid_full_path + Debug + AsRef>( fn is_path_above_base_dir + Debug>( base: P, full_path: &mut PathBuf, + new_file: bool, ) -> anyhow::Result { full_path .absolutize() @@ -169,7 +171,7 @@ fn is_path_above_base_dir + Debug>( .map_or_else( |e| Err(anyhow!(e)), |p| { - if p.starts_with(base) && p.exists() { + if p.starts_with(base) && (new_file || p.exists()) { Ok(p.into_owned()) } else if !p.exists() { Err(anyhow!("Path does not exist: {:?}", p)) @@ -196,7 +198,7 @@ impl RealFileSystem { impl FileSystemAccess for RealFileSystem { fn get_files_for_path(&self, path: &str) -> anyhow::Result> { - is_valid_full_path(&PathBuf::from(&self.base_path), &PathBuf::from(path)) + is_valid_full_path(&PathBuf::from(&self.base_path), &PathBuf::from(path), false) .map(|path| { debug!("Valid path: {:?}", path); list_files(&path).unwrap_or_default() @@ -457,23 +459,23 @@ mod tests { #[test] fn directory_traversal_test() { let base = env::temp_dir(); - assert_eq!(None, is_valid_full_path(&base, &PathBuf::from("../"))); - assert_eq!(None, is_valid_full_path(&base, &PathBuf::from(".."))); + assert_eq!(None, is_valid_full_path(&base, &PathBuf::from("../"), false)); + assert_eq!(None, is_valid_full_path(&base, &PathBuf::from(".."), false)); assert_eq!( None, - is_valid_full_path(&base, &PathBuf::from("fake/../../../")) + is_valid_full_path(&base, &PathBuf::from("fake/../../../"), false) ); assert_eq!( None, - is_valid_full_path(&base, &PathBuf::from("../../../etc/passwd")) + is_valid_full_path(&base, &PathBuf::from("../../../etc/passwd"), false) ); assert_eq!( None, - is_valid_full_path(&base, &PathBuf::from("..//etc/passwd")) + is_valid_full_path(&base, &PathBuf::from("..//etc/passwd"), false) ); assert_eq!( None, - is_valid_full_path(&base, &PathBuf::from("../../etc/passwd")) + is_valid_full_path(&base, &PathBuf::from("../../etc/passwd"), false) ); } @@ -484,7 +486,7 @@ mod tests { test_file.push("test.png"); File::create(test_file).unwrap(); - assert!(is_valid_full_path(&base, &PathBuf::from("test.png")).is_some()); + assert!(is_valid_full_path(&base, &PathBuf::from("test.png"), false).is_some()); } #[test] @@ -495,7 +497,7 @@ mod tests { let mut test_file = PathBuf::from(&base); test_file.push(path); - assert_eq!(None, is_valid_full_path(&base, &test_file)); + assert_eq!(None, is_valid_full_path(&base, &test_file, false)); } #[test] @@ -505,11 +507,11 @@ mod tests { test_file.push("test.png"); File::create(&test_file).unwrap(); - assert!(is_valid_full_path(&base, &test_file).is_some()); + assert!(is_valid_full_path(&base, &test_file, false).is_some()); assert_eq!( Some(PathBuf::from("/tmp/test.png")), - is_valid_full_path(&base, &PathBuf::from("/tmp/test.png")) + is_valid_full_path(&base, &PathBuf::from("/tmp/test.png"), false) ); } diff --git a/src/main.rs b/src/main.rs index 877f9dc..95aa587 100644 --- a/src/main.rs +++ b/src/main.rs @@ -76,7 +76,7 @@ async fn get_image( req: web::Query, app_state: Data, ) -> impl Responder { - if let Some(path) = is_valid_full_path(&app_state.base_path, &req.path) { + if let Some(path) = is_valid_full_path(&app_state.base_path, &req.path, false) { if req.size.is_some() { let relative_path = path .strip_prefix(&app_state.base_path) @@ -108,7 +108,7 @@ async fn get_file_metadata( path: web::Query, app_state: Data, ) -> impl Responder { - match is_valid_full_path(&app_state.base_path, &path.path) + match is_valid_full_path(&app_state.base_path, &path.path, false) .ok_or_else(|| ErrorKind::InvalidData.into()) .and_then(File::open) .and_then(|file| file.metadata()) @@ -159,6 +159,7 @@ async fn upload_image( if let Some(full_path) = is_valid_full_path( &app_state.base_path, &full_path.to_str().unwrap().to_string(), + true, ) { if !full_path.is_file() && is_image_or_video(&full_path) { let mut file = File::create(full_path).unwrap(); @@ -188,7 +189,7 @@ async fn generate_video( if let Some(name) = filename.file_stem() { let filename = name.to_str().expect("Filename should convert to string"); let playlist = format!("tmp/{}.m3u8", filename); - if let Some(path) = is_valid_full_path(&app_state.base_path, &body.path) { + if let Some(path) = is_valid_full_path(&app_state.base_path, &body.path, false) { if let Ok(child) = create_playlist(path.to_str().unwrap(), &playlist).await { app_state .stream_manager @@ -216,7 +217,7 @@ async fn stream_video( debug!("Playlist: {}", playlist); // Extract video playlist dir to dotenv - if !playlist.starts_with("tmp") && is_valid_full_path(&app_state.base_path, playlist).is_some() + if !playlist.starts_with("tmp") && is_valid_full_path(&app_state.base_path, playlist, false).is_some() { HttpResponse::BadRequest().finish() } else if let Ok(file) = NamedFile::open(playlist) {