diff --git a/src/auth/login_view.py b/src/auth/login_view.py index 4b60274..43b447e 100644 --- a/src/auth/login_view.py +++ b/src/auth/login_view.py @@ -1,5 +1,6 @@ """Login view with email/password and MFA support.""" +import logging from collections.abc import Callable from typing import Any @@ -8,6 +9,8 @@ from src.auth.session_manager import SessionManager +logger = logging.getLogger(__name__) + class LoginView(ft.Column): """Login form with email, password, and optional MFA fields.""" @@ -213,8 +216,9 @@ async def _handle_login(self, e: ft.Event[ft.Button]) -> None: self.status_text.color = ft.Colors.RED_400 await self.password_field.focus() - except Exception as ex: - self.status_text.value = f"Error: {ex}" + except Exception: + logger.exception("Unexpected error during login") + self.status_text.value = "Sign-in failed. Please try again." self.status_text.color = ft.Colors.RED_400 finally: diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py index a14137f..95f785c 100644 --- a/src/auth/session_manager.py +++ b/src/auth/session_manager.py @@ -1,5 +1,8 @@ """Manages Monarch Money authentication and session persistence.""" +import os +import stat +import sys from pathlib import Path import keyring @@ -11,6 +14,82 @@ SESSION_FILE = SESSION_DIR / "session.pickle" +def _session_file_is_safe_to_load(path: Path) -> bool: + """Refuse to deserialize session pickle if filesystem metadata is loose. + + The MonarchMoney library uses pickle, so loading an attacker-writable file + is RCE. This gate only defends against cross-user tampering: on POSIX we + require the path to be a non-symlink regular file owned by the current + uid and not group- or world-writable. It does NOT mitigate a malicious + process running as the same user — that attacker can write a 0o600 file + owned by the user that will pass this check. Removing pickle from the + persistence format (or adding a keyring-backed HMAC over the blob) is the + only real fix for that threat model. On Windows, mode bits don't reflect + NTFS ACLs, so permission checks are skipped, but we still require a + regular (non-symlink) file. + """ + try: + st = path.lstat() + except OSError: + return False + if stat.S_ISLNK(st.st_mode): + # Reject symlinks outright: a symlink owned by us could point at + # an attacker-controlled pickle, and the mode bits on the link + # itself aren't meaningful. + return False + if not stat.S_ISREG(st.st_mode): + # Directories, FIFOs, sockets, devices — none of these can be + # safely unpickled, and unlink() won't even clean up directories. + return False + if sys.platform == "win32": + return True + if st.st_uid != os.getuid(): + return False + return not st.st_mode & 0o022 + + +def _chmod_session_file() -> None: + """Tighten session file perms to 0o600, tolerating non-POSIX filesystems.""" + try: + SESSION_FILE.chmod(0o600) + except OSError: + pass + + +def _prepare_session_file_for_write() -> None: + """Clear any unsafe existing file before save_session. + + MonarchMoney's save_session() writes through whatever is at the path, + so a planted filesystem object there can redirect or expose the + pickle. An existing regular file is kept only when it's safe: on + POSIX, owned by the current uid and with no group or world + permission bits set at all (mask 0o077) — otherwise another uid + could pre-create the file or leave it group/world-readable before + our next save writes secrets into it, and _chmod_session_file()'s + 0o600 tighten wouldn't close that read window in time. On Windows + we accept any regular file because POSIX uid/mode bits don't map + to NTFS ACLs. Anything else gets unlinked, and if unlink fails + (e.g. a directory was planted) the OSError propagates and save + fails closed. + """ + try: + st = SESSION_FILE.lstat() + except FileNotFoundError: + return + if stat.S_ISREG(st.st_mode): + if sys.platform == "win32": + return + # Reject any group/other perms (0o077), not just writable bits. + # Otherwise a pre-existing 0o644 file (e.g. from a stale login + # with a loose umask) leaves a world-readable window between + # save_session() writing the pickle and _chmod_session_file() + # tightening it. Safer to unlink + let save_session create a + # fresh file that hits 0o600 immediately via our chmod. + if st.st_uid == os.getuid() and not st.st_mode & 0o077: + return + SESSION_FILE.unlink() + + class SessionManager: """Handles login, MFA, and session token persistence.""" @@ -45,7 +124,16 @@ def clear_credentials(self) -> None: async def try_restore_session(self) -> bool: """Attempt to restore a saved session. Returns True if successful.""" - if not SESSION_FILE.exists(): + # No early `SESSION_FILE.exists()` check — it returns False for + # dangling symlinks, which would skip the safety gate and leave a + # planted symlink in place for a later `save_session()` to follow. + # The safety gate handles missing files correctly (lstat → OSError + # → False); unlink() below handles "not there" via OSError. + if not _session_file_is_safe_to_load(SESSION_FILE): + try: + SESSION_FILE.unlink() + except OSError: + pass return False try: self._mm.load_session(str(SESSION_FILE)) @@ -65,19 +153,26 @@ async def try_restore_session(self) -> bool: async def login(self, email: str, password: str) -> None: """Login with email/password. Raises RequireMFAException if MFA needed.""" await self._mm.login(email=email, password=password) + _prepare_session_file_for_write() self._mm.save_session(str(SESSION_FILE)) - SESSION_FILE.chmod(0o600) + _chmod_session_file() self._authenticated = True async def login_with_mfa(self, email: str, password: str, mfa_code: str) -> None: """Login with email/password/MFA code.""" await self._mm.multi_factor_authenticate(email, password, mfa_code) + _prepare_session_file_for_write() self._mm.save_session(str(SESSION_FILE)) - SESSION_FILE.chmod(0o600) + _chmod_session_file() self._authenticated = True def logout(self) -> None: self._authenticated = False self.clear_credentials() - if SESSION_FILE.exists(): + # Best-effort cleanup. Catching OSError (not just FileNotFoundError) + # so a planted directory at SESSION_FILE doesn't crash logout — + # unlink() can't remove dirs and raises IsADirectoryError. + try: SESSION_FILE.unlink() + except OSError: + pass diff --git a/src/data/cache.py b/src/data/cache.py index 03316a6..9575442 100644 --- a/src/data/cache.py +++ b/src/data/cache.py @@ -1,7 +1,10 @@ """Simple SQLite cache for Monarch data to support offline/fast access.""" import json +import os import sqlite3 +import stat +import sys from datetime import datetime, timedelta from pathlib import Path from typing import Any @@ -16,7 +19,60 @@ class DataCache: def __init__(self, db_path: Path = CACHE_DB) -> None: db_path.parent.mkdir(parents=True, exist_ok=True, mode=0o700) + # Idempotent pre-create at 0o600 so there's no umask-default + # window where sqlite3.connect() creates the file with loose + # perms. Only run this path when O_NOFOLLOW is available: without + # it, O_CREAT could in theory follow a planted symlink and + # create/affect the target before the lstat gate below sees it. + # On platforms without O_NOFOLLOW (effectively Windows), skip + # the pre-create — we can't express "create without following + # symlinks", and the umask-default window doesn't carry the same + # meaning there (NTFS ACLs, not POSIX mode bits). + # FileExistsError covers the pre-existing regular-file case + # (legitimate prior run, or a race). Other OSErrors (EACCES, + # ENOSPC, ELOOP from a symlink) propagate. + if hasattr(os, "O_NOFOLLOW"): + flags = os.O_CREAT | os.O_WRONLY | os.O_EXCL | os.O_NOFOLLOW + try: + fd = os.open(str(db_path), flags, 0o600) + os.close(fd) + except FileExistsError: + pass + # Authoritative validation immediately before sqlite3.connect — + # closes the TOCTOU window where a symlink could be swapped in + # between pre-create and connect. lstat() so a planted symlink + # is seen as a symlink, not followed. On Windows (no pre-create) + # the file may not exist yet — let sqlite3.connect create it. + try: + st = db_path.lstat() + except FileNotFoundError: + st = None + if st is not None: + if stat.S_ISLNK(st.st_mode) or not stat.S_ISREG(st.st_mode): + raise OSError(f"refusing to use non-regular cache DB path: {db_path}") + if sys.platform != "win32" and st.st_uid != os.getuid(): + raise OSError(f"refusing to use cache DB not owned by current uid: {db_path}") + # Tighten perms BEFORE sqlite3.connect — a pre-existing 0o644 DB + # would otherwise be read/written with loose perms in the window + # before our chmod. For a just-pre-created file this is a no-op + # (os.open already set 0o600). On platforms where we skipped the + # pre-create (no O_NOFOLLOW) and the DB doesn't exist yet, this + # chmod will FileNotFoundError — swallowed — and sqlite3.connect + # below creates the file with umask defaults; the post-connect + # chmod catches that case. chmod() semantics vary on Windows, + # hence the broad except. + try: + db_path.chmod(0o600) + except OSError: + pass self._conn = sqlite3.connect(str(db_path)) + # Second chmod so a freshly-created-by-sqlite DB (the no-O_NOFOLLOW + # first-run case) lands on 0o600 rather than whatever the umask + # produced. No-op when the pre-connect chmod already succeeded. + try: + db_path.chmod(0o600) + except OSError: + pass self._conn.execute( """CREATE TABLE IF NOT EXISTS cache ( key TEXT PRIMARY KEY, diff --git a/tests/test_cache.py b/tests/test_cache.py index 9e1d1ed..b390e64 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -1,5 +1,8 @@ """Tests for the data cache.""" +import os +import stat +import sys import time from collections.abc import Iterator from pathlib import Path @@ -46,3 +49,90 @@ def test_stores_complex_types(self, cache: DataCache): data = [{"id": 1, "name": "test"}, {"id": 2, "values": [1, 2, 3]}] cache.set("list", data) assert cache.get("list") == data + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits only") + def test_db_file_has_restrictive_permissions(self, tmp_path: Path): + """The cache DB must be 0o600 so other local users can't read + cached balances / transactions (regression for PR #5 review).""" + db_path = tmp_path / "perms.db" + c = DataCache(db_path=db_path) + try: + mode = stat.S_IMODE(db_path.stat().st_mode) + assert mode == 0o600, f"expected 0o600, got {oct(mode)}" + finally: + c.close() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits only") + def test_db_file_created_restrictive_even_with_loose_umask(self, tmp_path: Path): + """Simulate a loose umask (0o000 → world-writable default) and + confirm the pre-create step still lands on 0o600 from the start.""" + old_umask = os.umask(0) + try: + db_path = tmp_path / "umask.db" + c = DataCache(db_path=db_path) + try: + mode = stat.S_IMODE(db_path.stat().st_mode) + assert mode == 0o600 + finally: + c.close() + finally: + os.umask(old_umask) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") + def test_db_path_refuses_symlink(self, tmp_path: Path): + """A symlink planted at db_path must be rejected outright — + otherwise sqlite3.connect() would follow it and we'd chmod a + file outside the cache dir.""" + target = tmp_path / "elsewhere" / "victim.db" + target.parent.mkdir() + target.write_bytes(b"") + db_path = tmp_path / "cache.db" + db_path.symlink_to(target) + + # Match OSError generally (not the specific message): O_NOFOLLOW and + # O_EXCL can race on different platforms — macOS raises EEXIST + # (caught, then the lstat gate raises "non-regular"), Linux can + # raise ELOOP from os.open directly. Both are correct rejections. + with pytest.raises(OSError): + DataCache(db_path=db_path) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") + def test_db_path_revalidates_after_file_exists( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ): + """Simulate the TOCTOU race: os.open raises FileExistsError (a file + was there), but by the time we sqlite3.connect the path could be + a symlink. The post-create lstat re-check must catch that.""" + target = tmp_path / "elsewhere" / "victim.db" + target.parent.mkdir() + target.write_bytes(b"") + db_path = tmp_path / "cache.db" + + real_open = os.open + + def racy_open(*_args, **_kwargs): + # Swap in a symlink between pre-create and the re-validation. + # Real attack would be an inter-process race; this monkeypatch + # reproduces the same end state deterministically. + db_path.symlink_to(target) + raise FileExistsError + + monkeypatch.setattr("src.data.cache.os.open", racy_open) + with pytest.raises(OSError, match="non-regular"): + DataCache(db_path=db_path) + # Restore so pytest's teardown doesn't trip. + monkeypatch.setattr("src.data.cache.os.open", real_open) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX uid") + def test_db_path_refuses_foreign_owned(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + """If a regular file at db_path is owned by another uid, refuse to + use it — another uid with dir-write access could pre-create a DB + owned by them, and sqlite3.connect() would dump cached data into + their file (chmod would silently no-op for a non-owned file).""" + db_path = tmp_path / "cache.db" + db_path.write_bytes(b"") # regular file, owned by us + real_uid = os.getuid() + monkeypatch.setattr("src.data.cache.os.getuid", lambda: real_uid + 1) + + with pytest.raises(OSError, match="not owned by current uid"): + DataCache(db_path=db_path) diff --git a/tests/test_session_manager.py b/tests/test_session_manager.py index 39804eb..10a62f9 100644 --- a/tests/test_session_manager.py +++ b/tests/test_session_manager.py @@ -1,5 +1,6 @@ """Tests for session manager (auth).""" +import sys from pathlib import Path from typing import Any, cast from unittest.mock import AsyncMock, MagicMock, patch @@ -95,6 +96,106 @@ async def test_restore_invalid_session(self, mock_keyring, tmp_session, tmp_path assert sm.is_authenticated is False assert not session_file.exists() + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits only") + @patch("src.auth.session_manager.keyring") + async def test_restore_refuses_world_writable_pickle(self, mock_keyring, tmp_session, tmp_path): + """A session file with group/world-write bits must NOT be unpickled + (defense against cross-user tampering; regression for PR #5).""" + session_file = tmp_path / "session.pickle" + session_file.write_bytes(b"fake") + session_file.chmod(0o666) + + sm = SessionManager() + load_session = MagicMock() + cast(Any, sm._mm).load_session = load_session + + assert await sm.try_restore_session() is False + assert sm.is_authenticated is False + # Unsafe file is deleted so we don't keep tripping the check. + assert not session_file.exists() + # Crucially, load_session must never have been called on the + # attacker-writable blob. + load_session.assert_not_called() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits only") + @patch("src.auth.session_manager.keyring") + async def test_restore_refuses_foreign_owned_pickle( + self, mock_keyring, tmp_session, tmp_path, monkeypatch + ): + """A session file owned by a different uid must NOT be unpickled.""" + session_file = tmp_path / "session.pickle" + session_file.write_bytes(b"fake") + session_file.chmod(0o600) + + import os as _os + + real_uid = _os.getuid() + monkeypatch.setattr("src.auth.session_manager.os.getuid", lambda: real_uid + 1) + + sm = SessionManager() + load_session = MagicMock() + cast(Any, sm._mm).load_session = load_session + + assert await sm.try_restore_session() is False + load_session.assert_not_called() + assert not session_file.exists() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") + @patch("src.auth.session_manager.keyring") + async def test_restore_refuses_symlink_pickle(self, mock_keyring, tmp_session, tmp_path): + """Even a symlink owned by us must not be unpickled — it could + point at attacker-controlled content.""" + real_target = tmp_path / "real.pickle" + real_target.write_bytes(b"fake") + session_file = tmp_path / "session.pickle" + session_file.symlink_to(real_target) + + sm = SessionManager() + load_session = MagicMock() + cast(Any, sm._mm).load_session = load_session + + assert await sm.try_restore_session() is False + load_session.assert_not_called() + + @patch("src.auth.session_manager.keyring") + async def test_restore_refuses_directory_at_session_path( + self, mock_keyring, tmp_session, tmp_path + ): + """If the path is a directory (not a regular file), the safety + gate must fail-closed without calling load_session — unlink() + can't remove a directory, so we must never reach it.""" + session_file = tmp_path / "session.pickle" + session_file.mkdir(mode=0o700) + + sm = SessionManager() + load_session = MagicMock() + cast(Any, sm._mm).load_session = load_session + + assert await sm.try_restore_session() is False + load_session.assert_not_called() + # Directory should still exist — unlink() can't delete it, but the + # OSError is swallowed so we don't crash the caller. + assert session_file.is_dir() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") + @patch("src.auth.session_manager.keyring") + async def test_restore_removes_dangling_symlink(self, mock_keyring, tmp_session, tmp_path): + """A dangling symlink at SESSION_FILE must be removed — not left + in place for a subsequent save_session() to follow. Path.exists() + returns False for dangling symlinks, so the safety gate has to + run unconditionally.""" + session_file = tmp_path / "session.pickle" + session_file.symlink_to(tmp_path / "does-not-exist") + + sm = SessionManager() + load_session = MagicMock() + cast(Any, sm._mm).load_session = load_session + + assert await sm.try_restore_session() is False + load_session.assert_not_called() + assert not session_file.is_symlink() + assert not session_file.exists() + class TestLogin: @patch("src.auth.session_manager.keyring") @@ -131,3 +232,97 @@ def test_logout(self, mock_keyring, tmp_session, tmp_path): assert sm.is_authenticated is False assert not session_file.exists() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") + @patch("src.auth.session_manager.keyring") + async def test_login_clears_planted_symlink_before_save( + self, mock_keyring, tmp_session, tmp_path + ): + """A symlink planted at SESSION_FILE (e.g. between logout and next + login) must be removed before save_session — otherwise the pickle + write would follow the symlink out of the intended directory.""" + target = tmp_path / "elsewhere.pickle" + session_file = tmp_path / "session.pickle" + session_file.symlink_to(target) + + sm = SessionManager() + mm = cast(Any, sm._mm) + mm.login = AsyncMock() + mm.save_session = MagicMock() + + await sm.login("user@test.com", "pass") + + assert not session_file.is_symlink() + assert not target.exists() + mm.save_session.assert_called_once() + + @patch("src.auth.session_manager.keyring") + def test_logout_survives_directory_at_session_path(self, mock_keyring, tmp_session, tmp_path): + """If a directory is sitting at SESSION_FILE, unlink() raises + IsADirectoryError. logout() must survive that — catching OSError, + not just FileNotFoundError.""" + session_file = tmp_path / "session.pickle" + session_file.mkdir() + + sm = SessionManager() + sm.logout() # must not raise + assert session_file.is_dir() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX uid") + @patch("src.auth.session_manager.keyring") + async def test_login_unlinks_foreign_owned_regular_file( + self, mock_keyring, tmp_session, tmp_path, monkeypatch + ): + """An attacker-owned regular file at SESSION_FILE must be unlinked + before save_session, so the next pickle write isn't captured by a + file owned by another uid (who could pre-create it in a + loosely-permissioned parent dir).""" + import os as _os + + session_file = tmp_path / "session.pickle" + session_file.write_bytes(b"attacker content") + session_file.chmod(0o600) + real_uid = _os.getuid() + monkeypatch.setattr("src.auth.session_manager.os.getuid", lambda: real_uid + 1) + + sm = SessionManager() + mm = cast(Any, sm._mm) + mm.login = AsyncMock() + + def fake_save(path: str) -> None: + with open(path, "wb") as f: + f.write(b"our session") + + mm.save_session = MagicMock(side_effect=fake_save) + + await sm.login("user@test.com", "pass") + + assert session_file.read_bytes() == b"our session" + mm.save_session.assert_called_once() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits") + @patch("src.auth.session_manager.keyring") + async def test_login_unlinks_world_writable_regular_file( + self, mock_keyring, tmp_session, tmp_path + ): + """Same class: a world-writable regular file is suspect even if + we own it (any other process could write into it) — must be + unlinked before save, so save_session produces a fresh 0o600 + file.""" + session_file = tmp_path / "session.pickle" + session_file.write_bytes(b"suspect") + session_file.chmod(0o666) + + sm = SessionManager() + mm = cast(Any, sm._mm) + mm.login = AsyncMock() + + def fake_save(path: str) -> None: + with open(path, "wb") as f: + f.write(b"fresh") + + mm.save_session = MagicMock(side_effect=fake_save) + + await sm.login("user@test.com", "pass") + + assert session_file.read_bytes() == b"fresh"