Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 60 additions & 61 deletions crates/uv-install-wheel/src/uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ pub fn uninstall_wheel(
})
}

static WARNED_FOR_PACKAGE: OnceLock<Mutex<HashSet<String>>> = OnceLock::new();
static WARNED_FOR_RECORD_ENTRY_PACKAGE: OnceLock<Mutex<HashSet<String>>> = OnceLock::new();
static WARNED_FOR_EGG_TOP_LEVEL_PACKAGE: OnceLock<Mutex<HashSet<String>>> = OnceLock::new();

/// Check if the path is inside the venv or a system interpreter path, and warn if it isn't.
///
Expand Down Expand Up @@ -194,7 +195,7 @@ fn is_path_in_scheme(
} else {
// A package that does this is malformed to the point of being a risk to the user, be
// annoying about it, but only once per package.
if WARNED_FOR_PACKAGE
if WARNED_FOR_RECORD_ENTRY_PACKAGE
.get_or_init(|| Mutex::new(HashSet::new()))
.lock()
.expect("The mutex is broken, did some other thread panic?")
Expand All @@ -210,14 +211,39 @@ fn is_path_in_scheme(
}
}

/// Check that a `top_level.txt` entry names a single top-level module or package.
///
/// Unlike wheel `RECORD` entries, egg `top_level.txt` entries refer to direct children of the
/// egg's base location, not arbitrary paths. Treating them as paths can make uninstall delete
/// directories outside `site-packages`.
fn is_valid_top_level_entry(entry: &str, distribution: impl Display) -> bool {
if is_safe_top_level_entry(entry) {
true
} else {
if WARNED_FOR_EGG_TOP_LEVEL_PACKAGE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be warn_user_once!, I think? That way we don't need another mutex?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the same pattern for record warnings; it's structured this way so we can warn once per package (rather than once overall).

.get_or_init(|| Mutex::new(HashSet::new()))
.lock()
.expect("The mutex is broken, did some other thread panic?")
.insert(distribution.to_string())
{
warn_user!(
"Invalid `top_level.txt` entry in {} that is not a top-level module or package, skipping: {}",
distribution,
entry
);
}
false
}
}

fn is_safe_top_level_entry(entry: &str) -> bool {
!entry.is_empty() && entry != "." && entry != ".." && !entry.contains(['/', '\\'])
}

