From 39ee6ca839ff60d629d3a7e0ce161b05872e951a Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sat, 3 Oct 2026 18:24:38 +0200 Subject: [PATCH] Entrypoint acts as root on nothing outside /data (closes #80) `bin/entrypoint.sh` now runs `netwatch-server prepare-data-dir`, which refuses a `DATA_DIR` that is not `/data` or a path below it written in full, then creates `DATA_DIR`, gives `/data` and everything in it to `netwatch`, and sets mode 750 on `/data` and `DATA_DIR`. Every step goes through a Go `os.Root` opened on `/data`, and the modes are set on the opened directories rather than by name, so neither a symbolic link already there nor one a host process swaps in while the container starts can make root create or change anything outside `/data`. The README says which `DATA_DIR` values are accepted. Model: opus-5-5 --- README.md | 6 +- TODO.md | 10 + backend/README.md | 5 +- backend/cmd/netwatch-server/main.go | 21 ++ backend/internal/reportbuf/datadir.go | 150 ++++++++++ backend/internal/reportbuf/datadir_test.go | 307 +++++++++++++++++++++ bin/entrypoint.sh | 25 +- 7 files changed, 501 insertions(+), 23 deletions(-) create mode 100644 backend/internal/reportbuf/datadir.go create mode 100644 backend/internal/reportbuf/datadir_test.go diff --git a/README.md b/README.md index d8fbffa..d124263 100644 --- a/README.md +++ b/README.md @@ -224,8 +224,10 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for netwatch needs: - `CORS_ALLOWED_ORIGINS`, default empty: other origins whose pages may call the API - `DEBUG`, default `false`: debug logging - - `DATA_DIR`, default `/data/reports`: leave unset; reports kept outside - `/data` do not survive a redeploy + - `DATA_DIR`, default `/data/reports`: the directory the reports are kept + in: `/data` or a path below it, with no `.` or `..` part and no extra `/`. + The container also stops if the path goes through a symbolic link that + leads out of `/data` or is written as a full path - `TRUSTED_PROXIES`, default empty: set it to the address the reverse proxy in front of the container connects from, as an IP address or CIDR; several are separated by commas. nginx takes the client address from diff --git a/TODO.md b/TODO.md index 8e10ec8..b4aee6d 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,16 @@ latest run passes. # Completed Steps +- 2026-10-03: root no longer acts outside `/data` when it prepares `DATA_DIR` + (issue #80): `bin/entrypoint.sh` runs `netwatch-server prepare-data-dir`, + which refuses a `DATA_DIR` that is not `/data` or a path below it written in + full, then creates `DATA_DIR`, gives `/data` and everything in it to + `netwatch` and sets the modes, all through a Go `os.Root` opened on `/data`. + That refuses any path leading out of `/data`, so neither a symbolic link + already there nor one a host process swaps in during the start can make root + create or change anything elsewhere, and `DATA_DIR=/etc` no longer gives + `/etc` to `netwatch`. The `README.md` section "Running under upaas" says which + values are accepted - 2026-10-03: `DATA_DIR_MAX_BYTES` is now how much of the report files is kept (issue #54): when a report would take them past it, the oldest report files are deleted to make room, each deletion logged, and at start files already diff --git a/backend/README.md b/backend/README.md index 42494ac..42faf05 100644 --- a/backend/README.md +++ b/backend/README.md @@ -107,8 +107,9 @@ this server. The image's entrypoint, `bin/entrypoint.sh`, starts the server as user `netwatch` (uid 1000) with `BIND_ADDRESS=127.0.0.1` and `PORT=8081`, so only nginx reaches it, and with `TRUSTED_PROXIES=127.0.0.1/32`, so it takes the client address nginx passes on and no other. `DATA_DIR` is `/data/reports`, on -the `/data` volume; the entrypoint creates it and gives it and `/data` to -`netwatch` before starting the server. nginx replaces the security headers +the `/data` volume; before starting the server, the entrypoint creates it and +gives it and `/data` to `netwatch` with `netwatch-server prepare-data-dir`, +which acts on nothing outside `/data`. nginx replaces the security headers this server sets with those in the root `security-headers.conf`, so those are what clients of the image see. diff --git a/backend/cmd/netwatch-server/main.go b/backend/cmd/netwatch-server/main.go index 7410367..0136dae 100644 --- a/backend/cmd/netwatch-server/main.go +++ b/backend/cmd/netwatch-server/main.go @@ -4,6 +4,7 @@ package main import ( "fmt" "os" + "os/user" "sneak.berlin/go/netwatch/internal/config" "sneak.berlin/go/netwatch/internal/globals" @@ -37,6 +38,26 @@ func main() { return } + // "netwatch-server prepare-data-dir DATA_DIR" gets DATA_DIR ready + // for the netwatch user, or exits 1 with the error; see + // reportbuf.PrepareDataDir. bin/entrypoint.sh runs it as root + // before it starts this server as that user. + if len(os.Args) == 3 && os.Args[1] == "prepare-data-dir" { + netwatch, err := user.Lookup("netwatch") + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + + err = reportbuf.PrepareDataDir("/data", os.Args[2], netwatch) + if err != nil { + fmt.Fprintf(os.Stderr, "DATA_DIR '%s': %v\n", os.Args[2], err) + os.Exit(1) + } + + return + } + globals.Appname = Appname globals.Version = Version diff --git a/backend/internal/reportbuf/datadir.go b/backend/internal/reportbuf/datadir.go new file mode 100644 index 0000000..c2fb9f6 --- /dev/null +++ b/backend/internal/reportbuf/datadir.go @@ -0,0 +1,150 @@ +package reportbuf + +import ( + "errors" + "io/fs" + "os" + "os/user" + "path/filepath" + "strconv" + "syscall" +) + +// ErrDataDirOutsideVolume is returned by PrepareDataDir for a DATA_DIR +// that is not the volume or a path below it, written in full. +var ErrDataDirOutsideVolume = errors.New( + "must be /data or a path below it, with no '.', '..' or extra '/'") + +// 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. +// +// bin/entrypoint.sh runs this as root, which would follow a symbolic +// link anywhere, so every step goes through an os.Root opened on +// volume: it follows a link only when it is written as a relative +// path that stays inside volume, and refuses any other. A process on +// the host can swap a link onto a path in volume at any moment while +// this runs. Even then, the os.Root checks each link as it reaches +// it. MkdirAll creates each directory inside a parent it already has +// 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. +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. + rel, err := filepath.Rel(volume, dir) + if err != nil || dir != filepath.Clean(dir) || !filepath.IsLocal(rel) { + return ErrDataDirOutsideVolume + } + + uid, err := strconv.Atoi(owner.Uid) + if err != nil { + return err + } + + gid, err := strconv.Atoi(owner.Gid) + if err != nil { + return err + } + + root, err := os.OpenRoot(volume) + if err != nil { + return err + } + + defer func() { _ = root.Close() }() + + err = root.MkdirAll(rel, dirPerms) + if err != nil { + return err + } + + err = lchownAll(root, ".", uid, gid) + if err != nil { + return err + } + + err = chmodDir(root, ".") + if err != nil { + return err + } + + 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() + + 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) + } + + if err != nil { + return err + } + } + + 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, +// following a link swapped in between. So chmodDir opens name through +// root and sets the mode on the open directory. It refuses anything +// but a directory: a directory has no second name (hard link), so the +// one opened is inside root, where any other file could be a hard link +// to one outside. +func chmodDir(root *os.Root, name string) error { + dir, err := root.Open(name) + if err != nil { + return err + } + + defer func() { _ = dir.Close() }() + + info, err := dir.Stat() + if err != nil { + return err + } + + if !info.IsDir() { + return &fs.PathError{Op: "chmod", Path: name, Err: syscall.ENOTDIR} + } + + return dir.Chmod(dirPerms) +} diff --git a/backend/internal/reportbuf/datadir_test.go b/backend/internal/reportbuf/datadir_test.go new file mode 100644 index 0000000..86b2048 --- /dev/null +++ b/backend/internal/reportbuf/datadir_test.go @@ -0,0 +1,307 @@ +package reportbuf_test + +import ( + "errors" + "io/fs" + "os" + "os/user" + "path/filepath" + "strconv" + "syscall" + "testing" + + "sneak.berlin/go/netwatch/internal/reportbuf" +) + +// reports is the last part of DATA_DIR in these tests, as in the +// image's /data/reports. +const reports = "reports" + +// currentUser is the user the test runs as, the only owner a test not +// run as root can give files to. +func currentUser() *user.User { + return &user.User{ + Uid: strconv.Itoa(os.Getuid()), + Gid: strconv.Itoa(os.Getgid()), + } +} + +// tempDirMode700 is a new directory in a t.TempDir with mode 0700, so +// a test can tell that PrepareDataDir left its mode alone. +func tempDirMode700(t *testing.T) string { + t.Helper() + + dir := filepath.Join(t.TempDir(), "d") + + err := os.Mkdir(dir, 0o700) + if err != nil { + t.Fatal(err) + } + + return dir +} + +func requireMode(t *testing.T, path string, want fs.FileMode) { + t.Helper() + + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + + if info.Mode() != want { + t.Errorf("%s: mode %v, want %v", path, info.Mode(), want) + } +} + +func requireMissing(t *testing.T, path string) { + t.Helper() + + _, err := os.Lstat(path) + if !errors.Is(err, fs.ErrNotExist) { + t.Errorf("%s: created, or Lstat failed: %v", path, err) + } +} + +func requireOwner(t *testing.T, path string, uid, gid uint32) { + t.Helper() + + info, err := os.Lstat(path) + if err != nil { + t.Fatal(err) + } + + stat, _ := info.Sys().(*syscall.Stat_t) + if stat.Uid != uid || stat.Gid != gid { + t.Errorf("%s: owner %d:%d, want %d:%d", path, stat.Uid, stat.Gid, + uid, gid) + } +} + +func TestPrepareDataDirCreatesDataDir(t *testing.T) { + t.Parallel() + + volume := t.TempDir() + dir := filepath.Join(volume, "a", reports) + + err := reportbuf.PrepareDataDir(volume, dir, currentUser()) + if err != nil { + t.Fatal(err) + } + + requireMode(t, volume, fs.ModeDir|0o750) + requireMode(t, dir, fs.ModeDir|0o750) +} + +// TestPrepareDataDirSetsModeOfExistingDataDir: a DATA_DIR already on +// the host with another mode gets the mode too, not only a new one. +func TestPrepareDataDirSetsModeOfExistingDataDir(t *testing.T) { + t.Parallel() + + volume := t.TempDir() + dir := filepath.Join(volume, reports) + + err := os.Mkdir(dir, 0o700) + if err != nil { + t.Fatal(err) + } + + err = reportbuf.PrepareDataDir(volume, dir, currentUser()) + if err != nil { + t.Fatal(err) + } + + requireMode(t, dir, fs.ModeDir|0o750) +} + +func TestPrepareDataDirTakesTheVolumeItself(t *testing.T) { + t.Parallel() + + volume := t.TempDir() + + err := reportbuf.PrepareDataDir(volume, volume, currentUser()) + if err != nil { + t.Fatal(err) + } + + requireMode(t, volume, fs.ModeDir|0o750) +} + +// TestPrepareDataDirRefusesDataDirOutsideVolume covers a DATA_DIR that +// is relative, outside the volume, or not written in full. +func TestPrepareDataDirRefusesDataDirOutsideVolume(t *testing.T) { + t.Parallel() + + volume := tempDirMode700(t) + for _, dir := range []string{ + reports, volume + "/../new", volume + "/", volume + "//" + reports, + volume + "/./" + reports, volume + "/" + reports + "/..", volume + "x", + "/etc", + } { + err := reportbuf.PrepareDataDir(volume, dir, currentUser()) + if !errors.Is(err, reportbuf.ErrDataDirOutsideVolume) { + t.Errorf("%q: error = %v, want ErrDataDirOutsideVolume", dir, err) + } + } + + requireMissing(t, filepath.Join(filepath.Dir(volume), "new")) + requireMode(t, volume, fs.ModeDir|0o700) +} + +// TestPrepareDataDirRefusesLinkOutOfVolume puts a symbolic link to a +// directory outside the volume on the path to DATA_DIR, written as a +// full path and as one that climbs out with '..', and as DATA_DIR +// itself, where the mode would be set through it. +func TestPrepareDataDirRefusesLinkOutOfVolume(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + climbsOut bool + link, dir string + }{ + {"full path", false, "x", "x/reports"}, + {"climbs out", true, "x", "x/reports"}, + {"DATA_DIR itself", false, reports, reports}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + outside := tempDirMode700(t) + volume := t.TempDir() + + climbOut, err := filepath.Rel(volume, outside) + if err != nil { + t.Fatal(err) + } + + target := outside + if tc.climbsOut { + target = climbOut + } + + err = os.Symlink(target, filepath.Join(volume, tc.link)) + if err != nil { + t.Fatal(err) + } + + err = reportbuf.PrepareDataDir(volume, + filepath.Join(volume, tc.dir), currentUser()) + if err == nil { + t.Error("no error") + } + + requireMissing(t, filepath.Join(outside, reports)) + requireMode(t, outside, fs.ModeDir|0o700) + }) + } +} + +// TestPrepareDataDirRefusesDanglingLink: DATA_DIR is a symbolic link +// to a name in the volume that does not exist, which is not created. +func TestPrepareDataDirRefusesDanglingLink(t *testing.T) { + t.Parallel() + + volume := t.TempDir() + + err := os.Symlink("missing", filepath.Join(volume, reports)) + if err != nil { + t.Fatal(err) + } + + err = reportbuf.PrepareDataDir(volume, filepath.Join(volume, reports), + currentUser()) + if err == nil { + t.Error("no error") + } + + requireMissing(t, filepath.Join(volume, "missing")) +} + +// TestPrepareDataDirTakesDirectoryNamedInLatin1: a host directory can +// hold names that are not valid UTF-8, here "café" written in Latin-1. +// A test not run as root can only check that PrepareDataDir goes into +// such a directory and gives it, and what it holds, to the current +// user. +func TestPrepareDataDirTakesDirectoryNamedInLatin1(t *testing.T) { + t.Parallel() + + volume := t.TempDir() + latin1 := filepath.Join(volume, "caf\xe9") + + err := os.Mkdir(latin1, 0o700) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(filepath.Join(latin1, "f"), nil, 0o600) + if err != nil { + t.Fatal(err) + } + + owner := currentUser() + + err = reportbuf.PrepareDataDir(volume, filepath.Join(volume, reports), + owner) + if err != nil { + t.Fatal(err) + } + + uid, _ := strconv.ParseUint(owner.Uid, 10, 32) + gid, _ := strconv.ParseUint(owner.Gid, 10, 32) + + requireOwner(t, latin1, uint32(uid), uint32(gid)) + 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. +func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { + t.Parallel() + + if os.Geteuid() != 0 { + t.Skip("only root can give files to another uid") + } + + outside := t.TempDir() + volume := t.TempDir() + old := filepath.Join(volume, "old") + + err := os.WriteFile(filepath.Join(outside, "f"), nil, 0o600) + if err != nil { + t.Fatal(err) + } + + err = os.Mkdir(old, 0o700) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(filepath.Join(old, "f"), nil, 0o600) + if err != nil { + t.Fatal(err) + } + + err = os.Symlink(outside, filepath.Join(old, "link")) + if err != nil { + t.Fatal(err) + } + + err = reportbuf.PrepareDataDir(volume, filepath.Join(volume, reports), + &user.User{Uid: "4242", Gid: "4343"}) + if err != nil { + t.Fatal(err) + } + + for _, path := range []string{ + volume, filepath.Join(volume, reports), old, + filepath.Join(old, "f"), filepath.Join(old, "link"), + } { + requireOwner(t, path, 4242, 4343) + } + + requireOwner(t, outside, 0, 0) + requireOwner(t, filepath.Join(outside, "f"), 0, 0) +} diff --git a/bin/entrypoint.sh b/bin/entrypoint.sh index 6f3bdf8..ee53bfe 100755 --- a/bin/entrypoint.sh +++ b/bin/entrypoint.sh @@ -63,26 +63,13 @@ 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. Both are given to the netwatch user here, with the mode the -# server gives a directory it creates, so the host directory needs no -# preparing. -# -# chown and chmod, run as root, change whatever a symbolic link on the -# path points to, anywhere in the container, and the netwatch user can -# put one in /data. So the start stops unless readlink -f, which -# follows every link on a path, gives /data and DATA_DIR back as they -# are. It also writes a path in full, so a DATA_DIR with '.', '..' or -# an extra '/' in it is refused too. +# 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. export DATA_DIR="${DATA_DIR:-/data/reports}" -mkdir -p "$DATA_DIR" || exit 1 -if [ "$(readlink -f /data)" != /data ] || - [ "$(readlink -f "$DATA_DIR")" != "$DATA_DIR" ]; then - echo "entrypoint: DATA_DIR must be a full path with no '.', '..'," \ - "extra '/' or symbolic link on it or on /data, not '$DATA_DIR'" >&2 - exit 1 -fi -chown -R netwatch:netwatch /data "$DATA_DIR" || exit 1 -chmod 750 /data "$DATA_DIR" || exit 1 +netwatch-server prepare-data-dir "$DATA_DIR" || exit 1 # A stop signal is only noted here; the loop below acts on it. stop_requested=""