commit 3ae3044916481f5c001f64a9d26110b878a676e0 (github/v1.7.1-fedora) Author: Aleksa Sarai Date: Wed Jan 12 01:01:30 2022 +1100 internal: proc: do not join the process user namespace The only reason we joined the process user namespace was to map a handful of fields into the same usernamepsace as that process. This procedure can be implemented entirely in Go without having to run code inside the container. In addition, since psgo is used inside "podman top", we were actually executing the nsenter binary *from the container* without all of the container's security profiles applied. At the very least this would allow a container process to return bad data to psgo (possibly confusing management scripts using psgo) and at the very worst it would allow the container process to escalate privileges by getting podman to execute code without all of the container security profiles applied. Fixes: CVE-2022-1227 Signed-off-by: Aleksa Sarai Backported-by: Valentin Rothberg Signed-off-by: Valentin Rothberg diff --git a/internal/proc/ns.go b/internal/proc/ns.go index 28ee6a2..9e77b86 100644 --- a/internal/proc/ns.go +++ b/internal/proc/ns.go @@ -19,13 +19,9 @@ import ( "fmt" "io" "os" -) -type IDMap struct { - ContainerID int - HostID int - Size int -} + "github.com/containers/storage/pkg/idtools" +) // ParsePIDNamespace returns the content of /proc/$pid/ns/pid. func ParsePIDNamespace(pid string) (string, error) { @@ -46,14 +42,14 @@ func ParseUserNamespace(pid string) (string, error) { } // ReadMappings reads the user namespace mappings at the specified path -func ReadMappings(path string) ([]IDMap, error) { +func ReadMappings(path string) ([]idtools.IDMap, error) { file, err := os.Open(path) if err != nil { return nil, err } defer file.Close() - mappings := []IDMap{} + var mappings []idtools.IDMap buf := bufio.NewReader(file) for { @@ -68,10 +64,10 @@ func ReadMappings(path string) ([]IDMap, error) { return mappings, nil } - containerID, hostID, size := 0, 0, 0 + var containerID, hostID, size int if _, err := fmt.Sscanf(string(line), "%d %d %d", &containerID, &hostID, &size); err != nil { return nil, fmt.Errorf("cannot parse %s: %w", string(line), err) } - mappings = append(mappings, IDMap{ContainerID: containerID, HostID: hostID, Size: size}) + mappings = append(mappings, idtools.IDMap{ContainerID: containerID, HostID: hostID, Size: size}) } } diff --git a/internal/proc/status.go b/internal/proc/status.go index 1896b5c..b018075 100644 --- a/internal/proc/status.go +++ b/internal/proc/status.go @@ -17,9 +17,13 @@ package proc import ( "bufio" "fmt" + "io/ioutil" "os" - "os/exec" + "strconv" "strings" + "sync" + + "github.com/containers/storage/pkg/idtools" ) // Status is a direct translation of a `/proc/[pid]/status`, which provides much @@ -173,23 +177,8 @@ type Status struct { NonvoluntaryCtxtSwitches string } -// readStatusUserNS joins the user namespace of pid and returns the content of -// /proc/pid/status as a string slice. -func readStatusUserNS(pid string) ([]string, error) { - path := fmt.Sprintf("/proc/%s/status", pid) - args := []string{"nsenter", "-U", "-t", pid, "cat", path} - - c := exec.Command(args[0], args[1:]...) - output, err := c.CombinedOutput() - if err != nil { - return nil, fmt.Errorf("error executing %q: %w", strings.Join(args, " "), err) - } - - return strings.Split(string(output), "\n"), nil -} - -// readStatusDefault returns the content of /proc/pid/status as a string slice. -func readStatusDefault(pid string) ([]string, error) { +// readStatus returns the content of /proc/pid/status as a string slice. +func readStatus(pid string) ([]string, error) { path := fmt.Sprintf("/proc/%s/status", pid) f, err := os.Open(path) if err != nil { @@ -203,21 +192,81 @@ func readStatusDefault(pid string) ([]string, error) { return lines, nil } -// ParseStatus parses the /proc/$pid/status file and returns a *Status. -func ParseStatus(pid string, joinUserNS bool) (*Status, error) { - var lines []string - var err error +// mapField maps a single string-typed ID field given the set of mappings. If +// no mapping exists, the overflow uid/gid is used. +func mapStatusField(field *string, mapping []idtools.IDMap, overflow string) { + hostId, err := strconv.Atoi(*field) + if err != nil { + *field = overflow + return + } + contId, err := idtools.RawToContainer(hostId, mapping) + if err != nil { + *field = overflow + return + } + *field = strconv.Itoa(contId) +} - if joinUserNS { - lines, err = readStatusUserNS(pid) - } else { - lines, err = readStatusDefault(pid) +var ( + overflowOnce sync.Once + overflowUid = "65534" + overflowGid = "65534" +) + +func overflowIds() (string, string) { + overflowOnce.Do(func() { + if uid, err := ioutil.ReadFile("/proc/sys/kernel/overflowuid"); err == nil { + overflowUid = strings.TrimSpace(string(uid)) + } + if gid, err := ioutil.ReadFile("/proc/sys/kernel/overflowgid"); err == nil { + overflowGid = strings.TrimSpace(string(gid)) + } + }) + return overflowUid, overflowGid +} + +// mapStatus takes a Status struct and remaps all of the relevant fields to +// match the user namespace of the target process. +func mapStatus(pid string, status *Status) (*Status, error) { + uidMap, err := ReadMappings(fmt.Sprintf("/proc/%s/uid_map", pid)) + if err != nil { + return nil, err + } + gidMap, err := ReadMappings(fmt.Sprintf("/proc/%s/gid_map", pid)) + if err != nil { + return nil, err + } + overflowUid, overflowGid := overflowIds() + for i := range status.Uids { + mapStatusField(&status.Uids[i], uidMap, overflowUid) + } + for i := range status.Gids { + mapStatusField(&status.Gids[i], gidMap, overflowGid) } + for i := range status.Groups { + mapStatusField(&status.Groups[i], gidMap, overflowGid) + } + return status, nil +} +// ParseStatus parses the /proc/$pid/status file and returns a *Status. +func ParseStatus(pid string, mapUserNS bool) (*Status, error) { + lines, err := readStatus(pid) + if err != nil { + return nil, err + } + status, err := parseStatus(pid, lines) if err != nil { return nil, err } - return parseStatus(pid, lines) + if mapUserNS { + status, err = mapStatus(pid, status) + if err != nil { + return nil, err + } + } + return status, nil } // parseStatus extracts data from lines and returns a *Status. diff --git a/psgo.go b/psgo.go index ea893e7..d6cfcef 100644 --- a/psgo.go +++ b/psgo.go @@ -41,28 +41,18 @@ import ( "github.com/containers/psgo/internal/dev" "github.com/containers/psgo/internal/proc" "github.com/containers/psgo/internal/process" + "github.com/containers/storage/pkg/idtools" "golang.org/x/sys/unix" ) -// IDMap specifies a mapping range from the host to the container IDs. -type IDMap struct { - // ContainerID is the first ID in the container. - ContainerID int - // HostID is the first ID in the host. - HostID int - // Size specifies how long is the range. e.g. 1 means a single user - // is mapped. - Size int -} - // JoinNamespaceOpts specifies different options for joining the specified namespaces. type JoinNamespaceOpts struct { // UIDMap specifies a mapping for UIDs in the container. If specified // huser will perform the reverse mapping. - UIDMap []IDMap + UIDMap []idtools.IDMap // GIDMap specifies a mapping for GIDs in the container. If specified // hgroup will perform the reverse mapping. - GIDMap []IDMap + GIDMap []idtools.IDMap // FillMappings specified whether UIDMap and GIDMap must be initialized // with the current user namespace. @@ -102,7 +92,7 @@ type aixFormatDescriptor struct { } // findID converts the specified id to the host mapping -func findID(idStr string, mapping []IDMap, lookupFunc func(uid string) (string, error), overflowFile string) (string, error) { +func findID(idStr string, mapping []idtools.IDMap, lookupFunc func(uid string) (string, error), overflowFile string) (string, error) { if len(mapping) == 0 { return idStr, nil } @@ -350,29 +340,16 @@ func JoinNamespaceAndProcessInfo(pid string, descriptors []string) ([][]string, return JoinNamespaceAndProcessInfoWithOptions(pid, descriptors, &JoinNamespaceOpts{}) } -func readMappings(path string) ([]IDMap, error) { - mappings, err := proc.ReadMappings(path) - if err != nil { - return nil, err - } - var res []IDMap - for _, i := range mappings { - m := IDMap{ContainerID: i.ContainerID, HostID: i.HostID, Size: i.Size} - res = append(res, m) - } - return res, nil -} - func contextFromOptions(options *JoinNamespaceOpts) (*psContext, error) { ctx := new(psContext) ctx.opts = options if ctx.opts != nil && ctx.opts.FillMappings { - uidMappings, err := readMappings("/proc/self/uid_map") + uidMappings, err := proc.ReadMappings("/proc/self/uid_map") if err != nil { return nil, err } - gidMappings, err := readMappings("/proc/self/gid_map") + gidMappings, err := proc.ReadMappings("/proc/self/gid_map") if err != nil { return nil, err }