Ignore setuid from owners the guest cannot mean - #267
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
execve took the new effective ID straight from a raw host fstat. A sysroot is an ordinary tree owned by whoever unpacked it and no host IDs are mapped into the guest, so a setuid binary left the process at an ID that exists nowhere in the guest -- neither root nor its own. Privilege checks against euid 0 failed, ownership comparisons against guest IDs failed, and the ID granted followed whoever owned the tree. Read ownership through chown_overlay_apply, the way fs-stat.c reports it, so exec and stat cannot disagree about who owns a file. Honour the bit only for root or the caller's own ID and leave the ID untouched otherwise, matching what Linux does for a setuid binary on a nosuid mount rather than failing the exec. Root stays reachable because a guest chown is recorded in the overlay, which is how an emulated-root guest marks a helper setuid-root. Fix sysprog21#264
02520c8 to
e2633d9
Compare
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test-setuid-exec.c">
<violation number="1" location="tests/test-setuid-exec.c:119">
P2: A transient `stat()` failure can leave a copied setuid executable in `/tmp` because cleanup is only reached at the normal end of `main`; the same leak occurs on helper-build errors after `mkstemp()`. Routing all post-creation failures through a cleanup path would prevent stale privileged artifacts from accumulating.</violation>
<violation number="2" location="tests/test-setuid-exec.c:133">
P2: The foreign-owner regression is not actually tested: on the QEMU matrix (and whenever the host UID equals the guest UID), this case is the caller-owned no-op path and passes even if exec incorrectly honors a foreign setuid owner. Making the helper deterministically owned by an ID other than 0 or the caller and asserting that `stat()` reports that ID would exercise the intended behavior.</violation>
</file>
<file name="src/syscall/exec.c">
<violation number="1" location="src/syscall/exec.c:718">
P0: An unprivileged guest can now gain euid 0: `chown()` records UID 0 in the overlay even outside fakeroot mode, and this new setuid check honors that overlay owner. The chown path needs to enforce the guest's chown privilege (or otherwise prevent unprivileged callers from recording UID 0) before overlay-backed root setuid is honored.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (have_exec_st && !exec_is_script && S_ISREG(exec_st.st_mode)) { | ||
| if (exec_st.st_mode & S_ISUID) { | ||
| if ((exec_st.st_mode & S_ISUID) && | ||
| exec_id_is_guest_meaningful((uint32_t) exec_st.st_uid, |
There was a problem hiding this comment.
P0: An unprivileged guest can now gain euid 0: chown() records UID 0 in the overlay even outside fakeroot mode, and this new setuid check honors that overlay owner. The chown path needs to enforce the guest's chown privilege (or otherwise prevent unprivileged callers from recording UID 0) before overlay-backed root setuid is honored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/syscall/exec.c, line 718:
<comment>An unprivileged guest can now gain euid 0: `chown()` records UID 0 in the overlay even outside fakeroot mode, and this new setuid check honors that overlay owner. The chown path needs to enforce the guest's chown privilege (or otherwise prevent unprivileged callers from recording UID 0) before overlay-backed root setuid is honored.</comment>
<file context>
@@ -681,10 +714,14 @@ int64_t sys_execve(hv_vcpu_t vcpu,
if (have_exec_st && !exec_is_script && S_ISREG(exec_st.st_mode)) {
- if (exec_st.st_mode & S_ISUID) {
+ if ((exec_st.st_mode & S_ISUID) &&
+ exec_id_is_guest_meaningful((uint32_t) exec_st.st_uid,
+ proc_get_uid())) {
new_euid = (uint32_t) exec_st.st_uid;
</file context>
| printf("test-setuid-exec: 1. build setuid helper... "); | ||
| if (build_setuid_helper() != 0) { | ||
| printf("FAIL (could not build helper: %m)\n"); | ||
| return 1; |
There was a problem hiding this comment.
P2: A transient stat() failure can leave a copied setuid executable in /tmp because cleanup is only reached at the normal end of main; the same leak occurs on helper-build errors after mkstemp(). Routing all post-creation failures through a cleanup path would prevent stale privileged artifacts from accumulating.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test-setuid-exec.c, line 119:
<comment>A transient `stat()` failure can leave a copied setuid executable in `/tmp` because cleanup is only reached at the normal end of `main`; the same leak occurs on helper-build errors after `mkstemp()`. Routing all post-creation failures through a cleanup path would prevent stale privileged artifacts from accumulating.</comment>
<file context>
@@ -0,0 +1,231 @@
+ printf("test-setuid-exec: 1. build setuid helper... ");
+ if (build_setuid_helper() != 0) {
+ printf("FAIL (could not build helper: %m)\n");
+ return 1;
+ }
+ struct stat st;
</file context>
| * real kernel the file we just created is owned by us, so the bit is a | ||
| * no-op and this only confirms the same expected euid. | ||
| */ | ||
| printf("test-setuid-exec: 2. foreign owner does not elevate... "); |
There was a problem hiding this comment.
P2: The foreign-owner regression is not actually tested: on the QEMU matrix (and whenever the host UID equals the guest UID), this case is the caller-owned no-op path and passes even if exec incorrectly honors a foreign setuid owner. Making the helper deterministically owned by an ID other than 0 or the caller and asserting that stat() reports that ID would exercise the intended behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test-setuid-exec.c, line 133:
<comment>The foreign-owner regression is not actually tested: on the QEMU matrix (and whenever the host UID equals the guest UID), this case is the caller-owned no-op path and passes even if exec incorrectly honors a foreign setuid owner. Making the helper deterministically owned by an ID other than 0 or the caller and asserting that `stat()` reports that ID would exercise the intended behavior.</comment>
<file context>
@@ -0,0 +1,231 @@
+ * real kernel the file we just created is owned by us, so the bit is a
+ * no-op and this only confirms the same expected euid.
+ */
+ printf("test-setuid-exec: 2. foreign owner does not elevate... ");
+ {
+ int rc = run_helper(self_euid);
</file context>
execve took the new effective ID straight from a raw host fstat. A sysroot is an ordinary tree owned by whoever unpacked it and no host IDs are mapped into the guest, so a setuid binary left the process at an ID that exists nowhere in the guest -- neither root nor its own. Privilege checks against euid 0 failed, ownership comparisons against guest IDs failed, and the ID granted followed whoever owned the tree.
Read ownership through chown_overlay_apply, the way fs-stat.c reports it, so exec and stat cannot disagree about who owns a file. Honour the bit only for root or the caller's own ID and leave the ID untouched otherwise, matching what Linux does for a setuid binary on a nosuid mount rather than failing the exec.
Root stays reachable because a guest chown is recorded in the overlay, which is how an emulated-root guest marks a helper setuid-root.
Fix #264
Summary by cubic
Fixes setuid/setgid on exec so the guest never switches to host-only IDs. Exec and interpreter paths now read ownership via the overlay and only elevate to IDs the guest can mean, preventing broken privilege checks.
chown_overlay_applyto both the main binary and shebang interpreter so exec and stat agree on ownership.test-setuid-execand wire it intotests/test-matrix.shto verify ignoring foreign owners, honoring overlay-backed root, and running owner-only executables the guest owns.Written for commit e2633d9. Summary will update on new commits.