fix(browser): real-profile snapshot auth files are owner-only (#96729)
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user