From f909434f805893b6f5628534ebd13b7c25d045cf Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Fri, 7 Jul 2023 14:50:59 +0100 Subject: [PATCH 2/3] fix: respect `umask` when unpacking `.crate` files Without this, an attacker can upload globally writable files buried in the `.crate` file. After a user downloaded and unpacked the file, the attacker can then write malicous code to the downloaded sources. --- Cargo.lock | 4 ++-- src/tools/cargo/Cargo.toml | 2 +- .../cargo/src/cargo/sources/registry/mod.rs | 21 +++++++++++++++++-- src/tools/cargo/src/cargo/util/mod.rs | 18 ++++++++++++++++ src/tools/cargo/tests/testsuite/registry.rs | 6 +++--- src/tools/cargo/tests/testsuite/vendor.rs | 5 +++-- 6 files changed, 46 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8b0972494..cd7052f3b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4876,9 +4876,9 @@ dependencies = [ [[package]] name = "tar" -version = "0.4.38" +version = "0.4.39" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4b55807c0344e1e6c04d7c965f5289c39a8d94ae23ed5c0b57aabac549f871c6" +checksum = "ec96d2ffad078296368d46ff1cb309be1c23c513b4ab0e22a45de0185275ac96" dependencies = [ "filetime", "libc", diff --git a/src/tools/cargo/Cargo.toml b/src/tools/cargo/Cargo.toml index 3c6597b5c..6442bb1c8 100644 --- a/src/tools/cargo/Cargo.toml +++ b/src/tools/cargo/Cargo.toml @@ -80,7 +80,7 @@ sha2 = "0.10.6" shell-escape = "0.1.4" snapbox = { version = "0.4.0", features = ["diff", "path"] } strip-ansi-escapes = "0.1.0" -tar = { version = "0.4.38", default-features = false } +tar = { version = "0.4.39", default-features = false } tempfile = "3.1.0" termcolor = "1.1.2" time = { version = "0.3", features = ["parsing", "formatting"] } diff --git a/src/tools/cargo/src/cargo/sources/registry/mod.rs b/src/tools/cargo/src/cargo/sources/registry/mod.rs index ee27df15d..4e4d794e0 100644 --- a/src/tools/cargo/src/cargo/sources/registry/mod.rs +++ b/src/tools/cargo/src/cargo/sources/registry/mod.rs @@ -162,7 +162,9 @@ use std::borrow::Cow; use std::collections::BTreeMap; use std::collections::HashSet; use std::fs::{File, OpenOptions}; -use std::io::{self, Write}; +use std::io; +use std::io::Read; +use std::io::Write; use std::path::{Path, PathBuf}; use std::task::{ready, Poll}; @@ -698,7 +700,9 @@ impl<'cfg> RegistrySource<'cfg> { let size_limit = max_unpack_size(self.config, tarball.metadata()?.len()); let gz = GzDecoder::new(tarball); let gz = LimitErrorReader::new(gz, size_limit); - Archive::new(gz) + let mut tar = Archive::new(gz); + set_mask(&mut tar); + tar }; let prefix = unpack_dir.file_name().unwrap(); let parent = unpack_dir.parent().unwrap(); @@ -1036,3 +1040,16 @@ mod tests { assert_eq!(make_dep_prefix("aBcDe"), "aB/cD"); } } + +/// Set the current [`umask`] value for the given tarball. No-op on non-Unix +/// platforms. +/// +/// On Windows, tar only looks at user permissions and tries to set the "read +/// only" attribute, so no-op as well. +/// +/// [`umask`]: https://man7.org/linux/man-pages/man2/umask.2.html +#[allow(unused_variables)] +fn set_mask(tar: &mut Archive) { + #[cfg(unix)] + tar.set_mask(crate::util::get_umask()); +} diff --git a/src/tools/cargo/src/cargo/util/mod.rs b/src/tools/cargo/src/cargo/util/mod.rs index e72f8183b..df8dcb0ac 100644 --- a/src/tools/cargo/src/cargo/util/mod.rs +++ b/src/tools/cargo/src/cargo/util/mod.rs @@ -207,6 +207,24 @@ pub fn try_canonicalize>(path: P) -> std::io::Result { }) } +/// Get the current [`umask`] value. +/// +/// [`umask`]: https://man7.org/linux/man-pages/man2/umask.2.html +#[cfg(unix)] +pub fn get_umask() -> u32 { + use std::sync::OnceLock; + static UMASK: OnceLock = OnceLock::new(); + // SAFETY: Syscalls are unsafe. Calling `umask` twice is even unsafer for + // multithreading program, since it doesn't provide a way to retrive the + // value without modifications. We use a static `OnceLock` here to ensure + // it only gets call once during the entire program lifetime. + *UMASK.get_or_init(|| unsafe { + let umask = libc::umask(0o022); + libc::umask(umask); + umask + }) as u32 // it is u16 on macos +} + #[cfg(test)] mod test { use super::*; diff --git a/src/tools/cargo/tests/testsuite/registry.rs b/src/tools/cargo/tests/testsuite/registry.rs index 42107a5e4..daa96d5a8 100644 --- a/src/tools/cargo/tests/testsuite/registry.rs +++ b/src/tools/cargo/tests/testsuite/registry.rs @@ -3452,9 +3452,9 @@ fn set_mask_during_unpacking() { .unwrap() }; - // Assuming umask is `0o022`. + let umask = cargo::util::get_umask(); let metadata = fs::metadata(src_file_path("src/lib.rs")).unwrap(); - assert_eq!(metadata.mode() & 0o777, 0o666); + assert_eq!(metadata.mode() & 0o777, 0o666 & !umask); let metadata = fs::metadata(src_file_path("example.sh")).unwrap(); - assert_eq!(metadata.mode() & 0o777, 0o777); + assert_eq!(metadata.mode() & 0o777, 0o777 & !umask); } diff --git a/src/tools/cargo/tests/testsuite/vendor.rs b/src/tools/cargo/tests/testsuite/vendor.rs index 21a1c097c..2b8b090c2 100644 --- a/src/tools/cargo/tests/testsuite/vendor.rs +++ b/src/tools/cargo/tests/testsuite/vendor.rs @@ -1051,10 +1051,11 @@ fn vendor_preserves_permissions() { p.cargo("vendor --respect-source-config").run(); + let umask = cargo::util::get_umask(); let metadata = fs::metadata(p.root().join("vendor/bar/src/lib.rs")).unwrap(); - assert_eq!(metadata.mode() & 0o777, 0o644); + assert_eq!(metadata.mode() & 0o777, 0o644 & !umask); let metadata = fs::metadata(p.root().join("vendor/bar/example.sh")).unwrap(); - assert_eq!(metadata.mode() & 0o777, 0o755); + assert_eq!(metadata.mode() & 0o777, 0o755 & !umask); } #[cargo_test] -- 2.34.1