From 76cd7c20bf0a6c9bd9e726b09660a1a8efb3be5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Wed, 12 Aug 2026 04:28:59 +0000 Subject: [PATCH] sever the ACL as well as the ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit severMemberTree reassigned the tree and left the access-control entries behind. confineUserTree grants each member a named ACL on their whole tree — u::rwx plus a default: copy — and chown does not remove them: they are xattrs rather than ownership, and they record the uid numerically. Measured before this change: after chown -h -R to the service user, user::rwx was still present on the directory, on its children and in their defaults. The tree read as the platform's while still granting the freed uid read and write on every byte, so the next account allocated that number would inherit the previous member's home, keys, credential and container storage — the hazard this file exists to prevent, reached through a door that find -uid cannot see. Now chown then setfacl -R -P -b. Proven on a scratch tree: owner 1001 with five entries naming 1001 becomes owner 1000 with none. -b rather than removing the member's entries alone, because the service user owns all of it afterwards and "no ACLs" is cheaper to verify than "no ACL naming one id". -P is already the default for a recursive setfacl — verified, a symlink out of the tree was not followed — and is stated for the same reason the chown above carries -h: a member chooses what their symlinks point at, and this argv should not rest on a traversal default holding. Found by running assert-uid-free.sh against a real tree; the spec and the checker had the same blind spot and were corrected in a2f63dc5. Co-Authored-By: Claude Opus 5 --- src/servers/os-user-deprovision.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/servers/os-user-deprovision.ts b/src/servers/os-user-deprovision.ts index b4f61ca5..e9bda318 100644 --- a/src/servers/os-user-deprovision.ts +++ b/src/servers/os-user-deprovision.ts @@ -345,6 +345,30 @@ async function severMemberTree(params: { const chowned = await sudo(['chown', '-h', '-R', `${who}:${who}`, params.tree]); if (!chowned.ok) return { ok: false, error: `could not reassign ${params.tree} to ${who}: ${chowned.out}` }; + // ── Ownership is not the only link to the uid ── + // + // `confineUserTree` grants the member a NAMED ACL entry on their whole tree — `u::rwx` plus a + // `default:` copy inherited by everything either party creates afterwards. `chown` does not remove those: + // they are xattrs rather than ownership, and they record the uid NUMERICALLY. + // + // Measured on this host: after `chown -h -R` to the service user, `user::rwx` was still present on the + // directory, on its children, and in their defaults. So the tree reads as the platform's while still + // granting the freed uid read and write on every byte of it — and the next account allocated that number + // inherits the previous member's home, keys, credential and container storage. Severing ownership without + // severing this just moves the hazard somewhere ownership checks cannot see it, including `find -uid`. + // + // `-b` removes every ACL rather than the member's entries alone. The service user owns all of it now, so a + // named entry granting themselves what ownership already grants is redundant, and "no ACLs" is a much + // cheaper thing to verify than "no ACL naming one particular id". + // + // `-P` is the physical walk. It is already the default for `setfacl -R` — verified here, a symlink out of + // the tree was not followed — but it is stated for the same reason `chown` above carries `-h`: a member + // chooses what their symlinks point at, and this argv should not depend on a traversal default holding. + const stripped = await sudo(['setfacl', '-R', '-P', '-b', params.tree]); + if (!stripped.ok) { + return { ok: false, error: `reassigned ${params.tree} but could not clear its ACLs: ${stripped.out}` }; + } + if (params.policy === 'preserve') return { ok: true }; try {