From 37f5234c41e1e0d2d965f20cfd89ec3872ab3206 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 16 Apr 2026 20:16:00 +0300 Subject: [PATCH 1/6] Add stageSymlink test helper for hook fixtures Enables upcoming tests that verify hook helpers refuse to follow symlink components planted in a rootfs. The helper places a symlink at rootfs/linkPath pointing to an arbitrary target. Co-Authored-By: Claude Opus 4.7 (1M context) --- hooks/hooks_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/hooks/hooks_test.go b/hooks/hooks_test.go index 58ca93f..441b5be 100644 --- a/hooks/hooks_test.go +++ b/hooks/hooks_test.go @@ -453,6 +453,35 @@ 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 TestShellEscape(t *testing.T) { t.Parallel() From ee0fecc43689c5771c7108dba7f129844eb8bb7c Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 16 Apr 2026 20:38:08 +0300 Subject: [PATCH 2/6] Reject symlink components in InjectAuthorizedKeys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hook previously used os.MkdirAll + os.WriteFile to place authorized_keys under a caller-configured home path inside the rootfs. pathutil.Contains guarded against lexical traversal but did not inspect the resolved filesystem — a symlink component planted inside the rootfs (e.g. by a malicious OCI layer) caused the write to follow the link out of the rootfs. Route the directory creation through image.MkdirAllNoSymlink (Lstat each component, refuse symlinks), gate the leaf with image.ValidateNoSymlinkLeaf, and open the final file with O_NOFOLLOW to close the TOCTOU window between validation and write. Covered by three new sub-tests: an absolute symlink at the home component, a relative escaping symlink at .ssh, and a symlink leaf at authorized_keys. All assert the out-of-rootfs target is untouched. Co-Authored-By: Claude Opus 4.7 (1M context) --- hooks/hooks.go | 24 +++++++++++++++-- hooks/hooks_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/hooks/hooks.go b/hooks/hooks.go index 0b85bed..4f3caff 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,6 +113,22 @@ 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. diff --git a/hooks/hooks_test.go b/hooks/hooks_test.go index 441b5be..b6f5539 100644 --- a/hooks/hooks_test.go +++ b/hooks/hooks_test.go @@ -427,6 +427,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() From aac4fab99f6325f628a6b01efe7d97e6670c1e98 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 16 Apr 2026 20:39:21 +0300 Subject: [PATCH 3/6] Reject symlink components in InjectFile and InjectBinary Route parent-directory creation through image.MkdirAllNoSymlink, gate the leaf with image.ValidateNoSymlinkLeaf, and open with O_NOFOLLOW via writeFileNoFollow. InjectBinary delegates to InjectFile so both are covered by the same code path. Tests cover the two realistic rootfs shapes: a parent directory replaced by a symlink and a leaf replaced by a symlink pointing at a host file. Both assert the out-of-rootfs target is untouched. Co-Authored-By: Claude Opus 4.7 (1M context) --- hooks/hooks.go | 11 ++++++--- hooks/hooks_test.go | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/hooks/hooks.go b/hooks/hooks.go index 4f3caff..4777ecf 100644 --- a/hooks/hooks.go +++ b/hooks/hooks.go @@ -131,17 +131,22 @@ func writeFileNoFollow(path string, data []byte, perm os.FileMode) error { // 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 diff --git a/hooks/hooks_test.go b/hooks/hooks_test.go index b6f5539..416510e 100644 --- a/hooks/hooks_test.go +++ b/hooks/hooks_test.go @@ -325,6 +325,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() From 7e81e1f1e027bafbe768a028b7b72e989caad119 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 16 Apr 2026 20:40:57 +0300 Subject: [PATCH 4/6] Reject symlink components in InjectEnvFile Last of the Inject* file-writing helpers. Same treatment as InjectAuthorizedKeys and InjectFile: MkdirAllNoSymlink for parent dirs, ValidateNoSymlinkLeaf for the target, O_NOFOLLOW on open. Tests cover parent-as-symlink and leaf-as-symlink variants. Co-Authored-By: Claude Opus 4.7 (1M context) --- hooks/hooks.go | 7 +++++-- hooks/hooks_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/hooks/hooks.go b/hooks/hooks.go index 4777ecf..31f1700 100644 --- a/hooks/hooks.go +++ b/hooks/hooks.go @@ -192,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 diff --git a/hooks/hooks_test.go b/hooks/hooks_test.go index 416510e..1958c05 100644 --- a/hooks/hooks_test.go +++ b/hooks/hooks_test.go @@ -390,6 +390,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 { From 7783f0ceb5a0baaeb97279bccc3c93c5f4361f8d Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 16 Apr 2026 21:46:26 +0300 Subject: [PATCH 5/6] Guard InjectVMConfig symlink-rejection via delegation chain InjectVMConfig writes /etc/go-microvm.json by delegating to InjectFile, which now refuses symlink components. A regression test locks the guarantee in at the delegation boundary so a future refactor that inlines or bypasses InjectFile cannot silently lose the protection. Co-Authored-By: Claude Opus 4.7 (1M context) --- hooks/hooks_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/hooks/hooks_test.go b/hooks/hooks_test.go index 1958c05..a8da03a 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() From 5de47e7c3b706dd9c3f992068e77a926921fded4 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Fri, 17 Apr 2026 07:45:39 +0300 Subject: [PATCH 6/6] Tighten BestEffortLchown to only swallow permission errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, any non-permission error from os.Lchown was logged at warn level and then silently dropped — a failure mode where the file's owner silently ended up wrong while the caller saw no error. Tighten the contract so only permission-denied errors (the expected non-root case) are absorbed; everything else (ENOENT, EROFS, EIO, ...) propagates to the caller. Tests cover both paths: a non-existent path returning ENOENT surfaces an error; a non-root chown to a foreign UID returning EPERM returns nil. The EPERM test is skipped when running as root since root can chown to any UID. Co-Authored-By: Claude Opus 4.7 (1M context) --- hooks/hooks.go | 17 +++++++++-------- hooks/hooks_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/hooks/hooks.go b/hooks/hooks.go index 31f1700..404c4cd 100644 --- a/hooks/hooks.go +++ b/hooks/hooks.go @@ -211,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 a8da03a..7823d25 100644 --- a/hooks/hooks_test.go +++ b/hooks/hooks_test.go @@ -658,6 +658,34 @@ func TestStageSymlink(t *testing.T) { 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()