From 0b4c86e3bed4be02f9302fc047fa712b39c10775 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 02:00:10 +0200 Subject: [PATCH] Count a setuid find only where the account can execute it, read at every judgement (hq issue 361) The two finds on the control-node sit in the operator's home of mode 0700, which the agent account cannot search. The search keeps every find; each judgement reads the modes on the way again, since an account can open its own home without root. A find in a home is said by count, its path in the journal alone. Also from review: a checkpoint racing SearchNow forgets the walk it just kept, and a walk kept by an older engine says its counts as lower bounds. --- internal/accounts/exec.go | 17 ++++ internal/accounts/root_test.go | 15 ++- internal/accounts/setuid_cache.go | 32 ++++-- internal/accounts/setuid_progress_test.go | 21 +++- internal/accounts/setuid_reach_test.go | 115 ++++++++++++++++++++++ internal/accounts/setuid_walk.go | 17 +++- internal/accounts/ways.go | 82 ++++++++++++++- internal/accounts/ways_test.go | 6 +- 8 files changed, 287 insertions(+), 18 deletions(-) create mode 100644 internal/accounts/setuid_reach_test.go diff --git a/internal/accounts/exec.go b/internal/accounts/exec.go index b7d8889..55a3eb4 100644 --- a/internal/accounts/exec.go +++ b/internal/accounts/exec.go @@ -28,6 +28,10 @@ type Exec struct { Proc string // Stat is a file's owner and mode; nil is the machine's own (os.Stat). A test gives files of its own. Stat func(path string) (FileMode, error) + // Lstat is a path's own owner and mode, never through a link, with its type bits in Perm (fs.ModeSymlink, + // fs.ModeDir); nil is the machine's own (os.Lstat). Whether an account can reach a setuid program is read + // with it at every judgement (novox/hq issue 361). + Lstat func(path string) (FileMode, error) // ReadFile, ReadDir and ACL read doas's and polkit's rules and a file's POSIX ACL; nil is the machine's. ReadFile func(path string) ([]byte, error) ReadDir func(path string) ([]fs.DirEntry, error) @@ -187,6 +191,19 @@ func statOf(path string) (FileMode, error) { return FileMode{UID: int(st.Uid), GID: int(st.Gid), Perm: info.Mode().Perm()}, nil } +// lstatOf is a path's own owner and mode, its type bits kept. +func lstatOf(path string) (FileMode, error) { + info, err := os.Lstat(path) + if err != nil { + return FileMode{}, err + } + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return FileMode{}, fmt.Errorf("%s has no owner this platform reports", path) + } + return FileMode{UID: int(st.Uid), GID: int(st.Gid), Perm: info.Mode()}, nil +} + // InDatabase is `id -nG`: every group the user database lists the account in. func (e Exec) InDatabase(ctx context.Context, account string) ([]string, error) { out, err := e.Run(ctx, "id", "-nG", account) diff --git a/internal/accounts/root_test.go b/internal/accounts/root_test.go index 72ae4ad..387a90c 100644 --- a/internal/accounts/root_test.go +++ b/internal/accounts/root_test.go @@ -40,6 +40,19 @@ type agentMachine struct { findArgs func([]string) // pacmanCalls counts the package manager's calls: one for every path found. pacmanCalls int + // nodes are paths' own owners and modes (Lstat); any other is root's, a directory searchable by all, or a + // file the walk found, setuid and executable by all. + nodes map[string]FileMode +} + +func (m *agentMachine) lstat(path string) (FileMode, error) { + if n, ok := m.nodes[path]; ok { + return n, nil + } + if strings.Contains(m.setuid, path+"\n") { + return FileMode{Perm: fs.ModeSetuid | 0o755}, nil + } + return FileMode{Perm: fs.ModeDir | 0o755}, nil } func (m *agentMachine) readFile(path string) ([]byte, error) { @@ -157,7 +170,7 @@ func clean() *agentMachine { func lookAgent(t *testing.T, m *agentMachine) Verdict { t.Helper() - j := New(Exec{Run: m.run, Walk: m.walk, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl}) + j := New(Exec{Run: m.run, Walk: m.walk, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl, Lstat: m.lstat}) j.Set([]Account{agent}) st, _ := j.Look(t.Context()) if len(st.Accounts) != 1 { diff --git a/internal/accounts/setuid_cache.go b/internal/accounts/setuid_cache.go index 7125a3d..777bbf5 100644 --- a/internal/accounts/setuid_cache.go +++ b/internal/accounts/setuid_cache.go @@ -123,6 +123,8 @@ type SearchProgress struct { // far, before the package manager is asked about them. Visited int64 Found int + // AtLeast says Visited is a lower bound: the walk was begun by an engine that did not count. + AtLeast bool // Kept is when its progress was last kept, zero before the first time. Kept time.Time // Finished is every filesystem walked to its end, in order, and how long its walk took. @@ -139,7 +141,7 @@ type SearchedFilesystem struct { // progressOf is p as anybody may read it. Counts and mount points, no I/O. func progressOf(p *SetuidProgress, kept time.Time) *SearchProgress { sp := &SearchProgress{Started: p.Started, Filesystems: len(p.Mounts), Done: len(p.Done), Visited: p.Visited, - Found: len(p.Found), Kept: kept} + Found: len(p.Found), Kept: kept, AtLeast: !p.Counted} for i, m := range p.Done { took := time.Duration(-1) if i < len(p.Took) { @@ -165,7 +167,11 @@ func (sp *SearchProgress) Words() string { if sp.Walking != "" { s += ", walking " + sp.Walking } - s += fmt.Sprintf(", %d entries visited, %d setuid- or setgid-root files found so far", sp.Visited, sp.Found) + atLeast := "" + if sp.AtLeast { + atLeast = "at least " + } + s += fmt.Sprintf(", %s%d entries visited, %d setuid- or setgid-root files found so far", atLeast, sp.Visited, sp.Found) if sp.Kept.IsZero() { s += ", not kept yet" } else { @@ -286,7 +292,7 @@ type SetuidSearch func(ctx context.Context, p *SetuidProgress, checkpoint func() // start, a result reads at most one search older than it is, which errs towards not judging. func (c *SetuidCache) Look(now time.Time, mounts []string, search SetuidSearch) ([]string, time.Time, error) { if c == nil { - p := &SetuidProgress{Started: now, Mounts: slices.Clone(mounts)} + p := &SetuidProgress{Started: now, Mounts: slices.Clone(mounts), Counted: true} found, err := search(context.Background(), p, func() {}) return found, now, err } @@ -360,7 +366,7 @@ func (c *SetuidCache) start(now time.Time, mounts []string, search SetuidSearch) resumed := p != nil if p == nil { p = &SetuidProgress{ID: fmt.Sprintf("%x", now.UnixNano()), Started: now, Mounts: slices.Clone(mounts), - Cursor: map[string]string{}} + Cursor: map[string]string{}, Counted: true} } c.running, c.startedAt, c.attemptAt, c.again = true, p.Started, now, false c.starts++ @@ -395,7 +401,16 @@ func (c *SetuidCache) start(now time.Time, mounts []string, search SetuidSearch) if !superseded { // root acted since this walk started: its progress is not to be carried on at := time.Now() p.Kept = at - if kerr := c.keepProgress(p); kerr != nil { + kerr := c.keepProgress(p) + // Superseded while it wrote (SearchAgain, SearchNow), after they forgot the kept walk: forgotten + // again here, so no engine restarted before this walk ends carries it on (the review of + // 2026-10-10). Written outside the lock, so a look never waits on the disk. + c.mu.Lock() + if c.again || c.gen != gen { + c.forgetProgress() + } + c.mu.Unlock() + if kerr != nil { p.Kept = kept // not kept: the file still says the time before if !unkept { unkept = true @@ -450,8 +465,13 @@ func (c *SetuidCache) start(now time.Time, mounts []string, search SetuidSearch) "restarts: " + err.Error()) } c.forgetProgress() + // The paths in the engine's journal alone: the verdict says one in a home by count. + named := "" + if len(found) > 0 { + named = ": " + strings.Join(found, ", ") + } c.say(fmt.Sprintf("the search for setuid programs started %s completed (%s this time): %d no package "+ - "owns", p.Started.Local().Format("2006-01-02 15:04"), took.Round(time.Second), len(found))) + "owns%s", p.Started.Local().Format("2006-01-02 15:04"), took.Round(time.Second), len(found), named)) }() } diff --git a/internal/accounts/setuid_progress_test.go b/internal/accounts/setuid_progress_test.go index 2fa03e2..3bcfe11 100644 --- a/internal/accounts/setuid_progress_test.go +++ b/internal/accounts/setuid_progress_test.go @@ -30,7 +30,7 @@ func TestTheProgressCountsEachEntryOnceAcrossAResume(t *testing.T) { w := mine() w.every = 0 ctx, stop := context.WithCancel(t.Context()) - p := &SetuidProgress{Mounts: []string{top, other}} + p := &SetuidProgress{Mounts: []string{top, other}, Counted: true} var kept []byte err := w.walk(ctx, p, func() { kept = mustJSON(t, p) @@ -49,13 +49,14 @@ func TestTheProgressCountsEachEntryOnceAcrossAResume(t *testing.T) { if err := mine().walk(t.Context(), &resumed, func() {}); err != nil { t.Fatal(err) } - if resumed.Visited != whole.Visited || len(resumed.Took) != 2 { + if resumed.Visited != whole.Visited || len(resumed.Took) != 2 || resumed.Took[0] < 0 { t.Fatalf("the resumed walk counted %d entries (the whole one %d), took %v", resumed.Visited, whole.Visited, resumed.Took) } } -// A walk kept before the times were counted has them not known, never shifted onto the next filesystem. +// A walk kept before the times were counted has them not known, never shifted onto the next filesystem, and +// its entries visited are a lower bound; the filesystem it was part-way through took a time not known. func TestAWalkKeptWithoutTimesSaysThemNotKnown(t *testing.T) { top, other := tree(t, "a"), tree(t, "b") p := &SetuidProgress{Mounts: []string{top, other}, Done: Paths{top}} @@ -65,7 +66,8 @@ func TestAWalkKeptWithoutTimesSaysThemNotKnown(t *testing.T) { if len(p.Took) != 2 || p.Took[0] >= 0 || p.Took[1] < 0 { t.Fatalf("took %v", p.Took) } - if w := progressOf(p, time.Time{}).Words(); !strings.Contains(w, top+" in a time not known") { + if w := progressOf(p, time.Time{}).Words(); !strings.Contains(w, top+" in a time not known") || + !strings.Contains(w, "at least ") { t.Fatalf("the words: %s", w) } } @@ -203,3 +205,14 @@ func TestSearchNowThrowsAwayTheLastSearchAndStartsAFullOne(t *testing.T) { t.Fatalf("the new search judges: %v", err) } } + +func TestAFilesystemPartWalkedByAnEngineThatDidNotCountTookATimeNotKnown(t *testing.T) { + top := tree(t, "a", "b") + p := &SetuidProgress{Mounts: []string{top}, Cursor: Cursors{top: filepath.Join(top, "a")}} + if err := mine().walk(t.Context(), p, func() {}); err != nil { + t.Fatal(err) + } + if len(p.Took) != 1 || p.Took[0] >= 0 { + t.Fatalf("took %v", p.Took) + } +} diff --git a/internal/accounts/setuid_reach_test.go b/internal/accounts/setuid_reach_test.go new file mode 100644 index 0000000..940f49a --- /dev/null +++ b/internal/accounts/setuid_reach_test.go @@ -0,0 +1,115 @@ +package accounts + +import ( + "io/fs" + "os" + "path/filepath" + "strings" + "testing" +) + +// A setuid program no package owns counts against an account only where it can execute it, read at every +// judgement from the modes on the way (novox/hq issue 361): the operator's copies in a home of mode 0700 are no +// way for the agent account, and become one the moment the home is opened, without a new search. + +const homeFind = "/home/op/usr/bin/mount" + +func homeMachine(home FileMode) *agentMachine { + m := clean() + m.setuid = homeFind + "\n" + m.nodes = map[string]FileMode{"/home/op": home} + return m +} + +func TestReachabilityDecidesWhetherAFindCounts(t *testing.T) { + op := func(perm fs.FileMode) FileMode { return FileMode{UID: 1500, GID: 1500, Perm: fs.ModeDir | perm} } + for _, c := range []struct { + name string + set func(*agentMachine) + counted bool + }{ + {"a home of 0700", func(m *agentMachine) {}, false}, + {"a home of 0755", func(m *agentMachine) { m.nodes["/home/op"] = op(0o755) }, true}, + {"a home its group may search, the agent in it", func(m *agentMachine) { + m.nodes["/home/op"], m.gids = op(0o710), "1600 1500" + }, true}, + {"a home its group may search, the agent not in it", func(m *agentMachine) { m.nodes["/home/op"] = op(0o710) }, false}, + {"an ACL on the home", func(m *agentMachine) { + m.files["/home/op"] = FileMode{UID: 1500, GID: 1500, Perm: 0o700} + m.acls = map[string][]ACLEntry{"/home/op": {{}}} + }, true}, + {"a link on the way", func(m *agentMachine) { + m.nodes["/home/op"] = FileMode{UID: 1500, GID: 1500, Perm: fs.ModeSymlink | 0o777} + }, true}, + {"a file the agent may not execute", func(m *agentMachine) { + m.nodes["/home/op"] = op(0o755) + m.nodes[homeFind] = FileMode{Perm: fs.ModeSetuid | 0o4750} + }, false}, + } { + t.Run(c.name, func(t *testing.T) { + m := homeMachine(FileMode{UID: 1500, GID: 1500, Perm: fs.ModeDir | 0o700}) + c.set(m) + v := lookAgent(t, m) + if c.counted != (v.State == Unhealthy) { + t.Fatalf("counted %v: %+v", c.counted, v) + } + if strings.Contains(v.Reason, "/home/op") { + t.Fatalf("a path in a home was said: %s", v.Reason) + } + if c.counted && !strings.Contains(v.Reason, "a setuid-root program no package owns in an account's home, which agent can execute") { + t.Fatalf("the reason: %s", v.Reason) + } + }) + } +} + +// The search keeps every find; whether one counts is read again at every look. +func TestAHomeOpenedAfterTheSearchCountsWithoutANewSearch(t *testing.T) { + m := homeMachine(FileMode{UID: 1500, GID: 1500, Perm: fs.ModeDir | 0o700}) + c := &SetuidCache{} + j := New(Exec{Run: m.run, Walk: m.walk, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl, + Lstat: m.lstat, Cache: c}) + j.Set([]Account{agent}) + j.Look(t.Context()) + c.Wait() + if st, _ := j.Look(t.Context()); st.Accounts[0].State != Healthy { + t.Fatalf("a find in a closed home: %+v", st.Accounts[0]) + } + m.nodes["/home/op"] = FileMode{UID: 1500, GID: 1500, Perm: fs.ModeDir | 0o755} + st, _ := j.Look(t.Context()) + if st.Accounts[0].State != Unhealthy || starts(c) != 1 { + t.Fatalf("the home opened: %+v, %d searches", st.Accounts[0], starts(c)) + } + m.nodes["/home/op"] = FileMode{UID: 1500, GID: 1500, Perm: fs.ModeDir | 0o700} + if st, _ := j.Look(t.Context()); st.Accounts[0].State != Healthy || starts(c) != 1 { + t.Fatalf("the home closed again: %+v", st.Accounts[0]) + } +} + +// On the machine's own modes, as their owner (the directories above a test's are its own): a directory without +// its owner's search bit hides what is in it. +func TestExecutableReadsTheMachinesModes(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root searches every directory") + } + top := tree(t, "home/bin/x!") + home := filepath.Join(top, "home") + me := os.Getuid() + noACL := func(string) ([]ACLEntry, error) { return nil, nil } + if err := os.Chmod(home, 0o600); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(home, 0o700) }) + if Executable(filepath.Join(home, "bin/x"), me, map[int]bool{}, lstatOf, noACL) { + t.Fatal("a file in a directory its account may not search was executable") + } + if err := os.Chmod(home, 0o700); err != nil { + t.Fatal(err) + } + if !Executable(filepath.Join(home, "bin/x"), me, map[int]bool{}, lstatOf, noACL) { + t.Fatal("a file every directory opens was not executable") + } + if Executable(filepath.Join(home, "bin/gone"), me, map[int]bool{}, lstatOf, noACL) { + t.Fatal("a file gone was executable") + } +} diff --git a/internal/accounts/setuid_walk.go b/internal/accounts/setuid_walk.go index 58f3e89..30c66f2 100644 --- a/internal/accounts/setuid_walk.go +++ b/internal/accounts/setuid_walk.go @@ -51,6 +51,10 @@ type SetuidProgress struct { Walking time.Duration `json:"walking"` // Kept is when this progress was kept, as the cache wrote it; zero before the first time. Kept time.Time `json:"kept"` + // Counted is a walk counted from its start: false for one kept by an engine before it counted, whose + // Visited is then a lower bound and whose filesystem under way took a time not known (the review of + // 2026-10-10). + Counted bool `json:"counted"` } // Paths are paths kept as their bytes. **Never as JSON strings**: encoding/json replaces bytes that are not @@ -135,6 +139,9 @@ func (w walker) walk(ctx context.Context, p *SetuidProgress, checkpoint func()) p.Took = append(p.Took, -1) } p.Took = p.Took[:len(p.Done)] + if !p.Counted && len(p.Cursor) > 0 { + p.Walking = -1 // walked partly by an engine that did not count its time + } last := time.Now() for _, m := range p.Mounts { if slices.Contains(p.Done, m) { @@ -185,7 +192,13 @@ func (w walker) walkMount(ctx context.Context, p *SetuidProgress, mount string, } // How long it walks this filesystem, carried on from what a resumed walk had spent in it. began, before := time.Now(), p.Walking - defer func() { p.Walking = before + time.Since(began) }() + spent := func() time.Duration { + if before < 0 { + return -1 + } + return before + time.Since(began) + } + defer func() { p.Walking = spent() }() return filepath.WalkDir(mount, func(path string, d fs.DirEntry, err error) error { if ctxErr := ctx.Err(); ctxErr != nil { return ctxErr @@ -238,7 +251,7 @@ func (w walker) walkMount(ctx context.Context, p *SetuidProgress, mount string, p.Visited++ } if time.Since(*last) >= w.every { - p.Walking = before + time.Since(began) + p.Walking = spent() checkpoint() *last = time.Now() } diff --git a/internal/accounts/ways.go b/internal/accounts/ways.go index 100770a..90d68e9 100644 --- a/internal/accounts/ways.go +++ b/internal/accounts/ways.go @@ -640,9 +640,31 @@ func (e Exec) moreWays(ctx context.Context, account string, uid int, groups []st if searchErr != nil && !errors.As(searchErr, ¬Judged) { return nil, searchErr } + // Counted only where the account can execute it, read now from the paths' modes, never kept with the + // search: an account may open its own home to others later without root, and no search would see it. + lstat := e.Lstat + if lstat == nil { + lstat = lstatOf + } + started := " (search started " + searchedAt.Local().Format("2006-01-02 15:04") + ")" + inHomes := 0 for _, p := range unowned { - ways = append(ways, "a setuid-root program no package owns: "+p+" (search started "+ - searchedAt.Local().Format("2006-01-02 15:04")+")") + if !Executable(p, uid, gids, lstat, acl) { + continue + } + if inAHome(p) { + inHomes++ // its path stays in the engine's journal: a home's names are its owner's + continue + } + ways = append(ways, "a setuid-root program no package owns, which "+account+" can execute: "+p+started) + } + switch { + case inHomes == 1: + ways = append(ways, "a setuid-root program no package owns in an account's home, which "+account+ + " can execute"+started) + case inHomes > 1: + ways = append(ways, fmt.Sprintf("%d setuid-root programs no package owns in accounts' homes, which %s can "+ + "execute%s", inHomes, account, started)) } // The kernel's protection of links: off, any account may link another's file where root then acts on it. @@ -682,6 +704,62 @@ func (e Exec) moreWays(ctx context.Context, account string, uid int, groups []st return ways, nil } +// Executable says whether an account of uid, in the groups gids, can execute path as the kernel decides it: +// every directory from the root down searchable for it, and the file executable for it, each by the owner's +// bits for its owner, the group's for a member, the others' for anybody else (novox/hq issue 361). It errs +// towards a way: a link on the way, an ACL on any of them, or a mode that could not be read is executable. +// A path gone is not: there is nothing to execute. +func Executable(path string, uid int, gids map[int]bool, lstat func(string) (FileMode, error), + acl func(string) ([]ACLEntry, error)) bool { + if uid == 0 { + return true + } + at := "/" + steps := []string{at} + for _, name := range components(path) { + at = filepath.Join(at, name) + steps = append(steps, at) + } + for i, p := range steps { + m, err := lstat(p) + if errors.Is(err, fs.ErrNotExist) { + return false + } + if err != nil || m.Perm&fs.ModeSymlink != 0 { + return true + } + if a, err := acl(p); (err != nil && !errors.Is(err, fs.ErrNotExist)) || len(a) > 0 { + return true + } + bit := fs.FileMode(0o001) + switch { + case m.UID == uid: + bit = 0o100 + case gids[m.GID]: + bit = 0o010 + } + if m.Perm&bit == 0 { + return false + } + if i < len(steps)-1 && !m.Perm.IsDir() { + return false // a file on the way: the path names nothing + } + } + return true +} + +// homes are where accounts' homes are: a path under one is said by count alone, its name its owner's. +var homes = []string{"/home/", "/root/", "/var/home/"} + +func inAHome(path string) bool { + for _, h := range homes { + if strings.HasPrefix(path, h) { + return true + } + } + return false +} + // walkFiles calls fn for every regular file below dir, through readDir. func walkFiles(readDir func(string) ([]fs.DirEntry, error), dir string, fn func(string) error) error { entries, err := readDir(dir) diff --git a/internal/accounts/ways_test.go b/internal/accounts/ways_test.go index c8be804..51451a3 100644 --- a/internal/accounts/ways_test.go +++ b/internal/accounts/ways_test.go @@ -71,11 +71,11 @@ func TestEachFurtherWayToRootIsSaid(t *testing.T) { panic(fmt.Sprintf("searched %v", args)) } } - }, "a setuid-root program no package owns: /srv/bin/rootshell"}, + }, "a setuid-root program no package owns, which agent can execute: /srv/bin/rootshell"}, {"setuid no package owns", func(m *agentMachine) { m.setuid = "/usr/bin/sudo\n/usr/local/bin/rootshell\n" m.packaged = map[string]bool{"/usr/bin/sudo": true} - }, "a setuid-root program no package owns: /usr/local/bin/rootshell"}, + }, "a setuid-root program no package owns, which agent can execute: /usr/local/bin/rootshell"}, } { t.Run(c.name, func(t *testing.T) { m := clean() @@ -124,7 +124,7 @@ func TestAFurtherQuestionUnansweredIsUnknown(t *testing.T) { return "", fmt.Errorf("%s: %w", name, exec.ErrNotFound) } return m.run(ctx, name, args...) - }, Walk: m.walk, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl}) + }, Walk: m.walk, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl, Lstat: m.lstat}) j.Set([]Account{agent}) if st, _ := j.Look(t.Context()); st.Accounts[0].State != Unknown { t.Fatalf("no package manager to ask: %+v", st.Accounts[0])