From 3cfa509869998edb85915d5c5c1cfd18ee0dbbea Mon Sep 17 00:00:00 2001 From: Chris Bainbridge Date: Wed, 14 Oct 2026 10:06:14 +0100 Subject: [PATCH] fix: preserve special file ownership from packages when cutting 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. --- internal/fsutil/create.go | 68 ++++++++++++++- internal/fsutil/create_test.go | 138 +++++++++++++++++++++++++++++++ internal/tarball/extract.go | 49 +++++++++-- internal/tarball/extract_test.go | 89 ++++++++++++++++++++ 4 files changed, 334 insertions(+), 10 deletions(-) diff --git a/internal/fsutil/create.go b/internal/fsutil/create.go index 0503c96f1..2db479354 100644 --- a/internal/fsutil/create.go +++ b/internal/fsutil/create.go @@ -3,6 +3,7 @@ package fsutil import ( "crypto/sha256" "encoding/hex" + "errors" "fmt" "hash" "io" @@ -10,6 +11,8 @@ import ( "os" "path/filepath" "strings" + "sync" + "syscall" ) type CreateOptions struct { @@ -27,6 +30,19 @@ type CreateOptions struct { // If OverrideMode is true and entry already exists, update the mode. Does // not affect symlinks. OverrideMode bool + // 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. + Uid *int + Gid *int } type Entry struct { @@ -143,10 +159,17 @@ func CreateWriter(options *CreateOptions) (io.WriteCloser, *Entry, error) { } } + _, existedErr := os.Lstat(path) file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, o.Mode) if err != nil { return nil, nil, err } + if existedErr != nil { + if err := applyOwnership(path, o); err != nil { + file.Close() + return nil, nil, err + } + } entry := &Entry{ Path: path, Mode: o.Mode, @@ -160,17 +183,51 @@ func CreateWriter(options *CreateOptions) (io.WriteCloser, *Entry, error) { return wp, entry, nil } +// applyOwnership applies the Uid/Gid of the created entry, if any. +// Ownership changes that fail due to lack of privilege are logged once +// and otherwise ignored, so unprivileged cuts keep working with the +// entries owned by the current process user and group. +func applyOwnership(path string, o *CreateOptions) error { + uid, gid := -1, -1 + if o.Uid != nil { + uid = *o.Uid + } + if o.Gid != nil { + gid = *o.Gid + } + if uid == -1 && gid == -1 { + return nil + } + err := os.Chown(path, uid, gid) + if errors.Is(err, syscall.EPERM) { + warnOwnership.Do(func() { + logf("WARNING: cannot preserve file ownership without sufficient privilege: entries will be owned by the current user and group") + }) + return nil + } + return err +} + +var warnOwnership sync.Once + func createDir(o *CreateOptions) error { debugf("Creating directory: %s (mode %#o)", o.Path, o.Mode) path, err := absPath(o.Root, o.Path) if err != nil { return err } + _, existedErr := os.Lstat(path) err = os.Mkdir(path, o.Mode) if os.IsExist(err) { + if existedErr == nil && o.OverrideMode { + return applyOwnership(path, o) + } return nil } - return err + if err != nil { + return err + } + return applyOwnership(path, o) } func createFile(o *CreateOptions) error { @@ -179,6 +236,7 @@ func createFile(o *CreateOptions) error { if err != nil { return err } + _, existedErr := os.Lstat(path) file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, o.Mode) if err != nil { return err @@ -188,7 +246,13 @@ func createFile(o *CreateOptions) error { if copyErr != nil { return copyErr } - return err + if err != nil { + return err + } + if existedErr != nil || o.OverrideMode { + return applyOwnership(path, o) + } + return nil } func createSymlink(o *CreateOptions) error { diff --git a/internal/fsutil/create_test.go b/internal/fsutil/create_test.go index 5e908fccc..def709aca 100644 --- a/internal/fsutil/create_test.go +++ b/internal/fsutil/create_test.go @@ -470,3 +470,141 @@ func (s *S) TestCreateEmptyRoot(c *C) { _, _, err = fsutil.CreateWriter(options) c.Assert(err, ErrorMatches, "internal error: CreateOptions.Root is unset") } + +func entryOwnership(path string) (uid, gid int) { + info, err := os.Lstat(path) + if err != nil { + panic(err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + panic("unsupported system: not a syscall.Stat_t") + } + return int(stat.Uid), int(stat.Gid) +} + +func (s *S) TestCreateOwnership(c *C) { + // Ownership can always be set to the current process user and group, + // even when running unprivileged. The unprivileged attempt to set + // other ids fails with EPERM, which is logged and otherwise ignored. + selfUid, selfGid := os.Getuid(), os.Getgid() + dir := c.MkDir() + + uid, gid := selfUid, selfGid + _, err := fsutil.Create(&fsutil.CreateOptions{ + Root: dir, + Path: "file", + Mode: 0644, + Data: bytes.NewBufferString("data1"), + Uid: &uid, + Gid: &gid, + }) + c.Assert(err, IsNil) + gotUid, gotGid := entryOwnership(filepath.Join(dir, "file")) + c.Assert(gotUid, Equals, selfUid) + c.Assert(gotGid, Equals, selfGid) + + // Directories are owned too. + _, err = fsutil.Create(&fsutil.CreateOptions{ + Root: dir, + Path: "somedir", + Mode: fs.ModeDir | 0755, + MakeParents: true, + Uid: &uid, + Gid: &gid, + }) + c.Assert(err, IsNil) + gotUid, gotGid = entryOwnership(filepath.Join(dir, "somedir")) + c.Assert(gotUid, Equals, selfUid) + c.Assert(gotGid, Equals, selfGid) + + // Ownership of pre-existing entries is only applied with + // OverrideMode, mirroring the mode semantics. + hack := func(root string) { + err := os.WriteFile(filepath.Join(root, "file"), []byte("data"), 0644) + c.Assert(err, IsNil) + } + root := c.MkDir() + hack(root) + rootUid, rootGid := 0, 0 + if selfUid == 0 { + // Use an id different from the current ones for a meaningful + // assertion when running as root. + rootUid, rootGid = 0, 0 + } + _, err = fsutil.Create(&fsutil.CreateOptions{ + Root: root, + Path: "file", + Mode: 0644, + Data: bytes.NewBufferString("data"), + Uid: &rootUid, + Gid: &rootGid, + }) + c.Assert(err, IsNil) + gotUid, gotGid = entryOwnership(filepath.Join(root, "file")) + if selfUid == 0 { + c.Assert(gotUid, Equals, rootUid) + c.Assert(gotGid, Equals, rootGid) + } else { + // Unchanged: the entry pre-existed and OverrideMode is unset. + c.Assert(gotUid, Equals, selfUid) + c.Assert(gotGid, Equals, selfGid) + } + + root = c.MkDir() + hack(root) + _, err = fsutil.Create(&fsutil.CreateOptions{ + Root: root, + Path: "file", + Mode: 0644, + Data: bytes.NewBufferString("data"), + OverrideMode: true, + Uid: &rootUid, + Gid: &rootGid, + }) + c.Assert(err, IsNil) + gotUid, gotGid = entryOwnership(filepath.Join(root, "file")) + if selfUid == 0 { + c.Assert(gotUid, Equals, rootUid) + c.Assert(gotGid, Equals, rootGid) + } else { + // EPERM is logged once and otherwise ignored. + c.Assert(gotUid, Equals, selfUid) + c.Assert(gotGid, Equals, selfGid) + c.Assert(c.GetTestLog(), Matches, "(?s).*WARNING: cannot preserve file ownership.*") + } + + // Ownership of symlinks is never applied, not even via their target. + root = c.MkDir() + err = os.WriteFile(filepath.Join(root, "bar"), []byte("data"), 0644) + c.Assert(err, IsNil) + _, err = fsutil.Create(&fsutil.CreateOptions{ + Root: root, + Path: "foo", + Link: "bar", + Mode: fs.ModeSymlink, + Uid: &rootUid, + Gid: &rootGid, + }) + c.Assert(err, IsNil) + gotUid, gotGid = entryOwnership(filepath.Join(root, "bar")) + c.Assert(gotUid, Equals, selfUid) + c.Assert(gotGid, Equals, selfGid) + + // CreateWriter applies ownership to newly created files too. + root = c.MkDir() + writer, _, err := fsutil.CreateWriter(&fsutil.CreateOptions{ + Root: root, + Path: "file", + Mode: 0644, + Uid: &uid, + Gid: &gid, + }) + c.Assert(err, IsNil) + _, err = writer.Write([]byte("data")) + c.Assert(err, IsNil) + c.Assert(writer.Close(), IsNil) + gotUid, gotGid = entryOwnership(filepath.Join(root, "file")) + c.Assert(gotUid, Equals, selfUid) + c.Assert(gotGid, Equals, selfGid) +} diff --git a/internal/tarball/extract.go b/internal/tarball/extract.go index 40db2cbac..62cd739dc 100644 --- a/internal/tarball/extract.go +++ b/internal/tarball/extract.go @@ -34,6 +34,14 @@ type ExtractInfo struct { Context any } +// tarDirInfo holds the mode and ownership of a directory as recorded in +// the package tarball. +type tarDirInfo struct { + mode fs.FileMode + uid int + gid int +} + func getValidOptions(options *ExtractOptions) (*ExtractOptions, error) { for extractPath, extractInfos := range options.Extract { isGlob := strings.ContainsAny(extractPath, "*?") @@ -114,12 +122,12 @@ func extractData(pkgReader io.ReadSeeker, options *ExtractOptions) error { pendingHardLinks := make(map[string][]pendingHardLink) // When creating a file we will iterate through its parent directories and - // create them with the permissions defined in the tarball. + // create them with the permissions and ownership defined in the tarball. // // The assumption is that the tar entries of the parent directories appear // 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) tarReader := tar.NewReader(dataReader) for { tarHeader, err := tarReader.Next() @@ -137,7 +145,11 @@ func extractData(pkgReader io.ReadSeeker, options *ExtractOptions) error { sourceIsDir := sourcePath[len(sourcePath)-1] == '/' if sourceIsDir { - tarDirMode[sourcePath] = tarHeader.FileInfo().Mode() + tarDirs[sourcePath] = tarDirInfo{ + mode: tarHeader.FileInfo().Mode(), + uid: tarHeader.Uid, + gid: tarHeader.Gid, + } } // Find all globs and copies that require this source, and map them by @@ -196,24 +208,35 @@ func extractData(pkgReader io.ReadSeeker, options *ExtractOptions) error { if mode != 0 { tarHeader.Mode = int64(mode) } - // Create the parent directories using the permissions from the tarball. + // Create the parent directories using the permissions and + // ownership from the tarball. parents := parentDirs(targetPath) for _, path := range parents { if path == "/" { continue } - mode, ok := tarDirMode[path] + dirInfo, ok := tarDirs[path] if !ok { continue } - delete(tarDirMode, path) + delete(tarDirs, path) createOptions := &fsutil.CreateOptions{ Root: options.TargetDir, Path: path, - Mode: mode, + Mode: dirInfo.mode, MakeParents: true, } + // Only special ownership (any non-root user or a + // non-root group) needs to be applied explicitly: + // root-owned entries already end up root-owned when + // cutting as root, and cannot be chowned when cutting + // as a regular user. + if dirInfo.uid != 0 || dirInfo.gid != 0 { + uid, gid := dirInfo.uid, dirInfo.gid + createOptions.Uid = &uid + createOptions.Gid = &gid + } err := options.Create(nil, createOptions) if err != nil { return err @@ -225,7 +248,12 @@ func extractData(pkgReader io.ReadSeeker, options *ExtractOptions) error { link = filepath.Join(options.TargetDir, link) } - // Create the entry itself. + // Create the entry itself, preserving any special + // ownership recorded in the tarball (e.g. the + // setgid-shadow group of the PAM chkpwd helpers). + // Root-owned entries are skipped: they already end up + // root-owned when cutting as root, and cannot be chowned + // when cutting as a regular user. createOptions := &fsutil.CreateOptions{ Root: options.TargetDir, Path: targetPath, @@ -235,6 +263,11 @@ func extractData(pkgReader io.ReadSeeker, options *ExtractOptions) error { MakeParents: true, OverrideMode: true, } + if tarHeader.Uid != 0 || tarHeader.Gid != 0 { + uid, gid := tarHeader.Uid, tarHeader.Gid + createOptions.Uid = &uid + createOptions.Gid = &gid + } err := options.Create(extractInfos, createOptions) if err != nil && os.IsNotExist(err) && tarHeader.Typeflag == tar.TypeLink { // The hard link could not be created because the content diff --git a/internal/tarball/extract_test.go b/internal/tarball/extract_test.go index fd5eb6147..86da056e5 100644 --- a/internal/tarball/extract_test.go +++ b/internal/tarball/extract_test.go @@ -1,12 +1,15 @@ package tarball_test import ( + "archive/tar" "bytes" + "io/fs" "os" "path" "path/filepath" "sort" "strings" + "syscall" . "gopkg.in/check.v1" @@ -628,3 +631,89 @@ func (s *S) TestExtractCreateCallback(c *C) { c.Assert(createExtractInfos, DeepEquals, test.calls) } } + +func (s *S) TestExtractOwnership(c *C) { + // Entries are created with the ownership recorded in the package + // tarball (e.g. the setgid-shadow group of the PAM chkpwd helpers). + // Applying ownership requires sufficient privilege (e.g. running as + // root); otherwise a warning is logged once and the entries keep the + // ownership of the current process user and group. + shadowGid := 42 + entries := []testutil.TarEntry{{ + Header: tar.Header{ + Typeflag: tar.TypeDir, + Name: "./usr/", + Mode: 0755, + Uid: 0, + Gid: shadowGid, + Uname: "root", + Gname: "shadow", + Format: tar.FormatGNU, + }, + }, { + Header: tar.Header{ + Typeflag: tar.TypeDir, + Name: "./usr/sbin/", + Mode: 0755, + Uid: 0, + Gid: shadowGid, + Uname: "root", + Gname: "shadow", + Format: tar.FormatGNU, + }, + }, { + Header: tar.Header{ + Typeflag: tar.TypeReg, + Name: "./usr/sbin/unix_chkpwd", + Mode: 02755, + Uid: 0, + Gid: shadowGid, + Uname: "root", + Gname: "shadow", + Format: tar.FormatGNU, + }, + Content: []byte("chkpwd-binary-content"), + }} + pkgdata, err := testutil.MakeDeb(entries) + c.Assert(err, IsNil) + + dir := c.MkDir() + // The ownership warnings are logged through fsutil's logger. + fsutil.SetLogger(c) + defer fsutil.SetLogger(nil) + options := tarball.ExtractOptions{ + Package: "test-package", + TargetDir: dir, + Extract: map[string][]tarball.ExtractInfo{ + "/usr/sbin/unix_chkpwd": []tarball.ExtractInfo{{ + Path: "/usr/sbin/unix_chkpwd", + }}, + }, + } + err = tarball.Extract(bytes.NewReader(pkgdata), &options) + c.Assert(err, IsNil) + + info, err := os.Lstat(filepath.Join(dir, "usr/sbin/unix_chkpwd")) + c.Assert(err, IsNil) + c.Assert(info.Mode()&fs.ModeSetgid, Not(Equals), 0) + stat, ok := info.Sys().(*syscall.Stat_t) + c.Assert(ok, Equals, true) + dirInfo, err := os.Lstat(filepath.Join(dir, "usr/sbin")) + c.Assert(err, IsNil) + dirStat, ok := dirInfo.Sys().(*syscall.Stat_t) + c.Assert(ok, Equals, true) + if os.Geteuid() == 0 { + // Ownership of the entry and its parent directories is + // preserved from the tarball. + c.Assert(stat.Uid, Equals, uint32(0)) + c.Assert(stat.Gid, Equals, uint32(shadowGid)) + c.Assert(dirStat.Gid, Equals, uint32(shadowGid)) + } else { + // Without sufficient privilege the entries are owned by the + // current process user and group, and a warning is logged. + c.Assert(stat.Uid, Equals, uint32(os.Getuid())) + c.Assert(stat.Gid, Equals, uint32(os.Getgid())) + c.Assert(dirStat.Gid, Equals, uint32(os.Getgid())) + c.Assert(c.GetTestLog(), Matches, "(?s).*WARNING: cannot preserve file ownership.*") + } +}