From 340d5069403404f665764a2551d56b97423cada2 Mon Sep 17 00:00:00 2001 From: goodboy Date: Thu, 13 Aug 2026 18:40:55 -0400 Subject: [PATCH] Bound generated UDS socket paths Actor names and custom runtime subdirs can otherwise produce unsafe or overlong pathname sockets after moving Darwin's bindspace to `/tmp`. Deats, - hash unsafe or over-budget actor names while retaining `@pid.sock` - share deterministic naming with post-kill socket cleanup - enforce Linux and Darwin `sun_path` byte budgets - restore non-Darwin dir validation and mode `0700` - validate every nested Darwin runtime-dir component Review: PR #480 (goodboy) https://github.com/goodboy/tractor/pull/480 (this patch was generated in some part by `opencode` using `gpt-5.6-sol` (`openai`)) --- tests/ipc/test_each_tpt.py | 137 +++++++++++++++++++++++++++++++++++++ tractor/ipc/_uds.py | 70 ++++++++++++++++++- tractor/runtime/_state.py | 65 +++++++++++------- tractor/spawn/_reap.py | 7 +- 4 files changed, 250 insertions(+), 29 deletions(-) diff --git a/tests/ipc/test_each_tpt.py b/tests/ipc/test_each_tpt.py index d7908cae..9c6d99bd 100644 --- a/tests/ipc/test_each_tpt.py +++ b/tests/ipc/test_each_tpt.py @@ -97,6 +97,143 @@ def test_macos_rt_dir_rejects_symlink( assert stat.S_IMODE(target_dir.stat().st_mode) == 0o755 +def test_rt_dir_rejects_non_directory( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +): + ''' + Preserve the non-Darwin runtime-directory type contract. + + Replacing `Path.is_dir()` with unguarded `lstat()` briefly made + existing files look like valid runtime directories on Linux. + This test points `platformdirs` at a regular file and proves + `get_rt_dir()` rejects it during initialization. + + ''' + rt_file: Path = tmp_path / 'runtime-file' + rt_file.touch() + monkeypatch.setattr(sys, 'platform', 'linux') + monkeypatch.setattr( + 'platformdirs.user_runtime_dir', + lambda appname: str(rt_file), + ) + + with pytest.raises(FileExistsError): + _state.get_rt_dir() + + new_rt_dir: Path = tmp_path / 'new-runtime-dir' + monkeypatch.setattr( + 'platformdirs.user_runtime_dir', + lambda appname: str(new_rt_dir), + ) + assert _state.get_rt_dir() == new_rt_dir + assert stat.S_IMODE(new_rt_dir.stat().st_mode) == 0o700 + + +def test_macos_rt_dir_rejects_intermediate_symlink( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +): + ''' + Reject symlinks in nested Darwin runtime subdirectories. + + The earlier final-component check allowed `link/child` to follow + an intermediate symlink and create `child` outside the secured + runtime root. This test installs that link and proves traversal + stops before anything is created in its target. + + ''' + rt_root: Path = tmp_path / f'tractor-{os.getuid()}' + target_dir: Path = tmp_path / 'target' + rt_root.mkdir(mode=0o700) + target_dir.mkdir() + (rt_root / 'link').symlink_to( + target_dir, + target_is_directory=True, + ) + monkeypatch.setattr(sys, 'platform', 'darwin') + monkeypatch.setattr(_state, '_DARWIN_TMPDIR', tmp_path) + + with pytest.raises(PermissionError, match='Unsafe Darwin'): + _state.get_rt_dir(subdir='link/child') + + assert not (target_dir / 'child').exists() + + +@pytest.mark.parametrize( + ('platform_name', 'path_limit'), + [ + ('darwin', 104), + ('linux', 108), + ], +) +def test_uds_sockname_compaction( + monkeypatch: pytest.MonkeyPatch, + platform_name: str, + path_limit: int, +): + ''' + Keep generated actor sockets safe and below Darwin's byte limit. + + Actor names are unrestricted identity strings. A long, multibyte, + or path-like name previously produced overlong or escaping socket + paths. These cases prove `UDSAddress.get_sockname()` preserves a + short legacy name, deterministically compacts unsafe names, keeps + the reaper's `@pid.sock` suffix, and stays within Darwin's byte + limit. + + ''' + from tractor.ipc._uds import UDSAddress + + bindspace: Path = Path('/tmp/tractor-501') + pid: int = 12345 + from tractor.ipc import _uds + + monkeypatch.setattr(sys, 'platform', platform_name) + monkeypatch.setattr(_uds, '_SUN_PATH_LIMIT', path_limit) + + short: Path = UDSAddress.get_sockname( + name='worker', + pid=pid, + bindspace=bindspace, + ) + long_name: str = 'actor-' + ('\u00e9' * 100) + compact: Path = UDSAddress.get_sockname( + name=long_name, + pid=pid, + bindspace=bindspace, + ) + unsafe: Path = UDSAddress.get_sockname( + name='../worker', + pid=pid, + bindspace=bindspace, + ) + + assert short == Path(f'worker@{pid}.sock') + assert compact == UDSAddress.get_sockname( + name=long_name, + pid=pid, + bindspace=bindspace, + ) + assert compact.name.endswith(f'@{pid}.sock') + assert unsafe.parent == Path('.') + assert '..' not in unsafe.name + assert len(os.fsencode(bindspace / compact)) < path_limit + + with pytest.raises(ValueError) as exc_info: + UDSAddress.get_sockname( + name=long_name, + pid=pid, + bindspace=Path('/tmp') / ('x' * 90), + ) + + errmsg: str = str(exc_info.value) + assert 'leaves no room' in errmsg + assert 'name was unsafe: False' in errmsg + assert 'name was over budget: True' in errmsg + assert f'AF_UNIX path limit: {path_limit}' in errmsg + + def test_uds_bindspace_created_implicitly( debug_mode: bool, bindspace_dir_str: str, diff --git a/tractor/ipc/_uds.py b/tractor/ipc/_uds.py index afd28848..f074820d 100644 --- a/tractor/ipc/_uds.py +++ b/tractor/ipc/_uds.py @@ -21,6 +21,7 @@ from __future__ import annotations from contextlib import ( contextmanager as cm, ) +import hashlib from pathlib import Path import os import sys @@ -96,6 +97,12 @@ else: log = get_logger() +_SUN_PATH_LIMIT: int = ( + 108 + if sys.platform == 'linux' + else 104 +) + def unwrap_sockpath( sockpath: Path, @@ -196,7 +203,7 @@ class UDSAddress( err_on_no_runtime=False, ) if actor: - sockname: str = f'{actor.aid.name}@{pid}' + sockname: str = actor.aid.name # XXX, orig version which broke both macOS (file-name # length) and `multiaddrs` ('::' invalid separator). # sockname: str = '::'.join(actor.uid) + f'@{pid}' @@ -222,15 +229,72 @@ class UDSAddress( # `(?P.+)@(?P\d+)\.sock` regex, and the # `spawn._reap` `{name}@{pid}.sock` reconstruction. token: str = uuid4().hex[:8] - sockname: str = f'{prefix}.{token}@{pid}' + sockname = f'{prefix}.{token}' - sockpath: Path = Path(f'{sockname}.sock') + sockpath: Path = cls.get_sockname( + name=sockname, + pid=pid, + bindspace=filedir, + ) return UDSAddress( filedir=filedir, filename=sockpath, maybe_pid=pid, ) + @classmethod + def get_sockname( + cls, + name: str, + pid: int, + bindspace: Path, + ) -> Path: + ''' + Build a safe, deterministic UDS socket filename. + + ''' + suffix: str = f'@{pid}.sock' + filename: str = f'{name}{suffix}' + unsafe: bool = ( + '\0' in name + or + '/' in name + or + bool(os.altsep and os.altsep in name) + or + Path(filename).is_absolute() + ) + too_long: bool = ( + len(os.fsencode(bindspace / filename)) + >= _SUN_PATH_LIMIT + ) + if ( + unsafe + or + too_long + ): + digest: str = hashlib.blake2s( + os.fsencode(name), + digest_size=16, + ).hexdigest() + filename = f'actor.{digest}{suffix}' + + sockpath: Path = bindspace / filename + path_nbytes: int = len(os.fsencode(sockpath)) + if path_nbytes >= _SUN_PATH_LIMIT: + raise ValueError( + f'UDS bindspace leaves no room for an AF_UNIX ' + f'socket filename!\n' + f'bindspace: {bindspace}\n' + f'name was unsafe: {unsafe}\n' + f'name was over budget: {too_long}\n' + f'compacted filename: {filename}\n' + f'encoded path bytes: {path_nbytes}\n' + f'AF_UNIX path limit: {_SUN_PATH_LIMIT}\n' + ) + + return Path(filename) + @classmethod def get_root(cls) -> UDSAddress: def_uds_filename: Path = 'registry@1616.sock' diff --git a/tractor/runtime/_state.py b/tractor/runtime/_state.py index df012cca..154266bb 100644 --- a/tractor/runtime/_state.py +++ b/tractor/runtime/_state.py @@ -383,6 +383,7 @@ def get_rt_dir( # Normalize and validate that `subdir` is a relative path # without any parent-directory ("..") components, to prevent # escaping the runtime directory. + subdir_path: Path|None = None if subdir: subdir_path = ( subdir @@ -400,32 +401,46 @@ def get_rt_dir( f'{subdir!r}\n' ) - rt_dir: Path = rt_dir / subdir_path - - try: - dir_stat: os.stat_result = rt_dir.lstat() - except FileNotFoundError: - rt_dir.mkdir( - mode=0o700, - parents=True, - exist_ok=True, # avoid `FileExistsError` from conc calls - ) - dir_stat = rt_dir.lstat() - - if rt_root is not None: - if ( - not stat.S_ISDIR(dir_stat.st_mode) - or - dir_stat.st_uid != os.getuid() - ): - raise PermissionError( - f'Unsafe Darwin runtime directory!\n' - f'path: {rt_dir}\n' - f'owner uid: {dir_stat.st_uid}\n' - f'mode: {stat.filemode(dir_stat.st_mode)}\n' + if rt_root is None: + if subdir_path is not None: + rt_dir = rt_dir / subdir_path + if not rt_dir.is_dir(): + rt_dir.mkdir( + # Runtime dirs hold IPC sockets; owner-only access + # prevents other users from traversing the bindspace. + mode=0o700, + parents=True, + exist_ok=True, ) - if rt_dir != rt_root: - rt_dir.relative_to(rt_root) + return rt_dir + + if subdir_path is not None: + for part in subdir_path.parts: + rt_dir = rt_dir / part + try: + dir_stat: os.stat_result = rt_dir.lstat() + except FileNotFoundError: + try: + # Every Darwin component is private so no other + # user can replace descendants below `rt_root`. + rt_dir.mkdir(mode=0o700) + except FileExistsError: + pass + dir_stat = rt_dir.lstat() + + if ( + not stat.S_ISDIR(dir_stat.st_mode) + or + dir_stat.st_uid != os.getuid() + ): + raise PermissionError( + f'Unsafe Darwin runtime directory!\n' + f'path: {rt_dir}\n' + f'owner uid: {dir_stat.st_uid}\n' + f'mode: {stat.filemode(dir_stat.st_mode)}\n' + ) + if stat.S_IMODE(dir_stat.st_mode) != 0o700: + rt_dir.chmod(0o700) return rt_dir diff --git a/tractor/spawn/_reap.py b/tractor/spawn/_reap.py index bf2c104d..f8ab4518 100644 --- a/tractor/spawn/_reap.py +++ b/tractor/spawn/_reap.py @@ -71,6 +71,7 @@ fd leak. Different bug class but same broader theme of from __future__ import annotations import os +from pathlib import Path from typing import TYPE_CHECKING import trio @@ -154,7 +155,11 @@ def unlink_uds_bind_addrs( and subactor is not None and proc.pid is not None ): - sockname: str = f'{subactor.aid.name}@{proc.pid}.sock' + sockname: Path = UDSAddress.get_sockname( + name=subactor.aid.name, + pid=proc.pid, + bindspace=UDSAddress.def_bindspace, + ) sockpath: str = str( UDSAddress.def_bindspace / sockname )