From f909434f805893b6f5628534ebd13b7c25d045cf Mon Sep 17 00:00:00 2001
From: Weihang Lo <me@weihanglo.tw>
Date: Fri, 7 Jul 2023 14:50:59 +0100
Subject: [PATCH 2/3] fix: respect `umask` when unpacking `.crate` files

Without this, an attacker can upload globally writable files buried
in the `.crate` file. After a user downloaded and unpacked the file,
the attacker can then write malicous code to the downloaded sources.
---
 Cargo.lock                                    |  4 ++--
 src/tools/cargo/Cargo.toml                    |  2 +-
 .../cargo/src/cargo/sources/registry/mod.rs   | 21 +++++++++++++++++--
 src/tools/cargo/src/cargo/util/mod.rs         | 18 ++++++++++++++++
 src/tools/cargo/tests/testsuite/registry.rs   |  6 +++---
 src/tools/cargo/tests/testsuite/vendor.rs     |  5 +++--
 6 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 8b0972494..cd7052f3b 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -4876,9 +4876,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/src/tools/cargo/Cargo.toml b/src/tools/cargo/Cargo.toml
index 3c6597b5c..6442bb1c8 100644
--- a/src/tools/cargo/Cargo.toml
+++ b/src/tools/cargo/Cargo.toml
@@ -80,7 +80,7 @@ sha2 = "0.10.6"
 shell-escape = "0.1.4"
 snapbox = { version = "0.4.0", features = ["diff", "path"] }
 strip-ansi-escapes = "0.1.0"
-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/tools/cargo/src/cargo/sources/registry/mod.rs b/src/tools/cargo/src/cargo/sources/registry/mod.rs
index ee27df15d..4e4d794e0 100644
--- a/src/tools/cargo/src/cargo/sources/registry/mod.rs
+++ b/src/tools/cargo/src/cargo/sources/registry/mod.rs
@@ -162,7 +162,9 @@ use std::borrow::Cow;
 use std::collections::BTreeMap;
 use std::collections::HashSet;
 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};
 
@@ -698,7 +700,9 @@ impl<'cfg> RegistrySource<'cfg> {
             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();
@@ -1036,3 +1040,16 @@ mod tests {
         assert_eq!(make_dep_prefix("aBcDe"), "aB/cD");
     }
 }
+
+/// 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());
+}
diff --git a/src/tools/cargo/src/cargo/util/mod.rs b/src/tools/cargo/src/cargo/util/mod.rs
index e72f8183b..df8dcb0ac 100644
--- a/src/tools/cargo/src/cargo/util/mod.rs
+++ b/src/tools/cargo/src/cargo/util/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::*;
diff --git a/src/tools/cargo/tests/testsuite/registry.rs b/src/tools/cargo/tests/testsuite/registry.rs
index 42107a5e4..daa96d5a8 100644
--- a/src/tools/cargo/tests/testsuite/registry.rs
+++ b/src/tools/cargo/tests/testsuite/registry.rs
@@ -3452,9 +3452,9 @@ fn set_mask_during_unpacking() {
         .unwrap()
     };
 
-    // Assuming umask is `0o022`.
+    let umask = cargo::util::get_umask();
     let metadata = fs::metadata(src_file_path("src/lib.rs")).unwrap();
-    assert_eq!(metadata.mode() & 0o777, 0o666);
+    assert_eq!(metadata.mode() & 0o777, 0o666 & !umask);
     let metadata = fs::metadata(src_file_path("example.sh")).unwrap();
-    assert_eq!(metadata.mode() & 0o777, 0o777);
+    assert_eq!(metadata.mode() & 0o777, 0o777 & !umask);
 }
diff --git a/src/tools/cargo/tests/testsuite/vendor.rs b/src/tools/cargo/tests/testsuite/vendor.rs
index 21a1c097c..2b8b090c2 100644
--- a/src/tools/cargo/tests/testsuite/vendor.rs
+++ b/src/tools/cargo/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]
-- 
2.34.1

