configs: disallow ambiguous userns and timens configurations

For userns and timens, the mappings (and offsets, respectively) cannot
be changed after the namespace is first configured. Thus, configuring a
container with a namespace path to join means that you cannot also
provide configuration for said namespace. Previously we would silently
ignore the configuration (and just join the provided path), but we
really should be returning an error (especially when you consider that
the configuration userns mappings are used quite a bit in runc with the
assumption that they are the correct mapping for the userns -- but in
this case they are not).

In the case of userns, the mappings are also required if you _do not_
specify a path, while in the case of the time namespace you can have a
container with a timens but no mappings specified.

It should be noted that the case checking that the user has not
specified a userns path and a userns mapping needs to be handled in
specconv (as opposed to the configuration validator) because with this
patchset we now cache the mappings of path-based userns configurations
and thus the validator can't be sure whether the mapping is a cached
mapping or a user-specified one. So we do the validation in specconv,
and thus the test for this needs to be an integration test.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

libcontainer/configs/validate/validator.go

@@ -104,11 +104,19 @@ func security(config *configs.Config) error {
 func namespaces(config *configs.Config) error {
 	if config.Namespaces.Contains(configs.NEWUSER) {
 		if _, err := os.Stat("/proc/self/ns/user"); os.IsNotExist(err) {
-			return errors.New("USER namespaces aren't enabled in the kernel")
+			return errors.New("user namespaces aren't enabled in the kernel")
 		}
+		hasPath := config.Namespaces.PathOf(configs.NEWUSER) != ""
+		hasMappings := config.UIDMappings != nil || config.GIDMappings != nil
+		if !hasPath && !hasMappings {
+			return errors.New("user namespaces enabled, but no namespace path to join nor mappings to apply specified")
+		}
+		// The hasPath && hasMappings validation case is handled in specconv --
+		// we cache the mappings in Config during specconv in the hasPath case,
+		// so we cannot do that validation here.
 	} else {
 		if config.UIDMappings != nil || config.GIDMappings != nil {
-			return errors.New("User namespace mappings specified, but USER namespace isn't enabled in the config")
+			return errors.New("user namespace mappings specified, but user namespace isn't enabled in the config")
 		}
 	}
 
@@ -122,6 +130,11 @@ func namespaces(config *configs.Config) error {
 		if _, err := os.Stat("/proc/self/timens_offsets"); os.IsNotExist(err) {
 			return errors.New("time namespaces aren't enabled in the kernel")
 		}
+		hasPath := config.Namespaces.PathOf(configs.NEWTIME) != ""
+		hasOffsets := config.TimeOffsets != nil
+		if hasPath && hasOffsets {
+			return errors.New("time namespace enabled, but both namespace path and time offsets specified -- you may only provide one")
+		}
 	} else {
 		if config.TimeOffsets != nil {
 			return errors.New("time namespace offsets specified, but time namespace isn't enabled in the config")

libcontainer/configs/validate/validator_test.go

@@ -170,7 +170,7 @@ func TestValidateSecurityWithoutNEWNS(t *testing.T) {
 	}
 }
 
-func TestValidateUsernamespace(t *testing.T) {
+func TestValidateUserNamespace(t *testing.T) {
 	if _, err := os.Stat("/proc/self/ns/user"); os.IsNotExist(err) {
 		t.Skip("Test requires userns.")
 	}
@@ -181,6 +181,8 @@ func TestValidateUsernamespace(t *testing.T) {
 				{Type: configs.NEWUSER},
 			},
 		),
+		UIDMappings: []configs.IDMap{{HostID: 0, ContainerID: 123, Size: 100}},
+		GIDMappings: []configs.IDMap{{HostID: 0, ContainerID: 123, Size: 100}},
 	}
 
 	err := Validate(config)
@@ -189,11 +191,11 @@ func TestValidateUsernamespace(t *testing.T) {
 	}
 }
 
-func TestValidateUsernamespaceWithoutUserNS(t *testing.T) {
-	uidMap := configs.IDMap{ContainerID: 123}
+func TestValidateUsernsMappingWithoutNamespace(t *testing.T) {
 	config := &configs.Config{
 		Rootfs:      "/var",
-		UIDMappings: []configs.IDMap{uidMap},
+		UIDMappings: []configs.IDMap{{HostID: 0, ContainerID: 123, Size: 100}},
+		GIDMappings: []configs.IDMap{{HostID: 0, ContainerID: 123, Size: 100}},
 	}
 
 	err := Validate(config)
@@ -221,6 +223,29 @@ func TestValidateTimeNamespace(t *testing.T) {
 	}
 }
 
+func TestValidateTimeNamespaceWithBothPathAndTimeOffset(t *testing.T) {
+	if _, err := os.Stat("/proc/self/ns/time"); os.IsNotExist(err) {
+		t.Skip("Test requires timens.")
+	}
+	config := &configs.Config{
+		Rootfs: "/var",
+		Namespaces: configs.Namespaces(
+			[]configs.Namespace{
+				{Type: configs.NEWTIME, Path: "/proc/1/ns/time"},
+			},
+		),
+		TimeOffsets: map[string]specs.LinuxTimeOffset{
+			"boottime":  {Secs: 150, Nanosecs: 314159},
+			"monotonic": {Secs: 512, Nanosecs: 271818},
+		},
+	}
+
+	err := Validate(config)
+	if err == nil {
+		t.Error("Expected error to occur but it was nil")
+	}
+}
+
 func TestValidateTimeOffsetsWithoutTimeNamespace(t *testing.T) {
 	config := &configs.Config{
 		Rootfs: "/var",

libcontainer/integration/exec_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/opencontainers/runc/libcontainer/cgroups"
 	"github.com/opencontainers/runc/libcontainer/cgroups/systemd"
 	"github.com/opencontainers/runc/libcontainer/configs"
+	"github.com/opencontainers/runc/libcontainer/userns"
 	"github.com/opencontainers/runtime-spec/specs-go"
 
 	"golang.org/x/sys/unix"
@@ -1550,6 +1551,11 @@ func TestInitJoinNetworkAndUser(t *testing.T) {
 	config2 := newTemplateConfig(t, &tParam{userns: true})
 	config2.Namespaces.Add(configs.NEWNET, netns1)
 	config2.Namespaces.Add(configs.NEWUSER, userns1)
+	// Emulate specconv.setupUserNamespace().
+	uidMap, gidMap, err := userns.GetUserNamespaceMappings(userns1)
+	ok(t, err)
+	config2.UIDMappings = uidMap
+	config2.GIDMappings = gidMap
 	config2.Cgroups.Path = "integration/test2"
 	container2, err := newContainer(t, config2)
 	ok(t, err)

libcontainer/specconv/spec_linux.go

@@ -516,6 +516,9 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
 }
 
 func toConfigIDMap(specMaps []specs.LinuxIDMapping) []configs.IDMap {
+	if specMaps == nil {
+		return nil
+	}
 	idmaps := make([]configs.IDMap, len(specMaps))
 	for i, id := range specMaps {
 		idmaps[i] = configs.IDMap{
@@ -970,6 +973,11 @@ 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.

libcontainer/specconv/spec_linux_test.go

@@ -608,6 +608,40 @@ func TestDupNamespaces(t *testing.T) {
 	}
 }
 
+func TestUserNamespaceMappingAndPath(t *testing.T) {
+	if _, err := os.Stat("/proc/self/ns/user"); os.IsNotExist(err) {
+		t.Skip("Test requires userns.")
+	}
+
+	spec := &specs.Spec{
+		Root: &specs.Root{
+			Path: "rootfs",
+		},
+		Linux: &specs.Linux{
+			UIDMappings: []specs.LinuxIDMapping{
+				{ContainerID: 0, HostID: 1000, Size: 1000},
+			},
+			GIDMappings: []specs.LinuxIDMapping{
+				{ContainerID: 0, HostID: 2000, Size: 1000},
+			},
+			Namespaces: []specs.LinuxNamespace{
+				{
+					Type: "user",
+					Path: "/proc/1/ns/user",
+				},
+			},
+		},
+	}
+
+	_, err := CreateLibcontainerConfig(&CreateOpts{
+		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")
+	}
+}
+
 func TestNonZeroEUIDCompatibleSpecconvValidate(t *testing.T) {
 	if _, err := os.Stat("/proc/self/ns/user"); os.IsNotExist(err) {
 		t.Skip("Test requires userns.")