From 80382206ed49ea6f80a93ae0652506fbe0194d95 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 23 Sep 2024 09:41:57 +0200 Subject: [PATCH 1/5] userns: fix off-by-one userns max size detection fix the detection for the maximum userns size from an image. If the maximum ID used in an image is X, we need to use a user namespace with size X+1 to include UID=X. Closes: https://github.com/containers/storage/issues/2104 Signed-off-by: Giuseppe Scrivano --- userns.go | 6 ++-- userns_test.go | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/userns.go b/userns.go index 57120731be..02813bc5f7 100644 --- a/userns.go +++ b/userns.go @@ -103,10 +103,10 @@ func parseMountedFiles(containerMount, passwdFile, groupFile string) uint32 { continue } if u.Uid > size && u.Uid != nobodyUser { - size = u.Uid + size = u.Uid + 1 } if u.Gid > size && u.Gid != nobodyUser { - size = u.Gid + size = u.Gid + 1 } } } @@ -118,7 +118,7 @@ func parseMountedFiles(containerMount, passwdFile, groupFile string) uint32 { continue } if g.Gid > size && g.Gid != nobodyUser { - size = g.Gid + size = g.Gid + 1 } } } diff --git a/userns_test.go b/userns_test.go index 89bb95ff27..800214870d 100644 --- a/userns_test.go +++ b/userns_test.go @@ -1,6 +1,8 @@ package storage import ( + "os" + "path/filepath" "reflect" "testing" @@ -170,3 +172,87 @@ func TestGetAutoUserNSMapping(t *testing.T) { }) } } + +func TestParseMountedFiles(t *testing.T) { + tests := []struct { + name string + passwdContent string + groupContent string + expectedMax uint32 + }{ + { + name: "basic case", + passwdContent: ` +root:x:0:0:root:/root:/bin/bash +user1:x:1000:1000::/home/user1:/bin/bash +nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin`, + groupContent: ` +root:x:0: +user1:x:1000: +nogroup:x:65534:`, + expectedMax: 1001, + }, + { + name: "only passwd", + passwdContent: ` +root:x:0:0:root:/root:/bin/bash +user1:x:4001:4001::/home/user1:/bin/bash +nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin`, + groupContent: "", + expectedMax: 4002, + }, + { + name: "only groups", + passwdContent: "", + groupContent: ` +root:x:0: +admin:x:3000: +nobody:x:65534:`, + expectedMax: 3001, + }, + { + name: "empty files", + passwdContent: "", + groupContent: "", + expectedMax: 0, + }, + { + name: "invalid passwd file", + passwdContent: "FOOBAR", + groupContent: "", + expectedMax: 0, + }, + { + name: "invalid groups file", + passwdContent: "", + groupContent: "FOOBAR", + expectedMax: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "containermount") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + passwdFile := filepath.Join(tmpDir, "passwd") + if err := os.WriteFile(passwdFile, []byte(tt.passwdContent), 0o644); err != nil { + t.Fatalf("Failed to write passwd file: %v", err) + } + + groupFile := filepath.Join(tmpDir, "group") + if err := os.WriteFile(groupFile, []byte(tt.groupContent), 0o644); err != nil { + t.Fatalf("Failed to write group file: %v", err) + } + + result := parseMountedFiles(tmpDir, passwdFile, groupFile) + + if result != tt.expectedMax { + t.Errorf("Expected max %d, but got %d", tt.expectedMax, result) + } + }) + } +} From 5401ebd1fd1f1eea6950e54b3f1cc2c77527bec1 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 23 Sep 2024 11:48:19 +0200 Subject: [PATCH 2/5] users: fix path for /etc/group in the container Signed-off-by: Giuseppe Scrivano --- userns.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/userns.go b/userns.go index 02813bc5f7..7b6a5b9bbb 100644 --- a/userns.go +++ b/userns.go @@ -89,7 +89,7 @@ func parseMountedFiles(containerMount, passwdFile, groupFile string) uint32 { passwdFile = filepath.Join(containerMount, "etc/passwd") } if groupFile == "" { - groupFile = filepath.Join(groupFile, "etc/group") + groupFile = filepath.Join(containerMount, "etc/group") } size := 0 From 9b22eeab7c90867a2538536c89df0f1e1fffe193 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Tue, 24 Sep 2024 18:34:24 +0200 Subject: [PATCH 3/5] userns: skip "nogroup" the alpine image defines a "nogroup": $ podman run --rm alpine grep nogroup /etc/group nogroup:x:65533: ignore it as we are already doing for the "nobody" user. Signed-off-by: Giuseppe Scrivano --- userns.go | 4 ++-- userns_test.go | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/userns.go b/userns.go index 7b6a5b9bbb..1b494ef12c 100644 --- a/userns.go +++ b/userns.go @@ -99,7 +99,7 @@ func parseMountedFiles(containerMount, passwdFile, groupFile string) uint32 { for _, u := range users { // Skip the "nobody" user otherwise we end up with 65536 // ids with most images - if u.Name == "nobody" { + if u.Name == "nobody" || u.Name == "nogroup" { continue } if u.Uid > size && u.Uid != nobodyUser { @@ -114,7 +114,7 @@ func parseMountedFiles(containerMount, passwdFile, groupFile string) uint32 { groups, err := libcontainerUser.ParseGroupFile(groupFile) if err == nil { for _, g := range groups { - if g.Name == "nobody" { + if g.Name == "nobody" || g.Name == "nogroup" { continue } if g.Gid > size && g.Gid != nobodyUser { diff --git a/userns_test.go b/userns_test.go index 800214870d..c560d552a4 100644 --- a/userns_test.go +++ b/userns_test.go @@ -228,6 +228,15 @@ nobody:x:65534:`, groupContent: "FOOBAR", expectedMax: 0, }, + { + name: "nogroup ignored", + passwdContent: "", + groupContent: ` +root:x:0: +admin:x:4000: +nogroup:x:65533:`, + expectedMax: 4001, + }, } for _, tt := range tests { From 097de28871f657a60cfafe25e7c53fc363a2f796 Mon Sep 17 00:00:00 2001 From: Matt Heon Date: Wed, 9 Oct 2024 09:54:22 -0400 Subject: [PATCH 4/5] Use securejoin.SecureJoin when forming userns paths We need to read /etc/passwd and /etc/group in the container to get an idea of how many UIDs and GIDs we need to allocate for a user namespace when `--userns=auto` is specified. We were forming paths for these using filepath.Join, which is not safe for paths within a container, resulting in this CVE allowing crafted symlinks in the container to access paths on the host instead. Addresses CVE-2024-9676 Signed-off-by: Matt Heon --- userns.go | 87 +++++++++++++++++++++++++++++-------------- userns_test.go | 2 + userns_unsupported.go | 14 +++++++ 3 files changed, 76 insertions(+), 27 deletions(-) create mode 100644 userns_unsupported.go diff --git a/userns.go b/userns.go index 1b494ef12c..09919394c0 100644 --- a/userns.go +++ b/userns.go @@ -1,18 +1,21 @@ +//go:build linux + package storage import ( "fmt" "os" "os/user" - "path/filepath" "strconv" drivers "github.com/containers/storage/drivers" "github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/unshare" "github.com/containers/storage/types" + securejoin "github.com/cyphar/filepath-securejoin" libcontainerUser "github.com/moby/sys/user" "github.com/sirupsen/logrus" + "golang.org/x/sys/unix" ) // getAdditionalSubIDs looks up the additional IDs configured for @@ -85,40 +88,59 @@ const nobodyUser = 65534 // parseMountedFiles returns the maximum UID and GID found in the /etc/passwd and // /etc/group files. func parseMountedFiles(containerMount, passwdFile, groupFile string) uint32 { + var ( + passwd *os.File + group *os.File + size int + err error + ) if passwdFile == "" { - passwdFile = filepath.Join(containerMount, "etc/passwd") - } - if groupFile == "" { - groupFile = filepath.Join(containerMount, "etc/group") + passwd, err = secureOpen(containerMount, "/etc/passwd") + } else { + // User-specified override from a volume. Will not be in + // container root. + passwd, err = os.Open(passwdFile) } - - size := 0 - - users, err := libcontainerUser.ParsePasswdFile(passwdFile) if err == nil { - for _, u := range users { - // Skip the "nobody" user otherwise we end up with 65536 - // ids with most images - if u.Name == "nobody" || u.Name == "nogroup" { - continue - } - if u.Uid > size && u.Uid != nobodyUser { - size = u.Uid + 1 - } - if u.Gid > size && u.Gid != nobodyUser { - size = u.Gid + 1 + defer passwd.Close() + + users, err := libcontainerUser.ParsePasswd(passwd) + if err == nil { + for _, u := range users { + // Skip the "nobody" user otherwise we end up with 65536 + // ids with most images + if u.Name == "nobody" || u.Name == "nogroup" { + continue + } + if u.Uid > size && u.Uid != nobodyUser { + size = u.Uid + 1 + } + if u.Gid > size && u.Gid != nobodyUser { + size = u.Gid + 1 + } } } } - groups, err := libcontainerUser.ParseGroupFile(groupFile) + if groupFile == "" { + group, err = secureOpen(containerMount, "/etc/group") + } else { + // User-specified override from a volume. Will not be in + // container root. + group, err = os.Open(groupFile) + } if err == nil { - for _, g := range groups { - if g.Name == "nobody" || g.Name == "nogroup" { - continue - } - if g.Gid > size && g.Gid != nobodyUser { - size = g.Gid + 1 + defer group.Close() + + groups, err := libcontainerUser.ParseGroup(group) + if err == nil { + for _, g := range groups { + if g.Name == "nobody" || g.Name == "nogroup" { + continue + } + if g.Gid > size && g.Gid != nobodyUser { + size = g.Gid + 1 + } } } } @@ -309,3 +331,14 @@ func getAutoUserNSIDMappings( gidMap := append(availableGIDs.zip(requestedContainerGIDs), additionalGIDMappings...) return uidMap, gidMap, nil } + +// Securely open (read-only) a file in a container mount. +func secureOpen(containerMount, file string) (*os.File, error) { + tmpFile, err := securejoin.OpenInRoot(containerMount, file) + if err != nil { + return nil, err + } + defer tmpFile.Close() + + return securejoin.Reopen(tmpFile, unix.O_RDONLY) +} diff --git a/userns_test.go b/userns_test.go index c560d552a4..14515a887d 100644 --- a/userns_test.go +++ b/userns_test.go @@ -1,3 +1,5 @@ +//go:build linux + package storage import ( diff --git a/userns_unsupported.go b/userns_unsupported.go new file mode 100644 index 0000000000..e37c18fe43 --- /dev/null +++ b/userns_unsupported.go @@ -0,0 +1,14 @@ +//go:build !linux + +package storage + +import ( + "errors" + + "github.com/containers/storage/pkg/idtools" + "github.com/containers/storage/types" +) + +func (s *store) getAutoUserNS(_ *types.AutoUserNsOptions, _ *Image, _ rwLayerStore, _ []roLayerStore) ([]idtools.IDMap, []idtools.IDMap, error) { + return nil, nil, errors.New("user namespaces are not supported on this platform") +} From eccfbd06f1d81cb4b9c574eea5df83ed98ece602 Mon Sep 17 00:00:00 2001 From: Matt Heon Date: Tue, 15 Oct 2024 14:46:15 -0400 Subject: [PATCH 5/5] Force lint to golang:1.21 Matches what we're compiling with. Signed-off-by: Matt Heon --- .cirrus.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.cirrus.yml b/.cirrus.yml index 50b9876169..49a6e33b70 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -120,7 +120,7 @@ lint_task: env: CIRRUS_WORKING_DIR: "/go/src/github.com/containers/storage" container: - image: golang + image: golang:1.21 modules_cache: fingerprint_script: cat go.sum folder: $GOPATH/pkg/mod