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

Author: charliermarsh

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> {
@@ -617,6 +630,25 @@ 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 {
+                            normalize_relative(dst.parent().unwrap(), 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",
+                                String::from_utf8_lossy(self.header.as_bytes())
+                            )));
+                        }
+                    }
+                }
+
                 match symlink(&src, dst).await {
                     Ok(()) => Ok(()),
                     Err(err) => {

src/fs.rs

@@ -0,0 +1,150 @@
+use std::path::{Component, Path, PathBuf};
+
+/// Normalize an absolute path.
+pub(crate) fn normalize_absolute(p: &Path) -> Option<PathBuf> {
+    debug_assert!(p.is_absolute());
+
+    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());
+    debug_assert!(dst.is_absolute());
+
+    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};
+
+    use crate::fs::{normalize_absolute, normalize_relative};
+
+    #[test]
+    #[cfg(unix)]
+    fn test_normalize_absolute() {
+        // Basic absolute path.
+        assert_eq!(
+            normalize_absolute(Path::new("/a/b/c")),
+            Some(PathBuf::from("/a/b/c"))
+        );
+
+        // Path with `..` (should remove `b`).
+        assert_eq!(
+            normalize_absolute(Path::new("/a/b/../c")),
+            Some(PathBuf::from("/a/c"))
+        );
+
+        // Path with `.`, should be ignored.
+        assert_eq!(
+            normalize_absolute(Path::new("/a/./b")),
+            Some(PathBuf::from("/a/b"))
+        );
+
+        // Excessive `..` that escapes root.
+        assert_eq!(normalize_absolute(Path::new("/../b")), None);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_normalize_absolute() {
+        // Basic absolute path.
+        assert_eq!(
+            normalize_absolute(Path::new(r"C:\a\b\c")),
+            Some(PathBuf::from(r"C:\a\b\c"))
+        );
+
+        // Path with `..` (should remove `b`).
+        assert_eq!(
+            normalize_absolute(Path::new(r"C:\a\b\..\c")),
+            Some(PathBuf::from(r"C:\a\c"))
+        );
+
+        // Path with `.`, should be ignored.
+        assert_eq!(
+            normalize_absolute(Path::new(r"C:\a\.\b")),
+            Some(PathBuf::from(r"C:\a\b"))
+        );
+
+        // Excessive `..` that escapes root.
+        assert_eq!(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!(
+            normalize_relative(dst, Path::new("a/b/c")),
+            Some(PathBuf::from("/home/user/dst/a/b/c"))
+        );
+
+        // Path with `..`, should remove `b`.
+        assert_eq!(
+            normalize_relative(dst, Path::new("a/b/../c")),
+            Some(PathBuf::from("/home/user/dst/a/c"))
+        );
+
+        // Path with `.` should be ignored.
+        assert_eq!(
+            normalize_relative(dst, Path::new("./a/b")),
+            Some(PathBuf::from("/home/user/dst/a/b"))
+        );
+
+        // Path escaping `dst`, should return None.
+        assert_eq!(
+            normalize_relative(dst, Path::new("../../../../outside")),
+            None
+        );
+
+        // Excessive `..`, escaping `dst`.
+        assert_eq!(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;
 

tests/entry.rs

@@ -6,6 +6,7 @@ extern crate tempfile;
 use tokio::{fs, fs::File, io::AsyncReadExt};
 use tokio_stream::*;
 
+use async_tar::ArchiveBuilder;
 use tempfile::Builder;
 
 macro_rules! t {
@@ -388,3 +389,76 @@ async fn modify_symlink_just_created() {
     t!(t!(File::open(&test).await).read_to_end(&mut contents).await);
     assert_eq!(contents.len(), 0);
 }
+
+#[tokio::test]
+async fn deny_absolute_symlink() {
+    let mut ar = async_tar::Builder::new(Vec::new());
+
+    let mut header = async_tar::Header::new_gnu();
+    header.set_size(0);
+    header.set_entry_type(async_tar::EntryType::Symlink);
+    t!(header.set_path("foo"));
+    t!(header.set_link_name("/bar"));
+    header.set_cksum();
+    t!(ar.append(&header, &[][..]).await);
+
+    let bytes = t!(ar.into_inner().await);
+
+    let builder = ArchiveBuilder::new(&bytes[..]).set_allow_external_symlinks(false);
+    let mut ar = builder.build();
+
+    let td = t!(Builder::new().prefix("tar").tempdir());
+    assert!(ar.unpack(td.path()).await.is_err());
+}
+
+#[tokio::test]
+async fn deny_relative_link() {
+    let mut ar = async_tar::Builder::new(Vec::new());
+
+    let mut header = async_tar::Header::new_gnu();
+    header.set_size(0);
+    header.set_entry_type(async_tar::EntryType::Symlink);
+    t!(header.set_path("foo"));
+    t!(header.set_link_name("../../../../"));
+    header.set_cksum();
+    t!(ar.append(&header, &[][..]).await);
+
+    let bytes = t!(ar.into_inner().await);
+
+    let builder = ArchiveBuilder::new(&bytes[..]).set_allow_external_symlinks(false);
+    let mut ar = builder.build();
+
+    let td = t!(Builder::new().prefix("tar").tempdir());
+    assert!(ar.unpack(td.path()).await.is_err());
+}
+
+#[tokio::test]
+#[cfg(unix)]
+async fn accept_relative_link() {
+    let mut ar = async_tar::Builder::new(Vec::new());
+
+    let mut header = async_tar::Header::new_gnu();
+    header.set_size(0);
+    header.set_entry_type(async_tar::EntryType::Symlink);
+    t!(header.set_path("foo/bar"));
+    t!(header.set_link_name("../baz"));
+    header.set_cksum();
+    t!(ar.append(&header, &[][..]).await);
+
+    let mut header = async_tar::Header::new_gnu();
+    header.set_size(1);
+    header.set_entry_type(async_tar::EntryType::Regular);
+    t!(header.set_path("baz"));
+    header.set_cksum();
+    t!(ar.append(&header, &b"x"[..]).await);
+
+    let bytes = t!(ar.into_inner().await);
+
+    let builder = ArchiveBuilder::new(&bytes[..]).set_allow_external_symlinks(false);
+    let mut ar = builder.build();
+
+    let td = t!(Builder::new().prefix("tar").tempdir());
+    t!(ar.unpack(td.path()).await);
+    t!(td.path().join("foo/bar").symlink_metadata());
+    t!(File::open(td.path().join("foo").join("bar")).await);
+}