From 0f7981b8a14ec96e7355e936ac148714c79af9c9 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 29 Aug 2026 19:19:46 -0700 Subject: [PATCH] fix(browser): real-profile snapshot auth files are owner-only (#96729) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The snapshot dirs were 0700 but every file inside landed umask-wide: shutil.copy2 preserves Chrome's own 0644 profile-file modes and sqlite3.connect creates the online-backup destinations as plain umask files — so the copied Cookies / Login Data / Web Data (the user's live session credentials) sat 0644. The 0700 parents contain it by default, but the documented HERMES_HOME_MODE traversal hatch makes group/world- readable children a real exposure. snapshot_real_profile now reconciles every file (0600) and nested dir (0700) inside the snapshot through the house helpers (_secure_file / _secure_dir — managed-mode and container carve-outs included) at the end of every pass, so snapshots written by older builds heal on their next launch. Best-effort, never blocks a launch. Tests: owner-only walk under umask 022 (fails on the pre-fix code — sabotage-verified) + heal-on-refresh for a pre-existing 0644 Cookies. The issue's other two findings are already fixed on main: mock-keychain flags eliminated by the direct native-binary launch (#98249, salvage of #96763); the 'Device not configured' TTY failure is superseded by the same launch-path rework. --- hermes_cli/browser_connect.py | 33 ++++++++++++++ tests/tools/test_browser_real_profile.py | 55 ++++++++++++++++++++++-- 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/hermes_cli/browser_connect.py b/hermes_cli/browser_connect.py index c2a27868d8..558f59a342 100644 --- a/hermes_cli/browser_connect.py +++ b/hermes_cli/browser_connect.py @@ -610,6 +610,33 @@ def _secure_snapshot_root(path: str) -> None: logger.debug("could not secure real-profile snapshot dir %s: %s", path, e) +def _secure_snapshot_contents(dst: str) -> None: + """Owner-only modes for every file/dir INSIDE the snapshot (#96729). + + ``_secure_snapshot_root`` covers the top-level dirs, but the copied files + inherit the umask: ``shutil.copy2`` preserves the source's mode (Chrome + keeps its own profile 0644 inside a 0700 dir) and ``sqlite3.connect`` on + the backup destination creates plain umask files — so Cookies / Login + Data / Web Data landed 0644 and any nested profile subdir 0755. The 0700 + parents contain the damage by default, but the documented + ``HERMES_HOME_MODE`` hatch (nginx traversal) makes world-readable children + a real exposure — these are the user's live session cookies. Reconciled + through the house helpers (``_secure_dir`` / ``_secure_file``) on EVERY + snapshot pass, so older snapshots heal too; both helpers already carry the + managed-mode / container carve-outs. Best-effort: never blocks a launch. + """ + try: + from hermes_cli.config import _secure_dir, _secure_file + + for root, dirs, files in os.walk(dst): + for d in dirs: + _secure_dir(os.path.join(root, d)) + for f in files: + _secure_file(os.path.join(root, f)) + except Exception as e: # best-effort, same policy as _secure_snapshot_root + logger.debug("could not secure real-profile snapshot contents %s: %s", dst, e) + + # Auth files that are SQLite databases: on Windows a running Chrome holds these # with an exclusive lock, so a raw file copy raises WinError 32 ("being used by # another process") and a naive best-effort skip leaves the copy signed-out. @@ -1059,6 +1086,12 @@ def snapshot_real_profile(browser: str, src: str | None = None) -> tuple[str | N fh.write(source_profile) except OSError as e: logger.debug("real-profile snapshot: could not write done marker: %s", e) + # Owner-only modes for everything the copies above created — copy2 + # preserves Chrome's 0644 and sqlite backup files land umask-wide; + # these are the user's session cookies (#96729). Runs AFTER the marker + # write so the marker itself is covered, and on every pass so + # snapshots from older builds heal on their next launch. + _secure_snapshot_contents(dst) except OSError as e: return None, f"could not snapshot the '{browser}' profile into {dst}: {e}" return dst, None diff --git a/tests/tools/test_browser_real_profile.py b/tests/tools/test_browser_real_profile.py index 68ff002fa7..cb5bed6659 100644 --- a/tests/tools/test_browser_real_profile.py +++ b/tests/tools/test_browser_real_profile.py @@ -139,6 +139,53 @@ class TestSnapshotRealProfile: assert dst is None assert err and "was not found" in err + def test_snapshot_files_are_owner_only(self, tmp_path, monkeypatch): + """Every copied file must be 0600 and every dir 0700 (#96729). + + copy2 preserves Chrome's 0644 source modes and sqlite-backup files + land umask-wide, so without explicit reconciliation the user's + session-cookie copies are group/world-readable. + """ + import stat + + import hermes_cli.browser_connect as bc + src = self._make_profile(tmp_path / "real") + home = tmp_path / "hermes-home" + monkeypatch.setattr(bc, "get_hermes_home", lambda: home) + old_umask = os.umask(0o022) # the common default that produced 0644 + try: + dst, err = bc.snapshot_real_profile("chrome", src=str(src)) + finally: + os.umask(old_umask) + assert err is None and dst + offenders = [] + for root, dirs, files in os.walk(dst): + for d in dirs: + mode = stat.S_IMODE(os.stat(os.path.join(root, d)).st_mode) + if mode & 0o077: + offenders.append((os.path.join(root, d), oct(mode))) + for f in files: + mode = stat.S_IMODE(os.stat(os.path.join(root, f)).st_mode) + if mode & 0o077: + offenders.append((os.path.join(root, f), oct(mode))) + assert not offenders, f"group/world-accessible snapshot entries: {offenders}" + + def test_existing_lax_snapshot_heals_on_refresh(self, tmp_path, monkeypatch): + """A snapshot left 0644 by an older build tightens on the next pass.""" + import stat + + import hermes_cli.browser_connect as bc + src = self._make_profile(tmp_path / "real") + home = tmp_path / "hermes-home" + monkeypatch.setattr(bc, "get_hermes_home", lambda: home) + dst, err = bc.snapshot_real_profile("chrome", src=str(src)) + assert err is None and dst + cookies = os.path.join(dst, "Default", "Cookies") + os.chmod(cookies, 0o644) # simulate the pre-fix on-disk state + dst2, err2 = bc.snapshot_real_profile("chrome", src=str(src)) + assert err2 is None and dst2 == dst + assert stat.S_IMODE(os.stat(cookies).st_mode) == 0o600 + class TestRealProfileCdpLaunch: """The agent-browser-based launcher in browser_tool._real_profile_cdp.""" @@ -564,12 +611,14 @@ class TestSnapshotIsCredentialStore: (tmp_path / "real" / "Local State").write_text("{}") (src / "Cookies").write_text("db") monkeypatch.setattr(bc, "get_hermes_home", lambda: tmp_path / "hh") - called = {} + called = {"paths": []} with patch("hermes_cli.config._secure_dir", - side_effect=lambda p: called.__setitem__("p", p)): + side_effect=lambda p: called["paths"].append(p)): dst, err = bc.snapshot_real_profile("chrome", src=str(tmp_path / "real")) assert err is None - assert called.get("p") == dst # secured through the canonical owner + # Secured through the canonical owner; since #96729 the walk also + # secures every nested dir, so dst is IN the set rather than last. + assert dst in called["paths"] class TestReviewBugFixes: