Skip to content

Commit b02826a

Browse files
JAORMXJAORMX
authored
Merge pull request #69 from stacklok/jaosorior/hooks-symlink-safe
hooks: harden path resolution against symlink components
2 parents c53560f + 5de47e7 commit b02826a

2 files changed

Lines changed: 277 additions & 15 deletions

File tree

‎hooks/hooks.go‎

Lines changed: 44 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"regexp"
1313
"sort"
1414
"strings"
15+
"syscall"
1516

1617
"github.com/stacklok/go-microvm/guest/vmconfig"
1718
"github.com/stacklok/go-microvm/image"
@@ -86,7 +87,7 @@ func InjectAuthorizedKeys(pubKey string, opts ...KeyOption) func(string, *image.
8687
if err != nil {
8788
return fmt.Errorf("validate .ssh path: %w", err)
8889
}
89-
if err := os.MkdirAll(sshDir, 0o700); err != nil {
90+
if err := image.MkdirAllNoSymlink(rootfsPath, sshDir, 0o700); err != nil {
9091
return fmt.Errorf("create .ssh dir: %w", err)
9192
}
9293
if err := cfg.chown(sshDir, cfg.uid, cfg.gid); err != nil {
@@ -98,7 +99,10 @@ func InjectAuthorizedKeys(pubKey string, opts ...KeyOption) func(string, *image.
9899
if err != nil {
99100
return fmt.Errorf("validate authorized_keys path: %w", err)
100101
}
101-
if err := os.WriteFile(akPath, []byte(pubKey+"\n"), 0o600); err != nil {
102+
if err := image.ValidateNoSymlinkLeaf(akPath); err != nil {
103+
return fmt.Errorf("validate authorized_keys: %w", err)
104+
}
105+
if err := writeFileNoFollow(akPath, []byte(pubKey+"\n"), 0o600); err != nil {
102106
return fmt.Errorf("write authorized_keys: %w", err)
103107
}
104108
if err := cfg.chown(akPath, cfg.uid, cfg.gid); err != nil {
@@ -109,19 +113,40 @@ func InjectAuthorizedKeys(pubKey string, opts ...KeyOption) func(string, *image.
109113
}
110114
}
111115

116+
// writeFileNoFollow writes data to path with O_NOFOLLOW on the final open,
117+
// refusing to write through a symlink leaf even if one races into place
118+
// between the caller's validation and this open. Creates the file if absent,
119+
// truncates if present. Parent directories must already exist.
120+
func writeFileNoFollow(path string, data []byte, perm os.FileMode) error {
121+
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|syscall.O_NOFOLLOW, perm)
122+
if err != nil {
123+
return err
124+
}
125+
if _, err := f.Write(data); err != nil {
126+
_ = f.Close()
127+
return err
128+
}
129+
return f.Close()
130+
}
131+
112132
// InjectFile returns a RootFSHook that writes content to the specified guest
113133
// path inside the rootfs with the given permissions. Parent directories are
114-
// created as needed.
134+
// created as needed. Symlink components — whether in parent directories or at
135+
// the leaf — are refused so that a rootfs planted with hostile symlinks cannot
136+
// redirect the write outside the rootfs.
115137
func InjectFile(guestPath string, content []byte, perm os.FileMode) func(string, *image.OCIConfig) error {
116138
return func(rootfsPath string, _ *image.OCIConfig) error {
117139
dst, err := pathutil.Contains(rootfsPath, guestPath)
118140
if err != nil {
119141
return fmt.Errorf("validate path %s: %w", guestPath, err)
120142
}
121-
if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil {
143+
if err := image.MkdirAllNoSymlink(rootfsPath, filepath.Dir(dst), 0o755); err != nil {
122144
return fmt.Errorf("create parent dirs for %s: %w", guestPath, err)
123145
}
124-
if err := os.WriteFile(dst, content, perm); err != nil {
146+
if err := image.ValidateNoSymlinkLeaf(dst); err != nil {
147+
return fmt.Errorf("validate %s: %w", guestPath, err)
148+
}
149+
if err := writeFileNoFollow(dst, content, perm); err != nil {
125150
return fmt.Errorf("write %s: %w", guestPath, err)
126151
}
127152
return nil
@@ -167,10 +192,13 @@ func InjectEnvFile(guestPath string, envMap map[string]string) func(string, *ima
167192
if err != nil {
168193
return fmt.Errorf("validate path %s: %w", guestPath, err)
169194
}
170-
if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil {
195+
if err := image.MkdirAllNoSymlink(rootfsPath, filepath.Dir(dst), 0o755); err != nil {
171196
return fmt.Errorf("create parent dirs for %s: %w", guestPath, err)
172197
}
173-
if err := os.WriteFile(dst, []byte(buf.String()), 0o600); err != nil {
198+
if err := image.ValidateNoSymlinkLeaf(dst); err != nil {
199+
return fmt.Errorf("validate %s: %w", guestPath, err)
200+
}
201+
if err := writeFileNoFollow(dst, []byte(buf.String()), 0o600); err != nil {
174202
return fmt.Errorf("write %s: %w", guestPath, err)
175203
}
176204
return nil
@@ -183,20 +211,21 @@ func shellEscape(s string) string {
183211
return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
184212
}
185213

186-
// BestEffortLchown attempts os.Lchown and silently ignores permission errors,
187-
// returning nil. On macOS non-root users cannot chown to a different UID;
188-
// the guest init will fix ownership at boot time. Non-permission errors are
189-
// logged at warn level and also swallowed. Callers that need strict chown
190-
// should call os.Lchown directly instead.
214+
// BestEffortLchown attempts os.Lchown and swallows permission errors (EPERM
215+
// and EACCES). On non-root Linux and on macOS the hook process cannot lchown
216+
// to a different UID; in those cases the override_stat xattr carries the
217+
// intended ownership to the guest, and the guest init fixes up ownership at
218+
// boot. Errors other than permission denied (e.g. ENOENT, EROFS, EIO) are
219+
// returned to the caller rather than silently dropped.
191220
// Lchown is used instead of Chown to avoid following symlinks in the rootfs.
192221
func BestEffortLchown(path string, uid, gid int) error {
193222
if err := os.Lchown(path, uid, gid); err != nil {
194223
if !os.IsPermission(err) {
195-
slog.Warn("lchown failed", "path", path, "uid", uid, "gid", gid, "err", err)
224+
return fmt.Errorf("lchown %s: %w", path, err)
196225
}
226+
slog.Debug("lchown permission denied; relying on xattr + guest fixup",
227+
"path", path, "uid", uid, "gid", gid)
197228
}
198-
// On macOS, set the override_stat xattr so libkrun's virtiofs reports
199-
// correct ownership to the guest (non-root cannot Lchown to a different UID).
200229
xattr.SetOverrideStatFromPath(path, uid, gid)
201230
return nil
202231
}

‎hooks/hooks_test.go‎

Lines changed: 233 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,24 @@ func TestInjectVMConfig(t *testing.T) {
9393
}
9494
}
9595

96+
func TestInjectVMConfig_RejectsSymlinkComponents(t *testing.T) {
97+
t.Parallel()
98+
99+
// Guard the delegation chain: InjectVMConfig -> InjectFile.
100+
// If InjectFile's symlink safety regresses, this test catches it.
101+
rootfs := t.TempDir()
102+
outside := t.TempDir()
103+
stageSymlink(t, rootfs, "etc", outside)
104+
105+
hook := InjectVMConfig(vmconfig.Config{TmpSizeMiB: 512})
106+
err := hook(rootfs, nil)
107+
require.Error(t, err)
108+
assert.Contains(t, err.Error(), "symlink")
109+
110+
_, statErr := os.Stat(filepath.Join(outside, "go-microvm.json"))
111+
assert.True(t, os.IsNotExist(statErr), "must not write under symlink target")
112+
}
113+
96114
func TestInjectFile_WritesContent(t *testing.T) {
97115
t.Parallel()
98116

@@ -325,6 +343,61 @@ func TestInjectBinary_RejectsPathTraversal(t *testing.T) {
325343
assert.Contains(t, err.Error(), "path traversal")
326344
}
327345

346+
func TestInjectFile_RejectsSymlinkComponents(t *testing.T) {
347+
t.Parallel()
348+
349+
t.Run("parent directory is a symlink", func(t *testing.T) {
350+
t.Parallel()
351+
352+
rootfs := t.TempDir()
353+
outside := t.TempDir()
354+
stageSymlink(t, rootfs, "etc", outside)
355+
356+
hook := InjectFile("/etc/myconfig.txt", []byte("hello"), 0o644)
357+
err := hook(rootfs, nil)
358+
require.Error(t, err)
359+
assert.Contains(t, err.Error(), "symlink")
360+
361+
_, statErr := os.Stat(filepath.Join(outside, "myconfig.txt"))
362+
assert.True(t, os.IsNotExist(statErr), "must not write under symlink target")
363+
})
364+
365+
t.Run("leaf is a symlink to a host file", func(t *testing.T) {
366+
t.Parallel()
367+
368+
rootfs := t.TempDir()
369+
require.NoError(t, os.MkdirAll(filepath.Join(rootfs, "etc"), 0o755))
370+
371+
victim := filepath.Join(t.TempDir(), "victim")
372+
require.NoError(t, os.WriteFile(victim, []byte("original"), 0o600))
373+
require.NoError(t, os.Symlink(victim, filepath.Join(rootfs, "etc", "myconfig.txt")))
374+
375+
hook := InjectFile("/etc/myconfig.txt", []byte("evil"), 0o644)
376+
err := hook(rootfs, nil)
377+
require.Error(t, err)
378+
379+
got, readErr := os.ReadFile(victim)
380+
require.NoError(t, readErr)
381+
assert.Equal(t, "original", string(got), "victim must not be overwritten")
382+
})
383+
}
384+
385+
func TestInjectBinary_RejectsSymlinkComponents(t *testing.T) {
386+
t.Parallel()
387+
388+
rootfs := t.TempDir()
389+
outside := t.TempDir()
390+
stageSymlink(t, rootfs, "usr", outside)
391+
392+
hook := InjectBinary("/usr/bin/mytool", []byte("#!/bin/sh\necho hi"))
393+
err := hook(rootfs, nil)
394+
require.Error(t, err)
395+
assert.Contains(t, err.Error(), "symlink")
396+
397+
_, statErr := os.Stat(filepath.Join(outside, "bin", "mytool"))
398+
assert.True(t, os.IsNotExist(statErr), "must not write under symlink target")
399+
}
400+
328401
func TestInjectEnvFile_RejectsPathTraversal(t *testing.T) {
329402
t.Parallel()
330403

@@ -335,6 +408,45 @@ func TestInjectEnvFile_RejectsPathTraversal(t *testing.T) {
335408
assert.Contains(t, err.Error(), "path traversal")
336409
}
337410

411+
func TestInjectEnvFile_RejectsSymlinkComponents(t *testing.T) {
412+
t.Parallel()
413+
414+
t.Run("parent directory is a symlink", func(t *testing.T) {
415+
t.Parallel()
416+
417+
rootfs := t.TempDir()
418+
outside := t.TempDir()
419+
stageSymlink(t, rootfs, "etc", outside)
420+
421+
hook := InjectEnvFile("/etc/env", map[string]string{"FOO": "bar"})
422+
err := hook(rootfs, nil)
423+
require.Error(t, err)
424+
assert.Contains(t, err.Error(), "symlink")
425+
426+
_, statErr := os.Stat(filepath.Join(outside, "env"))
427+
assert.True(t, os.IsNotExist(statErr), "must not write under symlink target")
428+
})
429+
430+
t.Run("leaf is a symlink to a host file", func(t *testing.T) {
431+
t.Parallel()
432+
433+
rootfs := t.TempDir()
434+
require.NoError(t, os.MkdirAll(filepath.Join(rootfs, "etc"), 0o755))
435+
436+
victim := filepath.Join(t.TempDir(), "victim")
437+
require.NoError(t, os.WriteFile(victim, []byte("original"), 0o600))
438+
require.NoError(t, os.Symlink(victim, filepath.Join(rootfs, "etc", "env")))
439+
440+
hook := InjectEnvFile("/etc/env", map[string]string{"FOO": "evil"})
441+
err := hook(rootfs, nil)
442+
require.Error(t, err)
443+
444+
got, readErr := os.ReadFile(victim)
445+
require.NoError(t, readErr)
446+
assert.Equal(t, "original", string(got), "victim must not be overwritten")
447+
})
448+
}
449+
338450
// failingChown returns a ChownFunc that returns an error when the path
339451
// ends with the given suffix, and succeeds otherwise.
340452
func failingChown(pathSuffix string) ChownFunc {
@@ -427,6 +539,70 @@ func TestInjectAuthorizedKeys_RejectsPathTraversal(t *testing.T) {
427539
assert.Contains(t, err.Error(), "path traversal")
428540
}
429541

542+
func TestInjectAuthorizedKeys_RejectsSymlinkComponents(t *testing.T) {
543+
t.Parallel()
544+
545+
chown, _ := recordingChown()
546+
pubKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAATEST test@example.com"
547+
548+
t.Run("home component is an absolute symlink out of rootfs", func(t *testing.T) {
549+
t.Parallel()
550+
551+
rootfs := t.TempDir()
552+
outside := t.TempDir()
553+
stageSymlink(t, rootfs, "home", outside)
554+
555+
hook := InjectAuthorizedKeys(pubKey, WithChown(chown))
556+
err := hook(rootfs, nil)
557+
require.Error(t, err)
558+
assert.Contains(t, err.Error(), "symlink")
559+
560+
// Nothing must have been written under the symlink target.
561+
_, statErr := os.Stat(filepath.Join(outside, "sandbox", ".ssh", "authorized_keys"))
562+
assert.True(t, os.IsNotExist(statErr), "must not write under symlink target")
563+
})
564+
565+
t.Run("dot-ssh component is a relative escaping symlink", func(t *testing.T) {
566+
t.Parallel()
567+
568+
rootfs := t.TempDir()
569+
require.NoError(t, os.MkdirAll(filepath.Join(rootfs, "home", "sandbox"), 0o755))
570+
// .ssh points two levels up, escaping the rootfs lexically after resolution.
571+
outside := t.TempDir()
572+
stageSymlink(t, rootfs, "home/sandbox/.ssh", outside)
573+
574+
hook := InjectAuthorizedKeys(pubKey, WithChown(chown))
575+
err := hook(rootfs, nil)
576+
require.Error(t, err)
577+
assert.Contains(t, err.Error(), "symlink")
578+
579+
_, statErr := os.Stat(filepath.Join(outside, "authorized_keys"))
580+
assert.True(t, os.IsNotExist(statErr), "must not write under symlink target")
581+
})
582+
583+
t.Run("authorized_keys leaf is a symlink to a host file", func(t *testing.T) {
584+
t.Parallel()
585+
586+
rootfs := t.TempDir()
587+
sshDir := filepath.Join(rootfs, "home", "sandbox", ".ssh")
588+
require.NoError(t, os.MkdirAll(sshDir, 0o700))
589+
590+
// An attacker-planted symlink at the leaf points to an arbitrary host file.
591+
victim := filepath.Join(t.TempDir(), "victim")
592+
require.NoError(t, os.WriteFile(victim, []byte("original"), 0o600))
593+
require.NoError(t, os.Symlink(victim, filepath.Join(sshDir, "authorized_keys")))
594+
595+
hook := InjectAuthorizedKeys(pubKey, WithChown(chown))
596+
err := hook(rootfs, nil)
597+
require.Error(t, err)
598+
599+
// The host-side file must be untouched.
600+
got, readErr := os.ReadFile(victim)
601+
require.NoError(t, readErr)
602+
assert.Equal(t, "original", string(got), "victim must not be overwritten")
603+
})
604+
}
605+
430606
func TestInjectEnvFile_RejectsInvalidKeyNames(t *testing.T) {
431607
t.Parallel()
432608

@@ -453,6 +629,63 @@ func TestInjectEnvFile_RejectsInvalidKeyNames(t *testing.T) {
453629
}
454630
}
455631

632+
// stageSymlink places a symlink at rootfs/linkPath pointing to target.
633+
// Parent directories of linkPath inside rootfs are created as 0o755.
634+
// Use this to build rootfs fixtures that exercise symlink-following behavior
635+
// in hook code — e.g. a malicious layer shipping `home/sandbox/.ssh` as a
636+
// symlink to somewhere outside the rootfs.
637+
func stageSymlink(t *testing.T, rootfs, linkPath, target string) {
638+
t.Helper()
639+
abs := filepath.Join(rootfs, linkPath)
640+
require.NoError(t, os.MkdirAll(filepath.Dir(abs), 0o755))
641+
require.NoError(t, os.Symlink(target, abs))
642+
}
643+
644+
func TestStageSymlink(t *testing.T) {
645+
t.Parallel()
646+
647+
rootfs := t.TempDir()
648+
outside := t.TempDir()
649+
650+
stageSymlink(t, rootfs, "home/sandbox/.ssh", outside)
651+
652+
info, err := os.Lstat(filepath.Join(rootfs, "home", "sandbox", ".ssh"))
653+
require.NoError(t, err)
654+
assert.NotZero(t, info.Mode()&os.ModeSymlink, ".ssh should be a symlink")
655+
656+
dest, err := os.Readlink(filepath.Join(rootfs, "home", "sandbox", ".ssh"))
657+
require.NoError(t, err)
658+
assert.Equal(t, outside, dest)
659+
}
660+
661+
func TestBestEffortLchown_PropagatesNonPermissionErrors(t *testing.T) {
662+
t.Parallel()
663+
664+
// ENOENT from a non-existent path is not a permission error; the function
665+
// must return it rather than silently swallowing.
666+
missing := filepath.Join(t.TempDir(), "does-not-exist")
667+
err := BestEffortLchown(missing, 1000, 1000)
668+
require.Error(t, err)
669+
assert.Contains(t, err.Error(), "lchown")
670+
}
671+
672+
func TestBestEffortLchown_SwallowsPermissionErrors(t *testing.T) {
673+
t.Parallel()
674+
675+
if os.Geteuid() == 0 {
676+
t.Skip("requires non-root: root can chown to any UID, so no EPERM")
677+
}
678+
679+
// Create a file we own; chowning to a UID we don't own should fail with
680+
// EPERM on Linux and macOS as a non-root user. BestEffortLchown must
681+
// swallow that specific error.
682+
target := filepath.Join(t.TempDir(), "target")
683+
require.NoError(t, os.WriteFile(target, []byte("x"), 0o600))
684+
685+
err := BestEffortLchown(target, 1, 1)
686+
require.NoError(t, err, "permission error must be swallowed, got: %v", err)
687+
}
688+
456689
func TestShellEscape(t *testing.T) {
457690
t.Parallel()
458691

0 commit comments

Comments
 (0)