From 131666bc1f6cd1c1cb297504b8e2e5ca33c50e92 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 19 Nov 2025 09:51:20 +0000 Subject: [PATCH] fix(sensor-proxy): lock file permissions and deadlock prevention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final security hardening based on second Codex review: **Lock File Permission Fix (Security)** - Lock file now created with 0600 instead of 0644 - Prevents unprivileged users from opening lock and holding LOCK_EX - Without this, any local user could DoS the installer/self-heal - Added f.Chmod(0600) to fix permissions on existing lock files **Deadlock Prevention (Future-Proofing)** - Added documentation for future multi-file locking scenarios - Specifies consistent lock ordering requirement (config.yaml.lock before allowed_nodes.yaml.lock) - Prevents potential deadlocks if future commands modify multiple files - Current implementation only locks one file, so no immediate issue **Testing:** ✅ Lock file created as `-rw-------` (0600) ✅ Existing lock files with wrong perms get fixed ✅ Unprivileged users can no longer DoS the lock **Codex Validation:** - Locking is now correct (persistent .lock file, held during entire operation) - Atomic writes complete while lock is held - Validation honors actual config paths - Empty lists supported for operational flexibility - Error propagation prevents silent failures - No remaining race conditions or security issues Phase 2 is now complete and Codex-verified as secure. Related to Phase 2 fixes commit 804a638ea --- cmd/pulse-sensor-proxy/config_cmd.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cmd/pulse-sensor-proxy/config_cmd.go b/cmd/pulse-sensor-proxy/config_cmd.go index d8d3218..27bd1cc 100644 --- a/cmd/pulse-sensor-proxy/config_cmd.go +++ b/cmd/pulse-sensor-proxy/config_cmd.go @@ -237,14 +237,23 @@ func setAllowedNodes(path string, newNodes []string, replace bool) error { } // withLockedFile opens a lock file with exclusive locking and runs a callback +// +// IMPORTANT: If future commands need to modify multiple files, use consistent lock ordering +// to avoid deadlocks (e.g., always lock config.yaml.lock before allowed_nodes.yaml.lock) func withLockedFile(lockPath string, fn func(f *os.File) error) error { // Open or create the lock file (never deleted, persists across renames) - f, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, 0644) + // Use 0600 to prevent unprivileged users from holding LOCK_EX and DoS'ing the installer + f, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, 0600) if err != nil { return fmt.Errorf("failed to open lock file: %w", err) } defer f.Close() + // Ensure correct permissions even if file already exists with broader perms + if err := f.Chmod(0600); err != nil { + return fmt.Errorf("failed to set lock file permissions: %w", err) + } + // Acquire exclusive lock if err := unix.Flock(int(f.Fd()), unix.LOCK_EX); err != nil { return fmt.Errorf("failed to acquire file lock: %w", err)