detached 982af7c

fix(vm): refuse an unsafe jailer chroot base

After creating <state>/jail, refuse it unless it is a real directory (a
symlink is not followed), owned by root and not writable by group or
others. The jailer runs as root and builds per-VM chroots there, so a
directory others can write to could redirect it. An existing directory is
only checked, never chmod-ed. The runner's own euid is accepted next to
root so the check is testable without root (the jailer itself needs root).
commit
982af7c268ea039b95c44db08b22e8ac15264ec9
author
brice · · 2026-10-12 22:02 UTC
parent
bbd315b
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
2 files · +107 -2
Unified
UnifiedSplit
M internal/sandbox/vm/driver.go +37 -2
@@ -15,6 +15,7 @@ import (
1515 "context"
1616 "errors"
1717 "fmt"
18+ "io/fs"
1819 "net"
1920 "net/netip"
2021 "os"
@@ -128,8 +129,8 @@ func New(ctx context.Context, cfg sandbox.Config, opt Options) (*Driver, error)
128129 if !cfg.JailerOff {
129130 // Jailer 1.17 canonicalizes --chroot-base-dir and fails when it does not
130131 // exist ("Failed to canonicalize path"); older releases created it.
131- if err := os.MkdirAll(jailBase(cfg.StateDir), 0o700); err != nil {
132- return fail(fmt.Errorf("jailer chroot base: %w", err))
132+ if err := ensureJailBase(jailBase(cfg.StateDir)); err != nil {
133+ return fail(err)
133134 }
134135 for _, p := range []string{cfg.KernelPath(), cfg.BaseImagePath()} {
135136 if err := checkWorldReadable(p); err != nil {
@@ -429,6 +430,40 @@ func checkSocketPaths(stateDir, execName string, jailed bool) error {
429430 return nil
430431 }
431432
433+// ensureJailBase creates the jailer's chroot base and refuses one that others
434+// could use to redirect the jailer: anything but a real directory (a symlink is
435+// not followed), owned by neither root nor the runner's own user, or writable by
436+// group or others. An existing directory is checked, never chmod-ed.
437+func ensureJailBase(dir string) error {
438+ if err := os.MkdirAll(dir, 0o700); err != nil {
439+ return fmt.Errorf("jailer chroot base: %w", err)
440+ }
441+ fi, err := os.Lstat(dir)
442+ if err != nil {
443+ return fmt.Errorf("jailer chroot base: %w", err)
444+ }
445+ return checkJailBaseInfo(dir, fi, os.Geteuid())
446+}
447+
448+func checkJailBaseInfo(dir string, fi fs.FileInfo, euid int) error {
449+ if !fi.IsDir() {
450+ return fmt.Errorf("jailer chroot base %s is not a directory (%v): remove it", dir, fi.Mode().Type())
451+ }
452+ if fi.Mode().Perm()&0o022 != 0 {
453+ return fmt.Errorf("jailer chroot base %s is writable by group or others (mode %04o): chmod go-w it", dir, fi.Mode().Perm())
454+ }
455+ st, ok := fi.Sys().(*syscall.Stat_t)
456+ if !ok {
457+ return fmt.Errorf("jailer chroot base %s: cannot read its owner", dir)
458+ }
459+ // The jailer needs root, so the runner is root in production; accepting its own
460+ // user as well keeps the driver testable without root.
461+ if int(st.Uid) != 0 && int(st.Uid) != euid {
462+ return fmt.Errorf("jailer chroot base %s is owned by uid %d, not root: chown it", dir, st.Uid)
463+ }
464+ return nil
465+}
466+
432467 func (s *vmSandbox) vmConfig(spec sandbox.JobSpec, netw netpolicy.Net, f vmFiles) vmConfig {
433468 cfg := s.d.cfg
434469 return vmConfig{
@@ -15,6 +15,7 @@ import (
15 "context"15 "context"
16 "errors"16 "errors"
17 "fmt"17 "fmt"
18+ "io/fs"
18 "net"19 "net"
19 "net/netip"20 "net/netip"
20 "os"21 "os"
@@ -128,8 +129,8 @@ func New(ctx context.Context, cfg sandbox.Config, opt Options) (*Driver, error)
128 if !cfg.JailerOff {129 if !cfg.JailerOff {
129 // Jailer 1.17 canonicalizes --chroot-base-dir and fails when it does not130 // Jailer 1.17 canonicalizes --chroot-base-dir and fails when it does not
130 // exist ("Failed to canonicalize path"); older releases created it.131 // exist ("Failed to canonicalize path"); older releases created it.
131- if err := os.MkdirAll(jailBase(cfg.StateDir), 0o700); err != nil {132+ if err := ensureJailBase(jailBase(cfg.StateDir)); err != nil {
132- return fail(fmt.Errorf("jailer chroot base: %w", err))133+ return fail(err)
133 }134 }
134 for _, p := range []string{cfg.KernelPath(), cfg.BaseImagePath()} {135 for _, p := range []string{cfg.KernelPath(), cfg.BaseImagePath()} {
135 if err := checkWorldReadable(p); err != nil {136 if err := checkWorldReadable(p); err != nil {
@@ -429,6 +430,40 @@ func checkSocketPaths(stateDir, execName string, jailed bool) error {
429 return nil430 return nil
430 }431 }
431 432
433+// ensureJailBase creates the jailer's chroot base and refuses one that others
434+// could use to redirect the jailer: anything but a real directory (a symlink is
435+// not followed), owned by neither root nor the runner's own user, or writable by
436+// group or others. An existing directory is checked, never chmod-ed.
437+func ensureJailBase(dir string) error {
438+ if err := os.MkdirAll(dir, 0o700); err != nil {
439+ return fmt.Errorf("jailer chroot base: %w", err)
440+ }
441+ fi, err := os.Lstat(dir)
442+ if err != nil {
443+ return fmt.Errorf("jailer chroot base: %w", err)
444+ }
445+ return checkJailBaseInfo(dir, fi, os.Geteuid())
446+}
447+
448+func checkJailBaseInfo(dir string, fi fs.FileInfo, euid int) error {
449+ if !fi.IsDir() {
450+ return fmt.Errorf("jailer chroot base %s is not a directory (%v): remove it", dir, fi.Mode().Type())
451+ }
452+ if fi.Mode().Perm()&0o022 != 0 {
453+ return fmt.Errorf("jailer chroot base %s is writable by group or others (mode %04o): chmod go-w it", dir, fi.Mode().Perm())
454+ }
455+ st, ok := fi.Sys().(*syscall.Stat_t)
456+ if !ok {
457+ return fmt.Errorf("jailer chroot base %s: cannot read its owner", dir)
458+ }
459+ // The jailer needs root, so the runner is root in production; accepting its own
460+ // user as well keeps the driver testable without root.
461+ if int(st.Uid) != 0 && int(st.Uid) != euid {
462+ return fmt.Errorf("jailer chroot base %s is owned by uid %d, not root: chown it", dir, st.Uid)
463+ }
464+ return nil
465+}
466+
432 func (s *vmSandbox) vmConfig(spec sandbox.JobSpec, netw netpolicy.Net, f vmFiles) vmConfig {467 func (s *vmSandbox) vmConfig(spec sandbox.JobSpec, netw netpolicy.Net, f vmFiles) vmConfig {
433 cfg := s.d.cfg468 cfg := s.d.cfg
434 return vmConfig{469 return vmConfig{
M internal/sandbox/vm/vm_test.go +70 -0
@@ -9,6 +9,7 @@ import (
99 "errors"
1010 "fmt"
1111 "io"
12+ "io/fs"
1213 "net"
1314 "net/http"
1415 "net/netip"
@@ -1096,6 +1097,75 @@ func TestNewRejectsAStateDirThatMakesASocketPathTooLong(t *testing.T) {
10961097 }
10971098 }
10981099
1100+// GO-003: the chroot base must be a real directory, owned by root (or the
1101+// runner's own user), not writable by group or others; an existing one is never
1102+// chmod-ed into compliance.
1103+func TestEnsureJailBase(t *testing.T) {
1104+ root := t.TempDir()
1105+ good := filepath.Join(root, "good")
1106+ if err := ensureJailBase(good); err != nil {
1107+ t.Fatal(err)
1108+ }
1109+ if fi, _ := os.Stat(good); fi.Mode().Perm() != 0o700 {
1110+ t.Fatalf("created with %v", fi.Mode().Perm())
1111+ }
1112+ if err := ensureJailBase(good); err != nil { // idempotent
1113+ t.Fatal(err)
1114+ }
1115+ for name, mode := range map[string]os.FileMode{"group-writable": 0o770, "other-writable": 0o707, "sticky-world": 0o777} {
1116+ d := filepath.Join(root, name)
1117+ _ = os.Mkdir(d, 0o700)
1118+ _ = os.Chmod(d, mode)
1119+ if err := ensureJailBase(d); err == nil || !strings.Contains(err.Error(), "writable by group or others") {
1120+ t.Errorf("%s: %v", name, err)
1121+ }
1122+ if fi, _ := os.Stat(d); fi.Mode().Perm() != mode {
1123+ t.Errorf("%s: mode was changed to %v", name, fi.Mode().Perm())
1124+ }
1125+ }
1126+ rx := filepath.Join(root, "rx")
1127+ _ = os.Mkdir(rx, 0o755) // readable by others is fine
1128+ _ = os.Chmod(rx, 0o755)
1129+ if err := ensureJailBase(rx); err != nil {
1130+ t.Errorf("0755 refused: %v", err)
1131+ }
1132+ file := filepath.Join(root, "file")
1133+ _ = os.WriteFile(file, nil, 0o600)
1134+ if err := ensureJailBase(file); err == nil {
1135+ t.Error("a file accepted")
1136+ }
1137+ link := filepath.Join(root, "link")
1138+ _ = os.Symlink(good, link)
1139+ if err := ensureJailBase(link); err == nil || !strings.Contains(err.Error(), "not a directory") {
1140+ t.Errorf("a symlink accepted: %v", err)
1141+ }
1142+}
1143+
1144+type fakeInfo struct {
1145+ fs.FileInfo
1146+ uid uint32
1147+}
1148+
1149+func (f fakeInfo) Sys() any { return &syscall.Stat_t{Uid: f.uid} }
1150+
1151+func TestJailBaseOwner(t *testing.T) {
1152+ fi, err := os.Stat(t.TempDir())
1153+ if err != nil {
1154+ t.Fatal(err)
1155+ }
1156+ d := "/x/jail"
1157+ mine := os.Geteuid()
1158+ if err := checkJailBaseInfo(d, fakeInfo{fi, 0}, mine); err != nil {
1159+ t.Errorf("root-owned refused: %v", err)
1160+ }
1161+ if err := checkJailBaseInfo(d, fakeInfo{fi, uint32(mine)}, mine); err != nil {
1162+ t.Errorf("own user refused: %v", err)
1163+ }
1164+ if err := checkJailBaseInfo(d, fakeInfo{fi, uint32(mine) + 4242}, mine); err == nil || !strings.Contains(err.Error(), "owned by uid") {
1165+ t.Errorf("foreign owner accepted: %v", err)
1166+ }
1167+}
1168+
10991169 // GO-001: a Firecracker with only the jailer's --start-time-us is ours only when
11001170 // it lives in our jail base; a jailer only when it was given our chroot base.
11011171 func TestScanIsScopedToOurJailBase(t *testing.T) {
@@ -9,6 +9,7 @@ import (
9 "errors"9 "errors"
10 "fmt"10 "fmt"
11 "io"11 "io"
12+ "io/fs"
12 "net"13 "net"
13 "net/http"14 "net/http"
14 "net/netip"15 "net/netip"
@@ -1096,6 +1097,75 @@ func TestNewRejectsAStateDirThatMakesASocketPathTooLong(t *testing.T) {
1096 }1097 }
1097 }1098 }
1098 1099
1100+// GO-003: the chroot base must be a real directory, owned by root (or the
1101+// runner's own user), not writable by group or others; an existing one is never
1102+// chmod-ed into compliance.
1103+func TestEnsureJailBase(t *testing.T) {
1104+ root := t.TempDir()
1105+ good := filepath.Join(root, "good")
1106+ if err := ensureJailBase(good); err != nil {
1107+ t.Fatal(err)
1108+ }
1109+ if fi, _ := os.Stat(good); fi.Mode().Perm() != 0o700 {
1110+ t.Fatalf("created with %v", fi.Mode().Perm())
1111+ }
1112+ if err := ensureJailBase(good); err != nil { // idempotent
1113+ t.Fatal(err)
1114+ }
1115+ for name, mode := range map[string]os.FileMode{"group-writable": 0o770, "other-writable": 0o707, "sticky-world": 0o777} {
1116+ d := filepath.Join(root, name)
1117+ _ = os.Mkdir(d, 0o700)
1118+ _ = os.Chmod(d, mode)
1119+ if err := ensureJailBase(d); err == nil || !strings.Contains(err.Error(), "writable by group or others") {
1120+ t.Errorf("%s: %v", name, err)
1121+ }
1122+ if fi, _ := os.Stat(d); fi.Mode().Perm() != mode {
1123+ t.Errorf("%s: mode was changed to %v", name, fi.Mode().Perm())
1124+ }
1125+ }
1126+ rx := filepath.Join(root, "rx")
1127+ _ = os.Mkdir(rx, 0o755) // readable by others is fine
1128+ _ = os.Chmod(rx, 0o755)
1129+ if err := ensureJailBase(rx); err != nil {
1130+ t.Errorf("0755 refused: %v", err)
1131+ }
1132+ file := filepath.Join(root, "file")
1133+ _ = os.WriteFile(file, nil, 0o600)
1134+ if err := ensureJailBase(file); err == nil {
1135+ t.Error("a file accepted")
1136+ }
1137+ link := filepath.Join(root, "link")
1138+ _ = os.Symlink(good, link)
1139+ if err := ensureJailBase(link); err == nil || !strings.Contains(err.Error(), "not a directory") {
1140+ t.Errorf("a symlink accepted: %v", err)
1141+ }
1142+}
1143+
1144+type fakeInfo struct {
1145+ fs.FileInfo
1146+ uid uint32
1147+}
1148+
1149+func (f fakeInfo) Sys() any { return &syscall.Stat_t{Uid: f.uid} }
1150+
1151+func TestJailBaseOwner(t *testing.T) {
1152+ fi, err := os.Stat(t.TempDir())
1153+ if err != nil {
1154+ t.Fatal(err)
1155+ }
1156+ d := "/x/jail"
1157+ mine := os.Geteuid()
1158+ if err := checkJailBaseInfo(d, fakeInfo{fi, 0}, mine); err != nil {
1159+ t.Errorf("root-owned refused: %v", err)
1160+ }
1161+ if err := checkJailBaseInfo(d, fakeInfo{fi, uint32(mine)}, mine); err != nil {
1162+ t.Errorf("own user refused: %v", err)
1163+ }
1164+ if err := checkJailBaseInfo(d, fakeInfo{fi, uint32(mine) + 4242}, mine); err == nil || !strings.Contains(err.Error(), "owned by uid") {
1165+ t.Errorf("foreign owner accepted: %v", err)
1166+ }
1167+}
1168+
1099 // GO-001: a Firecracker with only the jailer's --start-time-us is ours only when1169 // GO-001: a Firecracker with only the jailer's --start-time-us is ours only when
1100 // it lives in our jail base; a jailer only when it was given our chroot base.1170 // it lives in our jail base; a jailer only when it was given our chroot base.
1101 func TestScanIsScopedToOurJailBase(t *testing.T) {1171 func TestScanIsScopedToOurJailBase(t *testing.T) {

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 4

Tags 11

↑↓ moveenter openesc close