CVE-2018-20990

@@ -4,7 +4,7 @@ use std::fs;
 use std::io::prelude::*;
 use std::io::{self, Error, ErrorKind, SeekFrom};
 use std::marker;
-use std::path::{Component, Path};
+use std::path::{Component, Path, PathBuf};
 
 use filetime::{self, FileTime};
 
@@ -369,37 +369,14 @@ impl<'a> EntryFields<'a> {
             None    => return Ok(false),
         };
 
-        if !parent.exists() {
+        if parent.symlink_metadata().is_err() {
             fs::create_dir_all(&parent).map_err(|e| {
                 TarError::new(&format!("failed to create `{}`",
                                        parent.display()), e)
             })?;
         }
 
-        // Abort if target (canonical) parent is outside of `dst`
-        let canon_parent = parent.canonicalize().map_err(|err| {
-            Error::new(
-                err.kind(),
-                format!("{} while canonicalizing {}", err, parent.display()),
-            )
-        })?;
-        let canon_target = dst.canonicalize().map_err(|err| {
-            Error::new(
-                err.kind(),
-                format!("{} while canonicalizing {}", err, dst.display()),
-            )
-        })?;
-        if !canon_parent.starts_with(&canon_target) {
-            let err = TarError::new(
-                &format!(
-                    "trying to unpack outside of destination path: {}",
-                    canon_target.display()
-                ),
-                // TODO: use ErrorKind::InvalidInput here? (minor breaking change)
-                Error::new(ErrorKind::Other, "Invalid argument"),
-            );
-            return Err(err.into());
-        }
+        let canon_target = self.validate_inside_dst(&dst, parent)?;
 
         self.unpack(Some(&canon_target), &file_dst).map_err(|e| {
             TarError::new(&format!("failed to unpack `{}`", file_dst.display()), e)
@@ -441,8 +418,23 @@ impl<'a> EntryFields<'a> {
 
             return if kind.is_hard_link() {
                 let link_src = match target_base {
+                    // If we're unpacking within a directory then ensure that
+                    // the destination of this hard link is both present and
+                    // inside our own directory. This is needed because we want
+                    // to make sure to not overwrite anything outside the root.
+                    //
+                    // Note that this logic is only needed for hard links
+                    // currently. With symlinks the `validate_inside_dst` which
+                    // happens before this method as part of `unpack_in` will
+                    // use canonicalization to ensure this guarantee. For hard
+                    // links though they're canonicalized to their existing path
+                    // so we need to validate at this time.
+                    Some(ref p) => {
+                        let link_src = p.join(src);
+                        self.validate_inside_dst(p, &link_src)?;
+                        link_src
+                    }
                     None => src.into_owned(),
-                    Some(ref p) => p.join(src),
                 };
                 fs::hard_link(&link_src, dst).map_err(|err| Error::new(
                     err.kind(),
@@ -490,7 +482,16 @@ impl<'a> EntryFields<'a> {
         // As a result if we don't recognize the kind we just write out the file
         // as we would normally.
 
-        fs::File::create(dst).and_then(|mut f| {
+        // Remove an existing file, if any, to avoid writing through
+        // symlinks/hardlinks to weird locations. The tar archive says this is a
+        // regular file, so let's make it a regular file.
+        (|| -> io::Result<()> {
+            match fs::remove_file(dst) {
+                Ok(()) => {}
+                Err(ref e) if e.kind() == io::ErrorKind::NotFound => {}
+                Err(e) => return Err(e)
+            }
+            let mut f = fs::File::create(dst)?;
             for io in self.data.drain(..) {
                 match io {
                     EntryIo::Data(mut d) => {
@@ -508,7 +509,7 @@ impl<'a> EntryFields<'a> {
                 }
             }
             Ok(())
-        }).map_err(|e| {
+        })().map_err(|e| {
             let header = self.header.path_bytes();
             TarError::new(&format!("failed to unpack `{}` into `{}`",
                                    String::from_utf8_lossy(&header),
@@ -596,6 +597,34 @@ impl<'a> EntryFields<'a> {
             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| {
+            Error::new(
+                err.kind(),
+                format!("{} while canonicalizing {}", err, file_dst.display()),
+            )
+        })?;
+        let canon_target = dst.canonicalize().map_err(|err| {
+            Error::new(
+                err.kind(),
+                format!("{} while canonicalizing {}", err, dst.display()),
+            )
+        })?;
+        if !canon_parent.starts_with(&canon_target) {
+            let err = TarError::new(
+                &format!(
+                    "trying to unpack outside of destination path: {}",
+                    canon_target.display()
+                ),
+                // TODO: use ErrorKind::InvalidInput here? (minor breaking change)
+                Error::new(ErrorKind::Other, "Invalid argument"),
+            );
+            return Err(err.into());
+        }
+        Ok(canon_target)
+    }
 }
 
 impl<'a> Read for EntryFields<'a> {

@@ -2,6 +2,7 @@ extern crate tar;
 extern crate tempdir;
 
 use std::fs::File;
+use std::io::Read;
 
 use tempdir::TempDir;
 
@@ -247,3 +248,73 @@ fn good_parent_paths_ok() {
     let dst = t!(td.path().join("foo").join("bar").canonicalize());
     t!(File::open(dst));
 }
+
+#[test]
+fn modify_hard_link_just_created() {
+    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::Link);
+    t!(header.set_path("foo"));
+    t!(header.set_link_name("../test"));
+    header.set_cksum();
+    t!(ar.append(&header, &[][..]));
+
+    let mut header = tar::Header::new_gnu();
+    header.set_size(1);
+    header.set_entry_type(tar::EntryType::Regular);
+    t!(header.set_path("foo"));
+    header.set_cksum();
+    t!(ar.append(&header, &b"x"[..]));
+
+    let bytes = t!(ar.into_inner());
+    let mut ar = tar::Archive::new(&bytes[..]);
+
+    let td = t!(TempDir::new("tar"));
+
+    let test = td.path().join("test");
+    t!(File::create(&test));
+
+    let dir = td.path().join("dir");
+    assert!(ar.unpack(&dir).is_err());
+
+    let mut contents = Vec::new();
+    t!(t!(File::open(&test)).read_to_end(&mut contents));
+    assert_eq!(contents.len(), 0);
+}
+
+#[test]
+fn modify_symlink_just_created() {
+    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("foo"));
+    t!(header.set_link_name("../test"));
+    header.set_cksum();
+    t!(ar.append(&header, &[][..]));
+
+    let mut header = tar::Header::new_gnu();
+    header.set_size(1);
+    header.set_entry_type(tar::EntryType::Regular);
+    t!(header.set_path("foo"));
+    header.set_cksum();
+    t!(ar.append(&header, &b"x"[..]));
+
+    let bytes = t!(ar.into_inner());
+    let mut ar = tar::Archive::new(&bytes[..]);
+
+    let td = t!(TempDir::new("tar"));
+
+    let test = td.path().join("test");
+    t!(File::create(&test));
+
+    let dir = td.path().join("dir");
+    t!(ar.unpack(&dir));
+
+    let mut contents = Vec::new();
+    t!(t!(File::open(&test)).read_to_end(&mut contents));
+    assert_eq!(contents.len(), 0);
+}