From 09822c3da8ad8fa91b8796c5abf27ef06814a0c3 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 21 Nov 2023 17:20:36 +1100 Subject: [PATCH] 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 --- libcontainer/configs/validate/validator.go | 17 ++++++++-- .../configs/validate/validator_test.go | 33 +++++++++++++++--- libcontainer/integration/exec_test.go | 6 ++++ libcontainer/specconv/spec_linux.go | 8 +++++ libcontainer/specconv/spec_linux_test.go | 34 +++++++++++++++++++ 5 files changed, 92 insertions(+), 6 deletions(-) diff --git a/libcontainer/configs/validate/validator.go b/libcontainer/configs/validate/validator.go index 0df7242b38d..2594d05c723 100644 --- a/libcontainer/configs/validate/validator.go +++ b/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") diff --git a/libcontainer/configs/validate/validator_test.go b/libcontainer/configs/validate/validator_test.go index a882fa64cce..755d2a07be8 100644 --- a/libcontainer/configs/validate/validator_test.go +++ b/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", diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index 9795964caf2..1bc840116c2 100644 --- a/libcontainer/integration/exec_test.go +++ b/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) diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 2529e1e7e29..90275043eca 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/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. diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go index d84853bc75c..9fd64de7698 100644 --- a/libcontainer/specconv/spec_linux_test.go +++ b/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.")