diff --git a/tests/tools/test_bot_desktop_lease.py b/tests/tools/test_bot_desktop_lease.py index 9bd4d70aa5..4a763ddb6f 100644 --- a/tests/tools/test_bot_desktop_lease.py +++ b/tests/tools/test_bot_desktop_lease.py @@ -178,3 +178,26 @@ def test_lease_works_without_fcntl(tmp_path): out = subprocess.run([sys.executable, "-c", probe], capture_output=True, text=True, encoding="utf-8", timeout=60, stdin=subprocess.DEVNULL, env={**os.environ, "HERMES_HOME": str(tmp_path)}) assert out.stdout.strip() == "OK", out.stderr + + +@pytest.mark.linux_only +def test_lease_files_are_private_even_when_the_lease_is_written_before_the_screen_exists(tmp_path, monkeypatch): + """A takeover can be recorded before start() ever created bot-desktop/ 0700. The lease path then created + the directory and files with the umask (0755 / 0644): who holds the screen, and the lock the RFB bridge + serialises on, readable and clobberable by every other local user. Every piece must be owner-only.""" + import os + import stat + + home = tmp_path / "deep" / "home" + home.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(lease, "get_hermes_home", lambda: home) + old = os.umask(0o022) + try: + lease.acquire("v1") + finally: + os.umask(old) + sd = home / "bot-desktop" + for path in (sd, sd / "lease.json", sd / "lease.lock"): + assert path.exists(), path + assert stat.S_IMODE(path.stat().st_mode) & 0o077 == 0, f"{path.name} is {oct(path.stat().st_mode)}" diff --git a/tools/bot_desktop/launcher.sh b/tools/bot_desktop/launcher.sh index 4efecdaf9c..bfd02b1066 100755 --- a/tools/bot_desktop/launcher.sh +++ b/tools/bot_desktop/launcher.sh @@ -49,7 +49,10 @@ if [[ -e "$xlock" ]] && ! kill -0 "$(tr -d ' ' < "$xlock" 2>/dev/null)" 2>/dev/n rm -f "$xlock" "/tmp/.X11-unix/X${HERMES_BD_DISPLAY_NUM}" fi : > "$XAUTHORITY"; chmod 600 "$XAUTHORITY" -xauth -q -f "$XAUTHORITY" add "$DISPLAY" MIT-MAGIC-COOKIE-1 "$(od -An -N16 -tx1 /dev/urandom | tr -d ' \n')" +# The cookie goes in on stdin, not argv: a command line is readable by every local user via ps. +xauth -q -f "$XAUTHORITY" source - < Lease: return Lease(holder=HUMAN, viewer_id="unreadable-lease", reason="lease file corrupt") -def _write(path: Path, lease: Lease) -> None: +def _private_dir(path: Path) -> None: + """``bot-desktop/`` owner-only even when the lease is the first thing written there (a takeover can be + recorded before start() ever ran, and the umask would otherwise leave it 0755).""" path.parent.mkdir(parents=True, exist_ok=True) + secure_parent_dir(path) + + +def _open_private(path: str | bytes | os.PathLike, flags: int) -> int: + """``open(..., opener=_open_private)``: the file is created 0600 regardless of the umask.""" + return os.open(path, flags, 0o600) + + +def _write(path: Path, lease: Lease) -> None: + _private_dir(path) tmp = path.with_suffix(".json.tmp") - tmp.write_text(json.dumps(lease.as_dict()), encoding="utf-8") + with open(tmp, "w", encoding="utf-8", opener=_open_private) as fh: + fh.write(json.dumps(lease.as_dict())) os.replace(tmp, path) @@ -104,8 +117,8 @@ class _locked: def __enter__(self): if fcntl is None: return self - self._lockfile.parent.mkdir(parents=True, exist_ok=True) - self._fh = open(self._lockfile, "a+", encoding="utf-8") # noqa: SIM115 — closed in __exit__ + _private_dir(self._lockfile) + self._fh = open(self._lockfile, "a+", encoding="utf-8", opener=_open_private) # noqa: SIM115 — closed in __exit__ fcntl.flock(self._fh.fileno(), fcntl.LOCK_EX) return self diff --git a/tools/bot_desktop/runtime.py b/tools/bot_desktop/runtime.py index f0b7e293ed..8f003f8069 100644 --- a/tools/bot_desktop/runtime.py +++ b/tools/bot_desktop/runtime.py @@ -48,8 +48,10 @@ BINARY_PACKAGES = { "apt": {"Xvnc": "tigervnc-standalone-server", "xfwm4": "xfwm4", "xfce4-panel": "xfce4-panel", "xfdesktop": "xfdesktop4", "xfsettingsd": "xfce4-settings", "dbus-run-session": "dbus-x11", "xauth": "xauth", "xdpyinfo": "x11-utils", "setxkbmap": "x11-xkb-utils", "xprop": "x11-utils"}, - "dnf": {"Xvnc": "tigervnc-server-minimal", "xfwm4": "xfwm4", "xfce4-panel": "xfce4-panel", - "xfdesktop": "xfdesktop", "xfsettingsd": "xfce4-settings", "dbus-run-session": "dbus-x11", + # tigervnc-x11-server is the real package (tigervnc-server-minimal is only a Provides on it); dbus-run-session + # is in dbus-daemon (dbus-x11 ships dbus-launch only). + "dnf": {"Xvnc": "tigervnc-x11-server", "xfwm4": "xfwm4", "xfce4-panel": "xfce4-panel", + "xfdesktop": "xfdesktop", "xfsettingsd": "xfce4-settings", "dbus-run-session": "dbus-daemon", "xauth": "xorg-x11-xauth", "xdpyinfo": "xdpyinfo", "setxkbmap": "setxkbmap", "xprop": "xprop"}, "pacman": {"Xvnc": "tigervnc", "xfwm4": "xfwm4", "xfce4-panel": "xfce4-panel", "xfdesktop": "xfdesktop", "xfsettingsd": "xfce4-settings", "dbus-run-session": "dbus", @@ -60,8 +62,8 @@ PACKAGES = { "apt": ["tigervnc-standalone-server", "xfce4-panel", "xfwm4", "xfdesktop4", "xfce4-settings", "xfce4-terminal", "dbus-x11", "x11-xserver-utils", "x11-utils", "x11-xkb-utils", "xauth", "fonts-dejavu-core"], - "dnf": ["tigervnc-server-minimal", "xfce4-panel", "xfwm4", "xfdesktop", "xfce4-settings", - "xfce4-terminal", "dbus-x11", "xsetroot", "xset", "xdpyinfo", "xprop", "xorg-x11-xauth", "setxkbmap", + "dnf": ["tigervnc-x11-server", "xfce4-panel", "xfwm4", "xfdesktop", "xfce4-settings", + "xfce4-terminal", "dbus-daemon", "xsetroot", "xset", "xdpyinfo", "xprop", "xorg-x11-xauth", "setxkbmap", "dejavu-sans-fonts"], "pacman": ["tigervnc", "xfce4-panel", "xfwm4", "xfdesktop", "xfce4-settings", "xfce4-terminal", "dbus", "xorg-xsetroot", "xorg-xset", "xorg-xdpyinfo", "xorg-xprop", "xorg-xauth", "xorg-setxkbmap", @@ -229,7 +231,9 @@ def _kill_group_then_wait(pgid: Optional[int], pid: int, grace: float = 2.0) -> _signal(signal.SIGKILL) # windows-footgun: ok — Linux-only runtime (is_supported_host gates start/stop) -_ALLOC_LOCK = Path("/tmp/.hermes-bot-desktop-alloc.lock") # host-wide: profiles allocate from one band +# Host-wide (every profile allocates from one band), so it lives outside any profile home — but not in +# world-writable /tmp, where a predictable name lets another local user pre-create or squat the file. +_ALLOC_LOCK = Path(os.environ.get("XDG_RUNTIME_DIR") or Path.home() / ".cache") / "hermes-bot-desktop-alloc.lock" @contextlib.contextmanager @@ -256,6 +260,7 @@ def _pick_display() -> int: def _allocate_display() -> int: + _ALLOC_LOCK.parent.mkdir(parents=True, exist_ok=True, mode=0o700) with _flocked(_ALLOC_LOCK): return _pick_display() @@ -371,6 +376,7 @@ def start(*, wait_seconds: float = 15.0) -> DesktopStatus: return status() if _launcher_pid() is None: _reap_orphaned_server(sd) + _ALLOC_LOCK.parent.mkdir(parents=True, exist_ok=True, mode=0o700) with _flocked(_ALLOC_LOCK): return _spawn_and_wait(sd, _pick_display(), wait_seconds)