feat(security): filesystem permissions + owner validation (#5)#137
Conversation
…OW-07) Ship-order item #5 — the final P0 from the security audit. HIGH-03 (sensitive data stored unencrypted at rest): - ~/.rewind/ directory: chmod 0700 on every Store::open() - rewind.db: chmod 0600 after opening - objects/ (blob store): chmod 0700 on BlobStore::new() - All applied on unix only (#[cfg(unix)]); non-unix unchanged LOW-07 (REWIND_DATA env var hijack): - New validate_owner() in Store::open(): rejects data dirs owned by a different uid (via libc::geteuid). Clear error message. - Group/world-writable dirs are auto-tightened to 0700 with a tracing::warn instead of refusing (less disruptive for upgrades). Python SDK (store.py): - _harden_dir(): chmod 0700 on data dir + blob store dir - _harden_file(): chmod 0600 on rewind.db after connect - _validate_owner(): warns on uid mismatch (mirrors Rust behavior but warns instead of refusing, since Python SDK is typically the recorder side, not the server) Audit doc updated: HIGH-03 and LOW-07 marked as fixed. All 5 ship- order items now show ✅ in the status column. Tests: 370+ Rust, 303 Python, 0 failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
risjai
left a comment
There was a problem hiding this comment.
Code Review — PR #137: Filesystem Permissions + Owner Validation (#5)
Final P0 from the security audit. Clean, small PR (+104/−1). Overall looks solid — here's the breakdown.
Overview
Hardens filesystem permissions on the ~/.rewind/ data directory, DB file, and blob store:
- Rust:
Store::open()sets dir 0700, db 0600;BlobStore::new()sets dir 0700;validate_owner()rejects dirs owned by other UIDs - Python: mirrors same with
_harden_dir,_harden_file,_validate_owner(warn-only) - Audit doc: HIGH-03 and LOW-07 marked fixed
🟡 Should-fix (non-blocking but worth addressing)
1. Blob files written at default umask
blobs.rs:29 uses fs::write(&path, data) — individual blob files inherit the process umask (often 0644 = world-readable). The parent dir is 0700 which prevents directory traversal by other users, but if the umask is loose (e.g. 0000 in some container runtimes), blobs would be world-readable.
Consider OpenOptions::new().write(true).create_new(true).mode(0o600) similar to the auth token pattern from PR #133 — or document that the parent dir ACL is the intended defense.
2. WAL/SHM files not explicitly hardened
SQLite in WAL mode creates rewind.db-wal and rewind.db-shm. These are created by SQLite itself, not your code, so they inherit the process umask too. The parent dir 0700 defends against this, but worth a comment noting this is intentional (so a future contributor doesn't move the DB outside ~/.rewind/ and lose the protection).
3. Python _validate_owner silently catches AttributeError
except (OSError, AttributeError):
pass # non-unix or permission errorAttributeError is correct for non-unix (no os.geteuid), but OSError silently swallows real permission errors that may indicate something is wrong. Consider logging at debug level rather than silently passing.
4. Rust validate_owner ordering: chmod before ownership check
std::fs::set_permissions(root, dir_perms)?; // chmod 0700
validate_owner(root)?; // check uidIf root is owned by another user and is world-writable, this tightens permissions on their directory before rejecting it. The chmod will likely fail (EPERM) on a dir you don't own, but if you do own it via group write bits, the ordering is fine. Just noting it — not a real problem in practice.
🟢 Nits
5. let _ = on two set_permissions calls
blobs.rs and db.rs:55 both let _ = the permission result. In blobs.rs this makes sense (best-effort), but in db.rs the directory chmod is ?-propagated while the file chmod is silently dropped. Be consistent — either both propagate or both are best-effort, with a comment explaining why.
6. Python _harden_dir / _harden_file swallow all OSError
Same pattern — pass on OSError. Fine for non-unix, but on unix a failed chmod is worth a debug log.
✅ What looks good
libc::geteuid()over reading/proc/self/status— correct and portable across unix#[cfg(unix)]gating means Windows/WASI compilation is unaffected- Python mirrors Rust behavior (warn instead of fail is the right call for the SDK/recorder side)
- Audit doc updates are accurate and complete — all 5 ship-order items now ✅
create_dir_allbeforeset_permissions— correct ordering (dir must exist to chmod)
Security assessment
The permission hardening is sound. The parent dir 0700 is the real defense — individual file modes are defense-in-depth. The validate_owner check closes the REWIND_DATA hijack vector correctly. No blockers.
LGTM pending the nits above. 🚢
…gging - Blob put() uses OpenOptions mode(0o600) on unix to avoid umask leaks - DB file chmod propagates errors instead of silent let _ = - WAL/SHM comment documents parent dir as intended defense - Python helpers log at debug level instead of silently swallowing errors - _validate_owner separates AttributeError (non-unix) from OSError Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
risjai
left a comment
There was a problem hiding this comment.
Re-review — PR #137 (post-fix commit 7e3dd49)
All should-fix and nit items addressed:
| Finding | Verdict |
|---|---|
| 🟡 Blob files at default umask | ✅ Fixed — OpenOptions::create_new().mode(0o600) on unix, fs::write fallback on non-unix |
| 🟡 WAL/SHM not hardened | ✅ Documented — comment explains parent dir 0700 is the intended defense |
| 🟡 Python silently swallows OSError | ✅ Fixed — _log.debug() on all three helpers, AttributeError separated from OSError in _validate_owner |
| 🟡 chmod-before-ownership ordering | Unchanged (fine — no real risk) |
🟢 Inconsistent let _ = vs ? |
✅ Fixed — DB file chmod now propagates with ? |
🟢 Python _harden_* silent pass |
✅ Fixed — debug logging |
The blob create_new + mode(0o600) pattern matches what was done for auth tokens in PR #133 — consistent and race-free.
LGTM — ship it. 🚢
Summary
Ship-order item #5 — the final P0 from the security audit. Closes HIGH-03 (sensitive data at rest) and LOW-07 (REWIND_DATA hijack).
Changes
Rust (
crates/rewind-store/)Store::open():~/.rewind/→chmod 0700(owner-only)rewind.db→chmod 0600(owner read/write)validate_owner(): rejects data dirs owned by a different uid vialibc::geteuid(). Group/world-writable dirs auto-tightened to 0700 withtracing::warn.BlobStore::new():~/.rewind/objects/→chmod 0700All unix-only (
#[cfg(unix)]); non-unix behavior unchanged.Python (
python/rewind_agent/store.py)_harden_dir()— chmod 0700 on data dir + blob store dir_harden_file()— chmod 0600 on rewind.db_validate_owner()— warns on uid mismatch (logs instead of refusing, since Python SDK is the recorder side)Audit doc
HIGH-03 and LOW-07 marked as ✅ fixed. All 5 ship-order items now show ✅.
Test plan
cargo test --workspace— 370+ tests, 0 failurespython3 -m pytest tests/ -v— 303 passedcreate_dir_all+chmodon fresh dirs is fineComplete audit ship order
🤖 Generated with Claude Code