Repository: alexcrichton/tar-rs (language: rust)

Issue:
unpack_in allows creating directories outside the `dst` directory
The following code will create the folders `exploit` and `exploit/foo` outside of the `demo` folder:
```rust
use std::{fs::File, io, io::Result};
use tar::{Archive, Builder, EntryType, Header};

fn main() -> Result<()> {
    let mut buf = Vec::new();

    {
        let mut builder = Builder::new(&mut buf);

        // symlink: parent -> ..
        let mut header = Header::new_gnu();
        header.set_path("symlink")?;
        header.set_link_name("..")?;
        header.set_entry_type(EntryType::Symlink);
        header.set_size(0);
        header.set_cksum();
        builder.append(&header, io::empty())?;

        // file: symlink/exploit/foo/bar
        let mut header = Header::new_gnu();
        header.set_path("symlink/exploit/foo/bar")?;
        header.set_size(0);
        header.set_cksum();
        builder.append(&header, io::empty())?;

        builder.finish()?;
    };

    Archive::new(&*buf).unpack("demo")
}
```

`Entry::unpack_in` calls `fs::create_dir_all` ([src/entry.rs:406](https://github.com/alexcrichton/tar-rs/blob/8d51b53/src/entry.rs#L406-L408)) on the untrusted path and therefore can create directories outside of the `dst` directory.

_Note:_ The provided code will fail with the expected error that `symlink/exploit/foo/bar` is outside of `demo`, but the parent directories are already created outside of the `demo` folder.

This is kind of related to #129, but still works in the current master.


Discussion / hints:
~I'm having trouble reproducing this with tar v0.4.35:~
```
Error: Custom { kind: Other, error: TarError { desc: "trying to unpack outside of destination path: /redacted/exploit/demo", io: Custom { kind: Other, error: "Invalid argument" } } }
```

~Can this issue be closed?~

**Correction**: The file isn't written but the folders are still created in the parent directory with the most recent version.
@alexcrichton we have a request to add a RUSTSEC advisory for this, FYI https://github.com/RustSec/advisory-db/pull/965
Thanks for the heads up!

Fix patch:
diff --git a/src/entry.rs b/src/entry.rs
index b5e35123..d97c95ec 100644
--- a/src/entry.rs
+++ b/src/entry.rs
@@ -413,11 +413,8 @@ impl<'a> EntryFields<'a> {
             None => return Ok(false),
         };
 
-        if parent.symlink_metadata().is_err() {
-            fs::create_dir_all(&parent).map_err(|e| {
-                TarError::new(&format!("failed to create `{}`", parent.display()), e)
-            })?;
-        }
+        self.ensure_dir_created(&dst, parent)
+            .map_err(|e| TarError::new(&format!("failed to create `{}`", parent.display()), e))?;
 
         let canon_target = self.validate_inside_dst(&dst, parent)?;
 
@@ -761,6 +758,26 @@ impl<'a> EntryFields<'a> {
         }
     }
 
+    fn ensure_dir_created(&self, dst: &Path, dir: &Path) -> io::Result<()> {
+        let mut ancestor = dir;
+        let mut dirs_to_create = Vec::new();
+        while ancestor.symlink_metadata().is_err() {
+            dirs_to_create.push(ancestor);
+            if let Some(parent) = ancestor.parent() {
+                ancestor = parent;
+            } else {
+                break;
+            }
+        }
+        for ancestor in dirs_to_create.into_iter().rev() {
+            if let Some(parent) = ancestor.parent() {
+                self.validate_inside_dst(dst, parent)?;
+            }
+            fs::create_dir(ancestor)?;
+        }
+        Ok(())
+    }
+
     fn validate_inside_dst(&self, dst: &Path, file_dst: &Path) -> io::Result<PathBuf> {
         // Abort if target (canonical) parent is outside of `dst`
         let canon_parent = file_dst.canonicalize().map_err(|err| {


Test patch:
diff --git a/tests/entry.rs b/tests/entry.rs
index 757fd7da..fa8eeaee 100644
--- a/tests/entry.rs
+++ b/tests/entry.rs
@@ -1,7 +1,7 @@
 extern crate tar;
 extern crate tempfile;
 
-use std::fs::File;
+use std::fs::{create_dir, File};
 use std::io::Read;
 
 use tempfile::Builder;
@@ -219,6 +219,36 @@ fn modify_link_just_created() {
     t!(File::open(td.path().join("foo/bar")));
 }
 
+#[test]
+#[cfg(not(windows))] // dangling symlinks have weird permissions
+fn modify_outside_with_relative_symlink() {
+    let mut ar = tar::Builder::new(Vec::new());
+
+    let mut header = tar::Header::new_gnu();
+    header.set_size(0);
+    header.set_entry_type(tar::EntryType::Symlink);
+    t!(header.set_path("symlink"));
+    t!(header.set_link_name(".."));
+    header.set_cksum();
+    t!(ar.append(&header, &[][..]));
+
+    let mut header = tar::Header::new_gnu();
+    header.set_size(0);
+    header.set_entry_type(tar::EntryType::Regular);
+    t!(header.set_path("symlink/foo/bar"));
+    header.set_cksum();
+    t!(ar.append(&header, &[][..]));
+
+    let bytes = t!(ar.into_inner());
+    let mut ar = tar::Archive::new(&bytes[..]);
+
+    let td = t!(Builder::new().prefix("tar").tempdir());
+    let tar_dir = td.path().join("tar");
+    create_dir(&tar_dir).unwrap();
+    assert!(ar.unpack(tar_dir).is_err());
+    assert!(!td.path().join("foo").exists());
+}
+
 #[test]
 fn parent_paths_error() {
     let mut ar = tar::Builder::new(Vec::new());
diff --git a/tests/header/mod.rs b/tests/header/mod.rs
index a85a1472..86692e33 100644
--- a/tests/header/mod.rs
+++ b/tests/header/mod.rs
@@ -154,6 +154,10 @@ fn set_path() {
     assert!(h.set_path(&medium2).is_err());
     assert!(h.set_path("\0").is_err());
 
+    assert!(h.set_path("..").is_err());
+    assert!(h.set_path("foo/..").is_err());
+    assert!(h.set_path("foo/../bar").is_err());
+
     h = Header::new_ustar();
     t!(h.set_path("foo"));
     assert_eq!(t!(h.path()).to_str(), Some("foo"));