From c60c06585cdf4502260316bc012f06ba87269d0a Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Fri, 7 Jul 2023 18:18:39 +0100 Subject: [PATCH] fix: clear cache for old `.cargo-ok` format 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. --- src/cargo/sources/registry/mod.rs | 43 +++++++++++++---- tests/testsuite/registry.rs | 77 +++++++++++++++++++++++++++++-- 2 files changed, 108 insertions(+), 12 deletions(-) diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 97351625ff8..a0178db55e8 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -184,6 +184,7 @@ //! [`IndexPackage`]: index::IndexPackage use std::collections::HashSet; +use std::fs; use std::fs::{File, OpenOptions}; use std::io; use std::io::Read; @@ -196,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; @@ -219,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 @@ -546,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 @@ -567,15 +582,23 @@ 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 = { @@ -639,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()) } diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index daa96d5a842..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] @@ -3458,3 +3458,74 @@ fn set_mask_during_unpacking() { 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}"#); +}