mirror of
https://github.com/uutils/shadow.git
synced 2026-06-10 16:14:57 -07:00
review: address all 6 Copilot review comments
- sysroot: add try_resolve() that returns Option for user-controlled paths; resolve() uses unreachable! since only called with hardcoded paths like "/etc/passwd" - useradd: clamp subid start to at least 100_000 even when existing entries are below that threshold - grpck, pwck, userdel, usermod: use `let _ =` for harden_process() since these tools don't spawn child processes
This commit is contained in:
@@ -31,17 +31,28 @@ impl SysRoot {
|
||||
/// Resolve a path relative to the prefix.
|
||||
///
|
||||
/// Strips leading `/` from `relative` before joining with the prefix.
|
||||
#[must_use]
|
||||
pub fn resolve(&self, relative: &str) -> PathBuf {
|
||||
/// Returns `None` if the path contains `..` components (path traversal).
|
||||
pub fn try_resolve(&self, relative: &str) -> Option<PathBuf> {
|
||||
let stripped = relative.strip_prefix('/').unwrap_or(relative);
|
||||
let joined = self.prefix.join(stripped);
|
||||
// Reject path traversal: ".." components could escape the prefix.
|
||||
for component in joined.components() {
|
||||
if matches!(component, Component::ParentDir) {
|
||||
return self.prefix.clone();
|
||||
return None;
|
||||
}
|
||||
}
|
||||
joined
|
||||
Some(joined)
|
||||
}
|
||||
|
||||
/// Resolve a path relative to the prefix.
|
||||
///
|
||||
/// Strips leading `/` from `relative` before joining with the prefix.
|
||||
/// Only for hardcoded paths — use [`try_resolve`] for user-controlled input.
|
||||
#[must_use]
|
||||
pub fn resolve(&self, relative: &str) -> PathBuf {
|
||||
// All callers pass hardcoded paths like "/etc/passwd" — never ".."
|
||||
self.try_resolve(relative)
|
||||
.unwrap_or_else(|| unreachable!("resolve() called with path traversal: {relative:?}"))
|
||||
}
|
||||
|
||||
/// Path to `/etc/passwd`.
|
||||
@@ -138,13 +149,19 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_resolve_rejects_path_traversal() {
|
||||
fn test_try_resolve_rejects_path_traversal() {
|
||||
let root = SysRoot::new(Some(Path::new("/mnt/chroot")));
|
||||
// Attempting to escape the prefix via ".." returns None.
|
||||
assert_eq!(root.try_resolve("/../etc/shadow"), None);
|
||||
assert_eq!(root.try_resolve("/home/../../etc/shadow"), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_try_resolve_accepts_valid_paths() {
|
||||
let root = SysRoot::new(Some(Path::new("/mnt/chroot")));
|
||||
// Attempting to escape the prefix via ".." should return the prefix itself.
|
||||
assert_eq!(root.resolve("/../etc/shadow"), PathBuf::from("/mnt/chroot"));
|
||||
assert_eq!(
|
||||
root.resolve("/home/../../etc/shadow"),
|
||||
PathBuf::from("/mnt/chroot")
|
||||
root.try_resolve("/etc/shadow"),
|
||||
Some(PathBuf::from("/mnt/chroot/etc/shadow"))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -127,7 +127,7 @@ impl GrpckOptions {
|
||||
|
||||
#[uucore::main]
|
||||
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
|
||||
let _clean_env = shadow_core::hardening::harden_process();
|
||||
let _ = shadow_core::hardening::harden_process();
|
||||
|
||||
let matches = uu_app().try_get_matches_from(args)?;
|
||||
let opts = GrpckOptions::from_matches(&matches);
|
||||
|
||||
@@ -141,7 +141,7 @@ impl PwckOptions {
|
||||
|
||||
#[uucore::main]
|
||||
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
|
||||
let _clean_env = shadow_core::hardening::harden_process();
|
||||
let _ = shadow_core::hardening::harden_process();
|
||||
|
||||
let matches = uu_app().try_get_matches_from(args)?;
|
||||
let opts = PwckOptions::from_matches(&matches);
|
||||
|
||||
@@ -885,12 +885,13 @@ fn append_subid_entry(path: &Path, name: &str, count: u64) -> UResult<()> {
|
||||
}
|
||||
|
||||
// Find next available range by starting after the highest existing end.
|
||||
// This prevents overlapping ranges that could allow container escape.
|
||||
// Clamp to at least 100_000 even if existing entries are below that threshold.
|
||||
let start = entries
|
||||
.iter()
|
||||
.map(|e| e.start.saturating_add(e.count))
|
||||
.max()
|
||||
.unwrap_or(100_000);
|
||||
.unwrap_or(100_000)
|
||||
.max(100_000);
|
||||
|
||||
entries.push(SubIdEntry {
|
||||
name: name.to_string(),
|
||||
|
||||
@@ -72,7 +72,7 @@ impl UError for UserdelError {
|
||||
|
||||
#[uucore::main]
|
||||
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
|
||||
let _clean_env = shadow_core::hardening::harden_process();
|
||||
let _ = shadow_core::hardening::harden_process();
|
||||
|
||||
let matches = match uu_app().try_get_matches_from(args) {
|
||||
Ok(m) => m,
|
||||
|
||||
@@ -76,7 +76,7 @@ impl UError for UsermodError {
|
||||
#[uucore::main]
|
||||
#[allow(clippy::too_many_lines)]
|
||||
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
|
||||
let _clean_env = shadow_core::hardening::harden_process();
|
||||
let _ = shadow_core::hardening::harden_process();
|
||||
|
||||
let matches = match uu_app().try_get_matches_from(args) {
|
||||
Ok(m) => m,
|
||||
|
||||
Reference in New Issue
Block a user