/// Uninstall the egg represented by the `.egg-info` directory.
///
/// See: <https://github.com/pypa/pip/blob/41587f5e0017bcd849f42b314dc8a34a7db75621/src/pip/_internal/req/req_uninstall.py#L483>
pub fn uninstall_egg(
egg_info: &Path,
distribution: impl Display,
layout: &Layout,
) -> Result<Uninstall, Error> {
pub fn uninstall_egg(egg_info: &Path, distribution: impl Display) -> Result<Uninstall, Error> {
let mut file_count = 0usize;
let mut dir_count = 0usize;

Expand Down Expand Up @@ -268,12 +294,12 @@ pub fn uninstall_egg(

// Remove everything in `top_level.txt`.
for entry in top_level {
let path = dist_location.join(&entry);

if !is_path_in_scheme(&entry, dist_location, &distribution, layout) {
if !is_valid_top_level_entry(&entry, &distribution) {
continue;
}

let path = dist_location.join(&entry);
Comment thread
woodruffw marked this conversation as resolved.

// Remove as a directory.
match fs_err::remove_dir_all(&path) {
Ok(()) => {
Expand Down Expand Up @@ -435,7 +461,19 @@ mod tests {
use uv_pypi_types::Scheme;

use crate::Layout;
use crate::uninstall::{uninstall_egg, uninstall_wheel};
use crate::uninstall::{is_safe_top_level_entry, uninstall_egg, uninstall_wheel};

#[test]
fn test_top_level_entry_safe_name() {
assert!(is_safe_top_level_entry("package"));

assert!(!is_safe_top_level_entry(""));
assert!(!is_safe_top_level_entry("."));
assert!(!is_safe_top_level_entry(".."));
assert!(!is_safe_top_level_entry("../package"));
assert!(!is_safe_top_level_entry("package/name"));
assert!(!is_safe_top_level_entry(r"package\name"));
}

/// Uninstall must not remove files outside the install scheme.
#[test]
Expand Down Expand Up @@ -473,7 +511,7 @@ mod tests {
let metadata = dist_info.child("METADATA");
metadata.touch().unwrap();

// Something that looks sufficiently like a Unix venv.
// Something that looks sufficiently like a Unix environment.
let layout = Layout {
sys_executable: venv.path().join("bin/python"),
python_version: (3, 13),
Expand All @@ -500,20 +538,20 @@ mod tests {
fn test_uninstall_egg_info_path_traversal() {
let venv = assert_fs::TempDir::new().unwrap();
let site_packages = venv.child("lib/python3.12/site-packages");
let outside_dir = assert_fs::TempDir::new().unwrap();

// Create a directory outside site-packages that a malicious top_level.txt might target.
let target_dir = outside_dir.child("traversal_target");
// Create directories outside site-packages, but inside the environment. Egg uninstall should
// still reject them, even though wheel RECORD entries may target other install-scheme
// directories.
let target_dir = venv.child("traversal_target");
let target_file = target_dir.child("secret.txt");
target_file.write_str("I should not be deleted").unwrap();

// Build a relative traversal path from site-packages to the target directory.
let egg_info = site_packages.child("evilpkg-0.1.0.egg-info");
egg_info.create_dir_all().unwrap();
let target_path = pathdiff::diff_paths(target_dir.path(), site_packages.path()).unwrap();
assert!(site_packages.join(&target_path).exists());

// Create a fake egg-info directory with a top_level.txt containing a path traversal entry.
// Create a fake egg-info directory with a path traversal entry in `top_level.txt`.
egg_info
.child("top_level.txt")
.write_str(&format!("evilpkg\n{}\n", target_path.display()))
Expand All @@ -523,23 +561,10 @@ mod tests {
let init_py = site_packages.child("evilpkg").child("__init__.py");
init_py.touch().unwrap();

// Something that looks sufficiently like a Unix venv.
let layout = Layout {
sys_executable: venv.path().join("bin/python"),
python_version: (3, 13),
os_name: "posix".to_string(),
scheme: Scheme {
purelib: site_packages.to_path_buf(),
platlib: site_packages.to_path_buf(),
scripts: venv.path().join("bin"),
data: venv.path().to_path_buf(),
include: venv.path().join("include/python3.12"),
},
};
uninstall_egg(egg_info.path(), "evilpkg 0.1.0").unwrap();

uninstall_egg(egg_info.path(), "evilpkg 0.1.0", &layout).unwrap();

// The regular package directory has been removed, while the directory outside the scheme still exists.
// The regular package directory has been removed, while the directory outside
// site-packages still exists.
assert!(target_dir.exists());
assert!(target_file.exists());
assert!(!init_py.exists());
Expand Down Expand Up @@ -571,20 +596,7 @@ mod tests {
egg_info.create_dir_all().unwrap();
egg_info.child("top_level.txt").write_str("\n").unwrap();

let layout = Layout {
sys_executable: venv.path().join("bin/python"),
python_version: (3, 13),
os_name: "posix".to_string(),
scheme: Scheme {
purelib: site_packages.to_path_buf(),
platlib: site_packages.to_path_buf(),
scripts: venv.path().join("bin"),
data: venv.path().to_path_buf(),
include: venv.path().join("include/python3.12"),
},
};

uninstall_egg(egg_info.path(), "emptypkg 0.1.0", &layout).unwrap();
uninstall_egg(egg_info.path(), "emptypkg 0.1.0").unwrap();

// The egg-info is gone, but the rest of site-packages (including the sibling
// package) survives.
Expand Down Expand Up @@ -628,20 +640,7 @@ mod tests {
.write_str("\npkg_a\n \r\npkg_b\n\n")
.unwrap();

let layout = Layout {
sys_executable: venv.path().join("bin/python"),
python_version: (3, 13),
os_name: "posix".to_string(),
scheme: Scheme {
purelib: site_packages.to_path_buf(),
platlib: site_packages.to_path_buf(),
scripts: venv.path().join("bin"),
data: venv.path().to_path_buf(),
include: venv.path().join("include/python3.12"),
},
};

uninstall_egg(egg_info.path(), "mixedpkg 0.1.0", &layout).unwrap();
uninstall_egg(egg_info.path(), "mixedpkg 0.1.0").unwrap();

// The two named packages are gone, the egg-info is gone, and site-packages plus
// the sibling survive.
Expand Down
8 changes: 3 additions & 5 deletions crates/uv-installer/src/uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@ pub async fn uninstall(
InstalledDistKind::Registry(_) | InstalledDistKind::Url(_) => Ok(
uv_install_wheel::uninstall_wheel(dist.install_path(), &dist, &layout)?,
),
InstalledDistKind::EggInfoDirectory(_) => Ok(uv_install_wheel::uninstall_egg(
dist.install_path(),
&dist,
&layout,
)?),
InstalledDistKind::EggInfoDirectory(_) => {
Ok(uv_install_wheel::uninstall_egg(dist.install_path(), &dist)?)
}
InstalledDistKind::LegacyEditable(dist) => {
Ok(uv_install_wheel::uninstall_legacy_editable(&dist.egg_link)?)
}
Expand Down
58 changes: 57 additions & 1 deletion crates/uv/tests/it/pip_uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ fn uninstall_record_path_traversal() -> Result<()> {

// Build the relative traversal path from site-packages to a target file outside
// site-packages but inside the test temp dir. RECORD uses forward slashes, even on
// Windows, and the venv layout (and thus the traversal depth) differs by platform,
// Windows, and the environment layout (and thus the traversal depth) differs by platform,
// so we construct the path manually and filter the leading `../` sequence out of the
// snapshot above.
let target_file = context.temp_dir.child("traversal_target.txt");
Expand Down Expand Up @@ -580,6 +580,62 @@ fn uninstall_record_path_traversal() -> Result<()> {
Ok(())
}

/// Egg `top_level.txt` entries must be top-level names, not paths.
#[test]
fn uninstall_egg_info_top_level_path_traversal() -> Result<()> {
// The traversal-depth count differs between Unix (`.venv/lib/pythonX.Y/site-packages`)
// and Windows (`.venv/Lib/site-packages`), so normalize the `../` sequence in the warning.
let context = uv_test::test_context!("3.12")
.with_filter((r"(\.\./)+traversal_target", "[..]/traversal_target"));

let site_packages = ChildPath::new(context.site_packages());

// Manually create a `name-version.egg-info` directory, which is recognized by the shared egg
// filename parser.
let egg_info = site_packages.child("evilpkg-0.1.0.egg-info");
egg_info.create_dir_all()?;

// The traversal target is outside site-packages but inside the environment, so a wheel RECORD entry
// could validly target this scheme area. An egg `top_level.txt` entry must not.
let target_dir = context.venv.child("traversal_target");
let target_file = target_dir.child("secret.txt");
target_file.write_str("I should not be deleted")?;

let depth = context
.site_packages()
.strip_prefix(context.venv.path())?
.components()
.count();
let traversal_entry = format!("{}traversal_target", "../".repeat(depth));
assert!(context.site_packages().join(&traversal_entry).exists());

egg_info
.child("top_level.txt")
.write_str(&format!("evilpkg\n{traversal_entry}\n"))?;

let init_py = site_packages.child("evilpkg").child("__init__.py");
init_py.touch()?;

uv_snapshot!(context.filters(), context.pip_uninstall()
.arg("evilpkg"), @"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
warning: Invalid `top_level.txt` entry in evilpkg==0.1.0 that is not a top-level module or package, skipping: [..]/traversal_target
Uninstalled 1 package in [TIME]
- evilpkg==0.1.0
");

assert!(target_dir.exists());
assert!(target_file.exists());
assert!(!init_py.exists());
assert!(!egg_info.exists());

Ok(())
}

/// `--yes` is accepted for `pip uninstall` compatibility, but emits a warning.
#[test]
fn yes_flag() {
Expand Down
Loading