diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 25c83cc8d14..a0178db55e8 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -184,8 +184,11 @@ //! [`IndexPackage`]: index::IndexPackage use std::collections::HashSet; +use std::fs; 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}; @@ -194,6 +197,7 @@ use cargo_util::paths::{self, exclude_from_backups_and_indexing}; use flate2::read::GzDecoder; use log::debug; use serde::Deserialize; +use serde::Serialize; use tar::Archive; use crate::core::dependency::Dependency; @@ -217,6 +221,14 @@ pub const CRATES_IO_HTTP_INDEX: &str = "sparse+https://index.crates.io/"; pub const CRATES_IO_REGISTRY: &str = "crates-io"; pub const CRATES_IO_DOMAIN: &str = "crates.io"; +/// The content inside `.cargo-ok`. +/// See [`RegistrySource::unpack_package`] for more. +#[derive(Deserialize, Serialize)] +struct LockMetadata { + /// The version of `.cargo-ok` file + v: u32, +} + /// A [`Source`] implementation for a local or a remote registry. /// /// This contains common functionality that is shared between each registry @@ -544,6 +556,11 @@ impl<'cfg> RegistrySource<'cfg> { /// `.crate` files making `.cargo-ok` a symlink causing cargo to write "ok" /// to any arbitrary file on the filesystem it has permission to. /// + /// In 1.71, `.cargo-ok` changed to contain a JSON `{ v: 1 }` to indicate + /// the version of it. A failure of parsing will result in a heavy-hammer + /// approach that unpacks the `.crate` file again. This is in response to a + /// security issue that the unpacking didn't respect umask on Unix systems. + /// /// This is all a long-winded way of explaining the circumstances that might /// cause a directory to contain a `.cargo-ok` file that is empty or /// otherwise corrupted. Either this was extracted by a version of Rust @@ -565,22 +582,32 @@ impl<'cfg> RegistrySource<'cfg> { let path = dst.join(PACKAGE_SOURCE_LOCK); let path = self.config.assert_package_cache_locked(&path); let unpack_dir = path.parent().unwrap(); - match path.metadata() { - Ok(meta) if meta.len() > 0 => return Ok(unpack_dir.to_path_buf()), - Ok(_meta) => { - // See comment of `unpack_package` about why removing all stuff. - log::warn!("unexpected length of {path:?}, clearing cache"); - paths::remove_dir_all(dst.as_path_unlocked())?; - } + match fs::read_to_string(path) { + Ok(ok) => match serde_json::from_str::(&ok) { + Ok(lock_meta) if lock_meta.v == 1 => { + return Ok(unpack_dir.to_path_buf()); + } + _ => { + if ok == "ok" { + log::debug!("old `ok` content found, clearing cache"); + } else { + log::warn!("unrecognized .cargo-ok content, clearing cache: {ok}"); + } + // See comment of `unpack_package` about why removing all stuff. + paths::remove_dir_all(dst.as_path_unlocked())?; + } + }, Err(e) if e.kind() == io::ErrorKind::NotFound => {} - Err(e) => anyhow::bail!("failed to access package completion {path:?}: {e}"), + Err(e) => anyhow::bail!("unable to read .cargo-ok file at {path:?}: {e}"), } dst.create_dir()?; let mut tar = { 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(); @@ -635,7 +662,9 @@ impl<'cfg> RegistrySource<'cfg> { .write(true) .open(&path) .with_context(|| format!("failed to open `{}`", path.display()))?; - write!(ok, "ok")?; + + let lock_meta = LockMetadata { v: 1 }; + write!(ok, "{}", serde_json::to_string(&lock_meta).unwrap())?; Ok(unpack_dir.to_path_buf()) } @@ -908,3 +937,16 @@ fn max_unpack_size(config: &Config, size: u64) -> u64 { u64::max(max_unpack_size, size * max_compression_ratio as u64) } + +/// 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/cargo/util/mod.rs b/src/cargo/util/mod.rs index 6cefb97c2b7..26e97e2d20c 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -208,6 +208,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/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index f3964496509..bd5e42b4559 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -2546,7 +2546,7 @@ fn package_lock_inside_package_is_overwritten() { .join("bar-0.0.1") .join(".cargo-ok"); - assert_eq!(ok.metadata().unwrap().len(), 2); + assert_eq!(ok.metadata().unwrap().len(), 7); } #[cargo_test] @@ -2586,7 +2586,7 @@ fn package_lock_as_a_symlink_inside_package_is_overwritten() { let librs = pkg_root.join("src/lib.rs"); // Is correctly overwritten and doesn't affect the file linked to - assert_eq!(ok.metadata().unwrap().len(), 2); + assert_eq!(ok.metadata().unwrap().len(), 7); assert_eq!(fs::read_to_string(librs).unwrap(), "pub fn f() {}"); } @@ -3135,7 +3135,7 @@ fn corrupted_ok_overwritten() { fs::write(&ok, "").unwrap(); assert_eq!(fs::read_to_string(&ok).unwrap(), ""); p.cargo("fetch").with_stderr("").run(); - assert_eq!(fs::read_to_string(&ok).unwrap(), "ok"); + assert_eq!(fs::read_to_string(&ok).unwrap(), r#"{"v":1}"#); } #[cargo_test] @@ -3403,3 +3403,129 @@ Caused by: Please slow down ").run(); } + +#[cfg(unix)] +#[cargo_test] +fn set_mask_during_unpacking() { + use std::os::unix::fs::MetadataExt; + + Package::new("bar", "1.0.0") + .file_with_mode("example.sh", 0o777, "#!/bin/sh") + .file_with_mode("src/lib.rs", 0o666, "") + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("fetch") + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[DOWNLOADING] crates ... +[DOWNLOADED] bar v1.0.0 (registry `dummy-registry`) +", + ) + .run(); + let src_file_path = |path: &str| { + glob::glob( + paths::home() + .join(".cargo/registry/src/*/bar-1.0.0/") + .join(path) + .to_str() + .unwrap(), + ) + .unwrap() + .next() + .unwrap() + .unwrap() + }; + + let umask = cargo::util::get_umask(); + let metadata = fs::metadata(src_file_path("src/lib.rs")).unwrap(); + assert_eq!(metadata.mode() & 0o777, 0o666 & !umask); + let metadata = fs::metadata(src_file_path("example.sh")).unwrap(); + assert_eq!(metadata.mode() & 0o777, 0o777 & !umask); +} + +#[cargo_test] +fn unpack_again_when_cargo_ok_is_unrecognized() { + Package::new("bar", "1.0.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("fetch") + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[DOWNLOADING] crates ... +[DOWNLOADED] bar v1.0.0 (registry `dummy-registry`) +", + ) + .run(); + + let src_file_path = |path: &str| { + glob::glob( + paths::home() + .join(".cargo/registry/src/*/bar-1.0.0/") + .join(path) + .to_str() + .unwrap(), + ) + .unwrap() + .next() + .unwrap() + .unwrap() + }; + + // Change permissions to simulate the old behavior not respecting umask. + let lib_rs = src_file_path("src/lib.rs"); + let cargo_ok = src_file_path(".cargo-ok"); + let mut perms = fs::metadata(&lib_rs).unwrap().permissions(); + assert!(!perms.readonly()); + perms.set_readonly(true); + fs::set_permissions(&lib_rs, perms).unwrap(); + let ok = fs::read_to_string(&cargo_ok).unwrap(); + assert_eq!(&ok, r#"{"v":1}"#); + + p.cargo("fetch").with_stderr("").run(); + + // Without changing `.cargo-ok`, a unpack won't be triggered. + let perms = fs::metadata(&lib_rs).unwrap().permissions(); + assert!(perms.readonly()); + + // Write "ok" to simulate the old behavior and trigger the unpack again. + fs::write(&cargo_ok, "ok").unwrap(); + + p.cargo("fetch").with_stderr("").run(); + + // Permission has been restored and `.cargo-ok` is in the new format. + let perms = fs::metadata(lib_rs).unwrap().permissions(); + assert!(!perms.readonly()); + let ok = fs::read_to_string(&cargo_ok).unwrap(); + assert_eq!(&ok, r#"{"v":1}"#); +} diff --git a/tests/testsuite/vendor.rs b/tests/testsuite/vendor.rs index 21a1c097c0f..2b8b090c2e6 100644 --- a/tests/testsuite/vendor.rs +++ b/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]