From 04db051cfc08a2df2cb6c9639ff8fe357f4e8d43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Wed, 21 Oct 2026 12:29:10 +0200 Subject: [PATCH 1/3] host: Require a runtime dir for dependency callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without a runtime dir, the callback socket ended up as callback.sock in the working directory, and New removed any existing file with that name first. If the runtime dir did not exist and no binary was launched, New failed because only the launcher created it. Reject dependency providers without a runtime dir, and create the dir with 0o700 before listening, same as the launcher. Signed-off-by: Paweł Gronowski --- host/host.go | 6 ++++++ host/options_test.go | 14 ++++++++++++++ 2 files changed, 20 insertions(+) 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() From 014ecb1dbe862dc06c00806f38ec980021d319cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Wed, 21 Oct 2026 12:30:03 +0200 Subject: [PATCH 2/3] internal/launcher: Refuse symlinked extension binaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skip anything that is not a regular file with a warning. Signed-off-by: Paweł Gronowski --- docs/DESIGN.md | 2 +- internal/launcher/discovery.go | 23 ++++++++++++---- internal/launcher/discovery_unix_test.go | 35 ++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 7 deletions(-) create mode 100644 internal/launcher/discovery_unix_test.go diff --git a/docs/DESIGN.md b/docs/DESIGN.md index d2fa4f3..ff188b7 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -311,7 +311,7 @@ permission, or security boundary. 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. +- Only regular files are launched; symlinks and other special files are skipped with a warning. 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/internal/launcher/discovery.go b/internal/launcher/discovery.go index 545a67f..8a2a814 100644 --- a/internal/launcher/discovery.go +++ b/internal/launcher/discovery.go @@ -22,10 +22,10 @@ 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. +// Only regular files are launched. +// Symlinks and other special files, world-writable entries, entries owned by an +// untrusted user, and files without valid extension ids are skipped. +// Other trust decisions, including group policy, belong to the operator. func Binaries(ctx context.Context, dir string) ([]string, error) { entries, err := extensionEntries(ctx, dir) if err != nil || entries == nil { @@ -36,14 +36,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,6 +54,14 @@ 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() { + log.G(ctx).Warnf("extensions: refusing to run extension %q: not a regular file", path) + continue + } + if worldWritable(info) { log.G(ctx).Warnf("extensions: refusing to run world-writable extension binary %q", path) continue diff --git a/internal/launcher/discovery_unix_test.go b/internal/launcher/discovery_unix_test.go new file mode 100644 index 0000000..dbedfd6 --- /dev/null +++ b/internal/launcher/discovery_unix_test.go @@ -0,0 +1,35 @@ +// 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 only regular files are +// launched, 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)) + + dir := t.TempDir() + good := filepath.Join(dir, "org.example.good.v1") + assert.NilError(t, os.WriteFile(good, []byte("x"), 0o755)) + assert.NilError(t, os.Symlink(target, filepath.Join(dir, "org.example.link.v1"))) + fifo := filepath.Join(dir, "org.example.fifo.v1") + assert.NilError(t, syscall.Mkfifo(fifo, 0o755)) + // Mkfifo applies the umask; make sure the pipe looks executable. + assert.NilError(t, os.Chmod(fifo, 0o755)) + + bins, err := Binaries(t.Context(), dir) + assert.NilError(t, err) + assert.DeepEqual(t, bins, []string{good}) +} From 1c1f07cb7f56e061fe989cf6c12db096b35942a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Wed, 21 Oct 2026 15:22:51 +0200 Subject: [PATCH 3/3] internal/launcher: Fail discovery on untrusted extension binaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discovery skipped world-writable or foreign-owned dirs and binaries, and non-regular files, with just a warning. Loading is supposed to be all-or-nothing, and a skipped extension just disappears. If nothing depends on it, the daemon starts without it, which for an authz or veto extension means failing open. Return an error instead, so host.New fails. Directories, non-executable files, and files without a valid extension id are still ignored. Signed-off-by: Paweł Gronowski --- docs/AUTHORING.md | 2 +- docs/DESIGN.md | 6 ++-- internal/launcher/discovery.go | 27 ++++++++---------- internal/launcher/discovery_unix_test.go | 36 +++++++++++++++--------- internal/launcher/launcher_test.go | 30 ++++++++++---------- 5 files changed, 54 insertions(+), 47 deletions(-) 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 ff188b7..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. -- Only regular files are launched; symlinks and other special files are skipped with a warning. +- 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/internal/launcher/discovery.go b/internal/launcher/discovery.go index 8a2a814..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. -// Only regular files are launched. -// Symlinks and other special files, world-writable entries, entries owned by an -// untrusted user, and files without valid extension ids are skipped. +// 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 } @@ -58,24 +60,21 @@ func Binaries(ctx context.Context, dir string) ([]string, error) { // whose owner could replace the executed file. Other special files cannot // be executed and would only fail the launch. if !info.Mode().IsRegular() { - log.G(ctx).Warnf("extensions: refusing to run extension %q: not a regular file", path) - continue + 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) { @@ -84,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 index dbedfd6..b2b9c80 100644 --- a/internal/launcher/discovery_unix_test.go +++ b/internal/launcher/discovery_unix_test.go @@ -14,22 +14,32 @@ import ( "gotest.tools/v3/assert" ) -// TestBinariesRefusesNonRegularFiles verifies that only regular files are -// launched, even when a symlink's target would pass every other check. +// 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)) - dir := t.TempDir() - good := filepath.Join(dir, "org.example.good.v1") - assert.NilError(t, os.WriteFile(good, []byte("x"), 0o755)) - assert.NilError(t, os.Symlink(target, filepath.Join(dir, "org.example.link.v1"))) - fifo := filepath.Join(dir, "org.example.fifo.v1") - assert.NilError(t, syscall.Mkfifo(fifo, 0o755)) - // Mkfifo applies the umask; make sure the pipe looks executable. - assert.NilError(t, os.Chmod(fifo, 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)) - bins, err := Binaries(t.Context(), dir) - assert.NilError(t, err) - assert.DeepEqual(t, bins, []string{good}) + _, 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) {