Repository: opencontainers/runc (go)

Issue:
containerd/crio passes invalid config.json
> However, for this to work with containerd I had to do this change:
> ```
> $ git diff
> diff --git libcontainer/specconv/spec_linux.go libcontainer/specconv/spec_linux.go
> index 991962c4..203bf694 100644
> --- libcontainer/specconv/spec_linux.go
> +++ libcontainer/specconv/spec_linux.go
> @@ -1006,9 +1006,9 @@ func setupUserNamespace(spec *specs.Spec, config *configs.Config) error {
>         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")
> -               }
> +               //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.
> ```
> 
> Those lines are not part of this PR, though.
> 
> CRIO triggers the very same error too.
> 
> I think we should change that to a warning, change CRIO, containerd (and maybe more tools), and change it back to an error in a few releases. I haven't checked what is done today.
> 
> I think both (containerd and CRIO) when sending both, the path and the mappings, those are consistent (i.e. the path's mappings and the mappings in the config.json are the same). In that case, we can just print a warning here saying we will ignore one (probably the mappings and just use the path) would be safe.

_Originally posted by @rata in https://github.com/opencontainers/runc/pull/3985#pullrequestreview-1765313165_

Due to the invalid `config.json`s being passed by containerd and crio (possibly among others), we have to downgrade the relevant error added in 09822c3da8ad8fa91b8796c5abf27ef06814a0c3 to a warning if the mappings match the passed path.

Pull request description:
specconv: temporarily allow userns path and mapping if they match
It turns out that the error added in commit 09822c3da8ad ("configs: disallow ambiguous userns and timens configurations") causes issues with containerd and CRIO because they pass both userns mappings and a userns path.

These configurations are broken, but to avoid the regression in this one case, output a warning to tell the user that the configuration is incorrect but we will continue to use it if and only if the configured mappings are identical to the mappings of the provided namespace.

/cc @rata 

Fixes: 09822c3da8ad ("configs: disallow ambiguous userns and timens configurations")
Fixes #4133
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Patch:
diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go
index 722e1dca..a0a79d19 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 3c0e71e4..e401f533 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 f36abaf1..85b48f83 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 90275043..0ad03c1b 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 <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{
diff --git a/libcontainer/userns/userns_maps_linux.go b/libcontainer/userns/userns_maps_linux.go
index d81290de..7a8c2b02 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
+}


Test patch:
diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go
index 9fd64de7..8c7fb774 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)
 	}
 }