Auto merge of #12442 - pietroalbini:pa-cve-2023-38497-beta, r=weihanglo

Fix CVE-2023-38497 for beta 1.72.0

Changes have been made by `@weihanglo` and reviewed by `@ehuss` in a private repository.

Author: bors

Cargo.lock

@@ -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"] }

Cargo.toml

@@ -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::<LockMetadata>(&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<R: Read>(tar: &mut Archive<R>) {
+    #[cfg(unix)]
+    tar.set_mask(crate::util::get_umask());
+}

src/cargo/sources/registry/mod.rs

@@ -207,6 +207,24 @@ pub fn try_canonicalize<P: AsRef<Path>>(path: P) -> std::io::Result<PathBuf> {
     })
 }
 
+/// 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<libc::mode_t> = 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::*;

src/cargo/util/mod.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[..]")

tests/testsuite/bench.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();
 }
 

tests/testsuite/install.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}"#);
+}

tests/testsuite/registry.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:

tests/testsuite/test.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]