diff --git a/daemon/config/config.go b/daemon/config/config.go index 482bbe2d62..cf6e1248f0 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -7,9 +7,11 @@ import ( "net" "net/url" "os" + "path/filepath" "strings" "sync" + "github.com/containerd/containerd/runtime/v2/shim" "github.com/docker/docker/opts" "github.com/docker/docker/pkg/authorization" "github.com/docker/docker/registry" @@ -628,7 +630,7 @@ func Validate(config *Config) error { if defaultRuntime := config.GetDefaultRuntimeName(); defaultRuntime != "" { if !builtinRuntimes[defaultRuntime] { runtimes := config.GetAllRuntimes() - if _, ok := runtimes[defaultRuntime]; !ok { + if _, ok := runtimes[defaultRuntime]; !ok && !IsPermissibleC8dRuntimeName(defaultRuntime) { return fmt.Errorf("specified default runtime '%s' does not exist", defaultRuntime) } } @@ -662,3 +664,37 @@ func MaskCredentials(rawURL string) string { parsedURL.User = url.UserPassword("xxxxx", "xxxxx") return parsedURL.String() } + +// IsPermissibleC8dRuntimeName tests whether name is safe to pass into +// containerd as a runtime name, and whether the name is well-formed. +// It does not check if the runtime is installed. +// +// A runtime name containing slash characters is interpreted by containerd as +// the path to a runtime binary. If we allowed this, anyone with Engine API +// access could get containerd to execute an arbitrary binary as root. Although +// Engine API access is already equivalent to root on the host, the runtime name +// has not historically been a vector to run arbitrary code as root so users are +// not expecting it to become one. +// +// This restriction is not configurable. There are viable workarounds for +// legitimate use cases: administrators and runtime developers can make runtimes +// available for use with Docker by installing them onto PATH following the +// [binary naming convention] for containerd Runtime v2. +// +// [binary naming convention]: https://github.com/containerd/containerd/blob/main/runtime/v2/README.md#binary-naming +func IsPermissibleC8dRuntimeName(name string) bool { + // containerd uses a rather permissive test to validate runtime names: + // + // - Any name for which filepath.IsAbs(name) is interpreted as the absolute + // path to a shim binary. We want to block this behaviour. + // - Any name which contains at least one '.' character and no '/' characters + // and does not begin with a '.' character is a valid runtime name. The shim + // binary name is derived from the final two components of the name and + // searched for on the PATH. The name "a.." is technically valid per + // containerd's implementation: it would resolve to a binary named + // "containerd-shim---". + // + // https://github.com/containerd/containerd/blob/11ded166c15f92450958078cd13c6d87131ec563/runtime/v2/manager.go#L297-L317 + // https://github.com/containerd/containerd/blob/11ded166c15f92450958078cd13c6d87131ec563/runtime/v2/shim/util.go#L83-L93 + return !filepath.IsAbs(name) && !strings.ContainsRune(name, '/') && shim.BinaryName(name) != "" +} diff --git a/daemon/config/config_linux_test.go b/daemon/config/config_linux_test.go index 7b8d776281..f2ab051a38 100644 --- a/daemon/config/config_linux_test.go +++ b/daemon/config/config_linux_test.go @@ -140,18 +140,6 @@ func TestUnixValidateConfigurationErrors(t *testing.T) { }, expectedErr: `runtime name 'runc' is reserved`, }, - { - doc: `default runtime should be present in runtimes`, - config: &Config{ - Runtimes: map[string]types.Runtime{ - "foo": {}, - }, - CommonConfig: CommonConfig{ - DefaultRuntime: "bar", - }, - }, - expectedErr: `specified default runtime 'bar' does not exist`, - }, } for _, tc := range testCases { tc := tc diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index e33bc8383d..6686b15f87 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -764,7 +764,9 @@ func verifyDaemonSettings(conf *config.Config) error { configureRuntimes(conf) if rtName := conf.GetDefaultRuntimeName(); rtName != "" { if conf.GetRuntime(rtName) == nil { - return fmt.Errorf("specified default runtime '%s' does not exist", rtName) + if !config.IsPermissibleC8dRuntimeName(rtName) { + return fmt.Errorf("specified default runtime '%s' does not exist", rtName) + } } } return nil diff --git a/daemon/info_unix.go b/daemon/info_unix.go index eb7c01cc52..b9affb62a9 100644 --- a/daemon/info_unix.go +++ b/daemon/info_unix.go @@ -45,15 +45,16 @@ func (daemon *Daemon) fillPlatformInfo(v *types.Info, sysInfo *sysinfo.SysInfo) v.ContainerdCommit.ID = "N/A" v.InitCommit.ID = "N/A" - defaultRuntimeBinary := daemon.configStore.GetRuntime(v.DefaultRuntime).Path - if rv, err := exec.Command(defaultRuntimeBinary, "--version").Output(); err == nil { - if _, _, commit, err := parseRuntimeVersion(string(rv)); err != nil { - logrus.Warnf("failed to parse %s version: %v", defaultRuntimeBinary, err) + if rt := daemon.configStore.GetRuntime(v.DefaultRuntime); rt != nil { + if rv, err := exec.Command(rt.Path, "--version").Output(); err == nil { + if _, _, commit, err := parseRuntimeVersion(string(rv)); err != nil { + logrus.Warnf("failed to parse %s version: %v", rt.Path, err) + } else { + v.RuncCommit.ID = commit + } } else { - v.RuncCommit.ID = commit + logrus.Warnf("failed to retrieve %s version: %v", rt.Path, err) } - } else { - logrus.Warnf("failed to retrieve %s version: %v", defaultRuntimeBinary, err) } if rv, err := daemon.containerd.Version(context.Background()); err == nil { @@ -176,21 +177,22 @@ func (daemon *Daemon) fillPlatformVersion(v *types.Version) { } defaultRuntime := daemon.configStore.GetDefaultRuntimeName() - defaultRuntimeBinary := daemon.configStore.GetRuntime(defaultRuntime).Path - if rv, err := exec.Command(defaultRuntimeBinary, "--version").Output(); err == nil { - if _, ver, commit, err := parseRuntimeVersion(string(rv)); err != nil { - logrus.Warnf("failed to parse %s version: %v", defaultRuntimeBinary, err) + if rt := daemon.configStore.GetRuntime(defaultRuntime); rt != nil { + if rv, err := exec.Command(rt.Path, "--version").Output(); err == nil { + if _, ver, commit, err := parseRuntimeVersion(string(rv)); err != nil { + logrus.Warnf("failed to parse %s version: %v", rt.Path, err) + } else { + v.Components = append(v.Components, types.ComponentVersion{ + Name: defaultRuntime, + Version: ver, + Details: map[string]string{ + "GitCommit": commit, + }, + }) + } } else { - v.Components = append(v.Components, types.ComponentVersion{ - Name: defaultRuntime, - Version: ver, - Details: map[string]string{ - "GitCommit": commit, - }, - }) + logrus.Warnf("failed to retrieve %s version: %v", rt.Path, err) } - } else { - logrus.Warnf("failed to retrieve %s version: %v", defaultRuntimeBinary, err) } defaultInitBinary := daemon.configStore.GetInitPath() diff --git a/daemon/runtime_unix.go b/daemon/runtime_unix.go index 3b9fc4a765..df16aa14d7 100644 --- a/daemon/runtime_unix.go +++ b/daemon/runtime_unix.go @@ -11,7 +11,6 @@ import ( "strings" v2runcoptions "github.com/containerd/containerd/runtime/v2/runc/options" - "github.com/containerd/containerd/runtime/v2/shim" "github.com/docker/docker/api/types" "github.com/docker/docker/daemon/config" "github.com/docker/docker/errdefs" @@ -117,7 +116,7 @@ func (daemon *Daemon) rewriteRuntimePath(name, p string, args []string) (string, func (daemon *Daemon) getRuntime(name string) (*types.Runtime, error) { rt := daemon.configStore.GetRuntime(name) if rt == nil { - if !isPermissibleC8dRuntimeName(name) { + if !config.IsPermissibleC8dRuntimeName(name) { return nil, errdefs.InvalidParameter(errors.Errorf("unknown or invalid runtime name: %s", name)) } return &types.Runtime{Shim: &types.ShimConfig{Binary: name}}, nil @@ -138,37 +137,3 @@ func (daemon *Daemon) getRuntime(name string) (*types.Runtime, error) { return rt, nil } - -// isPermissibleC8dRuntimeName tests whether name is safe to pass into -// containerd as a runtime name, and whether the name is well-formed. -// It does not check if the runtime is installed. -// -// A runtime name containing slash characters is interpreted by containerd as -// the path to a runtime binary. If we allowed this, anyone with Engine API -// access could get containerd to execute an arbitrary binary as root. Although -// Engine API access is already equivalent to root on the host, the runtime name -// has not historically been a vector to run arbitrary code as root so users are -// not expecting it to become one. -// -// This restriction is not configurable. There are viable workarounds for -// legitimate use cases: administrators and runtime developers can make runtimes -// available for use with Docker by installing them onto PATH following the -// [binary naming convention] for containerd Runtime v2. -// -// [binary naming convention]: https://github.com/containerd/containerd/blob/main/runtime/v2/README.md#binary-naming -func isPermissibleC8dRuntimeName(name string) bool { - // containerd uses a rather permissive test to validate runtime names: - // - // - Any name for which filepath.IsAbs(name) is interpreted as the absolute - // path to a shim binary. We want to block this behaviour. - // - Any name which contains at least one '.' character and no '/' characters - // and does not begin with a '.' character is a valid runtime name. The shim - // binary name is derived from the final two components of the name and - // searched for on the PATH. The name "a.." is technically valid per - // containerd's implementation: it would resolve to a binary named - // "containerd-shim---". - // - // https://github.com/containerd/containerd/blob/11ded166c15f92450958078cd13c6d87131ec563/runtime/v2/manager.go#L297-L317 - // https://github.com/containerd/containerd/blob/11ded166c15f92450958078cd13c6d87131ec563/runtime/v2/shim/util.go#L83-L93 - return !filepath.IsAbs(name) && !strings.ContainsRune(name, '/') && shim.BinaryName(name) != "" -} diff --git a/integration/container/run_linux_test.go b/integration/container/run_linux_test.go index 0325c4050e..6a8322d9d8 100644 --- a/integration/container/run_linux_test.go +++ b/integration/container/run_linux_test.go @@ -270,4 +270,24 @@ func TestRunWithAlternativeContainerdShim(t *testing.T) { assert.NilError(t, err) assert.Equal(t, strings.TrimSpace(b.String()), "Hello, world!") + + d.Stop(t) + d.Start(t, "--default-runtime="+"io.containerd.realfake.v42") + + cID = container.Run(ctx, t, client, + container.WithImage("busybox"), + container.WithCmd("sh", "-c", `echo 'Hello, world!'`), + ) + + poll.WaitOn(t, container.IsStopped(ctx, client, cID), poll.WithDelay(100*time.Millisecond)) + + out, err = client.ContainerLogs(ctx, cID, types.ContainerLogsOptions{ShowStdout: true}) + assert.NilError(t, err) + defer out.Close() + + b.Reset() + _, err = stdcopy.StdCopy(&b, io.Discard, out) + assert.NilError(t, err) + + assert.Equal(t, strings.TrimSpace(b.String()), "Hello, world!") }