Repository navigation
fix: preserve special file ownership from packages when cutting - #338
chris-bainbridge wants to merge 1 commit into
Conversation
Files extracted from packages were always created with the ownership of the running process (root:root in build environments), discarding the uid/gid recorded in the package tarball. This breaks any file relying on non-root ownership for its function: most notably the PAM chkpwd helpers (/usr/sbin/unix_chkpwd and /usr/sbin/pam_extrausers_chkpwd) ship setgid shadow so they can read the root:shadow, mode 0640 shadow files on behalf of an unprivileged PAM caller -- cut with chisel they end up setgid root, which grants no such access, so every password check performed by a non-root caller (e.g. screen lock/unlock in a confined desktop session) fails. The same flattening silently affects chage, expiry (shadow), crontab (crontab), plocate (plocate) and ssh-agent (_ssh). Extend fsutil.CreateOptions with optional Uid/Gid, applied with os.Chown to newly created entries (and pre-existing ones when OverrideMode is set, mirroring the mode semantics). Ownership is never applied to symlinks, and hard links inherit it from their target's inode. Unprivileged runs that cannot chown log a single warning and keep the previous behavior. tarball.Extract now carries the tar header uid/gid through for entries and their parent directories, but only requests ownership that differs from root:root: root-owned entries already end up root-owned when cutting as root, and cannot be chowned when cutting as a regular user, so the common case keeps its previous behavior exactly. Verified cutting the real libpam-modules-bin_auth slice from chisel-ubuntu-core (26.04): the chkpwd helpers now come out root:shadow 2755, matching a plain dpkg -x of the same package.
upils
left a comment
upils
left a comment
There was a problem hiding this comment.
Thanks @chris-bainbridge.
I think it addresses a real gap in Chisel. As stated in the comments below I think the approach could be refined to avoid tricky bugs but this is going in the right direction.
Even if it seems to fix Chisel to "do the right thing" I will think about the consequences of this difference in behavior.
| Root: opts.TargetDir, | ||
| Path: links[0].path, | ||
| Mode: tarHeader.FileInfo().Mode(), | ||
| Data: tarReader, |
There was a problem hiding this comment.
The proper UID/GID must also be given here. We might be in a situation where the path associated to the target file in the tarball was not extracted (because it was not requested by the selected slices) so we will extract its content to the first entry in the links list, as a regular file. And in that case we must respect the UID/GID.
| // Uid and Gid, if not nil, are applied to the created entry with | ||
| // os.Chown, so the entry faithfully reflects the ownership recorded | ||
| // in the source package. A nil value leaves the corresponding id | ||
| // unchanged. Ownership is applied to newly created entries, and to | ||
| // pre-existing ones only when OverrideMode is true. It is never | ||
| // applied to symlinks (their ownership has no meaningful effect), | ||
| // nor to hard links (they share the inode, and therefore the | ||
| // ownership, of their already-created target). Applying ownership | ||
| // requires sufficient privilege (e.g. running as root); otherwise a | ||
| // warning is logged once and the entry keeps the ownership of the | ||
| // current process user and group. |
There was a problem hiding this comment.
This is a summary of all the implementation. This is documenting pieces of behavior from multiple functions below. This may become misleading if/when the implementation changes but this comment is not updated. I know this is somehow consistent with other comments in this struct but it pushes it a bit far I think.
| if existedErr != nil { | ||
| if err := applyOwnership(path, o); err != nil { | ||
| file.Close() | ||
| return nil, nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
There is an issue doing that here. Because applyOwnership does a chown it strips the setgid. So what is recorded in the entry below may not reflect the actual mode of the file. In the other places this is fine "by chance" because a call to chmod is putting back the expected mode but not here. This is going unnoticed here as CreateWriter is only used to write the manifest but is it conceptually wrong.
Suggestions to fix:
- In
applyOwnership, after a successful chown, re-chmod regular files too.Mode(restoring any setuid/setgid), or - Centralize: single
applyOwnershipcall inCreateplaced before the existingOverrideModechmod, and the same re-chmod inCreateWriter.
| // before the entry for the file itself. This is the case for .deb files but | ||
| // not for all tarballs. | ||
| tarDirMode := make(map[string]fs.FileMode) | ||
| tarDirs := make(map[string]tarDirInfo) |
There was a problem hiding this comment.
| tarDirs := make(map[string]tarDirInfo) | |
| tarDirInfos := make(map[string]tarDirInfo) |
?
| return int(stat.Uid), int(stat.Gid) | ||
| } | ||
|
|
||
| func (s *S) TestCreateOwnership(c *C) { |
There was a problem hiding this comment.
A table-driven test would probably be easier to follow, avoid unwanted side effect of a previous case and better fit the project convention.
Problem
Files extracted by
chisel cutare always created with the ownership of the running process (root:rootin build environments), discarding the uid/gid recorded in the package tarball. This silently breaks any file that relies on non-root ownership for its function.Most notable victim: the PAM chkpwd helpers
/usr/sbin/unix_chkpwdand/usr/sbin/pam_extrausers_chkpwd. These ship setgid shadow (mode 2755) so they can read theroot:shadow, mode 0640 shadow files on behalf of an unprivileged PAM caller (the standard Debian/Ubuntu privilege-separation design). Cut with chisel they end up setgid root, which grants no such access, so every password check performed by a non-root caller fails — e.g. screen lock/unlock in a confined desktop session on Ubuntu Core, where the session snap's PAM stack execs the helpers from the base snap. The same flattening affectschageandexpiry(shadow),crontab(crontab),plocate(plocate) andssh-agent(_ssh).The mode (including the setgid bit) is already preserved; only the uid/gid are dropped, in
internal/tarball/extract.go, which buildsfsutil.CreateOptionsfrom the tar header without carrying the header'sUid/Gidover —internal/fsutilhas no way to express them at all.Fix
internal/fsutil:CreateOptionsgains optionalUid/Gid, applied withos.Chownto newly created entries, and to pre-existing ones whenOverrideModeis set (mirroring the existing mode semantics). Ownership is never applied to symlinks; hard links inherit it from their target's inode. Unprivileged runs that cannot chown log a single warning and keep the previous behavior.internal/tarball: the tar header's uid/gid is carried through for entries and their parent directories — but only requested when it differs from root:root. Root-owned entries already end up root-owned when cutting as root, and cannot be chowned when cutting as a regular user, so the overwhelmingly common case keeps its previous behavior exactly (no extra syscalls, no behavior change for chiseled container images).Slice definitions are unchanged; the ownership comes from the packages themselves, so cuts stay deterministic.
Verification
internal/fsutil(ownership applied/withheld per the mode semantics, symlink and hard link handling,CreateWriter, unprivileged EPERM warning) andinternal/tarball(a setgid, gid-42 entry modelingunix_chkpwd; asserting preservation as root and the warning fallback as a regular user). Full suite passes both as root and as a regular user (17/17 packages).libpam-modules-bin_authslice from chisel-ubuntu-core (26.04) as root now producesdpkg -xof the same package (previouslyroot:root).