From 97b80919e404b0768ea31ae329c3b4da54bed05a Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Thu, 18 Aug 2022 17:17:19 +0200 Subject: [PATCH 1/5] CVE-2022-36113: avoid unpacking .cargo-ok from the crate --- src/cargo/sources/registry/mod.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index c17b822fd0f..a2863bf78a1 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -639,6 +639,13 @@ impl<'cfg> RegistrySource<'cfg> { prefix ) } + // Prevent unpacking the lockfile from the crate itself. + if entry_path + .file_name() + .map_or(false, |p| p == PACKAGE_SOURCE_LOCK) + { + continue; + } // Unpacking failed let mut result = entry.unpack_in(parent).map_err(anyhow::Error::from); if cfg!(windows) && restricted_names::is_windows_reserved_path(&entry_path) { @@ -654,16 +661,14 @@ impl<'cfg> RegistrySource<'cfg> { .with_context(|| format!("failed to unpack entry at `{}`", entry_path.display()))?; } - // The lock file is created after unpacking so we overwrite a lock file - // which may have been extracted from the package. + // Now that we've finished unpacking, create and write to the lock file to indicate that + // unpacking was successful. let mut ok = OpenOptions::new() - .create(true) + .create_new(true) .read(true) .write(true) .open(&path) .with_context(|| format!("failed to open `{}`", path.display()))?; - - // Write to the lock file to indicate that unpacking was successful. write!(ok, "ok")?; Ok(unpack_dir.to_path_buf()) From dafe4a7ea016739680ec7998aebe1bc6de131a5b Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Fri, 26 Aug 2022 01:12:25 +0100 Subject: [PATCH 2/5] CVE-2022-36113: add tests --- crates/cargo-test-support/src/registry.rs | 40 ++++++++++++++++++---- tests/testsuite/registry.rs | 41 +++++++++++++++++++++++ 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index 5a0a4c58280..667e3edd737 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -403,10 +403,17 @@ pub struct Dependency { optional: bool, } +/// Entry with data that corresponds to [`tar::EntryType`]. +#[non_exhaustive] +enum EntryData { + Regular(String), + Symlink(PathBuf), +} + /// A file to be created in a package. struct PackageFile { path: String, - contents: String, + contents: EntryData, /// The Unix mode for the file. Note that when extracted on Windows, this /// is mostly ignored since it doesn't have the same style of permissions. mode: u32, @@ -780,13 +787,24 @@ impl Package { pub fn file_with_mode(&mut self, path: &str, mode: u32, contents: &str) -> &mut Package { self.files.push(PackageFile { path: path.to_string(), - contents: contents.to_string(), + contents: EntryData::Regular(contents.into()), mode, extra: false, }); self } + /// Adds a symlink to a path to the package. + pub fn symlink(&mut self, dst: &str, src: &str) -> &mut Package { + self.files.push(PackageFile { + path: dst.to_string(), + contents: EntryData::Symlink(src.into()), + mode: DEFAULT_MODE, + extra: false, + }); + self + } + /// Adds an "extra" file that is not rooted within the package. /// /// Normal files are automatically placed within a directory named @@ -795,7 +813,7 @@ impl Package { pub fn extra_file(&mut self, path: &str, contents: &str) -> &mut Package { self.files.push(PackageFile { path: path.to_string(), - contents: contents.to_string(), + contents: EntryData::Regular(contents.to_string()), mode: DEFAULT_MODE, extra: true, }); @@ -1033,7 +1051,7 @@ impl Package { self.append_manifest(&mut a); } if self.files.is_empty() { - self.append(&mut a, "src/lib.rs", DEFAULT_MODE, ""); + self.append(&mut a, "src/lib.rs", DEFAULT_MODE, &EntryData::Regular("".into())); } else { for PackageFile { path, @@ -1107,10 +1125,10 @@ impl Package { manifest.push_str("[lib]\nproc-macro = true\n"); } - self.append(ar, "Cargo.toml", DEFAULT_MODE, &manifest); + self.append(ar, "Cargo.toml", DEFAULT_MODE, &EntryData::Regular(manifest.into())); } - fn append(&self, ar: &mut Builder, file: &str, mode: u32, contents: &str) { + fn append(&self, ar: &mut Builder, file: &str, mode: u32, contents: &EntryData) { self.append_raw( ar, &format!("{}-{}/{}", self.name, self.vers, file), @@ -1119,8 +1137,16 @@ impl Package { ); } - fn append_raw(&self, ar: &mut Builder, path: &str, mode: u32, contents: &str) { + fn append_raw(&self, ar: &mut Builder, path: &str, mode: u32, contents: &EntryData) { let mut header = Header::new_ustar(); + let contents = match contents { + EntryData::Regular(contents) => contents.as_str(), + EntryData::Symlink(src) => { + header.set_entry_type(tar::EntryType::Symlink); + t!(header.set_link_name(src)); + "" // Symlink has no contents. + } + }; header.set_size(contents.len() as u64); t!(header.set_path(path)); header.set_mode(mode); diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 7dbae00765f..4fcc4ffbec1 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -2583,6 +2583,47 @@ fn package_lock_inside_package_is_overwritten() { assert_eq!(ok.metadata().unwrap().len(), 2); } +#[cargo_test] +fn package_lock_as_a_symlink_inside_package_is_overwritten() { + let registry = registry::init(); + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + bar = ">= 0.0.0" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + Package::new("bar", "0.0.1") + .file("src/lib.rs", "pub fn f() {}") + .symlink(".cargo-ok", "src/lib.rs") + .publish(); + + p.cargo("build").run(); + + let id = SourceId::for_registry(registry.index_url()).unwrap(); + let hash = cargo::util::hex::short_hash(&id); + let pkg_root = cargo_home() + .join("registry") + .join("src") + .join(format!("-{}", hash)) + .join("bar-0.0.1"); + let ok = pkg_root.join(".cargo-ok"); + 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!(fs::read_to_string(librs).unwrap(), "pub fn f() {}"); +} + #[cargo_test] fn ignores_unknown_index_version_http() { let _server = setup_http(); From d1f9553c825f6d7481453be8d58d0e7f117988a7 Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Thu, 18 Aug 2022 17:45:45 +0200 Subject: [PATCH 3/5] CVE-2022-36114: limit the maximum unpacked size of a crate to 512MB This gives users of custom registries the same protections, using the same size limit that crates.io uses. `LimitErrorReader` code copied from crates.io. --- src/cargo/sources/registry/mod.rs | 6 +++++- src/cargo/util/io.rs | 27 +++++++++++++++++++++++++++ src/cargo/util/mod.rs | 2 ++ 3 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 src/cargo/util/io.rs diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index a2863bf78a1..c9c414e5005 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -182,7 +182,9 @@ use crate::util::hex; use crate::util::interning::InternedString; use crate::util::into_url::IntoUrl; use crate::util::network::PollExt; -use crate::util::{restricted_names, CargoResult, Config, Filesystem, OptVersionReq}; +use crate::util::{ + restricted_names, CargoResult, Config, Filesystem, LimitErrorReader, OptVersionReq, +}; const PACKAGE_SOURCE_LOCK: &str = ".cargo-ok"; pub const CRATES_IO_INDEX: &str = "https://github.com/rust-lang/crates.io-index"; @@ -194,6 +196,7 @@ const VERSION_TEMPLATE: &str = "{version}"; const PREFIX_TEMPLATE: &str = "{prefix}"; const LOWER_PREFIX_TEMPLATE: &str = "{lowerprefix}"; const CHECKSUM_TEMPLATE: &str = "{sha256-checksum}"; +const MAX_UNPACK_SIZE: u64 = 512 * 1024 * 1024; /// A "source" for a local (see `local::LocalRegistry`) or remote (see /// `remote::RemoteRegistry`) registry. @@ -615,6 +618,7 @@ impl<'cfg> RegistrySource<'cfg> { } } let gz = GzDecoder::new(tarball); + let gz = LimitErrorReader::new(gz, MAX_UNPACK_SIZE); let mut tar = Archive::new(gz); let prefix = unpack_dir.file_name().unwrap(); let parent = unpack_dir.parent().unwrap(); diff --git a/src/cargo/util/io.rs b/src/cargo/util/io.rs new file mode 100644 index 00000000000..f62672db035 --- /dev/null +++ b/src/cargo/util/io.rs @@ -0,0 +1,27 @@ +use std::io::{self, Read, Take}; + +#[derive(Debug)] +pub struct LimitErrorReader { + inner: Take, +} + +impl LimitErrorReader { + pub fn new(r: R, limit: u64) -> LimitErrorReader { + LimitErrorReader { + inner: r.take(limit), + } + } +} + +impl Read for LimitErrorReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + match self.inner.read(buf) { + Ok(0) if self.inner.limit() == 0 => Err(io::Error::new( + io::ErrorKind::Other, + "maximum limit reached when reading", + )), + e => e, + } + } +} + diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 28f685c209a..47bbf37aad9 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -14,6 +14,7 @@ pub use self::hasher::StableHasher; pub use self::hex::{hash_u64, short_hash, to_hex}; pub use self::into_url::IntoUrl; pub use self::into_url_with_base::IntoUrlWithBase; +pub(crate) use self::io::LimitErrorReader; pub use self::lev_distance::{closest, closest_msg, lev_distance}; pub use self::lockserver::{LockServer, LockServerClient, LockServerStarted}; pub use self::progress::{Progress, ProgressStyle}; @@ -44,6 +45,7 @@ pub mod important_paths; pub mod interning; pub mod into_url; mod into_url_with_base; +mod io; pub mod job; pub mod lev_distance; mod lockserver; From d87d57dbbda61754f4fab0f329a7ac520e062c46 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sun, 21 Aug 2022 11:21:41 +0100 Subject: [PATCH 4/5] CVE-2022-36114: add tests --- src/cargo/sources/registry/mod.rs | 16 +++++++++++- src/cargo/util/io.rs | 24 ++++++++++++++++++ tests/testsuite/registry.rs | 42 +++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index c9c414e5005..0c52d530ec8 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -618,7 +618,7 @@ impl<'cfg> RegistrySource<'cfg> { } } let gz = GzDecoder::new(tarball); - let gz = LimitErrorReader::new(gz, MAX_UNPACK_SIZE); + let gz = LimitErrorReader::new(gz, max_unpack_size()); let mut tar = Archive::new(gz); let prefix = unpack_dir.file_name().unwrap(); let parent = unpack_dir.parent().unwrap(); @@ -835,6 +835,20 @@ impl<'cfg> Source for RegistrySource<'cfg> { } } +/// For integration test only. +#[inline] +fn max_unpack_size() -> u64 { + const VAR: &str = "__CARGO_TEST_MAX_UNPACK_SIZE"; + if cfg!(debug_assertions) && std::env::var(VAR).is_ok() { + std::env::var(VAR) + .unwrap() + .parse() + .expect("a max unpack size in bytes") + } else { + MAX_UNPACK_SIZE + } +} + fn make_dep_prefix(name: &str) -> String { match name.len() { 1 => String::from("1"), diff --git a/src/cargo/util/io.rs b/src/cargo/util/io.rs index f62672db035..60f3ffe0560 100644 --- a/src/cargo/util/io.rs +++ b/src/cargo/util/io.rs @@ -25,3 +25,27 @@ impl Read for LimitErrorReader { } } +#[cfg(test)] +mod tests { + use super::LimitErrorReader; + + use std::io::Read; + + #[test] + fn under_the_limit() { + let buf = &[1; 7][..]; + let mut r = LimitErrorReader::new(buf, 8); + let mut out = Vec::new(); + assert!(matches!(r.read_to_end(&mut out), Ok(7))); + assert_eq!(buf, out.as_slice()); + } + + #[test] + #[should_panic = "maximum limit reached when reading"] + fn over_the_limit() { + let buf = &[1; 8][..]; + let mut r = LimitErrorReader::new(buf, 8); + let mut out = Vec::new(); + r.read_to_end(&mut out).unwrap(); + } +} diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 4fcc4ffbec1..a5032d2ac79 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -2697,3 +2697,45 @@ fn http_requires_trailing_slash() { .with_stderr("[ERROR] registry url must end in a slash `/`: sparse+https://index.crates.io") .run() } + +#[cargo_test] +fn reach_max_unpack_size() { + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + + [dependencies] + bar = ">= 0.0.0" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + Package::new("bar", "0.0.1").publish(); + + p.cargo("build") + .env("__CARGO_TEST_MAX_UNPACK_SIZE", "8") // hit 8 bytes limit and boom! + .with_status(101) + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 (registry `dummy-registry`) +[ERROR] failed to download replaced source registry `crates-io` + +Caused by: + failed to unpack package `bar v0.0.1 (registry `dummy-registry`)` + +Caused by: + failed to iterate over archive + +Caused by: + maximum limit reached when reading +", + ) + .run(); +} From 0bf436d0d2ca03dbe8a6a3636f7415750f78a2d7 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 14 Sep 2022 16:14:11 +0200 Subject: [PATCH 5/5] fix formatting --- crates/cargo-test-support/src/registry.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index 667e3edd737..14ea8275333 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -1051,7 +1051,12 @@ impl Package { self.append_manifest(&mut a); } if self.files.is_empty() { - self.append(&mut a, "src/lib.rs", DEFAULT_MODE, &EntryData::Regular("".into())); + self.append( + &mut a, + "src/lib.rs", + DEFAULT_MODE, + &EntryData::Regular("".into()), + ); } else { for PackageFile { path, @@ -1125,7 +1130,12 @@ impl Package { manifest.push_str("[lib]\nproc-macro = true\n"); } - self.append(ar, "Cargo.toml", DEFAULT_MODE, &EntryData::Regular(manifest.into())); + self.append( + ar, + "Cargo.toml", + DEFAULT_MODE, + &EntryData::Regular(manifest.into()), + ); } fn append(&self, ar: &mut Builder, file: &str, mode: u32, contents: &EntryData) { @@ -1137,7 +1147,13 @@ impl Package { ); } - fn append_raw(&self, ar: &mut Builder, path: &str, mode: u32, contents: &EntryData) { + fn append_raw( + &self, + ar: &mut Builder, + path: &str, + mode: u32, + contents: &EntryData, + ) { let mut header = Header::new_ustar(); let contents = match contents { EntryData::Regular(contents) => contents.as_str(),