diff --git a/hooks/hooks.go b/hooks/hooks.go index 0b85bed..404c4cd 100644 --- a/hooks/hooks.go +++ b/hooks/hooks.go @@ -12,6 +12,7 @@ import ( "regexp" "sort" "strings" + "syscall" "github.com/stacklok/go-microvm/guest/vmconfig" "github.com/stacklok/go-microvm/image" @@ -86,7 +87,7 @@ func InjectAuthorizedKeys(pubKey string, opts ...KeyOption) func(string, *image. if err != nil { return fmt.Errorf("validate .ssh path: %w", err) } - if err := os.MkdirAll(sshDir, 0o700); err != nil { + if err := image.MkdirAllNoSymlink(rootfsPath, sshDir, 0o700); err != nil { return fmt.Errorf("create .ssh dir: %w", err) } if err := cfg.chown(sshDir, cfg.uid, cfg.gid); err != nil { @@ -98,7 +99,10 @@ func InjectAuthorizedKeys(pubKey string, opts ...KeyOption) func(string, *image. if err != nil { return fmt.Errorf("validate authorized_keys path: %w", err) } - if err := os.WriteFile(akPath, []byte(pubKey+"\n"), 0o600); err != nil { + if err := image.ValidateNoSymlinkLeaf(akPath); err != nil { + return fmt.Errorf("validate authorized_keys: %w", err) + } + if err := writeFileNoFollow(akPath, []byte(pubKey+"\n"), 0o600); err != nil { return fmt.Errorf("write authorized_keys: %w", err) } if err := cfg.chown(akPath, cfg.uid, cfg.gid); err != nil { @@ -109,19 +113,40 @@ func InjectAuthorizedKeys(pubKey string, opts ...KeyOption) func(string, *image. } } +// writeFileNoFollow writes data to path with O_NOFOLLOW on the final open, +// refusing to write through a symlink leaf even if one races into place +// between the caller's validation and this open. Creates the file if absent, +// truncates if present. Parent directories must already exist. +func writeFileNoFollow(path string, data []byte, perm os.FileMode) error { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|syscall.O_NOFOLLOW, perm) + if err != nil { + return err + } + if _, err := f.Write(data); err != nil { + _ = f.Close() + return err + } + return f.Close() +} + // InjectFile returns a RootFSHook that writes content to the specified guest // path inside the rootfs with the given permissions. Parent directories are -// created as needed. +// created as needed. Symlink components — whether in parent directories or at +// the leaf — are refused so that a rootfs planted with hostile symlinks cannot +// redirect the write outside the rootfs. func InjectFile(guestPath string, content []byte, perm os.FileMode) func(string, *image.OCIConfig) error { return func(rootfsPath string, _ *image.OCIConfig) error { dst, err := pathutil.Contains(rootfsPath, guestPath) if err != nil { return fmt.Errorf("validate path %s: %w", guestPath, err) } - if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + if err := image.MkdirAllNoSymlink(rootfsPath, filepath.Dir(dst), 0o755); err != nil { return fmt.Errorf("create parent dirs for %s: %w", guestPath, err) } - if err := os.WriteFile(dst, content, perm); err != nil { + if err := image.ValidateNoSymlinkLeaf(dst); err != nil { + return fmt.Errorf("validate %s: %w", guestPath, err) + } + if err := writeFileNoFollow(dst, content, perm); err != nil { return fmt.Errorf("write %s: %w", guestPath, err) } return nil @@ -167,10 +192,13 @@ func InjectEnvFile(guestPath string, envMap map[string]string) func(string, *ima if err != nil { return fmt.Errorf("validate path %s: %w", guestPath, err) } - if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + if err := image.MkdirAllNoSymlink(rootfsPath, filepath.Dir(dst), 0o755); err != nil { return fmt.Errorf("create parent dirs for %s: %w", guestPath, err) } - if err := os.WriteFile(dst, []byte(buf.String()), 0o600); err != nil { + if err := image.ValidateNoSymlinkLeaf(dst); err != nil { + return fmt.Errorf("validate %s: %w", guestPath, err) + } + if err := writeFileNoFollow(dst, []byte(buf.String()), 0o600); err != nil { return fmt.Errorf("write %s: %w", guestPath, err) } return nil @@ -183,20 +211,21 @@ func shellEscape(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } -// BestEffortLchown attempts os.Lchown and silently ignores permission errors, -// returning nil. On macOS non-root users cannot chown to a different UID; -// the guest init will fix ownership at boot time. Non-permission errors are -// logged at warn level and also swallowed. Callers that need strict chown -// should call os.Lchown directly instead. +// BestEffortLchown attempts os.Lchown and swallows permission errors (EPERM +// and EACCES). On non-root Linux and on macOS the hook process cannot lchown +// to a different UID; in those cases the override_stat xattr carries the +// intended ownership to the guest, and the guest init fixes up ownership at +// boot. Errors other than permission denied (e.g. ENOENT, EROFS, EIO) are +// returned to the caller rather than silently dropped. // Lchown is used instead of Chown to avoid following symlinks in the rootfs. func BestEffortLchown(path string, uid, gid int) error { if err := os.Lchown(path, uid, gid); err != nil { if !os.IsPermission(err) { - slog.Warn("lchown failed", "path", path, "uid", uid, "gid", gid, "err", err) + return fmt.Errorf("lchown %s: %w", path, err) } + slog.Debug("lchown permission denied; relying on xattr + guest fixup", + "path", path, "uid", uid, "gid", gid) } - // On macOS, set the override_stat xattr so libkrun's virtiofs reports - // correct ownership to the guest (non-root cannot Lchown to a different UID). xattr.SetOverrideStatFromPath(path, uid, gid) return nil } diff --git a/hooks/hooks_test.go b/hooks/hooks_test.go index 58ca93f..7823d25 100644 --- a/hooks/hooks_test.go +++ b/hooks/hooks_test.go @@ -93,6 +93,24 @@ func TestInjectVMConfig(t *testing.T) { } } +func TestInjectVMConfig_RejectsSymlinkComponents(t *testing.T) { + t.Parallel() + + // Guard the delegation chain: InjectVMConfig -> InjectFile. + // If InjectFile's symlink safety regresses, this test catches it. + rootfs := t.TempDir() + outside := t.TempDir() + stageSymlink(t, rootfs, "etc", outside) + + hook := InjectVMConfig(vmconfig.Config{TmpSizeMiB: 512}) + err := hook(rootfs, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") + + _, statErr := os.Stat(filepath.Join(outside, "go-microvm.json")) + assert.True(t, os.IsNotExist(statErr), "must not write under symlink target") +} + func TestInjectFile_WritesContent(t *testing.T) { t.Parallel() @@ -325,6 +343,61 @@ func TestInjectBinary_RejectsPathTraversal(t *testing.T) { assert.Contains(t, err.Error(), "path traversal") } +func TestInjectFile_RejectsSymlinkComponents(t *testing.T) { + t.Parallel() + + t.Run("parent directory is a symlink", func(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + outside := t.TempDir() + stageSymlink(t, rootfs, "etc", outside) + + hook := InjectFile("/etc/myconfig.txt", []byte("hello"), 0o644) + err := hook(rootfs, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") + + _, statErr := os.Stat(filepath.Join(outside, "myconfig.txt")) + assert.True(t, os.IsNotExist(statErr), "must not write under symlink target") + }) + + t.Run("leaf is a symlink to a host file", func(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(rootfs, "etc"), 0o755)) + + victim := filepath.Join(t.TempDir(), "victim") + require.NoError(t, os.WriteFile(victim, []byte("original"), 0o600)) + require.NoError(t, os.Symlink(victim, filepath.Join(rootfs, "etc", "myconfig.txt"))) + + hook := InjectFile("/etc/myconfig.txt", []byte("evil"), 0o644) + err := hook(rootfs, nil) + require.Error(t, err) + + got, readErr := os.ReadFile(victim) + require.NoError(t, readErr) + assert.Equal(t, "original", string(got), "victim must not be overwritten") + }) +} + +func TestInjectBinary_RejectsSymlinkComponents(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + outside := t.TempDir() + stageSymlink(t, rootfs, "usr", outside) + + hook := InjectBinary("/usr/bin/mytool", []byte("#!/bin/sh\necho hi")) + err := hook(rootfs, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") + + _, statErr := os.Stat(filepath.Join(outside, "bin", "mytool")) + assert.True(t, os.IsNotExist(statErr), "must not write under symlink target") +} + func TestInjectEnvFile_RejectsPathTraversal(t *testing.T) { t.Parallel() @@ -335,6 +408,45 @@ func TestInjectEnvFile_RejectsPathTraversal(t *testing.T) { assert.Contains(t, err.Error(), "path traversal") } +func TestInjectEnvFile_RejectsSymlinkComponents(t *testing.T) { + t.Parallel() + + t.Run("parent directory is a symlink", func(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + outside := t.TempDir() + stageSymlink(t, rootfs, "etc", outside) + + hook := InjectEnvFile("/etc/env", map[string]string{"FOO": "bar"}) + err := hook(rootfs, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") + + _, statErr := os.Stat(filepath.Join(outside, "env")) + assert.True(t, os.IsNotExist(statErr), "must not write under symlink target") + }) + + t.Run("leaf is a symlink to a host file", func(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(rootfs, "etc"), 0o755)) + + victim := filepath.Join(t.TempDir(), "victim") + require.NoError(t, os.WriteFile(victim, []byte("original"), 0o600)) + require.NoError(t, os.Symlink(victim, filepath.Join(rootfs, "etc", "env"))) + + hook := InjectEnvFile("/etc/env", map[string]string{"FOO": "evil"}) + err := hook(rootfs, nil) + require.Error(t, err) + + got, readErr := os.ReadFile(victim) + require.NoError(t, readErr) + assert.Equal(t, "original", string(got), "victim must not be overwritten") + }) +} + // failingChown returns a ChownFunc that returns an error when the path // ends with the given suffix, and succeeds otherwise. func failingChown(pathSuffix string) ChownFunc { @@ -427,6 +539,70 @@ func TestInjectAuthorizedKeys_RejectsPathTraversal(t *testing.T) { assert.Contains(t, err.Error(), "path traversal") } +func TestInjectAuthorizedKeys_RejectsSymlinkComponents(t *testing.T) { + t.Parallel() + + chown, _ := recordingChown() + pubKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAATEST test@example.com" + + t.Run("home component is an absolute symlink out of rootfs", func(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + outside := t.TempDir() + stageSymlink(t, rootfs, "home", outside) + + hook := InjectAuthorizedKeys(pubKey, WithChown(chown)) + err := hook(rootfs, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") + + // Nothing must have been written under the symlink target. + _, statErr := os.Stat(filepath.Join(outside, "sandbox", ".ssh", "authorized_keys")) + assert.True(t, os.IsNotExist(statErr), "must not write under symlink target") + }) + + t.Run("dot-ssh component is a relative escaping symlink", func(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(rootfs, "home", "sandbox"), 0o755)) + // .ssh points two levels up, escaping the rootfs lexically after resolution. + outside := t.TempDir() + stageSymlink(t, rootfs, "home/sandbox/.ssh", outside) + + hook := InjectAuthorizedKeys(pubKey, WithChown(chown)) + err := hook(rootfs, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") + + _, statErr := os.Stat(filepath.Join(outside, "authorized_keys")) + assert.True(t, os.IsNotExist(statErr), "must not write under symlink target") + }) + + t.Run("authorized_keys leaf is a symlink to a host file", func(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + sshDir := filepath.Join(rootfs, "home", "sandbox", ".ssh") + require.NoError(t, os.MkdirAll(sshDir, 0o700)) + + // An attacker-planted symlink at the leaf points to an arbitrary host file. + victim := filepath.Join(t.TempDir(), "victim") + require.NoError(t, os.WriteFile(victim, []byte("original"), 0o600)) + require.NoError(t, os.Symlink(victim, filepath.Join(sshDir, "authorized_keys"))) + + hook := InjectAuthorizedKeys(pubKey, WithChown(chown)) + err := hook(rootfs, nil) + require.Error(t, err) + + // The host-side file must be untouched. + got, readErr := os.ReadFile(victim) + require.NoError(t, readErr) + assert.Equal(t, "original", string(got), "victim must not be overwritten") + }) +} + func TestInjectEnvFile_RejectsInvalidKeyNames(t *testing.T) { t.Parallel() @@ -453,6 +629,63 @@ func TestInjectEnvFile_RejectsInvalidKeyNames(t *testing.T) { } } +// stageSymlink places a symlink at rootfs/linkPath pointing to target. +// Parent directories of linkPath inside rootfs are created as 0o755. +// Use this to build rootfs fixtures that exercise symlink-following behavior +// in hook code — e.g. a malicious layer shipping `home/sandbox/.ssh` as a +// symlink to somewhere outside the rootfs. +func stageSymlink(t *testing.T, rootfs, linkPath, target string) { + t.Helper() + abs := filepath.Join(rootfs, linkPath) + require.NoError(t, os.MkdirAll(filepath.Dir(abs), 0o755)) + require.NoError(t, os.Symlink(target, abs)) +} + +func TestStageSymlink(t *testing.T) { + t.Parallel() + + rootfs := t.TempDir() + outside := t.TempDir() + + stageSymlink(t, rootfs, "home/sandbox/.ssh", outside) + + info, err := os.Lstat(filepath.Join(rootfs, "home", "sandbox", ".ssh")) + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink, ".ssh should be a symlink") + + dest, err := os.Readlink(filepath.Join(rootfs, "home", "sandbox", ".ssh")) + require.NoError(t, err) + assert.Equal(t, outside, dest) +} + +func TestBestEffortLchown_PropagatesNonPermissionErrors(t *testing.T) { + t.Parallel() + + // ENOENT from a non-existent path is not a permission error; the function + // must return it rather than silently swallowing. + missing := filepath.Join(t.TempDir(), "does-not-exist") + err := BestEffortLchown(missing, 1000, 1000) + require.Error(t, err) + assert.Contains(t, err.Error(), "lchown") +} + +func TestBestEffortLchown_SwallowsPermissionErrors(t *testing.T) { + t.Parallel() + + if os.Geteuid() == 0 { + t.Skip("requires non-root: root can chown to any UID, so no EPERM") + } + + // Create a file we own; chowning to a UID we don't own should fail with + // EPERM on Linux and macOS as a non-root user. BestEffortLchown must + // swallow that specific error. + target := filepath.Join(t.TempDir(), "target") + require.NoError(t, os.WriteFile(target, []byte("x"), 0o600)) + + err := BestEffortLchown(target, 1, 1) + require.NoError(t, err, "permission error must be swallowed, got: %v", err) +} + func TestShellEscape(t *testing.T) { t.Parallel()