useradd, skel: set mkdir mode atomically to close world-writable window (#157) - #158
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #157 by eliminating umask-influenced permission windows during home directory and skeleton file creation, using atomic mode-setting at mkdir(2) / open(2) time plus an RAII umask guard.
Changes:
useradd: create home directories viaDirBuilder::mode(0o700)underUmaskGuard::zero()and remove post-creation chmod.shadow-core/skel: copy skeleton dirs/files usingDirBuilder::mode(...)andOpenOptions::mode(...).create_new(true)underUmaskGuard::zero(), avoidingstd::fs::copy.shadow-core/atomic: exposeUmaskGuardpublicly for reuse by other crates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/uu/useradd/src/useradd.rs | Uses atomic mkdir mode + UmaskGuard for home creation; adds a regression test. |
| src/shadow-core/src/skel.rs | Makes skeleton copy set modes at creation time (dirs/files) under UmaskGuard. |
| src/shadow-core/src/atomic.rs | Makes UmaskGuard public and documents !Send behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
defff87 to
aa98a1c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aa98a1c to
af0e590
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ow (#157) std::fs::create_dir(path) calls mkdir(path, 0o777) and the actual mode is 0o777 & ~umask. With an attacker-controlled umask (inherited across setuid since the kernel does not reset it), the directory exists with permissive bits between mkdir and the subsequent set_permissions call. Fix uses DirBuilder::mode(...).create(...) so the mode is set in the syscall itself, wrapped in UmaskGuard::zero() to neutralize umask interference. Same pattern applied to skel.rs for subdirectory and file copies (file copies switch from fs::copy to OpenOptions::mode + io::copy for the same reason). Regression test asserts a process running with umask 0 still produces a 0o700 home directory. Reported by @collinfunk in uutils/coreutils#11828. Fixes #157.
af0e590 to
8b08f73
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fix #157. Reported by @collinfunk in uutils/coreutils#11828.
std::fs::create_dir(path)callsmkdir(path, 0o777)and the actual mode is0o777 & ~umask. With an attacker-controlled umask (inherited across setuid since the kernel does not reset it), the directory exists with permissive bits betweenmkdirand the subsequentset_permissionscall.Changes
useradd.rs—create_home_directorynow grabsUmaskGuard::zero()then usesDirBuilder::new().mode(0o700).create(...). Post-mkdirset_permissionsremoved.skel.rs— top-levelUmaskGuard::zero()for the entire copy. Subdirectories useDirBuilder::mode(src_mode).create(...). Files opened withOpenOptions::new().create_new(true).mode(src_mode).open(...)+io::copyinstead ofstd::fs::copy(which goes through umask internally).atomic.rs—UmaskGuardandUmaskGuard::zero()madepubso consumers in other crates can reuse the existing RAII guard.Test plan
test_integration_home_directory_ignores_inherited_umask(root-only): sets process umask to0o000, callscreate_home_directory, asserts the resulting directory mode is exactly0o700. Passes.cargo fmt --all --check— cleancargo clippy --workspace --all-targets -- -D warnings— zero warningscargo test --workspace— all pass