From 1b69407142a4a42d6d81cd0a3ec7db15fb270031 Mon Sep 17 00:00:00 2001 From: bwzhang Date: Tue, 19 Mar 2024 10:07:46 +0800 Subject: [PATCH] fix CVE-2023-25173 --- oci/spec_opts.go | 139 +++++++++++++++++------ pkg/cri/server/container_create_linux.go | 3 +- 2 files changed, 105 insertions(+), 37 deletions(-) diff --git a/oci/spec_opts.go b/oci/spec_opts.go index 36eae16..c9e1832 100644 --- a/oci/spec_opts.go +++ b/oci/spec_opts.go @@ -113,6 +113,17 @@ func setCapabilities(s *Spec) { } } +// ensureAdditionalGids ensures that the primary GID is also included in the additional GID list. +func ensureAdditionalGids(s *Spec) { + setProcess(s) + for _, f := range s.Process.User.AdditionalGids { + if f == s.Process.User.GID { + return + } + } + s.Process.User.AdditionalGids = append([]uint32{s.Process.User.GID}, s.Process.User.AdditionalGids...) +} + // WithDefaultSpec returns a SpecOpts that will populate the spec with default // values. // @@ -521,7 +532,9 @@ func WithNamespacedCgroup() SpecOpts { // user, uid, user:group, uid:gid, uid:group, user:gid func WithUser(userstr string) SpecOpts { return func(ctx context.Context, client Client, c *containers.Container, s *Spec) error { + defer ensureAdditionalGids(s) setProcess(s) + s.Process.User.AdditionalGids = nil // For LCOW it's a bit harder to confirm that the user actually exists on the host as a rootfs isn't // mounted on the host and shared into the guest, but rather the rootfs is constructed entirely in the @@ -614,7 +627,9 @@ func WithUser(userstr string) SpecOpts { // WithUIDGID allows the UID and GID for the Process to be set func WithUIDGID(uid, gid uint32) SpecOpts { return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { + defer ensureAdditionalGids(s) setProcess(s) + s.Process.User.AdditionalGids = nil s.Process.User.UID = uid s.Process.User.GID = gid return nil @@ -627,12 +642,11 @@ func WithUIDGID(uid, gid uint32) SpecOpts { // additionally sets the gid to 0, and does not return an error. func WithUserID(uid uint32) SpecOpts { return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) { + defer ensureAdditionalGids(s) setProcess(s) - if c.Snapshotter == "" && c.SnapshotKey == "" { - if !isRootfsAbs(s.Root.Path) { - return errors.New("rootfs absolute path is required") - } - user, err := UserFromPath(s.Root.Path, func(u user.User) bool { + s.Process.User.AdditionalGids = nil + setUser := func(root string) error { + user, err := UserFromPath(root, func(u user.User) bool { return u.Uid == int(uid) }) if err != nil { @@ -644,6 +658,12 @@ func WithUserID(uid uint32) SpecOpts { } s.Process.User.UID, s.Process.User.GID = uint32(user.Uid), uint32(user.Gid) return nil + } + if c.Snapshotter == "" && c.SnapshotKey == "" { + if !isRootfsAbs(s.Root.Path) { + return errors.New("rootfs absolute path is required") + } + return setUser(s.Root.Path) } if c.Snapshotter == "" { @@ -659,20 +679,7 @@ func WithUserID(uid uint32) SpecOpts { } mounts = tryReadonlyMounts(mounts) - return mount.WithTempMount(ctx, mounts, func(root string) error { - user, err := UserFromPath(root, func(u user.User) bool { - return u.Uid == int(uid) - }) - if err != nil { - if os.IsNotExist(err) || err == ErrNoUsersFound { - s.Process.User.UID, s.Process.User.GID = uid, 0 - return nil - } - return err - } - s.Process.User.UID, s.Process.User.GID = uint32(user.Uid), uint32(user.Gid) - return nil - }) + return mount.WithTempMount(ctx, mounts, setUser) } } @@ -684,13 +691,12 @@ func WithUserID(uid uint32) SpecOpts { // the container. func WithUsername(username string) SpecOpts { return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) { + defer ensureAdditionalGids(s) setProcess(s) + s.Process.User.AdditionalGids = nil if s.Linux != nil { - if c.Snapshotter == "" && c.SnapshotKey == "" { - if !isRootfsAbs(s.Root.Path) { - return errors.New("rootfs absolute path is required") - } - user, err := UserFromPath(s.Root.Path, func(u user.User) bool { + setUser := func(root string) error { + user, err := UserFromPath(root, func(u user.User) bool { return u.Name == username }) if err != nil { @@ -699,6 +705,12 @@ func WithUsername(username string) SpecOpts { s.Process.User.UID, s.Process.User.GID = uint32(user.Uid), uint32(user.Gid) return nil } + if c.Snapshotter == "" && c.SnapshotKey == "" { + if !isRootfsAbs(s.Root.Path) { + return errors.New("rootfs absolute path is required") + } + return setUser(s.Root.Path) + } if c.Snapshotter == "" { return errors.New("no snapshotter set for container") } @@ -712,16 +724,7 @@ func WithUsername(username string) SpecOpts { } mounts = tryReadonlyMounts(mounts) - return mount.WithTempMount(ctx, mounts, func(root string) error { - user, err := UserFromPath(root, func(u user.User) bool { - return u.Name == username - }) - if err != nil { - return err - } - s.Process.User.UID, s.Process.User.GID = uint32(user.Uid), uint32(user.Gid) - return nil - }) + return mount.WithTempMount(ctx, mounts, setUser) } else if s.Windows != nil { s.Process.User.Username = username } else { @@ -732,7 +735,7 @@ func WithUsername(username string) SpecOpts { } // WithAdditionalGIDs sets the OCI spec's additionalGids array to any additional groups listed -// for a particular user in the /etc/groups file of the image's root filesystem +// for a particular user in the /etc/group file of the image's root filesystem // The passed in user can be either a uid or a username. func WithAdditionalGIDs(userstr string) SpecOpts { return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) { @@ -741,7 +744,9 @@ func WithAdditionalGIDs(userstr string) SpecOpts { return nil } setProcess(s) + s.Process.User.AdditionalGids = nil setAdditionalGids := func(root string) error { + defer ensureAdditionalGids(s) var username string uid, err := strconv.Atoi(userstr) if err == nil { @@ -802,6 +807,68 @@ func WithAdditionalGIDs(userstr string) SpecOpts { } } +// WithAppendAdditionalGroups append additional groups within the container. +// The passed in groups can be either a gid or a groupname. +func WithAppendAdditionalGroups(groups ...string) SpecOpts { + return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) { + // For LCOW or on Darwin additional GID's are not supported + if s.Windows != nil || runtime.GOOS == "darwin" { + return nil + } + setProcess(s) + setAdditionalGids := func(root string) error { + defer ensureAdditionalGids(s) + gpath, err := fs.RootPath(root, "/etc/group") + if err != nil { + return err + } + ugroups, err := user.ParseGroupFile(gpath) + if err != nil { + return err + } + groupMap := make(map[string]user.Group) + for _, group := range ugroups { + groupMap[group.Name] = group + } + var gids []uint32 + for _, group := range groups { + gid, err := strconv.ParseUint(group, 10, 32) + if err == nil { + gids = append(gids, uint32(gid)) + } else { + g, ok := groupMap[group] + if !ok { + return fmt.Errorf("unable to find group %s", group) + } + gids = append(gids, uint32(g.Gid)) + } + } + s.Process.User.AdditionalGids = append(s.Process.User.AdditionalGids, gids...) + return nil + } + if c.Snapshotter == "" && c.SnapshotKey == "" { + if !filepath.IsAbs(s.Root.Path) { + return errors.New("rootfs absolute path is required") + } + return setAdditionalGids(s.Root.Path) + } + if c.Snapshotter == "" { + return errors.New("no snapshotter set for container") + } + if c.SnapshotKey == "" { + return errors.New("rootfs snapshot not created for container") + } + snapshotter := client.SnapshotService(c.Snapshotter) + mounts, err := snapshotter.Mounts(ctx, c.SnapshotKey) + if err != nil { + return err + } + + mounts = tryReadonlyMounts(mounts) + return mount.WithTempMount(ctx, mounts, setAdditionalGids) + } +} + // WithCapabilities sets Linux capabilities on the process func WithCapabilities(caps []string) SpecOpts { return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { @@ -906,7 +973,7 @@ func UserFromPath(root string, filter func(user.User) bool) (user.User, error) { // ErrNoGroupsFound can be returned from GIDFromPath var ErrNoGroupsFound = errors.New("no groups found") -// GIDFromPath inspects the GID using /etc/passwd in the specified rootfs. +// GIDFromPath inspects the GID using /etc/group in the specified rootfs. // filter can be nil. func GIDFromPath(root string, filter func(user.Group) bool) (gid uint32, err error) { gpath, err := fs.RootPath(root, "/etc/group") diff --git a/pkg/cri/server/container_create_linux.go b/pkg/cri/server/container_create_linux.go index 8fb41e2..c428fe8 100644 --- a/pkg/cri/server/container_create_linux.go +++ b/pkg/cri/server/container_create_linux.go @@ -347,7 +347,8 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon // Because it is still useful to get additional gids for uid 0. userstr = strconv.FormatInt(securityContext.GetRunAsUser().GetValue(), 10) } - specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr)) + specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr), + customopts.WithSupplementalGroups(securityContext.GetSupplementalGroups())) asp := securityContext.GetApparmor() if asp == nil { -- 2.20.1