From d68fa0382d1025cbeedd3a00deda854b034c510f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Oct 2022 18:51:42 +0200 Subject: [PATCH 1/4] pkg/idtools: don't use system.Stat() on unix Looks like we don't need the abstraction, so we can reduce the dependency on pkg/system. Signed-off-by: Sebastiaan van Stijn --- pkg/idtools/idtools_unix.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/idtools/idtools_unix.go b/pkg/idtools/idtools_unix.go index e2c464646b..dd39bc9bfe 100644 --- a/pkg/idtools/idtools_unix.go +++ b/pkg/idtools/idtools_unix.go @@ -14,7 +14,6 @@ import ( "sync" "syscall" - "github.com/docker/docker/pkg/system" "github.com/opencontainers/runc/libcontainer/user" ) @@ -29,7 +28,7 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting return err } - stat, err := system.Stat(path) + stat, err := os.Stat(path) if err == nil { if !stat.IsDir() { return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR} @@ -85,20 +84,21 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting // CanAccess takes a valid (existing) directory and a uid, gid pair and determines // if that uid, gid pair has access (execute bit) to the directory func CanAccess(path string, pair Identity) bool { - statInfo, err := system.Stat(path) + statInfo, err := os.Stat(path) if err != nil { return false } - perms := os.FileMode(statInfo.Mode()).Perm() + perms := statInfo.Mode().Perm() if perms&0o001 == 0o001 { // world access return true } - if statInfo.UID() == uint32(pair.UID) && (perms&0o100 == 0o100) { + ssi := statInfo.Sys().(*syscall.Stat_t) + if ssi.Uid == uint32(pair.UID) && (perms&0o100 == 0o100) { // owner access. return true } - if statInfo.GID() == uint32(pair.GID) && (perms&0o010 == 0o010) { + if ssi.Gid == uint32(pair.GID) && (perms&0o010 == 0o010) { // group access. return true } @@ -229,20 +229,21 @@ func getExitCode(err error) (int, error) { // Normally a Chown is a no-op if uid/gid match, but in some cases this can still cause an error, e.g. if the // dir is on an NFS share, so don't call chown unless we absolutely must. // Likewise for setting permissions. -func setPermissions(p string, mode os.FileMode, uid, gid int, stat *system.StatT) error { +func setPermissions(p string, mode os.FileMode, uid, gid int, stat os.FileInfo) error { if stat == nil { var err error - stat, err = system.Stat(p) + stat, err = os.Stat(p) if err != nil { return err } } - if os.FileMode(stat.Mode()).Perm() != mode.Perm() { + if stat.Mode().Perm() != mode.Perm() { if err := os.Chmod(p, mode.Perm()); err != nil { return err } } - if stat.UID() == uint32(uid) && stat.GID() == uint32(gid) { + ssi := stat.Sys().(*syscall.Stat_t) + if ssi.Uid == uint32(uid) && ssi.Gid == uint32(gid) { return nil } return os.Chown(p, uid, gid) From bca90530faec87bfe20d8eb248d7176f1c14108d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Oct 2022 14:44:25 +0200 Subject: [PATCH 2/4] pkg/idtools: simplify if-statement Signed-off-by: Sebastiaan van Stijn --- pkg/idtools/idtools_unix.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/idtools/idtools_unix.go b/pkg/idtools/idtools_unix.go index dd39bc9bfe..1965a3fa0d 100644 --- a/pkg/idtools/idtools_unix.go +++ b/pkg/idtools/idtools_unix.go @@ -59,17 +59,15 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting if dirPath == "/" { break } - if _, err := os.Stat(dirPath); err != nil && os.IsNotExist(err) { + if _, err = os.Stat(dirPath); err != nil && os.IsNotExist(err) { paths = append(paths, dirPath) } } - if err := os.MkdirAll(path, mode); err != nil { - return err - } - } else { - if err := os.Mkdir(path, mode); err != nil && !os.IsExist(err) { + if err = os.MkdirAll(path, mode); err != nil { return err } + } else if err = os.Mkdir(path, mode); err != nil { + return err } // even if it existed, we will chown the requested path + any subpaths that // didn't exist when we called MkdirAll From ee34a8ac29ad7e10eb45d34c3ce02ab4bd6d4614 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Oct 2022 14:47:04 +0200 Subject: [PATCH 3/4] pkg/idtools: setPermissions() accept Identity as argument Signed-off-by: Sebastiaan van Stijn --- pkg/idtools/idtools_unix.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/idtools/idtools_unix.go b/pkg/idtools/idtools_unix.go index 1965a3fa0d..2bd8140baa 100644 --- a/pkg/idtools/idtools_unix.go +++ b/pkg/idtools/idtools_unix.go @@ -37,8 +37,8 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting return nil } - // short-circuit--we were called with an existing directory and chown was requested - return setPermissions(path, mode, owner.UID, owner.GID, stat) + // short-circuit -- we were called with an existing directory and chown was requested + return setPermissions(path, mode, owner, stat) } // make an array containing the original path asked for, plus (for mkAll == true) @@ -72,7 +72,7 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting // even if it existed, we will chown the requested path + any subpaths that // didn't exist when we called MkdirAll for _, pathComponent := range paths { - if err := setPermissions(pathComponent, mode, owner.UID, owner.GID, nil); err != nil { + if err = setPermissions(pathComponent, mode, owner, nil); err != nil { return err } } @@ -227,7 +227,7 @@ func getExitCode(err error) (int, error) { // Normally a Chown is a no-op if uid/gid match, but in some cases this can still cause an error, e.g. if the // dir is on an NFS share, so don't call chown unless we absolutely must. // Likewise for setting permissions. -func setPermissions(p string, mode os.FileMode, uid, gid int, stat os.FileInfo) error { +func setPermissions(p string, mode os.FileMode, owner Identity, stat os.FileInfo) error { if stat == nil { var err error stat, err = os.Stat(p) @@ -241,10 +241,10 @@ func setPermissions(p string, mode os.FileMode, uid, gid int, stat os.FileInfo) } } ssi := stat.Sys().(*syscall.Stat_t) - if ssi.Uid == uint32(uid) && ssi.Gid == uint32(gid) { + if ssi.Uid == uint32(owner.UID) && ssi.Gid == uint32(owner.GID) { return nil } - return os.Chown(p, uid, gid) + return os.Chown(p, owner.UID, owner.GID) } // LoadIdentityMapping takes a requested username and From 69f72417f444c2e0589767019e697a161bb912a9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Oct 2022 22:23:41 +0200 Subject: [PATCH 4/4] pkg/idtools: remove CanAccess(), and move to daemon The implementation of CanAccess() is very rudimentary, and should not be used for anything other than a basic check (and maybe not even for that). It's only used in a single location in the daemon, so move it there, and un-export it to not encourage others to use it out of context. Signed-off-by: Sebastiaan van Stijn --- daemon/daemon_unix.go | 31 ++++++++++++++++++++++++++++++- pkg/idtools/idtools_unix.go | 24 ------------------------ pkg/idtools/idtools_unix_test.go | 9 ++++++++- 3 files changed, 38 insertions(+), 26 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 20b08e75e2..f15dff0af9 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -15,6 +15,7 @@ import ( "strconv" "strings" "sync" + "syscall" "time" "github.com/containerd/cgroups" @@ -1247,7 +1248,7 @@ func setupDaemonRoot(config *config.Config, rootDir string, remappedRoot idtools if dirPath == "/" { break } - if !idtools.CanAccess(dirPath, remappedRoot) { + if !canAccess(dirPath, remappedRoot) { return fmt.Errorf("a subdirectory in your graphroot path (%s) restricts access to the remapped root uid/gid; please fix by allowing 'o+x' permissions on existing directories", config.Root) } } @@ -1259,6 +1260,34 @@ func setupDaemonRoot(config *config.Config, rootDir string, remappedRoot idtools return nil } +// canAccess takes a valid (existing) directory and a uid, gid pair and determines +// if that uid, gid pair has access (execute bit) to the directory. +// +// Note: this is a very rudimentary check, and may not produce accurate results, +// so should not be used for anything other than the current use, see: +// https://github.com/moby/moby/issues/43724 +func canAccess(path string, pair idtools.Identity) bool { + statInfo, err := os.Stat(path) + if err != nil { + return false + } + perms := statInfo.Mode().Perm() + if perms&0o001 == 0o001 { + // world access + return true + } + ssi := statInfo.Sys().(*syscall.Stat_t) + if ssi.Uid == uint32(pair.UID) && (perms&0o100 == 0o100) { + // owner access. + return true + } + if ssi.Gid == uint32(pair.GID) && (perms&0o010 == 0o010) { + // group access. + return true + } + return false +} + func setupDaemonRootPropagation(cfg *config.Config) error { rootParentMount, mountOptions, err := getSourceMount(cfg.Root) if err != nil { diff --git a/pkg/idtools/idtools_unix.go b/pkg/idtools/idtools_unix.go index 2bd8140baa..7d31c69dae 100644 --- a/pkg/idtools/idtools_unix.go +++ b/pkg/idtools/idtools_unix.go @@ -79,30 +79,6 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting return nil } -// CanAccess takes a valid (existing) directory and a uid, gid pair and determines -// if that uid, gid pair has access (execute bit) to the directory -func CanAccess(path string, pair Identity) bool { - statInfo, err := os.Stat(path) - if err != nil { - return false - } - perms := statInfo.Mode().Perm() - if perms&0o001 == 0o001 { - // world access - return true - } - ssi := statInfo.Sys().(*syscall.Stat_t) - if ssi.Uid == uint32(pair.UID) && (perms&0o100 == 0o100) { - // owner access. - return true - } - if ssi.Gid == uint32(pair.GID) && (perms&0o010 == 0o010) { - // group access. - return true - } - return false -} - // LookupUser uses traditional local system files lookup (from libcontainer/user) on a username, // followed by a call to `getent` for supporting host configured non-files passwd and group dbs func LookupUser(name string) (user.User, error) { diff --git a/pkg/idtools/idtools_unix_test.go b/pkg/idtools/idtools_unix_test.go index 0979daf9e4..1e65e2af08 100644 --- a/pkg/idtools/idtools_unix_test.go +++ b/pkg/idtools/idtools_unix_test.go @@ -6,8 +6,10 @@ package idtools // import "github.com/docker/docker/pkg/idtools" import ( "fmt" "os" + "os/exec" "os/user" "path/filepath" + "syscall" "testing" "golang.org/x/sys/unix" @@ -425,7 +427,12 @@ func TestNewIDMappings(t *testing.T) { err = MkdirAllAndChown(dirName, 0o700, Identity{UID: rootUID, GID: rootGID}) assert.Check(t, err, "Couldn't change ownership of file path. Got error") - assert.Check(t, CanAccess(dirName, idMapping.RootPair()), "Unable to access %s directory with user UID:%d and GID:%d", dirName, rootUID, rootGID) + cmd := exec.Command("ls", "-la", dirName) + cmd.SysProcAttr = &syscall.SysProcAttr{ + Credential: &syscall.Credential{Uid: uint32(rootUID), Gid: uint32(rootGID)}, + } + out, err := cmd.CombinedOutput() + assert.Check(t, err, "Unable to access %s directory with user UID:%d and GID:%d:\n%s", dirName, rootUID, rootGID, string(out)) } func TestLookupUserAndGroup(t *testing.T) {