From 39066bc6144aab10d83d72cc8c35011b5db9fde7 Mon Sep 17 00:00:00 2001
From: Nathan Esquenazi <nesquena@gmail.com>
Date: Sat, 4 Apr 2026 22:25:08 -0700
Subject: [PATCH] security: fix env race, signing key, upload traversal,
 password hash (#106)

* security: fix four audit findings -- env race, signing key, upload traversal, password hash

1. Race condition in os.environ (HIGH): Per-session _agent_lock didn't
   prevent cross-session env writes from interleaving. Added global
   _ENV_LOCK in streaming.py that serializes the entire env save/restore
   block across all sessions.

2. Predictable signing key (MEDIUM): sha256(STATE_DIR) was deterministic.
   Now generates a random 32-byte key on first startup and persists it to
   STATE_DIR/.signing_key (chmod 600). Existing sessions invalidated on
   first restart (acceptable for a security fix).

3. Upload path traversal (MEDIUM): Filename '..' survived the regex
   sanitization (dots are allowed chars). Added explicit rejection of
   dot-only names and safe_resolve_ws() check to verify the resolved
   path stays within the workspace.

4. Weak password hashing (MEDIUM): Replaced bare SHA-256 with PBKDF2-
   SHA256 (600k iterations per OWASP). Uses stdlib hashlib.pbkdf2_hmac,
   no new dependencies. Note: existing passwords must be re-set after
   this change (hash format changed).

Closes #106

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: use random signing key as PBKDF2 salt (replaces predictable STATE_DIR salt)

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 api/auth.py      | 31 ++++++++++++++++++++++++++-----
 api/streaming.py |  8 +++++++-
 api/upload.py    |  6 +++++-
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/api/auth.py b/api/auth.py
index 2490c4e..0499591 100644
--- a/api/auth.py
+++ b/api/auth.py
@@ -26,14 +26,35 @@ _sessions = {}
 
 
 def _signing_key():
-    """Derive a stable signing key from STATE_DIR."""
-    return hashlib.sha256(str(STATE_DIR).encode()).digest()
+    """Return a random signing key, generating and persisting one on first call."""
+    key_file = STATE_DIR / '.signing_key'
+    if key_file.exists():
+        try:
+            raw = key_file.read_bytes()
+            if len(raw) >= 32:
+                return raw[:32]
+        except Exception:
+            pass
+    # Generate a new random key
+    key = secrets.token_bytes(32)
+    try:
+        STATE_DIR.mkdir(parents=True, exist_ok=True)
+        key_file.write_bytes(key)
+        key_file.chmod(0o600)
+    except Exception:
+        pass  # key works for this process even if persist fails
+    return key
 
 
 def _hash_password(password):
-    """SHA-256 hash with a salt derived from STATE_DIR."""
-    salt = str(STATE_DIR).encode()
-    return hashlib.sha256(salt + password.encode()).hexdigest()
+    """PBKDF2-SHA256 with 600k iterations (OWASP recommendation).
+    Salt is the persisted random signing key, which is secret and unique per
+    installation. This keeps the stored hash format a plain hex string
+    (no format change to settings.json) while replacing the predictable
+    STATE_DIR-derived salt from the original implementation."""
+    salt = _signing_key()
+    dk = hashlib.pbkdf2_hmac('sha256', password.encode(), salt, 600_000)
+    return dk.hex()
 
 
 def get_password_hash():
diff --git a/api/streaming.py b/api/streaming.py
index c6d4ab0..97bfba7 100644
--- a/api/streaming.py
+++ b/api/streaming.py
@@ -17,6 +17,12 @@ from api.config import (
     resolve_model_provider,
 )
 
+# Global lock for os.environ writes. Per-session locks (_agent_lock) prevent
+# concurrent runs of the SAME session, but two DIFFERENT sessions can still
+# interleave their os.environ writes. This global lock serializes the env
+# save/restore around the entire agent run.
+_ENV_LOCK = threading.Lock()
+
 # Lazy import to avoid circular deps -- hermes-agent is on sys.path via api/config.py
 try:
     from run_agent import AIAgent
@@ -101,7 +107,7 @@ def _run_agent_streaming(session_id, msg_text, model, workspace, stream_id, atta
             HERMES_HOME=_profile_home,
         )
         # Still set process-level env as fallback for tools that bypass thread-local
-        with _agent_lock:
+        with _ENV_LOCK:
           old_cwd = os.environ.get('TERMINAL_CWD')
           old_exec_ask = os.environ.get('HERMES_EXEC_ASK')
           old_session_key = os.environ.get('HERMES_SESSION_KEY')
diff --git a/api/upload.py b/api/upload.py
index 5cfe4d2..7e59da9 100644
--- a/api/upload.py
+++ b/api/upload.py
@@ -70,7 +70,11 @@ def handle_upload(handler):
             return j(handler, {'error': 'Session not found'}, status=404)
         workspace = Path(s.workspace)
         safe_name = _re.sub(r'[^\w.\-]', '_', Path(filename).name)[:200]
-        dest = workspace / safe_name
+        # Reject names that are purely dots (path traversal: ".." survives regex)
+        if not safe_name or safe_name.strip('.') == '':
+            return j(handler, {'error': 'Invalid filename'}, status=400)
+        # Verify the resolved path stays within the workspace
+        dest = safe_resolve_ws(workspace, safe_name)
         dest.write_bytes(file_bytes)
         return j(handler, {'filename': safe_name, 'path': str(dest), 'size': dest.stat().st_size})
     except Exception as e: