From b240085e0a5049500666563f61ff486a5835ece2 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 23:48:15 +0200 Subject: [PATCH] Close the review's gaps round the agent account (hq ADR 0266) The managed settings apply to every account's claude, so a key-helper script under the agent's home would run as the operator, and through sudo as root: the command is now carried whole in the root-owned managed settings and reads the key only as the agent. The helper reads only a regular file, bounded, without blocking, and a request it does not answer fails in time instead of holding every later one; a directory entry it answers that is not a name is refused. The guard's record is matched on its binary in the query, a half-named agent account is not taken as the operator's, the kept API key stays until the agent's home holds one, restic refuses to restore beside a directory others may write, and the agent-home check cleans a path before judging it. --- checks/agenthome/main.go | 4 + checks/agenthome/main_test.go | 4 +- modules/claude-code/README.md | 6 +- .../cmd/claude-code/agent_account.go | 4 + .../cmd/claude-code/agent_account_test.go | 120 +++++++++++++++++- .../claude-code/cmd/claude-code/guard_run.go | 6 +- modules/claude-code/cmd/claude-code/homefs.go | 92 ++++++++++++-- modules/claude-code/cmd/claude-code/main.go | 8 +- modules/claude-code/cmd/claude-code/node.go | 45 +++++-- modules/restic/cmd/restic-backups/backups.go | 18 ++- .../restic/cmd/restic-backups/backups_test.go | 21 ++- 11 files changed, 295 insertions(+), 33 deletions(-) diff --git a/checks/agenthome/main.go b/checks/agenthome/main.go index b8a06d95..813c4ae2 100644 --- a/checks/agenthome/main.go +++ b/checks/agenthome/main.go @@ -48,6 +48,10 @@ func resolve(p string, dirs map[string]string) string { // underAHome says whether a resolved path lies under the agent's home: by its machine fact, or written as a // literal home, which no definition may carry anyway (ADR 0112) and which may well be the agent's. func underAHome(p string) bool { + // Cleaned first, so //home/x or /./home/x is seen as the /home/x it is. + if p != "" { + p = filepath.Clean(p) + } return strings.HasPrefix(p, AgentHome) || strings.HasPrefix(p, "/home/") || p == "/home" } diff --git a/checks/agenthome/main_test.go b/checks/agenthome/main_test.go index 0d0368f1..b73129f2 100644 --- a/checks/agenthome/main_test.go +++ b/checks/agenthome/main_test.go @@ -29,11 +29,13 @@ func TestAnArchiveOrAFileWrittenIntoUnderTheAgentsHomeIsRefused(t *testing.T) { `{"id":"inner","type":"directory","path":"${dir:home}/inner"},`+ `{"id":"via-dir","type":"archive","path":"${dir:inner}/theme"}`) manifest(t, root, "e", `{"id":"literal","type":"file","path":"/home/agent/.bashrc","into":"block","content":"x"}`) + manifest(t, root, "f", `{"id":"unclean","type":"archive","path":"//./home/agent/x"}`) found, err := Check(root) if err != nil { t.Fatal(err) } - if len(found) != 4 || found[0].Module != "a" || found[1].Module != "b" || found[2].ID != "via-dir" || found[3].ID != "literal" { + if len(found) != 5 || found[0].Module != "a" || found[1].Module != "b" || found[2].ID != "via-dir" || + found[3].ID != "literal" || found[4].ID != "unclean" { t.Fatalf("refused %+v", found) } } diff --git a/modules/claude-code/README.md b/modules/claude-code/README.md index caffca75..541aa37c 100644 --- a/modules/claude-code/README.md +++ b/modules/claude-code/README.md @@ -205,8 +205,10 @@ named, and read as the operator's from a controller that sets neither — and ev agent goes to that account's home: - the licence token (`~/.claude/.credentials.json`) and the account it is logged in as (`~/.claude.json`); -- an API-key licence and its key-helper, in `~/.claude/mesh/`, since the account cannot enter the - module's state; +- an API-key licence, in `~/.claude/mesh/`, since the account cannot enter the module's state. Its key-helper + is no script there: the managed settings apply to every account's `claude`, the operator's and root's too, + so a script the agent owns would run as them. The managed settings carry the command whole instead, and it + reads the key only when run as the agent's account; - the items registered at the home scope, recorded per home (`home-placed-.json`). The runtime's account cannot enter that home (0700, the agent's), so a path under it is touched by a diff --git a/modules/claude-code/cmd/claude-code/agent_account.go b/modules/claude-code/cmd/claude-code/agent_account.go index 003d4bff..b6b71c06 100644 --- a/modules/claude-code/cmd/claude-code/agent_account.go +++ b/modules/claude-code/cmd/claude-code/agent_account.go @@ -80,6 +80,10 @@ func RetireOperatorHome(p Paths) []string { } for _, name := range []string{"api-key", "api-key-helper"} { old := filepath.Join(p.State, name) + if name == "api-key" && !holdsApplied(p, "api-key") { + // Kept, like the token above, until the agent's own home holds the key. + continue + } if _, err := os.Stat(old); err == nil { if err := os.Remove(old); err == nil { out = append(out, "removed "+old+": the key and its helper are kept where "+p.Agent+" reads them") diff --git a/modules/claude-code/cmd/claude-code/agent_account_test.go b/modules/claude-code/cmd/claude-code/agent_account_test.go index 33a938ac..489d2d6b 100644 --- a/modules/claude-code/cmd/claude-code/agent_account_test.go +++ b/modules/claude-code/cmd/claude-code/agent_account_test.go @@ -17,7 +17,9 @@ import ( "strings" "sync" "sync/atomic" + "syscall" "testing" + "time" ) // inProcess is a helper served in this process over pipes, counting what it was asked; kill ends it, as a @@ -198,6 +200,10 @@ func TestTheAgentsAccountAndHomeComeFromTheRuntime(t *testing.T) { if p.keyDir() != "/home/agent/.claude/mesh" || p.placed() != "/s/home-placed-agent.json" { t.Fatalf("the key and the record follow the agent's account: %s %s", p.keyDir(), p.placed()) } + with["MESH_AGENT_HOME"] = "" + if _, ok := PathsFrom(envOf(with)); ok { + t.Fatal("the agent's account without its home was taken as the operator's") + } with["MESH_AGENT_ACCOUNT"], with["MESH_AGENT_HOME"] = "operator", "/home/operator" if p, _ = PathsFrom(envOf(with)); p.Confined() || p.keyDir() != "/s" || p.placed() != "/s/home-placed.json" { t.Fatalf("the agent account named as the operator's is the operator's: %+v", p) @@ -255,8 +261,33 @@ func TestTheTokenTheModulePlacedInTheOperatorsHomeGoes(t *testing.T) { if _, err := os.Stat(file); !os.IsNotExist(err) { t.Fatalf("the access-token-only file is the module's and goes: %s", said) } + if _, err := os.Stat(filepath.Join(p.State, "api-key")); err != nil { + t.Fatal("the key kept in the state went before the agent's home held one") + } + if err := os.MkdirAll(filepath.Dir(p.apiKey()), 0o700); err != nil { + t.Fatal(err) + } + writeFile(t, p.apiKey(), "k\n") + RetireOperatorHome(p) if _, err := os.Stat(filepath.Join(p.State, "api-key")); !os.IsNotExist(err) { - t.Fatal("the key kept where the agent's account cannot read it goes") + t.Fatal("the key kept where the agent's account cannot read it goes once the agent's home holds it") + } +} + +// The managed settings apply to every account's claude, root's included: where the agent runs as an account of +// its own, the key-helper they name is never a file under any home, and reads the key only as that account. +func TestTheKeyHelperIsNeverAFileTheAgentOwns(t *testing.T) { + p := confined(t) + cmd := p.helperCommand() + if strings.HasPrefix(cmd, "/") || strings.HasPrefix(cmd, p.Home) || strings.HasPrefix(cmd, filepath.Dir(p.Home)) { + t.Fatalf("the key-helper is a path the agent may own: %q", cmd) + } + if !strings.Contains(cmd, `"$(id -un)" = 'agent'`) || !strings.Contains(cmd, "exec cat -- '"+p.apiKey()+"'") { + t.Fatalf("the key-helper reads the key only as the agent's account: %q", cmd) + } + p.Agent = p.Operator + if p.helperCommand() != p.helper() { + t.Fatalf("where the agent runs as the operator the helper stays in the state: %q", p.helperCommand()) } } @@ -453,3 +484,90 @@ func TestAnAnswerPastTheBoundIsRefused(t *testing.T) { t.Fatalf("a line within it is read whole across the buffer: %q %v", got, err) } } + +// A FIFO, a device or a link to one in the agent's home is answered at once, an error: it never holds the +// helper, and every request behind it (review of 2026-10-09). +func TestTheHelperReadsOnlyARegularFile(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, ".credentials.json") + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Skip("no FIFO here:", err) + } + zero := filepath.Join(dir, "zero") + if err := os.Symlink("/dev/zero", zero); err != nil { + t.Fatal(err) + } + for _, path := range []string{fifo, zero} { + done := make(chan Answer, 1) + go func() { done <- serveOne(Op{Op: "read", Path: path}) }() + select { + case a := <-done: + if a.Err == "" || len(a.Data) != 0 { + t.Fatalf("%s was read: %q", path, a.Data) + } + case <-time.After(5 * time.Second): + t.Fatalf("reading %s held the helper", path) + } + } + big := filepath.Join(dir, "big") + if err := os.WriteFile(big, make([]byte, helperReadMax+1), 0o600); err != nil { + t.Fatal(err) + } + if a := serveOne(Op{Op: "read", Path: big}); a.Err == "" { + t.Fatal("a file past the bound was read") + } +} + +// helperAnswering is a helper that answers every request with answer, or never when answer is empty. +func helperAnswering(account, answer string) *Helper { + return &Helper{Account: account, Start: func() (io.WriteCloser, io.Reader, func(), error) { + reqR, reqW := io.Pipe() + ansR, ansW := io.Pipe() + go func() { + r := bufio.NewReader(reqR) + for { + if _, err := r.ReadBytes('\n'); err != nil { + _ = ansW.Close() + return + } + if answer != "" { + _, _ = ansW.Write([]byte(answer + "\n")) + } + } + }() + return reqW, ansR, func() { _ = reqR.Close(); _ = ansW.Close() }, nil + }} +} + +// What the helper answers is the agent's word: an entry that is not a name in the directory is refused, so no +// path joined from it leaves the home and is read as the runtime's account. +func TestAnEntryThatLeavesTheDirectoryIsRefused(t *testing.T) { + for _, name := range []string{"../operator/.ssh/id_ed25519", "..", ".", "", "a/b"} { + raw, _ := json.Marshal(Answer{Entries: []*Info{{Name: name}}}) + f := &Files{Home: "/home/agent", Account: "agent", Helper: helperAnswering("agent", string(raw))} + if _, err := f.ReadDir("/home/agent/.claude"); err == nil { + t.Fatalf("the entry %q was taken", name) + } + } + raw, _ := json.Marshal(Answer{Entries: []*Info{{Name: "settings.json"}}}) + f := &Files{Home: "/home/agent", Account: "agent", Helper: helperAnswering("agent", string(raw))} + if got, err := f.ReadDir("/home/agent/.claude"); err != nil || len(got) != 1 { + t.Fatalf("a name in the directory was refused: %v %v", got, err) + } +} + +// A helper that does not answer fails the request in time, said, and is not asked again for it. +func TestAHelperThatDoesNotAnswerFailsTheRequestInTime(t *testing.T) { + was := helperAnswerWithin + helperAnswerWithin = 200 * time.Millisecond + defer func() { helperAnswerWithin = was }() + h := helperAnswering("agent", "") + start := time.Now() + _, err := h.Do(Op{Op: "read", Path: "/home/agent/.claude.json"}) + if err == nil || !errors.Is(err, errHelperSlow) { + t.Fatalf("a silent helper answered: %v", err) + } + if time.Since(start) > 2*time.Second { + t.Fatalf("the request waited %s", time.Since(start)) + } +} diff --git a/modules/claude-code/cmd/claude-code/guard_run.go b/modules/claude-code/cmd/claude-code/guard_run.go index 134fea29..c135271a 100644 --- a/modules/claude-code/cmd/claude-code/guard_run.go +++ b/modules/claude-code/cmd/claude-code/guard_run.go @@ -110,10 +110,12 @@ func recordInJournal(id string, e GuardEntry) error { // readJournal is the journal's last lines of the guard's record, read as root through the runtime account's // sudo where this process is not root: what other accounts said is not the runtime account's to read. As JSON, -// with the field the journal itself sets for the program that wrote the line (_EXE), which no writer chooses. +// with the field the journal itself sets for the program that wrote the line (_EXE), which no writer chooses — +// and matched on it in the query itself, so lines another program wrote under the guard's identifier cannot +// push the guard's own past the last ones read. var readJournal = func(id string, last int) ([]byte, error) { args := []string{"journalctl", "-q", "--no-pager", "-o", "json", "--output-fields=MESSAGE,_EXE", "-t", id, - "-n", strconv.Itoa(last)} + "_EXE=" + HelperPath(), "-n", strconv.Itoa(last)} if os.Geteuid() != 0 { args = append([]string{"sudo", "-n"}, args...) } diff --git a/modules/claude-code/cmd/claude-code/homefs.go b/modules/claude-code/cmd/claude-code/homefs.go index 409ae816..3bde64e7 100644 --- a/modules/claude-code/cmd/claude-code/homefs.go +++ b/modules/claude-code/cmd/claude-code/homefs.go @@ -32,6 +32,7 @@ import ( "sort" "strings" "sync" + "syscall" "time" ) @@ -184,6 +185,12 @@ func (f *Files) ReadDir(path string) ([]fs.DirEntry, error) { } out := make([]fs.DirEntry, 0, len(a.Entries)) for _, e := range a.Entries { + // A name is one entry of path, never a way out of it: the helper runs as the agent's account, so what it + // answers is the agent's word, and a name joined to path is read as the runtime's when it leaves the home. + if e == nil || !entryName(e.Name) { + return nil, fmt.Errorf("readdir %s as %s: the helper answered an entry that is not a name in it", path, + f.Helper.Account) + } out = append(out, fs.FileInfoToDirEntry(e.info())) } return out, nil @@ -207,6 +214,11 @@ func (f *Files) WalkDir(root string, fn fs.WalkDirFunc) error { return err } +// entryName says whether name is one entry of a directory: not empty, not . or .., and no separator. +func entryName(name string) bool { + return name != "" && name != "." && name != ".." && !strings.ContainsRune(name, '/') && !strings.ContainsRune(name, 0) +} + func (f *Files) walk(path string, d fs.DirEntry, fn fs.WalkDirFunc) error { if err := fn(path, d, nil); err != nil || !d.IsDir() { if err == filepath.SkipDir && d.IsDir() { @@ -317,6 +329,14 @@ type Helper struct { close func() } +// helperAnswerWithin bounds how long one request may take. The helper's own reads cannot block (serveOne), so +// a helper this slow has gone wrong: it is stopped and the request fails, said, never retried, so no request +// holds every later one behind it. +var helperAnswerWithin = 30 * time.Second + +// errHelperSlow is a helper that did not answer in time. +var errHelperSlow = errors.New("the helper did not answer in time") + // Do answers one request, starting the helper when it is not running. func (h *Helper) Do(op Op) (Answer, error) { h.mu.Lock() @@ -334,6 +354,9 @@ func (h *Helper) Do(op Op) (Answer, error) { } last = err h.stop() + if errors.Is(err, errHelperSlow) { + break + } } return Answer{}, fmt.Errorf("%s %s as %s: the helper did not answer: %w", op.Op, op.Path, h.Account, last) } @@ -343,12 +366,30 @@ func (h *Helper) roundTrip(op Op) (Answer, error) { if err != nil { return Answer{}, err } - if _, err := h.in.Write(append(raw, '\n')); err != nil { - return Answer{}, err + type read struct { + line []byte + err error } - line, err := readLine(h.out, helperAnswerMax) - if err != nil { - return Answer{}, err + done := make(chan read, 1) + in, out := h.in, h.out + go func() { + if _, err := in.Write(append(raw, '\n')); err != nil { + done <- read{err: err} + return + } + line, err := readLine(out, helperAnswerMax) + done <- read{line, err} + }() + var line []byte + select { + case r := <-done: + if r.err != nil { + return Answer{}, r.err + } + line = r.line + case <-time.After(helperAnswerWithin): + // The goroutine ends when stop closes the pipes and the helper goes. + return Answer{}, fmt.Errorf("%w (%s)", errHelperSlow, helperAnswerWithin) } var a Answer if err := json.Unmarshal(line, &a); err != nil { @@ -430,8 +471,14 @@ func (h *Helper) sudo() (io.WriteCloser, io.Reader, func(), error) { select { case <-done: case <-time.After(2 * time.Second): - _ = cmd.Process.Kill() - <-done + // sudo passes a SIGTERM on to the helper; a SIGKILL would end sudo alone and leave it running. + _ = cmd.Process.Signal(syscall.SIGTERM) + select { + case <-done: + case <-time.After(2 * time.Second): + _ = cmd.Process.Kill() + <-done + } } if s := strings.TrimSpace(stderr.String()); s != "" { say("the helper as %s said: %s", h.Account, s) @@ -459,6 +506,35 @@ func (l *limited) Write(p []byte) (int, error) { // ---- the helper ------------------------------------------------------------------------------------------- +// helperReadMax bounds a file the helper reads: its answer, as JSON, stays within helperAnswerMax. +const helperReadMax = 10 << 20 + +// readRegular reads a regular file of at most helperReadMax bytes. Opened without blocking and judged by what was +// opened, so a FIFO, a device or a link to one in the agent's home answers an error at once instead of holding +// the helper, and every request behind it, or filling its memory. +func readRegular(path string) ([]byte, error) { + f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, err + } + defer f.Close() + info, err := f.Stat() + if err != nil { + return nil, err + } + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("%s is not a regular file (%s)", path, info.Mode().Type()) + } + data, err := io.ReadAll(io.LimitReader(f, helperReadMax+1)) + if err != nil { + return nil, err + } + if len(data) > helperReadMax { + return nil, fmt.Errorf("%s is larger than %d bytes", path, helperReadMax) + } + return data, nil +} + // ServeHelper is the helper: one answer per request, until its input ends. It runs as the agent's account and // does only what that account may, on its own home: the account's permissions are the boundary. func ServeHelper(in io.Reader, out io.Writer) error { @@ -487,7 +563,7 @@ func serveOne(op Op) Answer { var err error switch op.Op { case "read": - a.Data, err = os.ReadFile(op.Path) + a.Data, err = readRegular(op.Path) case "write": err = os.WriteFile(op.Path, op.Data, mode) case "create": diff --git a/modules/claude-code/cmd/claude-code/main.go b/modules/claude-code/cmd/claude-code/main.go index 78da044e..a4ec3198 100644 --- a/modules/claude-code/cmd/claude-code/main.go +++ b/modules/claude-code/cmd/claude-code/main.go @@ -391,7 +391,13 @@ func main() { } if !launched { // Outside a launch — a build, a check — it serves nothing and says why. - say("not launched by the runtime with this module's words; serving no tools") + if (os.Getenv("MESH_AGENT_ACCOUNT") == "") != (os.Getenv("MESH_AGENT_HOME") == "") { + say("the runtime named the agent's account without its home, or its home without the account " + + "(MESH_AGENT_ACCOUNT, MESH_AGENT_HOME); serving no tools rather than writing the operator's home " + + "for an agent that runs as another account") + } else { + say("not launched by the runtime with this module's words; serving no tools") + } if err := stdio.Serve("", nil); err != nil { os.Exit(1) } diff --git a/modules/claude-code/cmd/claude-code/node.go b/modules/claude-code/cmd/claude-code/node.go index 7a0fdf2b..0e6b421e 100644 --- a/modules/claude-code/cmd/claude-code/node.go +++ b/modules/claude-code/cmd/claude-code/node.go @@ -64,7 +64,12 @@ func PathsFrom(env func(string) string) (Paths, bool) { Settings: env("MESH_CLAUDE_CODE_SETTINGS"), Home: env("MESH_AGENT_HOME"), Node: env("MESH_NODE"), Agent: env("MESH_AGENT_ACCOUNT"), Operator: env("MESH_OPERATOR_ACCOUNT"), AgentHome: env("MESH_AGENT_HOME"), OperatorHome: env("MESH_OPERATOR_HOME")} - if p.Home == "" || p.Agent == "" { + if (p.Home == "") != (p.Agent == "") { + // Half of the agent's account is not "the operator's": taken so, the agent's account rule of the guard + // would be off and its home never written, and nothing would say so. Not launched, said (main). + return p, false + } + if p.Home == "" { p.Home, p.Agent, p.AgentHome = env("MESH_OPERATOR_HOME"), p.Operator, env("MESH_OPERATOR_HOME") } return p, p.State != "" && p.Facts != "" && p.Settings != "" && p.Home != "" && p.Node != "" @@ -122,10 +127,27 @@ func (p Paths) binding() string { return filepath.Join(p.State, "licence.jso func (p Paths) apiKey() string { return filepath.Join(p.keyDir(), "api-key") } func (p Paths) helper() string { return filepath.Join(p.keyDir(), "api-key-helper") } -// keyDir is where an API-key licence and its key-helper live: the module's state, which the agent runs the -// helper from as the operator — or, where the agent runs as an account of its own, which cannot enter the -// module's state, a directory of the module's in that account's agent directory (novox/hq ADR 0266). Either -// way readable by the account the agent runs as, and by it alone. +// helperCommand is the managed settings' apiKeyHelper: the key-helper's path in the module's state where the agent +// runs as the operator. Where it runs as an account of its own, never a file under that account's home: the +// managed settings apply to every account's `claude`, the operator's and root's included, so a script the agent +// owns there would run as them at their next start (novox/hq ADR 0266, review of 2026-10-09). It is a command +// written whole in the root-owned managed settings instead, which reads the key only when run as the agent's +// account, and as it. +func (p Paths) helperCommand() string { + if !p.Confined() { + return p.helper() + } + return "[ \"$(id -un)\" = " + shellQuote(p.Agent) + " ] && exec cat -- " + shellQuote(p.apiKey()) +} + +// shellQuote is s as one word of a POSIX shell. +func shellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } + +// keyDir is where an API-key licence lives: the module's state, with the key-helper the agent runs from it as +// the operator — or, where the agent runs as an account of its own, which cannot enter the module's state, a +// directory of the module's in that account's agent directory (novox/hq ADR 0266), and no helper there: the +// managed settings carry its command (helperCommand). Either way readable by the account the agent runs as, +// and by it alone. func (p Paths) keyDir() string { if p.Confined() { return filepath.Join(p.Home, ".claude", "mesh") @@ -210,7 +232,7 @@ func RenderNow(p Paths, write WriteManaged) ([]string, error) { if mesh != nil { mesh.Refused = RefusedCounts(GuardDataOf(p, mesh).Log) } - files := RenderWithMesh(facts, settings, binding, p.helper(), Registered(p), config, mesh) + files := RenderWithMesh(facts, settings, binding, p.helperCommand(), Registered(p), config, mesh) tree, _ := json.Marshal(MarketplaceWith(config, guardFiles(p, mesh))) files[MarketplaceDir+"/"] = string(tree) names := make([]string, 0, len(files)) @@ -432,10 +454,15 @@ func Apply(p Paths, c Current, write WriteManaged) (map[string]any, error) { if err := fsys.WriteFile(p.apiKey(), []byte(strings.TrimSpace(plain)+"\n"), 0o600); err != nil { return nil, err } - if err := fsys.WriteFile(p.helper(), []byte("#!/bin/sh\nexec cat '"+p.apiKey()+"'\n"), 0o700); err != nil { - return nil, err + if p.Confined() { + // No script under the agent's home: the managed settings carry the command whole (helperCommand). + _ = fsys.Remove(p.helper()) + } else { + if err := fsys.WriteFile(p.helper(), []byte("#!/bin/sh\nexec cat "+shellQuote(p.apiKey())+"\n"), 0o700); err != nil { + return nil, err + } + _ = fsys.Chmod(p.helper(), 0o700) } - _ = fsys.Chmod(p.helper(), 0o700) } else { var g Grant if err := json.Unmarshal([]byte(plain), &g); err != nil { diff --git a/modules/restic/cmd/restic-backups/backups.go b/modules/restic/cmd/restic-backups/backups.go index c365010e..10fbe8b0 100644 --- a/modules/restic/cmd/restic-backups/backups.go +++ b/modules/restic/cmd/restic-backups/backups.go @@ -495,8 +495,8 @@ func (b *Backups) Restore(ctx context.Context, module, snapshot, path string) (* } // besideIsUnsafe says why root must not restore at target, beside the live data: a directory on the way to it -// is a symbolic link, or belongs to an account other than root and the one this process runs as — one that -// could swap a link in while root writes. Empty when every directory on the way is root's or the runtime's. +// is a symbolic link, belongs to an account other than root and the one this process runs as, or may be written +// by its group or everyone — any of which could swap a link in while root writes. Empty when every directory on the way is root's or the runtime's. func (b *Backups) besideIsUnsafe(ctx context.Context, target string) string { me := strconv.Itoa(os.Getuid()) dir := filepath.Dir(filepath.Clean(target)) @@ -508,17 +508,27 @@ func (b *Backups) besideIsUnsafe(ctx context.Context, target string) string { } } for i := len(chain) - 1; i >= 0; i-- { - out, err := b.Run(ctx, "stat", "-c", "%u %F", chain[i]) + out, err := b.Run(ctx, "stat", "-c", "%u %a %F", chain[i]) if err != nil { return chain[i] + " could not be read" } - owner, kind, _ := strings.Cut(strings.TrimSpace(out), " ") + owner, rest, _ := strings.Cut(strings.TrimSpace(out), " ") + perm, kind, _ := strings.Cut(rest, " ") if kind == "symbolic link" { return chain[i] + " is a symbolic link" } if owner != "0" && owner != me { return chain[i] + " belongs to another account" } + // Another account may write into a directory open to its group or to everyone, sticky or not, and so + // put a link where root is about to write. + mode, err := strconv.ParseUint(perm, 8, 32) + if err != nil { + return chain[i] + "'s mode could not be read" + } + if mode&0o022 != 0 { + return chain[i] + " may be written by other accounts" + } } return "" } diff --git a/modules/restic/cmd/restic-backups/backups_test.go b/modules/restic/cmd/restic-backups/backups_test.go index 1d9e936f..77b0dc1f 100644 --- a/modules/restic/cmd/restic-backups/backups_test.go +++ b/modules/restic/cmd/restic-backups/backups_test.go @@ -190,8 +190,13 @@ func TestWithTheRealResticAMistakeIsUndoneBesideTheLiveData(t *testing.T) { settings := filepath.Join(root, "settings.xml") must(t, os.WriteFile(settings, []byte("kept\n"), 0o600)) where := placed(t, "# mail\npath "+store+"\npath "+settings+"\n# pg\nrun echo 'every row' > "+dumps+"/all.dump\npath "+dumps+"\n") - // As whoever runs the test, against its own repository: no sudo. + // As whoever runs the test, against its own repository: no sudo. The test's directory lies in a temporary + // directory everyone may write, which a real restore is right to refuse beside; the directories above the + // test's own are said to be root's here, so the restore beside is what is tested. run := func(ctx context.Context, name string, args ...string) (string, error) { + if name == "stat" && len(args) > 0 && strings.HasPrefix(root, strings.TrimSuffix(args[len(args)-1], "/")+"/") { + return "0 755 directory\n", nil + } out, err := exec.CommandContext(ctx, name, args...).Output() if ee, ok := err.(*exec.ExitError); ok { return string(out), errors.New(string(ee.Stderr)) @@ -294,9 +299,12 @@ func TestARestoreBesideAnotherAccountsDirectoryGoesToTheState(t *testing.T) { ran = append(ran, name+" "+strings.Join(args, " ")) if name == "stat" { if args[len(args)-1] == "/home/agent" { - return "1001 directory\n", nil + return "1001 700 directory\n", nil } - return "0 directory\n", nil + if args[len(args)-1] == "/srv/shared" { + return "0 1777 directory\n", nil + } + return "0 755 directory\n", nil } return "", nil } @@ -307,11 +315,14 @@ func TestARestoreBesideAnotherAccountsDirectoryGoesToTheState(t *testing.T) { if why := b.besideIsUnsafe(context.Background(), "/srv/data.restored-x"); why != "" { t.Fatalf("root's directories are safe beside: %q", why) } + if why := b.besideIsUnsafe(context.Background(), "/srv/shared/data.restored-x"); !strings.Contains(why, "other accounts") { + t.Fatalf("a directory everyone may write is unsafe beside, sticky or not: %q", why) + } run2 := func(ctx context.Context, name string, args ...string) (string, error) { if name == "stat" && args[len(args)-1] == "/home" { - return "0 symbolic link\n", nil + return "0 777 symbolic link\n", nil } - return "0 directory\n", nil + return "0 755 directory\n", nil } b.Run = run2 if why := b.besideIsUnsafe(context.Background(), "/home/agent/x"); !strings.Contains(why, "symbolic link") {