Merge pull request #4134 from cyphar/ns-path-regression

specconv: temporarily allow userns path and mapping if they match

Author: lifubang

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

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)

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)
 }

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,18 +973,32 @@ 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.
 		uidMap, gidMap, err := userns.GetUserNamespaceMappings(path)
 		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 <https://github.com/opencontainers/runc> if you see this warning and cannot update your configuration.")
+		}
+
 		config.UIDMappings = uidMap
 		config.GIDMappings = gidMap
 		logrus.WithFields(logrus.Fields{

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)
 	}
 }
 

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
+}