From 2673e7a2c95e00a83a109617d3ae252c509fb72d Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 12:11:57 +0200 Subject: [PATCH] Let the setuid search run to its 15-minute bound, ask pacman once, and call a walk with errors incomplete The search kept a two-minute limit of its own inside its 15-minute bound, so a long walk always ended incomplete; pacman was asked once per path; and find exiting with an error but some output was read as a complete walk. The bound and the not-judged reasons move to the public rootsearch package so the controller reads the same values (the review of 2026-10-09). --- internal/accounts/root_test.go | 22 ++++-- internal/accounts/ways.go | 124 +++++++++++++++++++++++---------- internal/accounts/ways_test.go | 70 +++++++++++++++++++ rootsearch/rootsearch.go | 19 +++++ 4 files changed, 195 insertions(+), 40 deletions(-) create mode 100644 rootsearch/rootsearch.go diff --git a/internal/accounts/root_test.go b/internal/accounts/root_test.go index 8e9a040..a738238 100644 --- a/internal/accounts/root_test.go +++ b/internal/accounts/root_test.go @@ -38,6 +38,8 @@ type agentMachine struct { // proc is /proc's files, aSafeProc unless a test changes them; findArgs sees find's arguments. proc map[string]string findArgs func([]string) + // pacmanCalls counts the package manager's calls: one for every path found. + pacmanCalls int } func (m *agentMachine) readFile(path string) ([]byte, error) { @@ -107,11 +109,21 @@ func (m *agentMachine) run(_ context.Context, name string, args ...string) (stri m.findArgs(args) } return m.setuid, m.findErr - case name == "pacman" && len(args) == 2 && args[0] == "-Qqo": - if m.packaged[args[1]] { - return "somepackage\n", nil + case name == "pacman" && len(args) >= 2 && args[0] == "-Qo": + m.pacmanCalls++ + var out, errs []string + for _, p := range args[1:] { + if m.packaged[p] { + out = append(out, p+" is owned by somepackage 1.0-1") + } else { + errs = append(errs, "error: No package owns "+p) + } } - return "", errors.New("pacman exited 1: error: No package owns " + args[1]) + said := strings.Join(out, "\n") + if len(errs) > 0 { + return said, errors.New("pacman exited 1: " + strings.Join(errs, "\n")) + } + return said, nil } return "", errors.New("not a command the judge may run: " + line) } @@ -144,7 +156,7 @@ func lookAgent(t *testing.T, m *agentMachine) Verdict { } for _, a := range m.asked { if !strings.HasPrefix(a, "id ") && a != "sudo -l -U agent" && !strings.HasPrefix(a, "find / ") && - !strings.HasPrefix(a, "pacman -Qqo ") { + !strings.HasPrefix(a, "pacman -Qo ") { t.Errorf("the judge asked something that is not a read: %q", a) } } diff --git a/internal/accounts/ways.go b/internal/accounts/ways.go index 883de20..9aa525c 100644 --- a/internal/accounts/ways.go +++ b/internal/accounts/ways.go @@ -10,6 +10,7 @@ import ( "encoding/binary" "errors" "fmt" + "github.com/novox/mesh-host/rootsearch" "io/fs" "os" "os/exec" @@ -398,9 +399,11 @@ type SetuidCache struct { backoff time.Duration } -// SearchBound is how long one search may run in the background; MaxBackoff the longest wait after failures. +// SearchBound is how long one search may run in the background — the whole search, the walk and the package +// manager's answers together (rootsearch.Bound, which the controller reads too); MaxBackoff the longest wait +// after failures. const ( - SearchBound = 15 * time.Minute + SearchBound = rootsearch.Bound MaxBackoff = 6 * time.Hour ) @@ -424,8 +427,8 @@ func (e *SetuidNotJudged) Error() string { // The reasons of a root verdict the setuid search could not answer. const ( - ReasonPending = "not judged yet (search running)" - ReasonIncomplete = "not judged (search incomplete)" + ReasonPending = rootsearch.ReasonPending + ReasonIncomplete = rootsearch.ReasonIncomplete ) // Look answers the last result and when its search finished, starting a search in the background when one is @@ -475,11 +478,10 @@ func (c *SetuidCache) Look(now time.Time, search func(ctx context.Context) ([]st func (c *SetuidCache) Wait() { c.wg.Wait() } // setuidSearch is every setuid- or setgid-root regular file on the root filesystem that no installed package -// owns, found with find and asked of the package manager. Bounded: a search that does not finish is an -// unanswered question, never "none". +// owns, found with find and asked of the package manager. Bounded by its caller (SearchBound, the whole search): +// a search that does not finish is an unanswered question, never "none". A walk find could not complete — it +// exited with an error — is incomplete too, unless every error was a file that vanished while it walked. func (e Exec) setuidSearch(ctx context.Context, mounts []string) ([]string, error) { - ctx, cancel := context.WithTimeout(ctx, 2*time.Minute) - defer cancel() // Each mount point a starting point of its own, -xdev keeping each to its own filesystem: every local // filesystem mounted without nosuid is searched once (the re-review of 2026-10-08), and a program setgid to // root's group counts whoever owns it. @@ -489,10 +491,10 @@ func (e Exec) setuidSearch(ctx context.Context, mounts []string) ([]string, erro "-2000", ")", ")", "-print") out, err := e.Run(ctx, "find", args...) if ctx.Err() != nil { - return nil, fmt.Errorf("the search for setuid programs did not finish within two minutes") + return nil, fmt.Errorf("the search for setuid programs did not finish within %s", SearchBound) } - if err != nil && strings.TrimSpace(out) == "" { - return nil, fmt.Errorf("the search for setuid programs did not finish: %w", err) + if err != nil && (strings.TrimSpace(out) == "" || !onlyVanished(err)) { + return nil, fmt.Errorf("the search for setuid programs did not complete its walk: %w", err) } var paths []string for _, l := range strings.Split(out, "\n") { @@ -501,36 +503,88 @@ func (e Exec) setuidSearch(ctx context.Context, mounts []string) ([]string, erro } } sort.Strings(paths) - var unowned []string - for _, p := range paths { - owned, err := e.packaged(ctx, p) - if err != nil { - return nil, err - } - if !owned { - unowned = append(unowned, p) - } - } - return unowned, nil + return e.unpackaged(ctx, paths) } -// packaged says whether an installed package owns a path: pacman's or apk's answer, never a guess. -func (e Exec) packaged(ctx context.Context, path string) (bool, error) { - out, err := e.Run(ctx, "pacman", "-Qqo", path) - if err == nil { - return strings.TrimSpace(out) != "", nil +// onlyVanished says every error find reported was a file that went while it walked — the one error that leaves +// the walk complete for what still exists. find's own words come after its exit, one per line. +func onlyVanished(err error) bool { + said := false + for _, l := range errorLines(err, "find") { + said = true + if !strings.Contains(l, "No such file or directory") { + return false + } } - if strings.Contains(out+err.Error(), "No package owns") || strings.Contains(err.Error(), "exited 1") { - return false, nil + return said +} + +// errorLines are the lines a command said on its error stream, as the runner words a failure: " exited N: +// ", the prefix taken off the first line. Empty lines are dropped. +func errorLines(err error, name string) []string { + words := err.Error() + if strings.HasPrefix(words, name+" exited") { + if i := strings.Index(words, ": "); i >= 0 { + words = words[i+2:] + } } - if !errors.Is(err, exec.ErrNotFound) && !errors.Is(err, fs.ErrNotExist) { - return false, fmt.Errorf("pacman could not say who owns %s: %w", path, err) + var out []string + for _, l := range strings.Split(words, "\n") { + if l = strings.TrimSpace(l); l != "" { + out = append(out, l) + } } - out, err = e.Run(ctx, "apk", "info", "-W", path) - if err != nil { - return false, fmt.Errorf("no package manager could say who owns %s: %w", path, err) + return out +} + +// unpackaged is every path no installed package owns: pacman asked about all of them in one call, else apk +// about each. Never a guess: a path neither said owned nor said unowned is an unanswered question. +func (e Exec) unpackaged(ctx context.Context, paths []string) ([]string, error) { + if len(paths) == 0 { + return nil, nil } - return strings.Contains(out, " is owned by "), nil + out, err := e.Run(ctx, "pacman", append([]string{"-Qo"}, paths...)...) + if err == nil || !errors.Is(err, exec.ErrNotFound) && !errors.Is(err, fs.ErrNotExist) { + owned := map[string]bool{} + for _, l := range strings.Split(out, "\n") { + if p, _, ok := strings.Cut(strings.TrimSpace(l), " is owned by "); ok { + owned[p] = true + } + } + unowned := map[string]bool{} + if err != nil { + // pacman exits 1 when a path is owned by nothing, and says which on its error stream. + for _, l := range errorLines(err, "pacman") { + if _, p, ok := strings.Cut(l, "No package owns "); ok { + unowned[strings.TrimSpace(p)] = true + } else { + return nil, fmt.Errorf("pacman could not say who owns the setuid programs found: %s", l) + } + } + } + var out []string + for _, p := range paths { + switch { + case owned[p]: + case unowned[p]: + out = append(out, p) + default: + return nil, fmt.Errorf("pacman said nothing of who owns %s", p) + } + } + return out, nil + } + var none []string + for _, p := range paths { + said, err := e.Run(ctx, "apk", "info", "-W", p) + if err != nil { + return nil, fmt.Errorf("no package manager could say who owns %s: %w", p, err) + } + if !strings.Contains(said, " is owned by ") { + none = append(none, p) + } + } + return none, nil } // moreWays is every way beyond uid, groups and sudo; an unanswered question is an error, never none. diff --git a/internal/accounts/ways_test.go b/internal/accounts/ways_test.go index d0979bb..b62de60 100644 --- a/internal/accounts/ways_test.go +++ b/internal/accounts/ways_test.go @@ -5,6 +5,7 @@ import ( "encoding/binary" "errors" "fmt" + "github.com/novox/mesh-host/rootsearch" "os/exec" "strings" "testing" @@ -318,3 +319,72 @@ polkit.addRule(function(a, s) { /* isInGroup("wheel") */ return polkit.Result.YE t.Error("each .pkla section on its own") } } + +// The search's bound is the one the controller reads (rootsearch.Bound): a search may run its whole fifteen +// minutes, with no shorter limit inside it that would end every long walk incomplete (the review of 2026-10-09). +func TestTheSearchRunsToItsBoundWithNoShorterLimitInside(t *testing.T) { + if rootsearch.Bound != 15*time.Minute { + t.Fatalf("the shared bound is %s", rootsearch.Bound) + } + m := clean() + var left time.Duration + run := func(ctx context.Context, name string, args ...string) (string, error) { + if name == "find" { + if d, ok := ctx.Deadline(); ok { + left = time.Until(d) + } + } + return m.run(ctx, name, args...) + } + ctx, cancel := context.WithTimeout(t.Context(), SearchBound) + defer cancel() + if _, err := (Exec{Run: run}).setuidSearch(ctx, []string{"/"}); err != nil { + t.Fatal(err) + } + if left < SearchBound-time.Minute { + t.Fatalf("find was given %s, less than the search's bound of %s", left, SearchBound) + } +} + +// The package manager is asked about every path found in one call, and answers each: owned, or owned by nothing. +func TestThePackageManagerIsAskedOnceForEveryPathFound(t *testing.T) { + m := clean() + m.setuid = "/usr/bin/sudo\n/usr/bin/passwd\n/usr/local/bin/x\n/opt/y\n" + m.packaged = map[string]bool{"/usr/bin/sudo": true, "/usr/bin/passwd": true} + found, err := (Exec{Run: m.run}).setuidSearch(t.Context(), []string{"/"}) + if err != nil { + t.Fatal(err) + } + if m.pacmanCalls != 1 { + t.Errorf("pacman was asked %d times for four paths", m.pacmanCalls) + } + if strings.Join(found, ",") != "/opt/y,/usr/local/bin/x" { + t.Errorf("unowned: %v", found) + } + // An error pacman says that is not "owned by nothing" is no answer. + odd := func(ctx context.Context, name string, args ...string) (string, error) { + if name == "pacman" { + return "", errors.New("pacman exited 1: error: could not open database") + } + return m.run(ctx, name, args...) + } + if _, err := (Exec{Run: odd}).setuidSearch(t.Context(), []string{"/"}); err == nil { + t.Error("a package manager that could not answer was read as an answer") + } +} + +// find that exits with an error has not walked everything: the search is incomplete, unless every error was a +// file that vanished while it walked. +func TestAWalkFindDidNotCompleteIsIncomplete(t *testing.T) { + m := clean() + m.setuid = "/usr/local/bin/x\n" + m.findErr = errors.New("find exited 1: find: '/mnt/data/private': Permission denied") + if _, err := (Exec{Run: m.run}).setuidSearch(t.Context(), []string{"/"}); err == nil { + t.Fatal("a walk that could not enter a directory was read as complete") + } + m.findErr = errors.New("find exited 1: find: '/tmp/a': No such file or directory\nfind: '/tmp/b': No such file or directory") + found, err := (Exec{Run: m.run}).setuidSearch(t.Context(), []string{"/"}) + if err != nil || len(found) != 1 { + t.Fatalf("files that vanished while find walked: %v %v", found, err) + } +} diff --git a/rootsearch/rootsearch.go b/rootsearch/rootsearch.go new file mode 100644 index 0000000..a1c70c0 --- /dev/null +++ b/rootsearch/rootsearch.go @@ -0,0 +1,19 @@ +// Package rootsearch holds what the node-engine's search for setuid programs and the controller that reads its +// verdicts must agree on (novox/hq ADR 0266): how long one search may run, and the words a verdict starts with +// while it has no answer. Public, so the controller imports these rather than keeps copies that could drift. +package rootsearch + +import "time" + +// Bound is how long one search for setuid programs may run. It governs the whole search, the walk and the +// package manager's answers together; the controller does not raise an agent account as not judged while the +// first search after a start is within it. +const Bound = 15 * time.Minute + +// The reasons of a root verdict the search could not answer yet. +const ( + // ReasonPending starts the reason while the first search since the engine started runs. + ReasonPending = "not judged yet (search running)" + // ReasonIncomplete starts the reason when no search has finished: one failed or ran out its bound. + ReasonIncomplete = "not judged (search incomplete)" +)