CVE-2025-59825

@@ -19,7 +19,7 @@ checksum = "512761e0bb2578dd7380c6baaa0f4ce03e84f95e960231d1dec8bf4d7d6e2627"
 
 [[package]]
 name = "astral-tokio-tar"
-version = "0.5.3"
+version = "0.5.4"
 dependencies = [
  "filetime",
  "futures-core",

@@ -1,6 +1,6 @@
 [package]
 name = "astral-tokio-tar"
-version = "0.5.3"
+version = "0.5.4"
 authors = [
     "Alex Crichton <alex@alexcrichton.com>",
     "dignifiedquire <me@dignifiequire.com>",

@@ -1,5 +1,12 @@
 # Changelog
 
+## 0.5.4
+
+* Fixed a path traversal vulnerability when using the `unpack_in_raw` API
+  by @charliermarsh
+
+    This vulnerability is being tracked as GHSA-3wgq-wrwc-vqmv.
+
 ## 0.5.3
 
 * Expose `TarError` publicly by @konstin in https://github.com/astral-sh/tokio-tar/pull/52

@@ -51,6 +51,7 @@ pub(crate) fn normalize(path: &Path) -> Option<PathBuf> {
 
 #[cfg(test)]
 mod tests {
+    #[cfg(unix)]
     use std::path::{Path, PathBuf};
 
     #[test]

@@ -1,4 +1,4 @@
-use crate::fs::{normalize_absolute, normalize_relative};
+use crate::fs::normalize;
 use crate::{
     error::TarError, header::bytes2path, other, pax::pax_extensions, Archive, Header, PaxExtensions,
 };
@@ -519,6 +519,13 @@ impl<R: Read + Unpin> EntryFields<R> {
             None => return Ok(None),
         };
 
+        // If the target is a link, clear the memoized set entirely. If we don't clear the set, then
+        // a malicious tarball could create a symlink to change the effective parent directory
+        // of an unpacked file _after_ it has been validated.
+        if self.header.entry_type().is_symlink() || self.header.entry_type().is_hard_link() {
+            memo.clear();
+        }
+
         // Validate the parent, if we haven't seen it yet.
         if !memo.contains(parent) {
             self.ensure_dir_created(dst, parent).await.map_err(|e| {
@@ -581,17 +588,25 @@ impl<R: Read + Unpin> EntryFields<R> {
             }
             return Ok(Unpacked::Other);
         } else if kind.is_hard_link() || kind.is_symlink() {
-            let src = match self.link_name()? {
+            let link_name = match self.link_name()? {
                 Some(name) => name,
                 None => {
                     return Err(other("hard link listed but no link name found"));
                 }
             };
 
-            if src.iter().count() == 0 {
+            // Reject absolute paths entirely.
+            if !self.allow_external_symlinks && link_name.is_absolute() {
+                return Err(other(&format!(
+                    "symlink path `{}` is absolute, but external symlinks are not allowed",
+                    link_name.display()
+                )));
+            }
+
+            if link_name.iter().count() == 0 {
                 return Err(other(&format!(
                     "symlink destination for {} is empty",
-                    src.display()
+                    link_name.display()
                 )));
             }
 
@@ -609,11 +624,11 @@ impl<R: Read + Unpin> EntryFields<R> {
                     // links though they're canonicalized to their existing path
                     // so we need to validate at this time.
                     Some(p) => {
-                        let link_src = p.join(src);
+                        let link_src = p.join(link_name);
                         self.validate_inside_dst(p, &link_src).await?;
                         link_src
                     }
-                    None => src.into_owned(),
+                    None => link_name.into_owned(),
                 };
                 fs::hard_link(&link_src, dst).await.map_err(|err| {
                     Error::new(
@@ -627,34 +642,56 @@ 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()
-                            )));
-                        }
+                let normalized_src = if self.allow_external_symlinks {
+                    // If external symlinks are allowed, use the source path as is.
+                    link_name
+                } else {
+                    // Ensure that we were able to normalize the path (e.g., `a/b/../c` to `a/c`).
+                    let Some(normalized_src) = normalize(&link_name) else {
+                        return Err(other(&format!(
+                            "symlink destination for {} is not a valid path",
+                            link_name.display()
+                        )));
+                    };
+
+                    // Join the normalized path with the parent of `dst`.
+                    let Some(absolute_normalized_path) = dst
+                        .parent()
+                        .map(|parent| parent.join(&normalized_src))
+                        .and_then(|path| normalize(&path))
+                    else {
+                        return Err(other(&format!(
+                            "symlink destination for {} lacks a parent path",
+                            link_name.display()
+                        )));
+                    };
+
+                    println!(
+                        "Absolute normalized symlink source: {}",
+                        absolute_normalized_path.display()
+                    );
+
+                    // If the normalized path points outside the target directory, reject it.
+                    if !target_base
+                        .is_some_and(|target| absolute_normalized_path.starts_with(target))
+                    {
+                        return Err(other(&format!(
+                            "symlink destination for {} is outside of the target directory",
+                            link_name.display()
+                        )));
                     }
-                }
 
-                match symlink(&src, dst).await {
+                    Cow::Owned(normalized_src)
+                };
+
+                match symlink(&normalized_src, dst).await {
                     Ok(()) => Ok(()),
                     Err(err) => {
                         if err.kind() == io::ErrorKind::AlreadyExists && self.overwrite {
                             match remove_file(dst).await {
-                                Ok(()) => symlink(&src, dst).await,
+                                Ok(()) => symlink(&normalized_src, dst).await,
                                 Err(ref e) if e.kind() == io::ErrorKind::NotFound => {
-                                    symlink(&src, dst).await
+                                    symlink(&normalized_src, dst).await
                                 }
                                 Err(e) => Err(e),
                             }

@@ -1,62 +1,52 @@
 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() {
+/// Normalize a path, like Python's `os.path.normpath`.
+///
+/// Adapted from <https://github.com/rust-lang/cargo/blob/fede83ccf973457de319ba6fa0e36ead454d2e20/src/cargo/util/paths.rs#L61>.
+pub(crate) fn normalize(path: &Path) -> Option<PathBuf> {
+    let mut components = path.components().peekable();
+    let mut ret = if let Some(c @ Component::Prefix(..)) = components.peek() {
+        let buf = PathBuf::from(c.as_os_str());
+        components.next();
+        buf
+    } else {
+        PathBuf::new()
+    };
+    let mut has_root = false;
+
+    for component in components {
         match component {
-            Component::Prefix(prefix) => {
-                // Windows-specific: preserve drive letter and prefix.
-                resolved.push(prefix.as_os_str());
-            }
+            Component::Prefix(..) => unreachable!(),
             Component::RootDir => {
-                // Append root directory after prefix.
-                resolved.push(component.as_os_str());
-            }
-            Component::CurDir => {
-                // Ignore `.`
+                ret.push(component.as_os_str());
+                has_root = true;
             }
+            Component::CurDir => {}
             Component::ParentDir => {
-                if !resolved.pop() {
+                // Preserve leading `..` components.
+                if ret
+                    .components()
+                    .next_back()
+                    .is_some_and(|component| component == Component::ParentDir)
+                {
+                    ret.push(component.as_os_str());
+                } else if ret.pop() {
+                    // We successfully removed a component.
+                } else if has_root {
+                    // An absolute path tried to go above the root.
                     return None;
+                } else {
+                    // If we don't have a root, we can just push the `..` component.
+                    ret.push(component.as_os_str());
                 }
             }
-            Component::Normal(segment) => {
-                resolved.push(segment);
+            Component::Normal(c) => {
+                ret.push(c);
             }
         }
     }
-    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)
+    Some(ret)
 }
 
 #[cfg(test)]
@@ -65,87 +55,107 @@ mod tests {
 
     #[test]
     #[cfg(unix)]
-    fn test_normalize_absolute() {
-        // Basic absolute path.
+    fn test_normalize() {
+        // Basic relative path.
         assert_eq!(
-            crate::fs::normalize_absolute(Path::new("/a/b/c")),
-            Some(PathBuf::from("/a/b/c"))
+            crate::fs::normalize(Path::new("a/b/c")),
+            Some(PathBuf::from("a/b/c"))
         );
 
-        // Path with `..` (should remove `b`).
+        // Path with `..`, should remove `b`.
         assert_eq!(
-            crate::fs::normalize_absolute(Path::new("/a/b/../c")),
-            Some(PathBuf::from("/a/c"))
+            crate::fs::normalize(Path::new("a/b/../c")),
+            Some(PathBuf::from("a/c"))
         );
 
-        // Path with `.`, should be ignored.
+        // Path with `.` should be ignored.
         assert_eq!(
-            crate::fs::normalize_absolute(Path::new("/a/./b")),
-            Some(PathBuf::from("/a/b"))
+            crate::fs::normalize(Path::new("./a/b")),
+            Some(PathBuf::from("a/b"))
         );
 
-        // Excessive `..` that escapes root.
-        assert_eq!(crate::fs::normalize_absolute(Path::new("/../b")), None);
-    }
+        // Path with no relative components should be unchanged.
+        assert_eq!(
+            crate::fs::normalize(Path::new("outside")),
+            Some(PathBuf::from("outside"))
+        );
 
-    #[test]
-    #[cfg(windows)]
-    fn test_normalize_absolute() {
-        // Basic absolute path.
+        // Excessive `..` should be ignored.
         assert_eq!(
-            crate::fs::normalize_absolute(Path::new(r"C:\a\b\c")),
-            Some(PathBuf::from(r"C:\a\b\c"))
+            crate::fs::normalize(Path::new("../../../../")),
+            Some(PathBuf::from("../../../../"),)
         );
 
-        // Path with `..` (should remove `b`).
+        // Multiple `..` should stack.
         assert_eq!(
-            crate::fs::normalize_absolute(Path::new(r"C:\a\b\..\c")),
-            Some(PathBuf::from(r"C:\a\c"))
+            crate::fs::normalize(Path::new("a/b/../../c")),
+            Some(PathBuf::from("c"))
         );
 
-        // Path with `.`, should be ignored.
+        // Rooted absolute path, `..` should not go above root.
+        assert_eq!(crate::fs::normalize(Path::new("/a/../..")), None);
+
+        // Root with dot and parent.
         assert_eq!(
-            crate::fs::normalize_absolute(Path::new(r"C:\a\.\b")),
-            Some(PathBuf::from(r"C:\a\b"))
+            crate::fs::normalize(Path::new("/./a/../b")),
+            Some(PathBuf::from("/b"))
         );
 
-        // Excessive `..` that escapes root.
-        assert_eq!(crate::fs::normalize_absolute(Path::new(r"C:\..\b")), None);
-    }
+        // Trailing slash should be ignored.
+        assert_eq!(
+            crate::fs::normalize(Path::new("a/b/c/")),
+            Some(PathBuf::from("a/b/c"))
+        );
 
-    #[test]
-    #[cfg(unix)]
-    fn test_normalize_relative() {
-        let dst = Path::new("/home/user/dst");
+        // Trailing `/.` should be dropped.
+        assert_eq!(
+            crate::fs::normalize(Path::new("a/b/.")),
+            Some(PathBuf::from("a/b"))
+        );
 
-        // Basic relative path.
+        // Trailing `/..` should pop last component.
         assert_eq!(
-            crate::fs::normalize_relative(dst, Path::new("a/b/c")),
-            Some(PathBuf::from("/home/user/dst/a/b/c"))
+            crate::fs::normalize(Path::new("a/b/..")),
+            Some(PathBuf::from("a"))
         );
 
-        // Path with `..`, should remove `b`.
+        // Leading `..` in a relative path should be preserved.
         assert_eq!(
-            crate::fs::normalize_relative(dst, Path::new("a/b/../c")),
-            Some(PathBuf::from("/home/user/dst/a/c"))
+            crate::fs::normalize(Path::new("../x/y")),
+            Some(PathBuf::from("../x/y"))
         );
 
-        // Path with `.` should be ignored.
+        // Mix of preserved leading `..` and collapsed internals.
+        assert_eq!(
+            crate::fs::normalize(Path::new("../../a/b/../c")),
+            Some(PathBuf::from("../../a/c"))
+        );
+
+        // Windows drive absolute: C:\a\..\b
+        #[cfg(windows)]
+        assert_eq!(
+            crate::fs::normalize(Path::new(r"C:\a\..\b")),
+            Some(PathBuf::from(r"C:\b"))
+        );
+
+        // Windows drive-relative (no backslash): C:..\a
+        // should preserve the `..`
+        #[cfg(windows)]
         assert_eq!(
-            crate::fs::normalize_relative(dst, Path::new("./a/b")),
-            Some(PathBuf::from("/home/user/dst/a/b"))
+            crate::fs::normalize(Path::new(r"C:..\a")),
+            Some(PathBuf::from(r"C:..\a"))
         );
 
-        // Path escaping `dst`, should return None.
+        // Root-only should normalize to root.
         assert_eq!(
-            crate::fs::normalize_relative(dst, Path::new("../../../../outside")),
-            None
+            crate::fs::normalize(Path::new("/")),
+            Some(PathBuf::from("/"))
         );
 
-        // Excessive `..`, escaping `dst`.
+        // Just `..` should normalize to `..`
         assert_eq!(
-            crate::fs::normalize_relative(dst, Path::new("../../../../")),
-            None
+            crate::fs::normalize(Path::new("..")),
+            Some(PathBuf::from(".."))
         );
     }
 }