diff --git a/Cargo.lock b/Cargo.lock index 8d6fef0a37a..1c345bcbe40 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3123,9 +3123,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/Cargo.toml b/Cargo.toml index d4b229759ee..571f372fd56 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -80,7 +80,7 @@ shell-escape = "0.1.4" snapbox = { version = "0.4.0", features = ["diff", "path"] } strip-ansi-escapes = "0.1.0" syn = { version = "2.0.14", features = ["extra-traits", "full"] } -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/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 e72f8183b5f..df8dcb0acfe 100644 --- a/src/cargo/util/mod.rs +++ b/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/tests/testsuite/bench.rs b/tests/testsuite/bench.rs index 60ad2b60d34..581acbe1505 100644 --- a/tests/testsuite/bench.rs +++ b/tests/testsuite/bench.rs @@ -313,9 +313,8 @@ fn cargo_bench_failing_test() { [FINISHED] bench [optimized] target(s) in [..] [RUNNING] [..] (target/release/deps/foo-[..][EXE])", ) - .with_stdout_contains( - "[..]thread '[..]' panicked at 'assertion failed: `(left == right)`[..]", - ) + .with_stdout_contains("[..]thread '[..]' panicked at[..]") + .with_stdout_contains("[..]assertion failed[..]") .with_stdout_contains("[..]left: `\"hello\"`[..]") .with_stdout_contains("[..]right: `\"nope\"`[..]") .with_stdout_contains("[..]src/main.rs:15[..]") diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 0f6d9909a42..0c7fc5037e4 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -1284,7 +1284,8 @@ fn reports_unsuccessful_subcommand_result() { .run(); cargo_process("fail") .with_status(101) - .with_stderr_contains("thread '[..]' panicked at 'explicit panic', [..]") + .with_stderr_contains("thread '[..]' panicked at [..]src/main.rs:1:[..]") + .with_stderr_contains("[..]explicit panic[..]") .run(); } 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/test.rs b/tests/testsuite/test.rs index add0a991f24..6a062cfb60f 100644 --- a/tests/testsuite/test.rs +++ b/tests/testsuite/test.rs @@ -387,8 +387,9 @@ test test_hello ... FAILED failures: ---- test_hello stdout ---- -[..]thread '[..]' panicked at 'assertion failed:[..]", +[..]thread '[..]' panicked at [..]", ) + .with_stdout_contains("[..]assertion failed[..]") .with_stdout_contains("[..]`(left == right)`[..]") .with_stdout_contains("[..]left: `\"hello\"`,[..]") .with_stdout_contains("[..]right: `\"nope\"`[..]") @@ -437,10 +438,10 @@ test test_hello ... FAILED failures: ---- test_hello stdout ---- -[..]thread '[..]' panicked at 'assertion failed: false', \ - tests/footest.rs:1[..] +[..]thread '[..]' panicked at [..]tests/footest.rs:1:[..] ", ) + .with_stdout_contains("[..]assertion failed[..]") .with_stdout_contains( "\ failures: @@ -473,10 +474,10 @@ test test_hello ... FAILED failures: ---- test_hello stdout ---- -[..]thread '[..]' panicked at 'assertion failed: false', \ - src/lib.rs:1[..] +[..]thread '[..]' panicked at [..]src/lib.rs:1:[..] ", ) + .with_stdout_contains("[..]assertion failed[..]") .with_stdout_contains( "\ failures: 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]