v0.328.0 (unreleased): the security review's four findings — the entry is bound to its image and start; symlinks out of a root; the missed classes; roots only under the drive namespace or system data

Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS

controller/internal/privpolicy/privpolicy.go

@@ -54,8 +54,11 @@ type Entry struct {
 	App     string `json:"app"`
 	Service string `json:"service"`
 	Setting string `json:"setting"`
-	Why     string `json:"why"`
-	Doc     string `json:"doc"`
+	// Image is the image REPOSITORY (no tag, no digest) the service must run for the entry to hold. Without it a push
+	// could keep the allowed setting and swap the image under it (security review 2026-10-17, finding 1).
+	Image string `json:"image"`
+	Why   string `json:"why"`
+	Doc   string `json:"doc"`
 }
 
 type allowList struct {
@@ -65,14 +68,17 @@ type allowList struct {
 
 var allowed = mustLoad(allowListJSON)
 
-func mustLoad(b []byte) map[string]bool {
+func mustLoad(b []byte) map[string]Entry {
 	var l allowList
 	if err := json.Unmarshal(b, &l); err != nil {
 		panic("privpolicy: allowlist.json does not parse: " + err.Error())
 	}
-	m := map[string]bool{}
+	m := map[string]Entry{}
 	for _, e := range l.Entries {
-		m[key(e.App, e.Service, e.Setting)] = true
+		if e.Image == "" {
+			panic("privpolicy: allowlist.json entry " + e.App + "/" + e.Service + " names no image")
+		}
+		m[key(e.App, e.Service, e.Setting)] = e
 	}
 	return m
 }
@@ -115,9 +121,17 @@ func (e *RefusedError) Error() string {
 
 // Policy carries what the box knows about its own paths.
 type Policy struct {
-	SystemDataPath string // paths.system_data_path — a data root may live under it (or under /mnt/)
+	SystemDataPath string // paths.system_data_path — a data root may live under it, or under /mnt/felhom-drives/
+	// Resolve, when set, resolves symlinks of an EXISTING bind source and of each root (filepath.EvalSymlinks on the
+	// box): a path inside a root that is a symlink out of it is outside (security review 2026-10-17, finding 2). nil =
+	// no filesystem (the catalog CI, a template with no app yet).
+	Resolve func(string) (string, error)
 }
 
+// DriveNamespace is where the box mounts every drive in the guest; a data root lives under it or under the system
+// data path — never anywhere else under /mnt (finding 4: /mnt/<anything> was accepted).
+const DriveNamespace = "/mnt/felhom-drives/"
+
 // RootVars are the only variables a bind source may use.
 var RootVars = []string{"HDD_PATH", "USERDATA_PATH", "IMPORT_PATH"}
 
@@ -141,10 +155,21 @@ func Admit(app string, compose []byte, env map[string]string, p Policy) error {
 	if err != nil {
 		return fmt.Errorf("the catalog definition of %s could not be checked: %w", app, err)
 	}
+	svcs := serviceFacts(compose, env)
 	var refused []Violation
 	for _, v := range vs {
-		if !allowed[key(app, v.Service, v.Setting)] {
+		e, ok := allowed[key(app, v.Service, v.Setting)]
+		if !ok {
 			refused = append(refused, v)
+			continue
+		}
+		// Finding 1: the entry holds only for the image it names, and only while the service runs that image's own
+		// start — a changed entrypoint or command would run anything with the allowed right.
+		f := svcs[v.Service]
+		if f.repo != e.Image {
+			refused = append(refused, Violation{Service: v.Service, Setting: v.Setting + " with image " + f.repo + " (allowed only for " + e.Image + ")"})
+		} else if f.startOverride {
+			refused = append(refused, Violation{Service: v.Service, Setting: v.Setting + " with a changed entrypoint or command"})
 		}
 	}
 	if len(refused) > 0 {
@@ -153,6 +178,39 @@ func Admit(app string, compose []byte, env map[string]string, p Policy) error {
 	return nil
 }
 
+type facts struct {
+	repo          string // the rendered image's repository (no tag, no digest)
+	startOverride bool   // entrypoint or command set
+}
+
+// serviceFacts reads, per service, what an allow-list entry is bound to.
+func serviceFacts(compose []byte, env map[string]string) map[string]facts {
+	var doc map[string]any
+	_ = yaml.Unmarshal(compose, &doc)
+	c := &checker{env: env}
+	out := map[string]facts{}
+	svcs, _ := doc["services"].(map[string]any)
+	for name, v := range svcs {
+		sd, _ := v.(map[string]any)
+		_, ep := sd["entrypoint"]
+		_, cmd := sd["command"]
+		out[name] = facts{repo: imageRepo(c.render(sd["image"])), startOverride: ep || cmd}
+	}
+	return out
+}
+
+// imageRepo is an image reference without its digest and tag ("ghcr.io/x/y:1.2@sha256:…" → "ghcr.io/x/y").
+func imageRepo(ref string) string {
+	ref = strings.TrimSpace(ref)
+	if i := strings.Index(ref, "@"); i >= 0 {
+		ref = ref[:i]
+	}
+	if i := strings.LastIndex(ref, ":"); i > strings.LastIndex(ref, "/") {
+		ref = ref[:i]
+	}
+	return ref
+}
+
 // Check lists every setting in a class, allowed or not, sorted.
 func Check(compose []byte, env map[string]string, p Policy) ([]Violation, error) {
 	var doc map[string]any
@@ -211,11 +269,13 @@ func (c *checker) add(svc, setting string) {
 }
 
 // scalarKeys are the classes whose whole value is the setting.
-var scalarKeys = []string{"network_mode", "pid", "ipc", "uts", "userns_mode", "cgroup"}
+var scalarKeys = []string{"network_mode", "pid", "ipc", "uts", "userns_mode", "cgroup", "cgroup_parent", "runtime",
+	"isolation"}
 
 // privilegedKeys are every key whose value must be literal (no ${VAR}).
 var privilegedKeys = []string{"privileged", "cap_add", "devices", "device_cgroup_rules", "network_mode", "pid", "ipc",
-	"uts", "userns_mode", "cgroup", "security_opt", "sysctls", "volumes_from"}
+	"uts", "userns_mode", "cgroup", "cgroup_parent", "runtime", "isolation", "security_opt", "sysctls", "volumes_from",
+	"gpus", "provider", "post_start", "pre_stop"}
 
 func (c *checker) service(name string, sd map[string]any) {
 	c.svc = name
@@ -269,6 +329,28 @@ func (c *checker) service(name string, sd map[string]any) {
 	if _, has := sd["build"]; has {
 		c.add(name, "build")
 	}
+	// Finding 3 (security review 2026-10-17): device requests outside `devices:`, a provider service (compose runs a
+	// plugin binary ON THE HOST for it), and lifecycle hooks (they can run a command privileged).
+	if v, has := sd["gpus"]; has {
+		c.add(name, "gpus "+c.canon(v))
+	}
+	if dep, ok := sd["deploy"].(map[string]any); ok {
+		if res, ok := dep["resources"].(map[string]any); ok {
+			if rv, ok := res["reservations"].(map[string]any); ok {
+				if d, has := rv["devices"]; has {
+					c.add(name, "device_request "+c.canon(d))
+				}
+			}
+		}
+	}
+	if v, has := sd["provider"]; has {
+		c.add(name, "provider "+c.canon(v))
+	}
+	for _, hook := range []string{"post_start", "pre_stop"} {
+		if v, has := sd[hook]; has {
+			c.add(name, hook+" "+c.canon(v))
+		}
+	}
 	if ex, ok := sd["extends"].(map[string]any); ok {
 		if f, has := ex["file"]; has {
 			c.add(name, "extends_file "+c.shown(f))
@@ -396,7 +478,7 @@ func (c *checker) validRoots() []string {
 		if v == "" || !strings.HasPrefix(v, "/") || filepath.Clean(v) != v || v == "/" || hasDotDot(v) {
 			continue
 		}
-		if strings.HasPrefix(v, "/mnt/") || (c.p.SystemDataPath != "" && within(v, filepath.Clean(c.p.SystemDataPath))) {
+		if strings.HasPrefix(v, DriveNamespace) || (c.p.SystemDataPath != "" && within(v, filepath.Clean(c.p.SystemDataPath))) {
 			out = append(out, v)
 		}
 	}
@@ -408,8 +490,25 @@ func (c *checker) underRoot(p string) bool {
 		return false
 	}
 	p = filepath.Clean(p)
+	in := false
 	for _, r := range c.roots {
 		if within(p, r) {
+			in = true
+			break
+		}
+	}
+	if !in || c.p.Resolve == nil {
+		return in
+	}
+	// On the box: an existing source is judged by where it really points. A source that does not exist yet (the
+	// deploy belt creates it as a directory) is judged by its name; a root that cannot be resolved is no root.
+	rp, err := c.p.Resolve(p)
+	if err != nil {
+		return true
+	}
+	for _, r := range c.roots {
+		rr, rerr := c.p.Resolve(r)
+		if rerr == nil && within(rp, rr) {
 			return true
 		}
 	}