Skip to content

Commit

Permalink
Sanitize filenames during zip extraction (#8732)
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh authored Oct 31, 2024
1 parent 8d3408f commit f326458
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 3 deletions.
41 changes: 38 additions & 3 deletions crates/uv-extract/src/stream.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
use std::path::Path;
use std::path::{Component, Path, PathBuf};
use std::pin::Pin;

use crate::Error;
use futures::StreamExt;
use rustc_hash::FxHashSet;
use tokio_util::compat::{FuturesAsyncReadCompatExt, TokioAsyncReadCompatExt};
use tracing::warn;

use uv_distribution_filename::SourceDistExtension;

use crate::Error;

const DEFAULT_BUF_SIZE: usize = 128 * 1024;

/// Unpack a `.zip` archive into the target directory, without requiring `Seek`.
Expand All @@ -19,6 +21,26 @@ pub async fn unzip<R: tokio::io::AsyncRead + Unpin>(
reader: R,
target: impl AsRef<Path>,
) -> Result<(), Error> {
/// Ensure the file path is safe to use as a [`Path`].
///
/// See: <https://docs.rs/zip/latest/zip/read/struct.ZipFile.html#method.enclosed_name>
pub(crate) fn enclosed_name(file_name: &str) -> Option<PathBuf> {
if file_name.contains('\0') {
return None;
}
let path = PathBuf::from(file_name);
let mut depth = 0usize;
for component in path.components() {
match component {
Component::Prefix(_) | Component::RootDir => return None,
Component::ParentDir => depth = depth.checked_sub(1)?,
Component::Normal(_) => depth += 1,
Component::CurDir => (),
}
}
Some(path)
}

let target = target.as_ref();
let mut reader = futures::io::BufReader::with_capacity(DEFAULT_BUF_SIZE, reader.compat());
let mut zip = async_zip::base::read::stream::ZipFileReader::new(&mut reader);
Expand All @@ -28,6 +50,16 @@ pub async fn unzip<R: tokio::io::AsyncRead + Unpin>(
while let Some(mut entry) = zip.next_with_entry().await? {
// Construct the (expected) path to the file on-disk.
let path = entry.reader().entry().filename().as_str()?;

// Sanitize the file name to prevent directory traversal attacks.
let Some(path) = enclosed_name(path) else {
warn!("Skipping unsafe file name: {path}");

// Close current file prior to proceeding, as per:
// https://docs.rs/async_zip/0.0.16/async_zip/base/read/stream/
zip = entry.skip().await?;
continue;
};
let path = target.join(path);
let is_dir = entry.reader().entry().dir()?;

Expand Down Expand Up @@ -55,7 +87,7 @@ pub async fn unzip<R: tokio::io::AsyncRead + Unpin>(
tokio::io::copy(&mut reader, &mut writer).await?;
}

// Close current file to get access to the next one. See docs:
// Close current file prior to proceeding, as per:
// https://docs.rs/async_zip/0.0.16/async_zip/base/read/stream/
zip = entry.skip().await?;
}
Expand Down Expand Up @@ -84,6 +116,9 @@ pub async fn unzip<R: tokio::io::AsyncRead + Unpin>(
if has_any_executable_bit != 0 {
// Construct the (expected) path to the file on-disk.
let path = entry.filename().as_str()?;
let Some(path) = enclosed_name(path) else {
continue;
};
let path = target.join(path);

let permissions = fs_err::tokio::metadata(&path).await?.permissions();
Expand Down
2 changes: 2 additions & 0 deletions crates/uv-extract/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::sync::Mutex;

use rayon::prelude::*;
use rustc_hash::FxHashSet;
use tracing::warn;
use zip::ZipArchive;

use crate::vendor::{CloneableSeekableReader, HasLength};
Expand All @@ -25,6 +26,7 @@ pub fn unzip<R: Send + std::io::Read + std::io::Seek + HasLength>(

// Determine the path of the file within the wheel.
let Some(enclosed_name) = file.enclosed_name() else {
warn!("Skipping unsafe file name: {}", file.name());
return Ok(());
};

Expand Down
31 changes: 31 additions & 0 deletions crates/uv/tests/it/pip_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5607,3 +5607,34 @@ fn sync_seed() -> Result<()> {

Ok(())
}

/// Sanitize zip files during extraction.
#[test]
fn sanitize() -> Result<()> {
let context = TestContext::new("3.12");

// Install a zip file that includes a path that extends outside the parent.
let requirements_txt = context.temp_dir.child("requirements.txt");
requirements_txt.write_str("payload-package @ https://github.com/astral-sh/sanitize-wheel-test/raw/bc59283d5b4b136a191792e32baa51b477fdf65e/payload_package-0.1.0-py3-none-any.whl")?;

uv_snapshot!(context.pip_sync()
.arg("requirements.txt"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Resolved 1 package in [TIME]
Prepared 1 package in [TIME]
Installed 1 package in [TIME]
+ payload-package==0.1.0 (from https://github.com/astral-sh/sanitize-wheel-test/raw/bc59283d5b4b136a191792e32baa51b477fdf65e/payload_package-0.1.0-py3-none-any.whl)
"###
);

// There should be no `payload` file in the root.
if let Some(parent) = context.temp_dir.parent() {
assert!(!parent.join("payload").exists());
}

Ok(())
}

0 comments on commit f326458

Please sign in to comment.