Enable opt-in to deny creation of symlinks outside target directory

Author: charliermarsh

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## 0.5.2
+
+* Enable opt-in to deny creation of symlinks outside target directory by @charliermarsh in https://github.com/astral-sh/tokio-tar/pull/46
+
 ## 0.5.1
 
 * Add test to reproduce issue in `impl Stream for Entries` causing filename truncation by @charliermarsh in https://github.com/astral-sh/tokio-tar/pull/41

README.md

@@ -19,6 +19,9 @@ the following modifications:
   In [`alexcrichton/tar-rs`](https://github.com/alexcrichton/tar-rs), setting `preserve_permissions`
   to `false` will still set read, write, and execute permissions on extracted files, but will avoid
   setting extended permissions (e.g., `setuid`, `setgid`, and `sticky` bits).
+- Setting `allow_external_symlinks` to `false` will avoid extracting symlinks that point outside the
+  unpack target. Operations that _write_ outside the unpack directory are _always_ denied; but by
+  default, symlinks that _read_ outside the unpack directory are allowed.
 
 See the [changelog](CHANGELOG.md) for a more detailed list of changes.
 

src/archive.rs

@@ -44,6 +44,7 @@ pub struct ArchiveInner<R> {
     unpack_xattrs: bool,
     preserve_permissions: bool,
     preserve_mtime: bool,
+    allow_external_symlinks: bool,
     overwrite: bool,
     ignore_zeros: bool,
     obj: Mutex<R>,
@@ -55,6 +56,7 @@ pub struct ArchiveBuilder<R: Read + Unpin> {
     unpack_xattrs: bool,
     preserve_permissions: bool,
     preserve_mtime: bool,
+    allow_external_symlinks: bool,
     overwrite: bool,
     ignore_zeros: bool,
 }
@@ -66,6 +68,7 @@ impl<R: Read + Unpin> ArchiveBuilder<R> {
             unpack_xattrs: false,
             preserve_permissions: false,
             preserve_mtime: true,
+            allow_external_symlinks: true,
             overwrite: true,
             ignore_zeros: false,
             obj,
@@ -118,12 +121,23 @@ impl<R: Read + Unpin> ArchiveBuilder<R> {
         self
     }
 
+    /// Indicate whether to deny symlinks that point outside the destination
+    /// directory when unpacking this entry. (Writing to locations outside the
+    /// destination directory is _always_ forbidden.)
+    ///
+    /// This flag is enabled by default.
+    pub fn set_allow_external_symlinks(mut self, allow_external_symlinks: bool) -> Self {
+        self.allow_external_symlinks = allow_external_symlinks;
+        self
+    }
+
     /// Construct the archive, ready to accept inputs.
     pub fn build(self) -> Archive<R> {
         let Self {
             unpack_xattrs,
             preserve_permissions,
             preserve_mtime,
+            allow_external_symlinks,
             overwrite,
             ignore_zeros,
             obj,
@@ -134,6 +148,7 @@ impl<R: Read + Unpin> ArchiveBuilder<R> {
                 unpack_xattrs,
                 preserve_permissions,
                 preserve_mtime,
+                allow_external_symlinks,
                 overwrite,
                 ignore_zeros,
                 obj: Mutex::new(obj),
@@ -151,6 +166,7 @@ impl<R: Read + Unpin> Archive<R> {
                 unpack_xattrs: false,
                 preserve_permissions: false,
                 preserve_mtime: true,
+                allow_external_symlinks: true,
                 overwrite: true,
                 ignore_zeros: false,
                 obj: Mutex::new(obj),
@@ -565,6 +581,7 @@ fn poll_next_raw<R: Read + Unpin>(
         preserve_permissions: archive.inner.preserve_permissions,
         preserve_mtime: archive.inner.preserve_mtime,
         overwrite: archive.inner.overwrite,
+        allow_external_symlinks: archive.inner.allow_external_symlinks,
         read_state: None,
     };
 

src/entry.rs

@@ -1,3 +1,4 @@
+use crate::fs::{normalize_absolute, normalize_relative};
 use crate::{
     error::TarError, header::bytes2path, other, pax::pax_extensions, Archive, Header, PaxExtensions,
 };
@@ -54,6 +55,7 @@ pub struct EntryFields<R: Read + Unpin> {
     pub preserve_permissions: bool,
     pub preserve_mtime: bool,
     pub overwrite: bool,
+    pub allow_external_symlinks: bool,
     pub(crate) read_state: Option<EntryIo<R>>,
 }
 
@@ -71,6 +73,8 @@ impl<R: Read + Unpin> fmt::Debug for EntryFields<R> {
             .field("unpack_xattrs", &self.unpack_xattrs)
             .field("preserve_permissions", &self.preserve_permissions)
             .field("preserve_mtime", &self.preserve_mtime)
+            .field("overwrite", &self.overwrite)
+            .field("allow_external_symlinks", &self.allow_external_symlinks)
             .field("read_state", &self.read_state)
             .finish()
     }
@@ -321,6 +325,15 @@ impl<R: Read + Unpin> Entry<R> {
     pub fn set_preserve_mtime(&mut self, preserve: bool) {
         self.fields.preserve_mtime = preserve;
     }
+
+    /// Indicate whether to deny symlinks that point outside the destination
+    /// directory when unpacking this entry. (Writing to locations outside the
+    /// destination directory is _always_ forbidden.)
+    ///
+    /// This flag is enabled by default.
+    pub fn set_allow_external_symlinks(&mut self, allow_external_symlinks: bool) {
+        self.fields.allow_external_symlinks = allow_external_symlinks;
+    }
 }
 
 impl<R: Read + Unpin> Read for Entry<R> {
@@ -571,17 +584,14 @@ impl<R: Read + Unpin> EntryFields<R> {
             let src = match self.link_name()? {
                 Some(name) => name,
                 None => {
-                    return Err(other(&format!(
-                        "hard link listed for {} but no link name found",
-                        String::from_utf8_lossy(self.header.as_bytes())
-                    )));
+                    return Err(other("hard link listed but no link name found"));
                 }
             };
 
             if src.iter().count() == 0 {
                 return Err(other(&format!(
                     "symlink destination for {} is empty",
-                    String::from_utf8_lossy(self.header.as_bytes())
+                    src.display()
                 )));
             }
 
@@ -617,6 +627,26 @@ impl<R: Read + Unpin> EntryFields<R> {
                     )
                 })?;
             } else {
+                if !self.allow_external_symlinks {
+                    if let Some(target_base) = target_base {
+                        // Determine the normalized, absolute destination of the symlink.
+                        let link_dst = if src.is_absolute() {
+                            normalize_absolute(src.as_ref())
+                        } else {
+                            dst.parent()
+                                .and_then(|parent| normalize_relative(parent, src.as_ref()))
+                        };
+
+                        // Verify that the symlink destination is inside the target directory.
+                        if !link_dst.is_some_and(|link_dst| link_dst.starts_with(target_base)) {
+                            return Err(other(&format!(
+                                "symlink destination for {} is outside of the target directory",
+                                src.display()
+                            )));
+                        }
+                    }
+                }
+
                 match symlink(&src, dst).await {
                     Ok(()) => Ok(()),
                     Err(err) => {

src/fs.rs

@@ -0,0 +1,151 @@
+use std::path::{Component, Path, PathBuf};
+
+/// Normalize an absolute path.
+pub(crate) fn normalize_absolute(p: &Path) -> Option<PathBuf> {
+    debug_assert!(p.is_absolute(), "Target must be an absolute path");
+
+    let mut resolved = PathBuf::new();
+    for component in p.components() {
+        match component {
+            Component::Prefix(prefix) => {
+                // Windows-specific: preserve drive letter and prefix.
+                resolved.push(prefix.as_os_str());
+            }
+            Component::RootDir => {
+                // Append root directory after prefix.
+                resolved.push(component.as_os_str());
+            }
+            Component::CurDir => {
+                // Ignore `.`
+            }
+            Component::ParentDir => {
+                if !resolved.pop() {
+                    return None;
+                }
+            }
+            Component::Normal(segment) => {
+                resolved.push(segment);
+            }
+        }
+    }
+    Some(resolved)
+}
+
+/// Normalize a path relative to a destination directory.
+pub(crate) fn normalize_relative(dst: &Path, p: &Path) -> Option<PathBuf> {
+    debug_assert!(!p.is_absolute(), "Target must be a relative path");
+    debug_assert!(dst.is_absolute(), "Destination must be an absolute path");
+
+    let mut resolved = dst.to_path_buf();
+    for component in p.components() {
+        match component {
+            Component::RootDir | Component::Prefix(_) => {
+                // E.g., `/usr` on Windows.
+                return None;
+            }
+            Component::CurDir => {
+                // Ignore `.`
+            }
+            Component::ParentDir => {
+                if !resolved.pop() {
+                    return None;
+                }
+            }
+            Component::Normal(segment) => {
+                resolved.push(segment);
+            }
+        }
+    }
+    Some(resolved)
+}
+
+#[cfg(test)]
+mod tests {
+    use std::path::{Path, PathBuf};
+
+    #[test]
+    #[cfg(unix)]
+    fn test_normalize_absolute() {
+        // Basic absolute path.
+        assert_eq!(
+            crate::fs::normalize_absolute(Path::new("/a/b/c")),
+            Some(PathBuf::from("/a/b/c"))
+        );
+
+        // Path with `..` (should remove `b`).
+        assert_eq!(
+            crate::fs::normalize_absolute(Path::new("/a/b/../c")),
+            Some(PathBuf::from("/a/c"))
+        );
+
+        // Path with `.`, should be ignored.
+        assert_eq!(
+            crate::fs::normalize_absolute(Path::new("/a/./b")),
+            Some(PathBuf::from("/a/b"))
+        );
+
+        // Excessive `..` that escapes root.
+        assert_eq!(crate::fs::normalize_absolute(Path::new("/../b")), None);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_normalize_absolute() {
+        // Basic absolute path.
+        assert_eq!(
+            crate::fs::normalize_absolute(Path::new(r"C:\a\b\c")),
+            Some(PathBuf::from(r"C:\a\b\c"))
+        );
+
+        // Path with `..` (should remove `b`).
+        assert_eq!(
+            crate::fs::normalize_absolute(Path::new(r"C:\a\b\..\c")),
+            Some(PathBuf::from(r"C:\a\c"))
+        );
+
+        // Path with `.`, should be ignored.
+        assert_eq!(
+            crate::fs::normalize_absolute(Path::new(r"C:\a\.\b")),
+            Some(PathBuf::from(r"C:\a\b"))
+        );
+
+        // Excessive `..` that escapes root.
+        assert_eq!(crate::fs::normalize_absolute(Path::new(r"C:\..\b")), None);
+    }
+
+    #[test]
+    #[cfg(unix)]
+    fn test_normalize_relative() {
+        let dst = Path::new("/home/user/dst");
+
+        // Basic relative path.
+        assert_eq!(
+            crate::fs::normalize_relative(dst, Path::new("a/b/c")),
+            Some(PathBuf::from("/home/user/dst/a/b/c"))
+        );
+
+        // Path with `..`, should remove `b`.
+        assert_eq!(
+            crate::fs::normalize_relative(dst, Path::new("a/b/../c")),
+            Some(PathBuf::from("/home/user/dst/a/c"))
+        );
+
+        // Path with `.` should be ignored.
+        assert_eq!(
+            crate::fs::normalize_relative(dst, Path::new("./a/b")),
+            Some(PathBuf::from("/home/user/dst/a/b"))
+        );
+
+        // Path escaping `dst`, should return None.
+        assert_eq!(
+            crate::fs::normalize_relative(dst, Path::new("../../../../outside")),
+            None
+        );
+
+        // Excessive `..`, escaping `dst`.
+        assert_eq!(
+            crate::fs::normalize_relative(dst, Path::new("../../../../")),
+            None
+        );
+    }
+}

src/lib.rs

@@ -37,6 +37,7 @@ mod builder;
 mod entry;
 mod entry_type;
 mod error;
+mod fs;
 mod header;
 mod pax;