Skip to content

Commit

Permalink
fix: respect umask when unpacking .crate files
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
weihanglo committed Jul 29, 2023
1 parent ac6f044 commit f315a70
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 10 deletions.
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
Expand Down
21 changes: 19 additions & 2 deletions src/cargo/sources/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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());
}
18 changes: 18 additions & 0 deletions src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down
6 changes: 3 additions & 3 deletions tests/testsuite/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
5 changes: 3 additions & 2 deletions tests/testsuite/vendor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit f315a70

Please sign in to comment.