diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5def952..70b41fe 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -90,7 +90,12 @@ For day-to-day workflow and commands, see [`CLAUDE.md`](../CLAUDE.md) and region. TTL lives in the shared header and governs expiry for every reader, so a new header field that changes behavior must be added to this reuse check too — otherwise a process opening with a different value silently inherits the creator's, producing config-dependent behavior. -- **No second shard guard while one is live (reentrancy, issue #30).** A memory-backend +- **Shm files are owner-private (issue #39).** The mmap files hold serialized (and possibly + pickled) return values, so they live in a per-user directory `warp_cache-` created `0o700` + (under `/dev/shm` on Linux, `$TMPDIR` otherwise) and are created `0o600`. On a shared parent + like `/dev/shm`, `ensure_secure_dir` refuses a pre-existing dir not owned by us or with + group/other access, so a co-located user can't read the cache or pre-create a crafted file that + the victim would `pickle.loads`. Any new shm file-creation site must set `.mode(0o600)`. A memory-backend lookup runs arbitrary Python `__eq__` (via `PyObject_RichCompareBool`) *while a shard guard is held*. That `__eq__` can re-enter the same `CachedFunction` (or, on GIL builds, hand off the GIL to another thread that calls in) and take a second, conflicting guard — aliasing diff --git a/src/shm/region.rs b/src/shm/region.rs index f21e7eb..68606f6 100644 --- a/src/shm/region.rs +++ b/src/shm/region.rs @@ -4,6 +4,7 @@ /// cache: header + lock + hash table + slab arena. use std::fs; use std::io; +use std::os::unix::fs::{DirBuilderExt, MetadataExt, OpenOptionsExt, PermissionsExt}; use std::path::{Path, PathBuf}; use memmap2::MmapMut; @@ -11,14 +12,61 @@ use memmap2::MmapMut; use super::layout::{self, Bucket, Header, SlotHeader, BUCKET_EMPTY, MAGIC, SLOT_NONE, VERSION}; use super::lock::{ShmSeqLock, LOCK_SIZE}; -/// Where to store the mmap files. +/// File mode for the cache files: owner read/write only. They hold serialized +/// (possibly pickled) return values, so no group/other access (#39). +const FILE_MODE: u32 = 0o600; + +/// Current real user id. +fn current_uid() -> u32 { + // SAFETY: getuid() always succeeds and is thread-safe. + unsafe { libc::getuid() } +} + +/// Per-user directory holding the mmap cache files. +/// +/// The files contain serialized (and possibly pickled) return values, so they +/// must not be readable or pre-creatable by other local users (#39). On Linux the +/// parent is the world-writable, sticky `/dev/shm`, so the directory name is scoped +/// to the uid and created `0o700` (see `ensure_secure_dir`); on macOS/other `$TMPDIR` +/// is already per-user, but we keep the same layout for defense in depth. fn shm_dir() -> PathBuf { - if cfg!(target_os = "linux") { + let base = if cfg!(target_os = "linux") { PathBuf::from("/dev/shm") } else { - // macOS and other Unix: use TMPDIR - std::env::temp_dir().join("warp_cache") + std::env::temp_dir() + }; + base.join(format!("warp_cache-{}", current_uid())) +} + +/// Create the per-user shm directory as `0o700`, or — if it already exists — verify +/// it is a directory we own with no group/other access. This refuses to use a +/// directory a hostile local user pre-created on a shared parent like `/dev/shm`, +/// which would otherwise let them read or tamper with the cache files (#39). +fn ensure_secure_dir(dir: &Path) -> io::Result<()> { + match fs::DirBuilder::new().mode(0o700).create(dir) { + Ok(()) => return Ok(()), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {} + Err(e) => return Err(e), } + // symlink_metadata (not metadata): a symlink must not redirect us elsewhere. + let meta = fs::symlink_metadata(dir)?; + if !meta.file_type().is_dir() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + "shm path exists but is not a directory", + )); + } + if meta.uid() != current_uid() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "shm directory is owned by another user", + )); + } + // We own it — tighten perms if a previous run (or anyone) left it group/other-accessible. + if meta.permissions().mode() & 0o077 != 0 { + fs::set_permissions(dir, fs::Permissions::from_mode(0o700))?; + } + Ok(()) } /// The full shared-memory region, owning the mmap handle and providing @@ -50,9 +98,7 @@ impl ShmRegion { } let dir = shm_dir(); - if !dir.exists() { - fs::create_dir_all(&dir)?; - } + ensure_secure_dir(&dir)?; // Hash table must be power-of-2 for bitmask probing. `capacity * 2` and its // next-power-of-two must both fit in u32, or the table is silently mis-sized @@ -73,21 +119,23 @@ impl ShmRegion { let data_path = dir.join(format!("{name}.data")); let lock_path = dir.join(format!("{name}.lock")); - // Create or truncate the data file + // Create or truncate the data file (owner-only, #39) let data_file = fs::OpenOptions::new() .read(true) .write(true) .create(true) .truncate(true) + .mode(FILE_MODE) .open(&data_path)?; data_file.set_len(total_size as u64)?; - // Create or truncate the lock file + // Create or truncate the lock file (owner-only, #39) let lock_file = fs::OpenOptions::new() .read(true) .write(true) .create(true) .truncate(true) + .mode(FILE_MODE) .open(&lock_path)?; lock_file.set_len(LOCK_SIZE as u64)?; @@ -189,9 +237,7 @@ impl ShmRegion { ttl_nanos: u64, ) -> io::Result { let dir = shm_dir(); - if !dir.exists() { - fs::create_dir_all(&dir)?; - } + ensure_secure_dir(&dir)?; let data_path = dir.join(format!("{name}.data")); let lock_path = dir.join(format!("{name}.lock")); let init_path = dir.join(format!("{name}.init")); @@ -208,6 +254,7 @@ impl ShmRegion { .write(true) .create(true) .truncate(false) // never truncate — it's purely an advisory lock file + .mode(FILE_MODE) // owner-only (#39) .open(&init_path)?; init_file.lock()?; diff --git a/tests/test_complex_objects.py b/tests/test_complex_objects.py index 0fbb9ea..bd5ea20 100644 --- a/tests/test_complex_objects.py +++ b/tests/test_complex_objects.py @@ -34,7 +34,7 @@ class User: def _cleanup_shm(): tmpdir = tempfile.gettempdir() - shm_dir = os.path.join(tmpdir, "warp_cache") + shm_dir = os.path.join(tmpdir, f"warp_cache-{os.getuid()}") if os.path.isdir(shm_dir): for f in glob.glob(os.path.join(shm_dir, "*")): with contextlib.suppress(OSError): diff --git a/tests/test_introspection.py b/tests/test_introspection.py index 0f22af1..5349968 100644 --- a/tests/test_introspection.py +++ b/tests/test_introspection.py @@ -23,7 +23,9 @@ def _cleanup_shm(): - shm_dir = os.path.join(tempfile.gettempdir(), "warp_cache") + if not hasattr(os, "getuid"): # Windows: no shared backend / shm files + return + shm_dir = os.path.join(tempfile.gettempdir(), f"warp_cache-{os.getuid()}") if os.path.isdir(shm_dir): for f in glob.glob(os.path.join(shm_dir, "*")): with contextlib.suppress(OSError): @@ -36,9 +38,7 @@ def _cleanup_shm(): "memory", pytest.param( "shared", - marks=pytest.mark.skipif( - sys.platform == "win32", reason="shared memory is Unix-only" - ), + marks=pytest.mark.skipif(sys.platform == "win32", reason="shared memory is Unix-only"), ), ], ) diff --git a/tests/test_shared_basic.py b/tests/test_shared_basic.py index ad3f2e2..de4c0a8 100644 --- a/tests/test_shared_basic.py +++ b/tests/test_shared_basic.py @@ -3,6 +3,8 @@ import contextlib import glob import os +import stat +import sys import tempfile import pytest @@ -13,7 +15,7 @@ def _cleanup_shm(): """Remove any leftover shared memory files.""" tmpdir = tempfile.gettempdir() - shm_dir = os.path.join(tmpdir, "warp_cache") + shm_dir = os.path.join(tmpdir, f"warp_cache-{os.getuid()}") if os.path.isdir(shm_dir): for f in glob.glob(os.path.join(shm_dir, "*")): with contextlib.suppress(OSError): @@ -425,3 +427,39 @@ def test_oversized_max_size_raises(self, max_size): @cache(max_size=max_size, backend="shared") def fn(x): return x + + +class TestSharedFilePermissions: + """Regression for #39: the mmap cache files hold serialized (and possibly + pickled) return values, so they must be owner-only (0o600) inside a per-user + 0o700 directory — not world-readable in a shared dir like /dev/shm, where + another local user could read them or pre-create a crafted file.""" + + def setup_method(self): + _cleanup_shm() + + def teardown_method(self): + _cleanup_shm() + + @pytest.mark.skipif(sys.platform == "win32", reason="shared memory is Unix-only") + def test_shm_dir_and_files_are_owner_only(self): + # Mirror src/shm/region.rs::shm_dir. + base = "/dev/shm" if sys.platform.startswith("linux") else tempfile.gettempdir() + shm_dir = os.path.join(base, f"warp_cache-{os.getuid()}") + + @cache(max_size=16, backend="shared") + def fn(x): + return x * 2 + + assert fn(1) == 2 # creates the region + files + + # The per-user directory must be private (owner rwx only). + dmode = stat.S_IMODE(os.stat(shm_dir).st_mode) + assert dmode == 0o700, f"{shm_dir} is {oct(dmode)}, expected 0o700" + + # Every cache file must be owner read/write only — no group/other bits. + files = glob.glob(os.path.join(shm_dir, "*")) + assert files, "no shm files were created" + for f in files: + fmode = stat.S_IMODE(os.stat(f).st_mode) + assert fmode & 0o077 == 0, f"{f} is group/other-accessible: {oct(fmode)}" diff --git a/tests/test_shared_multiprocess.py b/tests/test_shared_multiprocess.py index e006002..2188067 100644 --- a/tests/test_shared_multiprocess.py +++ b/tests/test_shared_multiprocess.py @@ -20,22 +20,21 @@ from warp_cache._warp_cache_rs import SharedCachedFunction +def _shm_dir(): + # Must match src/shm/region.rs::shm_dir: per-user dir (#39) — warp_cache- + # under /dev/shm on Linux, $TMPDIR otherwise. + base = "/dev/shm" if sys.platform.startswith("linux") else tempfile.gettempdir() + return os.path.join(base, f"warp_cache-{os.getuid()}") + + def _cleanup_shm(): - tmpdir = tempfile.gettempdir() - shm_dir = os.path.join(tmpdir, "warp_cache") + shm_dir = _shm_dir() if os.path.isdir(shm_dir): for f in glob.glob(os.path.join(shm_dir, "*")): with contextlib.suppress(OSError): os.unlink(f) -def _shm_dir(): - # Must match src/shm/region.rs::shm_dir: /dev/shm on Linux, $TMPDIR/warp_cache otherwise. - if sys.platform.startswith("linux"): - return "/dev/shm" - return os.path.join(tempfile.gettempdir(), "warp_cache") - - def _unlink_shm(name): """Remove all files for a shm name so the next open is a cold create.""" for suffix in (".data", ".lock", ".init"): diff --git a/tests/test_shared_realistic.py b/tests/test_shared_realistic.py index 08d5e7b..2040205 100644 --- a/tests/test_shared_realistic.py +++ b/tests/test_shared_realistic.py @@ -16,7 +16,7 @@ def _cleanup_shm(): tmpdir = tempfile.gettempdir() - shm_dir = os.path.join(tmpdir, "warp_cache") + shm_dir = os.path.join(tmpdir, f"warp_cache-{os.getuid()}") if os.path.isdir(shm_dir): for f in glob.glob(os.path.join(shm_dir, "*")): with contextlib.suppress(OSError):