diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go index 722e1dcad61..a0a79d19d53 100644 --- a/libcontainer/configs/config.go +++ b/libcontainer/configs/config.go @@ -22,9 +22,9 @@ type Rlimit struct { // IDMap represents UID/GID Mappings for User Namespaces. type IDMap struct { - ContainerID int `json:"container_id"` - HostID int `json:"host_id"` - Size int `json:"size"` + ContainerID int64 `json:"container_id"` + HostID int64 `json:"host_id"` + Size int64 `json:"size"` } // Seccomp represents syscall restrictions diff --git a/libcontainer/configs/config_linux.go b/libcontainer/configs/config_linux.go index 3c0e71e4b2d..e401f5331b4 100644 --- a/libcontainer/configs/config_linux.go +++ b/libcontainer/configs/config_linux.go @@ -3,6 +3,7 @@ package configs import ( "errors" "fmt" + "math" ) var ( @@ -30,11 +31,18 @@ func (c Config) HostUID(containerId int) (int, error) { if len(c.UIDMappings) == 0 { return -1, errNoUIDMap } - id, found := c.hostIDFromMapping(containerId, c.UIDMappings) + id, found := c.hostIDFromMapping(int64(containerId), c.UIDMappings) if !found { return -1, fmt.Errorf("user namespaces enabled, but no mapping found for uid %d", containerId) } - return id, nil + // If we are a 32-bit binary running on a 64-bit system, it's possible + // the mapped user is too large to store in an int, which means we + // cannot do the mapping. We can't just return an int64, because + // os.Setuid() takes an int. + if id > math.MaxInt { + return -1, fmt.Errorf("mapping for uid %d (host id %d) is larger than native integer size (%d)", containerId, id, math.MaxInt) + } + return int(id), nil } // Return unchanged id. return containerId, nil @@ -53,11 +61,18 @@ func (c Config) HostGID(containerId int) (int, error) { if len(c.GIDMappings) == 0 { return -1, errNoGIDMap } - id, found := c.hostIDFromMapping(containerId, c.GIDMappings) + id, found := c.hostIDFromMapping(int64(containerId), c.GIDMappings) if !found { return -1, fmt.Errorf("user namespaces enabled, but no mapping found for gid %d", containerId) } - return id, nil + // If we are a 32-bit binary running on a 64-bit system, it's possible + // the mapped user is too large to store in an int, which means we + // cannot do the mapping. We can't just return an int64, because + // os.Setgid() takes an int. + if id > math.MaxInt { + return -1, fmt.Errorf("mapping for gid %d (host id %d) is larger than native integer size (%d)", containerId, id, math.MaxInt) + } + return int(id), nil } // Return unchanged id. return containerId, nil @@ -71,7 +86,7 @@ func (c Config) HostRootGID() (int, error) { // Utility function that gets a host ID for a container ID from user namespace map // if that ID is present in the map. -func (c Config) hostIDFromMapping(containerID int, uMap []IDMap) (int, bool) { +func (c Config) hostIDFromMapping(containerID int64, uMap []IDMap) (int64, bool) { for _, m := range uMap { if (containerID >= m.ContainerID) && (containerID <= (m.ContainerID + m.Size - 1)) { hostID := m.HostID + (containerID - m.ContainerID) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index f36abaf1032..85b48f83981 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -1351,7 +1351,7 @@ func ignoreTerminateErrors(err error) error { func requiresRootOrMappingTool(c *configs.Config) bool { gidMap := []configs.IDMap{ - {ContainerID: 0, HostID: os.Getegid(), Size: 1}, + {ContainerID: 0, HostID: int64(os.Getegid()), Size: 1}, } return !reflect.DeepEqual(c.GIDMappings, gidMap) } diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 90275043eca..0ad03c1bef3 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -522,9 +522,9 @@ func toConfigIDMap(specMaps []specs.LinuxIDMapping) []configs.IDMap { idmaps := make([]configs.IDMap, len(specMaps)) for i, id := range specMaps { idmaps[i] = configs.IDMap{ - ContainerID: int(id.ContainerID), - HostID: int(id.HostID), - Size: int(id.Size), + ContainerID: int64(id.ContainerID), + HostID: int64(id.HostID), + Size: int64(id.Size), } } return idmaps @@ -973,11 +973,6 @@ func setupUserNamespace(spec *specs.Spec, config *configs.Config) error { config.GIDMappings = toConfigIDMap(spec.Linux.GIDMappings) } if path := config.Namespaces.PathOf(configs.NEWUSER); path != "" { - // We cannot allow uid or gid mappings to be set if we are also asked - // to join a userns. - if config.UIDMappings != nil || config.GIDMappings != nil { - return errors.New("user namespaces enabled, but both namespace path and mapping specified -- you may only provide one") - } // Cache the current userns mappings in our configuration, so that we // can calculate uid and gid mappings within runc. These mappings are // never used for configuring the container if the path is set. @@ -985,6 +980,25 @@ func setupUserNamespace(spec *specs.Spec, config *configs.Config) error { if err != nil { return fmt.Errorf("failed to cache mappings for userns: %w", err) } + // We cannot allow uid or gid mappings to be set if we are also asked + // to join a userns. + if config.UIDMappings != nil || config.GIDMappings != nil { + // FIXME: It turns out that containerd and CRIO pass both a userns + // path and the mappings of the namespace in the same config.json. + // Such a configuration is technically not valid, but we used to + // require mappings be specified, and thus users worked around our + // bug -- so we can't regress it at the moment. But we also don't + // want to produce broken behaviour if the mapping doesn't match + // the userns. So (for now) we output a warning if the actual + // userns mappings match the configuration, otherwise we return an + // error. + if !userns.IsSameMapping(uidMap, config.UIDMappings) || + !userns.IsSameMapping(gidMap, config.GIDMappings) { + return errors.New("user namespaces enabled, but both namespace path and non-matching mapping specified -- you may only provide one") + } + logrus.Warnf("config.json has both a userns path to join and a matching userns mapping specified -- you may only provide one. Future versions of runc may return an error with this configuration, please report a bug on if you see this warning and cannot update your configuration.") + } + config.UIDMappings = uidMap config.GIDMappings = gidMap logrus.WithFields(logrus.Fields{ diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go index 9fd64de7698..8c7fb774f97 100644 --- a/libcontainer/specconv/spec_linux_test.go +++ b/libcontainer/specconv/spec_linux_test.go @@ -637,8 +637,8 @@ func TestUserNamespaceMappingAndPath(t *testing.T) { Spec: spec, }) - if !strings.Contains(err.Error(), "user namespaces enabled, but both namespace path and mapping specified") { - t.Errorf("user namespace with mapping and namespace path should be forbidden") + if !strings.Contains(err.Error(), "both namespace path and non-matching mapping specified") { + t.Errorf("user namespace with path and non-matching mapping should be forbidden, got error %v", err) } } diff --git a/libcontainer/userns/userns_maps_linux.go b/libcontainer/userns/userns_maps_linux.go index d81290de2cb..7a8c2b023b3 100644 --- a/libcontainer/userns/userns_maps_linux.go +++ b/libcontainer/userns/userns_maps_linux.go @@ -169,3 +169,18 @@ func GetUserNamespaceMappings(nsPath string) (uidMap, gidMap []configs.IDMap, er return uidMap, gidMap, nil } + +// IsSameMapping returns whether or not the two id mappings are the same. Note +// that if the order of the mappings is different, or a mapping has been split, +// the mappings will be considered different. +func IsSameMapping(a, b []configs.IDMap) bool { + if len(a) != len(b) { + return false + } + for idx := range a { + if a[idx] != b[idx] { + return false + } + } + return true +}