From eccb45ced04d13c8dba8299526397b4eef775b5a Mon Sep 17 00:00:00 2001
From: Cameron Cordes <cameronc.dev@gmail.com>
Date: Sat, 17 Oct 2020 19:22:55 -0400
Subject: [PATCH] Use Absolutize for files that do not exist

Canonicalize relies on the file existing to resolve the potential
traversal, which won't work for file upload in case the file name has a
traversal inside it.
---
 Cargo.lock   | 19 +++++++++++++++++++
 Cargo.toml   |  1 +
 src/files.rs | 13 ++++++-------
 src/main.rs  | 12 +++---------
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index e05df4c..f19b113 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1133,6 +1133,7 @@ dependencies = [
  "image",
  "jsonwebtoken",
  "notify",
+ "path-absolutize",
  "rayon",
  "serde",
  "serde_json",
@@ -1596,6 +1597,24 @@ dependencies = [
  "winapi 0.3.9",
 ]
 
+[[package]]
+name = "path-absolutize"
+version = "3.0.6"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7a6ab2aaa5faefed84db46e4398eab15fa51325606462b5da8b0e230af3ac59a"
+dependencies = [
+ "path-dedot",
+]
+
+[[package]]
+name = "path-dedot"
+version = "3.0.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "658c6e985fce9c25289fe7c86c08a3cbe82c19a3cd5b3bc5945c8c632552e460"
+dependencies = [
+ "once_cell",
+]
+
 [[package]]
 name = "pem"
 version = "0.8.1"
diff --git a/Cargo.toml b/Cargo.toml
index 3298ec2..9bf8492 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -26,3 +26,4 @@ walkdir = "2"
 rayon = "1.3"
 notify = "4.0"
 tokio = "0.2"
+path-absolutize = "3.0.6"
diff --git a/src/files.rs b/src/files.rs
index 9ef7be4..cff8813 100644
--- a/src/files.rs
+++ b/src/files.rs
@@ -1,3 +1,4 @@
+use path_absolutize::*;
 use std::ffi::OsStr;
 use std::fs::read_dir;
 use std::io;
@@ -47,10 +48,10 @@ fn is_valid_full_path(base: &Path, path: &str) -> Option<PathBuf> {
         let mut full_path = PathBuf::from(base);
         full_path.push(&path);
         full_path
-            .canonicalize()
+            .absolutize()
             .and_then(|p| {
                 if p.starts_with(base) {
-                    Ok(p)
+                    Ok(p.into_owned())
                 } else {
                     Err(io::Error::new(
                         io::ErrorKind::Other,
@@ -59,9 +60,9 @@ fn is_valid_full_path(base: &Path, path: &str) -> Option<PathBuf> {
                 }
             })
             .ok()
-    } else if let Ok(path) = path.canonicalize().and_then(|path| {
+    } else if let Ok(path) = path.absolutize().and_then(|path| {
         if path.starts_with(base) {
-            Ok(path)
+            Ok(path.into_owned())
         } else {
             Err(io::Error::new(
                 io::ErrorKind::Other,
@@ -88,9 +89,7 @@ mod tests {
         assert_eq!(None, is_valid_path("fake/../../../"));
         assert_eq!(None, is_valid_path("../../../etc/passwd"));
         assert_eq!(None, is_valid_path("..//etc/passwd"));
-        assert_eq!(None, is_valid_path("..%2f..%2f/etc/passwd"));
-        assert_eq!(None, is_valid_path("%2e%2e%2f%2e%2e%2f/etc/passwd"));
-        assert_eq!(None, is_valid_path("\\\\attacker.com\\shared\\mal.php"));
+        assert_eq!(None, is_valid_path("../../etc/passwd"));
     }
 
     #[test]
diff --git a/src/main.rs b/src/main.rs
index 072c4b0..cadb131 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -153,13 +153,8 @@ async fn upload_image(_: Claims, mut payload: mp::Multipart) -> impl Responder {
 
     let path = file_path.unwrap_or_else(|| dotenv::var("BASE_PATH").unwrap());
     if !file_content.is_empty() {
-        let full_path = PathBuf::from(&path);
-
-        if let Some(mut full_path) = is_valid_path(full_path.to_str().unwrap_or("")) {
-            // TODO: Validate this file_name as is subject to path traversals which could lead to
-            // writing outside the base dir.
-            full_path = full_path.join(file_name.unwrap());
-
+        let full_path = PathBuf::from(&path).join(file_name.unwrap());
+        if let Some(full_path) = is_valid_path(full_path.to_str().unwrap_or("")) {
             if !full_path.is_file() {
                 let mut file = File::create(full_path).unwrap();
                 file.write_all(&file_content).unwrap();
@@ -206,8 +201,7 @@ async fn stream_video(
     // Extract video playlist dir to dotenv
     if !playlist.starts_with("tmp") || playlist.contains("..") {
         HttpResponse::NotFound().finish()
-    }
-    else if let Ok(file) = NamedFile::open(playlist) {
+    } else if let Ok(file) = NamedFile::open(playlist) {
         file.into_response(&request).unwrap()
     } else {
         HttpResponse::NotFound().finish()