From 6898e5a3553ba762556057c97723ca6db7018714 Mon Sep 17 00:00:00 2001 From: Fangliquan Date: Sun, 26 Jul 2026 10:46:38 +0800 Subject: [PATCH] fix(hermes_cli): remove restartable root chown from s6 gateway log/run Root-context log/run used to pathname-chown hermes-writable log paths, which a hermes user can race through a symlink swap via the writable log control FIFO. Create the leaf with s6-setuidgid hermes mkdir instead; parent logs/gateways ownership stays a stage2 boot concern (#45258). --- hermes_cli/service_manager.py | 23 +-- tests/hermes_cli/test_service_manager.py | 232 +++++++++++++++++++++-- 2 files changed, 223 insertions(+), 32 deletions(-) diff --git a/hermes_cli/service_manager.py b/hermes_cli/service_manager.py index 471c6a412cb66..b01fce9302c37 100644 --- a/hermes_cli/service_manager.py +++ b/hermes_cli/service_manager.py @@ -783,17 +783,18 @@ class S6ServiceManager: f"# shellcheck shell=sh\n" f': "${{HERMES_HOME:=/opt/data}}"\n' f'log_dir="$HERMES_HOME/logs/gateways/{prof}"\n' - f'mkdir -p "$log_dir"\n' - # The gateways/ parent must be chowned too (non-recursively): - # `mkdir -p` creates it root-owned on a root-context boot, and a - # leaf-only chown leaves it that way — every profile registered - # later then runs its log service as hermes and crash-loops on - # `mkdir: Permission denied`. The parent chown runs on every - # root-context boot, so it also heals volumes already poisoned - # by older images. Non-recursive on purpose: sibling profile - # dirs are each managed by their own log/run. See #45258. - f'chown hermes:hermes "$HERMES_HOME/logs/gateways" 2>/dev/null || true\n' - f'chown -R hermes:hermes "$log_dir" 2>/dev/null || true\n' + # Create the leaf as hermes when this script starts as root. + # Never chown hermes-writable volume paths from this restartable + # root-context script: log/supervise/control is hermes-owned, so + # an unprivileged user can race a pathname check/chown through a + # symlink swap (CWE-59 / CWE-367). Parent logs/gateways is seeded + # hermes-owned at stage2 boot (#45258; + # tests/docker/test_log_dir_seed.py). + f'if [ "$(id -u)" = 0 ]; then\n' + f' s6-setuidgid hermes mkdir -p "$log_dir"\n' + f'else\n' + f' mkdir -p "$log_dir"\n' + f'fi\n' f'rm -f "$log_dir/lock"\n' # Skip the drop when already non-root (CAP_SETGID). f'[ "$(id -u)" = 0 ] || exec s6-log 1 n10 s1000000 T "$log_dir"\n' diff --git a/tests/hermes_cli/test_service_manager.py b/tests/hermes_cli/test_service_manager.py index 21107c248dc3a..99826b08cf779 100644 --- a/tests/hermes_cli/test_service_manager.py +++ b/tests/hermes_cli/test_service_manager.py @@ -1103,36 +1103,226 @@ def test_s6_stop_tolerates_marker_write_failure(monkeypatch, s6_scandir): assert any(cmd[0] == "s6-svc" and "-d" in cmd for cmd in svc_calls) -def test_s6_log_run_chowns_gateways_parent(s6_scandir, fake_subprocess_run) -> None: - """The log/run script must chown the logs/gateways/ parent, not just the leaf. +def _log_run_setup_fragment(rendered: str) -> str: + """Keep mkdir/rm setup from ``_render_log_run``; stop before ``s6-log``.""" + keep: list[str] = [] + for line in rendered.splitlines(keepends=True): + if line.startswith("#!/") or "shellcheck" in line: + continue + if "s6-log" in line: + break + keep.append(line) + return "#!/bin/sh\n" + "".join(keep) - Regression guard for #45258: `mkdir -p` creates the gateways/ parent - root-owned on a root-context boot, and a leaf-only chown leaves it that - way. Every profile registered later then runs its log service as the - dropped hermes user and s6-log crash-loops on `mkdir: Permission denied`. + +def test_s6_log_run_creates_leaf_as_hermes_without_chown( + s6_scandir, fake_subprocess_run, +) -> None: + """log/run must not root-chown volume paths; create the leaf as hermes. + + #45258 parent ownership is stage2's job (``logs/gateways`` seeded as + hermes). Restartable log/run must not pathname-chown a hermes-writable + tree from root — that is a symlink TOCTOU privilege-escalation hole. """ mgr = S6ServiceManager(scandir=s6_scandir) mgr.register_profile_gateway("coder") log_text = (s6_scandir / "gateway-coder" / "log" / "run").read_text() - parent_chown = 'chown hermes:hermes "$HERMES_HOME/logs/gateways"' - assert parent_chown in log_text, ( - "log/run must chown the logs/gateways parent so profiles added " - f"after a root-context boot can create their leaf dirs. Saw: {log_text!r}" + assert not any(line.lstrip().startswith("chown ") for line in log_text.splitlines()), ( + "restartable log/run must not invoke chown on hermes-writable paths; " + f"saw: {log_text!r}" ) - # Non-recursive on purpose: sibling profile leaf dirs are each managed - # by their own log/run; a recursive parent chown would race them. - assert 'chown -R hermes:hermes "$HERMES_HOME/logs/gateways"' not in log_text + assert 's6-setuidgid hermes mkdir -p "$log_dir"' in log_text + assert 'mkdir -p "$log_dir"' in log_text - # Ordering: mkdir creates the parent, then the parent chown repairs its - # ownership, then the leaf chown — all before s6-log execs. - mkdir_idx = log_text.index('mkdir -p "$log_dir"') - parent_idx = log_text.index(parent_chown) - leaf_idx = log_text.index('chown -R hermes:hermes "$log_dir"') + mkdir_as_hermes_idx = log_text.index('s6-setuidgid hermes mkdir -p "$log_dir"') exec_idx = log_text.index("s6-log 1 ") - assert mkdir_idx < parent_idx < leaf_idx < exec_idx + assert mkdir_as_hermes_idx < exec_idx - # The parent path must be a runtime env expansion, never a baked-in - # absolute path (same contract as the log_dir itself). + # Runtime path expansion, never a baked-in absolute path. assert '/opt/data/logs/gateways"' not in log_text + + +def test_s6_log_run_never_invokes_chown_with_symlinked_log_dir(tmp_path) -> None: + """Symlinked ``$log_dir`` must not cause any chown of the referent.""" + import os + import stat + import subprocess + import threading + import time + + import pytest + + if os.name == "nt": + pytest.skip("POSIX symlink + /bin/sh required") + + hermes_home = tmp_path / "hermes" + gateways = hermes_home / "logs" / "gateways" + gateways.mkdir(parents=True) + leaf = gateways / "coder" + leaf.mkdir() + + victim = tmp_path / "victim" + victim.mkdir() + (victim / "marker").write_text("keep", encoding="utf-8") + before = victim.stat() + + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + recorder = tmp_path / "chown_calls.txt" + (bin_dir / "chown").write_text( + "#!/bin/sh\n" + f'printf "%s\\n" "$*" >> "{recorder.as_posix()}"\n' + "exit 0\n", + encoding="utf-8", + ) + # Pretend we are root so the script takes the s6-setuidgid mkdir path, + # and make s6-setuidgid a no-op drop that just runs the command. + (bin_dir / "id").write_text( + "#!/bin/sh\n" + 'if [ "$1" = "-u" ]; then echo 0; exit 0; fi\n' + "exit 1\n", + encoding="utf-8", + ) + (bin_dir / "s6-setuidgid").write_text( + "#!/bin/sh\n" + "shift\n" + 'exec "$@"\n', + encoding="utf-8", + ) + for name in ("chown", "id", "s6-setuidgid"): + p = bin_dir / name + p.chmod(p.stat().st_mode | stat.S_IXUSR) + + script_path = tmp_path / "log_run_setup.sh" + script_path.write_text( + _log_run_setup_fragment(S6ServiceManager._render_log_run("coder")), + encoding="utf-8", + ) + script_path.chmod(script_path.stat().st_mode | stat.S_IXUSR) + + stop = threading.Event() + + def _clear_leaf() -> None: + if leaf.is_symlink(): + leaf.unlink() + elif leaf.is_dir(): + leaf.rmdir() + elif leaf.exists(): + leaf.unlink() + + def _swap_race() -> None: + # Alternate leaf between a real dir and a symlink to the victim while + # the setup fragment runs — proves there is no privileged chown window + # to win, unlike a check-then-chown preflight. + while not stop.is_set(): + try: + _clear_leaf() + leaf.symlink_to(victim) + time.sleep(0.001) + _clear_leaf() + leaf.mkdir() + except OSError: + pass + time.sleep(0.001) + + env = os.environ.copy() + env["HERMES_HOME"] = str(hermes_home) + env["PATH"] = f"{bin_dir.as_posix()}{os.pathsep}{env.get('PATH', '')}" + + racer = threading.Thread(target=_swap_race, daemon=True) + racer.start() + try: + for _ in range(40): + proc = subprocess.run( + ["/bin/sh", str(script_path)], + env=env, + capture_output=True, + text=True, + check=False, + ) + assert proc.returncode == 0, (proc.stdout, proc.stderr) + finally: + stop.set() + racer.join(timeout=2) + + assert not recorder.exists() or recorder.read_text(encoding="utf-8").strip() == "" + after = victim.stat() + assert after.st_uid == before.st_uid + assert after.st_gid == before.st_gid + assert (victim / "marker").read_text(encoding="utf-8") == "keep" + + +def test_s6_log_run_mkdir_as_hermes_on_real_dirs(tmp_path) -> None: + """Root-context setup creates ``$log_dir`` via ``s6-setuidgid hermes mkdir``.""" + import os + import stat + import subprocess + + import pytest + + if os.name == "nt": + pytest.skip("POSIX /bin/sh required") + + hermes_home = tmp_path / "hermes" + (hermes_home / "logs" / "gateways").mkdir(parents=True) + + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + mkdir_recorder = tmp_path / "mkdir_via_setuidgid.txt" + chown_recorder = tmp_path / "chown_calls.txt" + + (bin_dir / "id").write_text( + "#!/bin/sh\n" + 'if [ "$1" = "-u" ]; then echo 0; exit 0; fi\n' + "exit 1\n", + encoding="utf-8", + ) + (bin_dir / "s6-setuidgid").write_text( + "#!/bin/sh\n" + f'printf "%s\\n" "$*" >> "{mkdir_recorder.as_posix()}"\n' + "shift\n" + 'exec "$@"\n', + encoding="utf-8", + ) + (bin_dir / "chown").write_text( + "#!/bin/sh\n" + f'printf "%s\\n" "$*" >> "{chown_recorder.as_posix()}"\n' + "exit 0\n", + encoding="utf-8", + ) + for name in ("id", "s6-setuidgid", "chown"): + p = bin_dir / name + p.chmod(p.stat().st_mode | stat.S_IXUSR) + + script_path = tmp_path / "log_run_setup.sh" + script_path.write_text( + _log_run_setup_fragment(S6ServiceManager._render_log_run("coder")), + encoding="utf-8", + ) + script_path.chmod(script_path.stat().st_mode | stat.S_IXUSR) + + env = os.environ.copy() + env["HERMES_HOME"] = str(hermes_home) + env["PATH"] = f"{bin_dir.as_posix()}{os.pathsep}{env.get('PATH', '')}" + + proc = subprocess.run( + ["/bin/sh", str(script_path)], + env=env, + capture_output=True, + text=True, + check=False, + ) + assert proc.returncode == 0, (proc.stdout, proc.stderr) + + mkdir_calls = mkdir_recorder.read_text(encoding="utf-8").strip().splitlines() + assert any( + c.split()[:3] == ["hermes", "mkdir", "-p"] and "gateways/coder" in c + for c in mkdir_calls + ), mkdir_calls + assert (hermes_home / "logs" / "gateways" / "coder").is_dir() + assert ( + not chown_recorder.exists() + or chown_recorder.read_text(encoding="utf-8").strip() == "" + )