diff --git a/docs/AUTHORING.md b/docs/AUTHORING.md index 7e82337..1d00ba1 100644 --- a/docs/AUTHORING.md +++ b/docs/AUTHORING.md @@ -513,7 +513,7 @@ See the [lifecycle limitations](./DESIGN.md#resolution-ordering-and-lifecycle). - [ ] Offer selected implemented Points with `servicev0.Offer`. - [ ] Add Host `WithProviderPolicy` and in-process `WithPointServers` wiring as needed. - [ ] Keep handshake output on stdout and logs on stderr. -- [ ] Install one correctly named, non-world-writable binary in the trusted extension directory. +- [ ] Install one correctly named, non-world-writable binary (not a symlink) in the trusted extension directory. ## Quick reference diff --git a/docs/DESIGN.md b/docs/DESIGN.md index d2fa4f3..55469d4 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -304,14 +304,14 @@ permission, or security boundary. - Directories supplied through `host.WithDirs` are trusted; the framework has no default discovery directory. Treat each as a root-owned program directory: only package managers or operators should install files there, and unprivileged users must not be able to write to it. -- A world-writable directory or binary is skipped with a warning. - A binary or directory owned by anyone other than root or the daemon user is also skipped because its owner could rewrite daemon-executed code. +- A world-writable directory or binary fails startup. + So does a binary or directory owned by anyone other than root or the daemon user, because its owner could rewrite daemon-executed code. Only executable files with valid extension-id names are launched; stray tools and build leftovers are not executed. - These checks are backstops, not the complete trust model. Group-writable binaries are **not** refused; only the world-writable (`o+w`) bit is checked. An untrusted group could therefore rewrite a group-writable binary and gain daemon code execution. Keep the directory and binaries writable only by root or the daemon user, and do not grant their group to untrusted users. -- Symlink targets are checked as ordinary files. +- Symlinks and other non-regular files fail startup. Where ownership cannot be determined from file metadata, notably on Windows, the owner check is not enforced; ACL and group policy remain the operator's responsibility. - One accepted extension that cannot launch, describe, initialize, or coexist with the set fails daemon startup. Because every configured directory is scanned at Host construction, a broken or incompatible binary there blocks startup until removed or its directory is no longer configured. diff --git a/host/host.go b/host/host.go index a7620ca..84766b9 100644 --- a/host/host.go +++ b/host/host.go @@ -340,6 +340,9 @@ func New(ctx context.Context, optionList ...Option) (_ *Host, retErr error) { // Put the callback path in each handshake before starting the server. callbackEndpoint := "" if len(opts.dependencyProviders) > 0 { + if opts.runtimeDir == "" { + return nil, errors.New("extension runtime dir is required for dependency providers") + } callbackEndpoint = filepath.Join(opts.runtimeDir, "callback.sock") } @@ -703,6 +706,9 @@ func serveCallback(endpoint string, deps []serverpoint.Registration, b *broker.B if err := registerDependencyProviders(srv, deps, b); err != nil { return nil, err } + if err := os.MkdirAll(filepath.Dir(endpoint), 0o700); err != nil { + return nil, fmt.Errorf("create extension runtime dir: %w", err) + } if err := os.Remove(endpoint); err != nil && !os.IsNotExist(err) { return nil, fmt.Errorf("remove stale dependency callback socket: %w", err) } diff --git a/host/options_test.go b/host/options_test.go index 7d7b7f4..90bc657 100644 --- a/host/options_test.go +++ b/host/options_test.go @@ -9,6 +9,8 @@ import ( "github.com/moby/extensions" "github.com/moby/extensions/host" + "github.com/moby/extensions/serverpoint" + "google.golang.org/grpc" "gotest.tools/v3/assert" ) @@ -32,6 +34,18 @@ func TestNewRejectsNilOption(t *testing.T) { assert.Equal(t, err.Error(), "nil host option") } +func TestNewRejectsDependencyProvidersWithoutRuntimeDir(t *testing.T) { + t.Parallel() + + dep := serverpoint.Registration{ + Point: "org.example.dependency.v1", + Register: func(grpc.ServiceRegistrar, any) {}, + } + h, err := host.New(t.Context(), host.WithDependencyProviders(dep)) + assert.Assert(t, h == nil) + assert.Error(t, err, "extension runtime dir is required for dependency providers") +} + func TestRepeatedExtensionOptionsComposeAndCopyInput(t *testing.T) { t.Parallel() diff --git a/internal/launcher/discovery.go b/internal/launcher/discovery.go index 545a67f..fabc60f 100644 --- a/internal/launcher/discovery.go +++ b/internal/launcher/discovery.go @@ -22,12 +22,14 @@ import ( // A missing directory yields no binaries. // // Discovery is a root-code-execution boundary. -// World-writable entries, entries owned by an untrusted user, and files without -// valid extension ids are skipped. -// Other trust decisions, including group policy and symlinks, belong to the -// operator. +// Directories, non-executable files, and files without valid extension ids are +// skipped. +// A world-writable or untrusted-owned dir, or an extension binary that is not a +// regular file, is world-writable, or is owned by an untrusted user, is an +// error: skipping it could silently drop a security extension. +// Other trust decisions, including group policy, belong to the operator. func Binaries(ctx context.Context, dir string) ([]string, error) { - entries, err := extensionEntries(ctx, dir) + entries, err := extensionEntries(dir) if err != nil || entries == nil { return nil, err } @@ -36,14 +38,17 @@ func Binaries(ctx context.Context, dir string) ([]string, error) { if entry.IsDir() { continue } + + path := filepath.Join(dir, entry.Name()) + info, err := entry.Info() if err != nil { - return nil, fmt.Errorf("stat extension %q: %w", filepath.Join(dir, entry.Name()), err) + return nil, fmt.Errorf("stat extension %q: %w", path, err) } + if !isExecutable(info) { continue } - path := filepath.Join(dir, entry.Name()) // The file name is the extension id. Validate it before launching so a // shared directory cannot execute arbitrary helper binaries. name := extensionName(entry.Name()) @@ -51,20 +56,25 @@ func Binaries(ctx context.Context, dir string) ([]string, error) { log.G(ctx).WithError(err).Warnf("extensions: skipping %q: not a valid extension binary name", path) continue } + // A symlink target can live in a directory these checks do not cover, + // whose owner could replace the executed file. Other special files cannot + // be executed and would only fail the launch. + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("extension binary %q is not a regular file", path) + } + if worldWritable(info) { - log.G(ctx).Warnf("extensions: refusing to run world-writable extension binary %q", path) - continue + return nil, fmt.Errorf("extension binary %q is world-writable", path) } if uid, untrusted := untrustedOwner(info); untrusted { - log.G(ctx).Warnf("extensions: refusing to run extension binary %q owned by untrusted uid %d", path, uid) - continue + return nil, fmt.Errorf("extension binary %q is owned by untrusted uid %d", path, uid) } bins = append(bins, path) } return bins, nil } -func extensionEntries(ctx context.Context, dir string) ([]os.DirEntry, error) { +func extensionEntries(dir string) ([]os.DirEntry, error) { dirInfo, err := os.Stat(dir) if err != nil { if errors.Is(err, fs.ErrNotExist) { @@ -73,12 +83,10 @@ func extensionEntries(ctx context.Context, dir string) ([]os.DirEntry, error) { return nil, fmt.Errorf("stat extension dir %q: %w", dir, err) } if worldWritable(dirInfo) { - log.G(ctx).Warnf("extensions: ignoring world-writable extension directory %q", dir) - return nil, nil + return nil, fmt.Errorf("extension dir %q is world-writable", dir) } if uid, untrusted := untrustedOwner(dirInfo); untrusted { - log.G(ctx).Warnf("extensions: ignoring extension directory %q owned by untrusted uid %d", dir, uid) - return nil, nil + return nil, fmt.Errorf("extension dir %q is owned by untrusted uid %d", dir, uid) } entries, err := os.ReadDir(dir) if err != nil { diff --git a/internal/launcher/discovery_unix_test.go b/internal/launcher/discovery_unix_test.go new file mode 100644 index 0000000..b2b9c80 --- /dev/null +++ b/internal/launcher/discovery_unix_test.go @@ -0,0 +1,45 @@ +// SPDX-FileCopyrightText: Copyright The Moby Authors +// SPDX-License-Identifier: Apache-2.0 + +//go:build unix + +package launcher + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "gotest.tools/v3/assert" +) + +// TestBinariesRefusesNonRegularFiles verifies that an extension binary that is +// not a regular file fails discovery, even when a symlink's target would pass +// every other check. +func TestBinariesRefusesNonRegularFiles(t *testing.T) { + target := filepath.Join(t.TempDir(), "target") + assert.NilError(t, os.WriteFile(target, []byte("x"), 0o755)) + + for _, tc := range []struct { + name string + create func(path string) error + }{ + {name: "symlink", create: func(path string) error { return os.Symlink(target, path) }}, + {name: "fifo", create: func(path string) error { + if err := syscall.Mkfifo(path, 0o755); err != nil { + return err + } + // Mkfifo applies the umask; make sure the pipe looks executable. + return os.Chmod(path, 0o755) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + bin := filepath.Join(t.TempDir(), "org.example.special.v1") + assert.NilError(t, tc.create(bin)) + + _, err := Binaries(t.Context(), filepath.Dir(bin)) + assert.Error(t, err, `extension binary "`+bin+`" is not a regular file`) + }) + } +} diff --git a/internal/launcher/launcher_test.go b/internal/launcher/launcher_test.go index 262ff85..69cb734 100644 --- a/internal/launcher/launcher_test.go +++ b/internal/launcher/launcher_test.go @@ -165,31 +165,29 @@ func TestBinaries(t *testing.T) { assert.Check(t, is.Len(missing, 0)) } -// TestBinariesRefusesWorldWritable verifies the root-exec safety filter. +// TestBinariesRefusesWorldWritable verifies that world-writable binaries and +// dirs fail discovery rather than being silently dropped. func TestBinariesRefusesWorldWritable(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("world-writable bit is not meaningful on Windows") } dir := t.TempDir() - good := filepath.Join(dir, "org.example.good.v1") bad := filepath.Join(dir, "org.example.bad.v1") - assert.NilError(t, os.WriteFile(good, []byte("x"), 0o755)) assert.NilError(t, os.WriteFile(bad, []byte("x"), 0o755)) assert.NilError(t, os.Chmod(bad, 0o757)) // o+w - bins, err := Binaries(t.Context(), dir) - assert.NilError(t, err) - assert.DeepEqual(t, bins, []string{good}) + _, err := Binaries(t.Context(), dir) + assert.Error(t, err, `extension binary "`+bad+`" is world-writable`) wwDir := t.TempDir() assert.NilError(t, os.WriteFile(filepath.Join(wwDir, "org.example.x.v1"), []byte("x"), 0o755)) assert.NilError(t, os.Chmod(wwDir, 0o777)) - bins, err = Binaries(t.Context(), wwDir) - assert.NilError(t, err) - assert.Check(t, is.Len(bins, 0)) + _, err = Binaries(t.Context(), wwDir) + assert.Error(t, err, `extension dir "`+wwDir+`" is world-writable`) } -// TestBinariesRefusesUntrustedOwner verifies the ownership filter. +// TestBinariesRefusesUntrustedOwner verifies that binaries and dirs owned by an +// untrusted user fail discovery. func TestBinariesRefusesUntrustedOwner(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("ownership is not enforced on Windows") @@ -198,15 +196,17 @@ func TestBinariesRefusesUntrustedOwner(t *testing.T) { t.Skip("changing a file's owner requires root") } dir := t.TempDir() - good := filepath.Join(dir, "org.example.good.v1") bad := filepath.Join(dir, "org.example.bad.v1") - assert.NilError(t, os.WriteFile(good, []byte("x"), 0o755)) assert.NilError(t, os.WriteFile(bad, []byte("x"), 0o755)) assert.NilError(t, os.Chown(bad, 65534, 65534)) // nobody: not root, not us - bins, err := Binaries(t.Context(), dir) - assert.NilError(t, err) - assert.DeepEqual(t, bins, []string{good}) + _, err := Binaries(t.Context(), dir) + assert.Error(t, err, `extension binary "`+bad+`" is owned by untrusted uid 65534`) + + badDir := t.TempDir() + assert.NilError(t, os.Chown(badDir, 65534, 65534)) + _, err = Binaries(t.Context(), badDir) + assert.Error(t, err, `extension dir "`+badDir+`" is owned by untrusted uid 65534`) } func TestLaunchOutOfProcess(t *testing.T) {