tar-rs: unpack_in can chmod arbitrary directories by following symlinks

@@ -93,9 +93,21 @@ impl<R: Read> Archive<R> {
     /// extracting each file in turn to the location specified by the entry's
     /// path name.
     ///
-    /// This operation is relatively sensitive in that it will not write files
-    /// outside of the path specified by `dst`. Files in the archive which have
-    /// a '..' in their path are skipped during the unpacking process.
+    /// # Security
+    ///
+    /// A best-effort is made to prevent writing files outside `dst` (paths
+    /// containing `..` are skipped, symlinks are validated). However, there
+    /// have been historical bugs in this area, and more may exist. For this
+    /// reason, when processing untrusted archives, stronger sandboxing is
+    /// encouraged: e.g. the [`cap-std`] crate and/or OS-level
+    /// containerization/virtualization.
+    ///
+    /// If `dst` does not exist, it is created. Unpacking into an existing
+    /// directory merges content. This function assumes `dst` is not
+    /// concurrently modified by untrusted processes. Protecting against
+    /// TOCTOU races is out of scope for this crate.
+    ///
+    /// [`cap-std`]: https://docs.rs/cap-std/
     ///
     /// # Examples
     ///

@@ -212,8 +212,9 @@ impl<'a, R: Read> Entry<'a, R> {
     /// also be propagated to the path `dst`. Any existing file at the location
     /// `dst` will be overwritten.
     ///
-    /// This function carefully avoids writing outside of `dst`. If the file has
-    /// a '..' in its path, this function will skip it and return false.
+    /// # Security
+    ///
+    /// See [`Archive::unpack`].
     ///
     /// # Examples
     ///
@@ -446,7 +447,7 @@ impl<'a> EntryFields<'a> {
         // If the directory already exists just let it slide
         fs::create_dir(dst).or_else(|err| {
             if err.kind() == ErrorKind::AlreadyExists {
-                let prev = fs::metadata(dst);
+                let prev = fs::symlink_metadata(dst);
                 if prev.map(|m| m.is_dir()).unwrap_or(false) {
                     return Ok(());
                 }

@@ -408,3 +408,59 @@ fn modify_symlink_just_created() {
         .unwrap();
     assert_eq!(contents.len(), 0);
 }
+
+/// Test that unpacking a tarball with a symlink followed by a directory entry
+/// with the same name does not allow modifying permissions of arbitrary directories
+/// outside the extraction path.
+#[test]
+#[cfg(unix)]
+fn symlink_dir_collision_does_not_modify_external_dir_permissions() {
+    use ::std::fs;
+    use ::std::os::unix::fs::PermissionsExt;
+
+    let td = Builder::new().prefix("tar").tempdir().unwrap();
+
+    let target_dir = td.path().join("target-dir");
+    fs::create_dir(&target_dir).unwrap();
+    fs::set_permissions(&target_dir, fs::Permissions::from_mode(0o700)).unwrap();
+    let before_mode = fs::metadata(&target_dir).unwrap().permissions().mode() & 0o7777;
+    assert_eq!(before_mode, 0o700);
+
+    let extract_dir = td.path().join("extract-dir");
+    fs::create_dir(&extract_dir).unwrap();
+
+    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);
+    header.set_path("foo").unwrap();
+    header.set_link_name(&target_dir).unwrap();
+    header.set_mode(0o777);
+    header.set_cksum();
+    ar.append(&header, &[][..]).unwrap();
+
+    let mut header = tar::Header::new_gnu();
+    header.set_size(0);
+    header.set_entry_type(tar::EntryType::Directory);
+    header.set_path("foo").unwrap();
+    header.set_mode(0o777);
+    header.set_cksum();
+    ar.append(&header, &[][..]).unwrap();
+
+    let bytes = ar.into_inner().unwrap();
+    let mut ar = tar::Archive::new(&bytes[..]);
+
+    let result = ar.unpack(&extract_dir);
+    assert!(result.is_err());
+
+    let symlink_path = extract_dir.join("foo");
+    assert!(symlink_path
+        .symlink_metadata()
+        .unwrap()
+        .file_type()
+        .is_symlink());
+
+    let after_mode = fs::metadata(&target_dir).unwrap().permissions().mode() & 0o7777;
+    assert_eq!(after_mode, 0o700);
+}