Bunch of fixes

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
 

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.

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)
 	}

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()
 

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,35 +38,43 @@ 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())
 		if err := extensions.ValidateExtensionID(extensions.ExtensionID(name)); err != nil {
 			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 {

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`)
+		})
+	}
+}

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) {