From 14e4ff908d4ee86cd1019b3df21344760439f3f4 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 18 Oct 2026 20:38:58 +0000 Subject: [PATCH] prepare-data-dir leaves files with a second name to their owner (closes #143) A hard link in /data shares its owner with a file that can be anywhere on the same filesystem, so chowning it gave that file to the netwatch user. A regular file with more than one name now keeps its owner and its path is logged. Owners are now set on the opened directory or file after checking it, as chmodDir sets modes, so a hard link swapped in afterwards is caught. Symbolic links and other files that are neither directories nor regular files are no longer given away: they cannot be opened safely, and setting their owner by name could reach a hard link swapped in. Model: opus-5-5 --- TODO.md | 10 +++ bin/entrypoint.sh | 9 ++- internal/reportbuf/datadir.go | 126 +++++++++++++++++++++-------- internal/reportbuf/datadir_test.go | 18 +++-- 4 files changed, 119 insertions(+), 44 deletions(-) diff --git a/TODO.md b/TODO.md index 302faf8..161092b 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,16 @@ Run `make frontend-viewport-test` in CI as its own step; it is not part of # Completed Steps +- 2026-10-18: `netwatch-server prepare-data-dir` no longer changes the owner of + a file outside `/data` through a hard link in the volume + ([#143](https://git.eeqj.de/sneak/netwatch/issues/143)). A regular file with a + second name (hard link) keeps its owner, and the start logs its path. Owners + are now set on the opened directory or file, never by name, so a hard link + swapped in after the check is caught too; symbolic links and other files that + are neither directories nor regular files keep their owner, as setting it by + name could reach a hard link swapped in at that name. The test that runs as + root hard-links a file from outside the volume into it and finds its owner + unchanged - 2026-10-14: stopping the server waits for a report file write already under way, the periodic flush's or the one `Append` starts when 10 MiB are waiting, before the final flush diff --git a/bin/entrypoint.sh b/bin/entrypoint.sh index 8bf4ec4..fcc9595 100755 --- a/bin/entrypoint.sh +++ b/bin/entrypoint.sh @@ -66,10 +66,11 @@ done > /etc/nginx/trusted-proxies.conf # netwatch-server keeps its report files in DATA_DIR, on the /data # volume, which may be a host directory owned by root or by another # uid. Here, as root, netwatch-server prepare-data-dir creates DATA_DIR -# and gives /data and everything in it to the netwatch user, so the -# host directory needs no preparing. It stops the start, naming -# DATA_DIR, unless DATA_DIR is /data or a path below it, and it acts on -# nothing outside /data, whatever symbolic links it meets there. +# and gives /data and the directories and regular files in it to the +# netwatch user, so the host directory needs no preparing. It stops the +# start, naming DATA_DIR, unless DATA_DIR is /data or a path below it, +# and it acts on nothing outside /data, whatever symbolic or hard links +# it meets there. export DATA_DIR="${DATA_DIR:-/data/reports}" netwatch-server prepare-data-dir "$DATA_DIR" || exit 1 diff --git a/internal/reportbuf/datadir.go b/internal/reportbuf/datadir.go index c2fb9f6..6609154 100644 --- a/internal/reportbuf/datadir.go +++ b/internal/reportbuf/datadir.go @@ -3,6 +3,7 @@ package reportbuf import ( "errors" "io/fs" + "log/slog" "os" "os/user" "path/filepath" @@ -18,9 +19,13 @@ var ErrDataDirOutsideVolume = errors.New( // PrepareDataDir gets dir, the server's DATA_DIR, ready for owner, the // user the server runs as, so that a host directory mounted at volume, // /data in the image, needs no preparing: it creates dir, gives volume -// and everything in it to owner, and gives volume and dir the mode the -// server gives a directory it creates. dir must be volume or a path -// below it, with no '.', '..', empty part or '/' at the end. +// and the directories and regular files in it to owner, and gives +// volume and dir the mode the server gives a directory it creates. It +// leaves as they are a regular file with a second name (hard link), as +// that name could be outside volume, and any file that is neither a +// directory nor a regular file, such as a symbolic link (see +// chownAll). dir must be volume or a path below it, with no '.', '..', +// empty part or '/' at the end. // // bin/entrypoint.sh runs this as root, which would follow a symbolic // link anywhere, so every step goes through an os.Root opened on @@ -32,10 +37,9 @@ var ErrDataDirOutsideVolume = errors.New( // open, never following a link at the name it creates, and follows a // link on the path only as the os.Root allows, so a relative link // inside volume can lead it to create directories elsewhere inside -// volume. Lchown never changes what a link points to, and the modes -// are set on directories already opened (see chmodDir), so the most -// that process can do is make a step fail or wait, or act on -// something else inside volume. +// volume. The owners and modes are set on files already opened (see +// chownAll and chmodDir), so the most that process can do is make a +// step fail or wait, or act on something else inside volume. func PrepareDataDir(volume, dir string, owner *user.User) error { // rel is dir as a path from volume; IsLocal is false for one that // leads out of it. @@ -66,7 +70,7 @@ func PrepareDataDir(volume, dir string, owner *user.User) error { return err } - err = lchownAll(root, ".", uid, gid) + err = chownAll(root, ".", uid, gid) if err != nil { return err } @@ -79,38 +83,32 @@ func PrepareDataDir(volume, dir string, owner *user.User) error { return chmodDir(root, rel) } -// lchownAll gives name, a directory inside root, and everything in it -// to uid and gid. It reads each directory opened through root, not -// through root.FS(), which refuses a name that is not valid UTF-8, and -// calls Lchown on every entry, which gives a symbolic link itself to -// them, not what it points to. It goes into an entry only when the -// read found a directory there, so it follows no link it finds; one -// swapped in for that directory afterwards is followed only as the -// os.Root allows. -func lchownAll(root *os.Root, name string, uid, gid int) error { - err := root.Lchown(name, uid, gid) - if err != nil { - return err - } - - dir, err := root.Open(name) - if err != nil { - return err - } - - entries, err := dir.ReadDir(-1) - _ = dir.Close() - +// chownAll gives name, a directory inside root, and the directories +// and regular files in it to uid and gid. It opens each through root +// and gives the open file to them, as chmodDir sets a mode on the open +// directory. Root.Lchown would not hold: it acts on whatever is at the +// name by then, and a hard link swapped in there shares its owner with +// a file that could be outside root. chownAll goes into an entry only +// when the read found a directory there, so it follows no link it +// finds; one swapped in afterwards for a directory or a regular file +// is followed only as the os.Root allows. It leaves any file that is +// neither a directory nor a regular file, such as a symbolic link: +// opening one would follow the link or open the device, Root.Lchown +// would not hold, and the server needs to own none of them. +func chownAll(root *os.Root, name string, uid, gid int) error { + entries, err := chownDir(root, name, uid, gid) if err != nil { return err } for _, entry := range entries { entryName := filepath.Join(name, entry.Name()) - if entry.IsDir() { - err = lchownAll(root, entryName, uid, gid) - } else { - err = root.Lchown(entryName, uid, gid) + + switch { + case entry.IsDir(): + err = chownAll(root, entryName, uid, gid) + case entry.Type().IsRegular(): + err = chownFile(root, entryName, uid, gid) } if err != nil { @@ -121,6 +119,66 @@ func lchownAll(root *os.Root, name string, uid, gid int) error { return nil } +// chownDir gives name, a directory inside root, to uid and gid, and +// returns what it holds. Like chmodDir, it opens name through root and +// refuses anything but a directory, which has no second name (hard +// link). It reads the open directory, not root.FS(), which refuses a +// name that is not valid UTF-8. +func chownDir(root *os.Root, name string, uid, gid int) ([]fs.DirEntry, error) { + dir, err := root.Open(name) + if err != nil { + return nil, err + } + + defer func() { _ = dir.Close() }() + + info, err := dir.Stat() + if err != nil { + return nil, err + } + + if !info.IsDir() { + return nil, &fs.PathError{Op: "chown", Path: name, Err: syscall.ENOTDIR} + } + + err = dir.Chown(uid, gid) + if err != nil { + return nil, err + } + + return dir.ReadDir(-1) +} + +// chownFile gives name, a regular file inside root, to uid and gid, +// unless it has a second name (hard link): all the names of a file +// share its owner, and the other could be outside root. It opens name +// through root, reads the number of names from the open file and gives +// that file to them, so a hard link swapped in at name is caught too. +// It logs a file it leaves. +func chownFile(root *os.Root, name string, uid, gid int) error { + file, err := root.Open(name) + if err != nil { + return err + } + + defer func() { _ = file.Close() }() + + info, err := file.Stat() + if err != nil { + return err + } + + stat, ok := info.Sys().(*syscall.Stat_t) + if ok && stat.Nlink == 1 { + return file.Chown(uid, gid) + } + + slog.Warn("owner not changed: file has a second name (hard link)", + "path", file.Name()) + + return nil +} + // chmodDir gives name, a directory inside root, the mode the server // gives a directory it creates. Root.Chmod would not hold: on Linux it // checks that name is not a symbolic link, then sets the mode by name, diff --git a/internal/reportbuf/datadir_test.go b/internal/reportbuf/datadir_test.go index 86b2048..cb454eb 100644 --- a/internal/reportbuf/datadir_test.go +++ b/internal/reportbuf/datadir_test.go @@ -254,10 +254,11 @@ func TestPrepareDataDirTakesDirectoryNamedInLatin1(t *testing.T) { requireOwner(t, filepath.Join(latin1, "f"), uint32(uid), uint32(gid)) } -// TestPrepareDataDirGivesVolumeToOwner gives everything in the volume -// to a uid and gid that own nothing, which only root can do. A -// symbolic link in the volume to a directory outside it is given to -// them itself; what it points to is left as it was. +// TestPrepareDataDirGivesVolumeToOwner gives the directories and +// regular files in the volume to a uid and gid that own nothing, which +// only root can do. A symbolic link in the volume to a directory +// outside it, and a hard link to a file outside it, are left as they +// were, and so is what they lead to. func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { t.Parallel() @@ -289,6 +290,11 @@ func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { t.Fatal(err) } + err = os.Link(filepath.Join(outside, "f"), filepath.Join(old, "hardlink")) + if err != nil { + t.Fatal(err) + } + err = reportbuf.PrepareDataDir(volume, filepath.Join(volume, reports), &user.User{Uid: "4242", Gid: "4343"}) if err != nil { @@ -296,12 +302,12 @@ func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { } for _, path := range []string{ - volume, filepath.Join(volume, reports), old, - filepath.Join(old, "f"), filepath.Join(old, "link"), + volume, filepath.Join(volume, reports), old, filepath.Join(old, "f"), } { requireOwner(t, path, 4242, 4343) } + requireOwner(t, filepath.Join(old, "link"), 0, 0) requireOwner(t, outside, 0, 0) requireOwner(t, filepath.Join(outside, "f"), 0, 0) }