officerdev/platform
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

severing ownership does not sever the ACL, and neither the spec nor the checker said so

Browse Source 
Reviewing 46799dad against a real tree: severMemberTree reassigns ownership and leaves the
access-control entries behind. confineUserTree grants each member a named ACL on their whole
tree — u:<uid>:rwx plus a default: copy — and chown does not remove them. They are xattrs
rather than ownership, and they store the uid NUMERICALLY.

Measured: chown -h -R to the service user leaves user:<uid>:rwx intact on the directory, its
children and their defaults. So a preserved tree owned by the platform still grants the freed
uid read and write on every byte, and the next account allocated that number inherits the
previous member's home, SSH keys, credentials, transcripts and container storage. That is the
hazard the function exists to prevent, reached through ACLs instead of ownership.

This was my omission as much as the implementation's: the spec said "sever the data from the
uid" and specified only chown, and assert-uid-free.sh checked find -uid, which reads ownership
and cannot see an ACL. Both are fixed here — the spec now requires setfacl -R -b alongside the
chown, and the checker scans DATA_PATH with getfacl -R -n for entries naming the freed uid.

The checker was verified to catch it: run against green's live tree it now reports
"ACL entries still grant uid 1001 (user:1001:rwx)", which it did not before.

The code fix is one line in severMemberTree and is not mine to make.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
pastilhas
2026-08-12 04:24:31 +00:00
co-authored by Claude Opus 5
parent 46799dada8
commit a2f63dc534
2 changed files with 35 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
scripts/assert-uid-free.sh
+13
View File
@@ -60,6 +60,19 @@ owned="$(find "${SEARCH_ROOTS[@]}" -uid "$uid" -print -quit 2>/dev/null)"
mapped="$(find "${SEARCH_ROOTS[@]}" -uid +"$((sub_start-1))" ! -uid +"$sub_end" -print -quit 2>/dev/null)"
[[ -z "$mapped" ]] && ok "no files in the freed subuid range" || bad "files owned by the freed subuid range (e.g. $mapped)"
# ACL entries, which ownership checks cannot see. `confineUserTree` grants the member a NAMED entry on their
# whole tree — `u:<uid>:rwx` plus a `default:` copy — and `chown` does not remove them: they are xattrs, not
# ownership, and they store the uid NUMERICALLY. So a tree reassigned to the service user can still carry
# `user:1001:rwx` on every file, and the next account allocated 1001 inherits read/write on all of it.
#
# `-n` forces numeric output; after `userdel` the uid has no name to resolve to, and relying on the name
# would make this check depend on the very passwd entry that is supposed to be gone.
#
# Scoped to DATA_PATH: member trees live there, and a recursive getfacl over /home would walk the owner's
# entire account for no gain.
acl_hit="$(getfacl -R -n -p "$DATA_PATH" 2>/dev/null | grep -m1 -E "^(default:)?user:$uid:")"
[[ -z "$acl_hit" ]] && ok "no ACL entries naming uid $uid" || bad "ACL entries still grant uid $uid ($acl_hit)"
echo
if [[ "$fails" -eq 0 ]]; then
echo "CLEAN — uid $uid and its subuid range are safe to reissue"