security: fix #4 — chmod SQLite DB + WAL/SHM sidecars to 0o600
The sites table stores api_key + api_user in plaintext. Previous behavior left the DB file at the inherited umask (0o644 on most Linux systems) so any other local user could sqlite3 it open and exfiltrate every booru API key. Adds Database._restrict_perms(), called from the lazy conn init right after _migrate(). Tightens the main file plus the -wal and -shm sidecars to 0o600. The sidecars only exist after the first write, so the FileNotFoundError path is expected and silenced. Filesystem chmod failures are also swallowed for FUSE-mount compatibility. behavior change from v0.2.5: ~/.local/share/booru-viewer/booru.db is now 0o600 even if a previous version created it 0o644. Audit-Ref: SECURITY_AUDIT.md finding #4 Severity: Medium
This commit is contained in:
parent
6801a0b45e
commit
a6a73fed61
@ -11,7 +11,7 @@ from dataclasses import dataclass, field
|
||||
from datetime import datetime, timezone
|
||||
from pathlib import Path
|
||||
|
||||
from .config import db_path
|
||||
from .config import IS_WINDOWS, db_path
|
||||
|
||||
|
||||
def _validate_folder_name(name: str) -> str:
|
||||
@ -210,8 +210,30 @@ class Database:
|
||||
self._conn.execute("PRAGMA foreign_keys=ON")
|
||||
self._conn.executescript(_SCHEMA)
|
||||
self._migrate()
|
||||
self._restrict_perms()
|
||||
return self._conn
|
||||
|
||||
def _restrict_perms(self) -> None:
|
||||
"""Tighten the DB file (and WAL/SHM sidecars) to 0o600 on POSIX.
|
||||
|
||||
The sites table stores api_key + api_user in plaintext, so the
|
||||
file must not be readable by other local users. Sidecars only
|
||||
exist after the first WAL checkpoint, so we tolerate
|
||||
FileNotFoundError. Windows: NTFS ACLs handle this; chmod is a
|
||||
no-op there. Filesystem-level chmod failures are swallowed —
|
||||
better to keep working than refuse to start.
|
||||
"""
|
||||
if IS_WINDOWS:
|
||||
return
|
||||
for suffix in ("", "-wal", "-shm"):
|
||||
target = Path(str(self._path) + suffix) if suffix else self._path
|
||||
try:
|
||||
os.chmod(target, 0o600)
|
||||
except FileNotFoundError:
|
||||
pass
|
||||
except OSError:
|
||||
pass
|
||||
|
||||
@contextmanager
|
||||
def _write(self):
|
||||
"""Context manager for write methods.
|
||||
|
||||
@ -13,6 +13,9 @@ These tests lock in the `54ccc40` security/correctness fixes:
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
|
||||
from booru_viewer.core.db import _validate_folder_name
|
||||
@ -42,6 +45,34 @@ def test_validate_folder_name_rejects_traversal():
|
||||
_validate_folder_name("~user") # leading tilde
|
||||
|
||||
|
||||
@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only chmod check")
|
||||
def test_db_file_chmod_600(tmp_db):
|
||||
"""Audit finding #4: the SQLite file must be 0o600 on POSIX so the
|
||||
plaintext api_key/api_user columns aren't readable by other local
|
||||
users on shared workstations."""
|
||||
# The conn property triggers _restrict_perms() the first time it's
|
||||
# accessed; tmp_db calls it via add_site/etc., but a defensive
|
||||
# access here makes the assertion order-independent.
|
||||
_ = tmp_db.conn
|
||||
mode = os.stat(tmp_db._path).st_mode & 0o777
|
||||
assert mode == 0o600, f"expected 0o600, got {oct(mode)}"
|
||||
|
||||
|
||||
@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only chmod check")
|
||||
def test_db_wal_sidecar_chmod_600(tmp_db):
|
||||
"""The -wal sidecar created by PRAGMA journal_mode=WAL must also
|
||||
be 0o600. It carries in-flight transactions including the most
|
||||
recent api_key writes — same exposure as the main DB file."""
|
||||
# Force a write so the WAL file actually exists.
|
||||
tmp_db.add_site("test", "http://example.test", "danbooru")
|
||||
# Re-trigger the chmod pass now that the sidecar exists.
|
||||
tmp_db._restrict_perms()
|
||||
wal = type(tmp_db._path)(str(tmp_db._path) + "-wal")
|
||||
if wal.exists():
|
||||
mode = os.stat(wal).st_mode & 0o777
|
||||
assert mode == 0o600, f"expected 0o600 on WAL sidecar, got {oct(mode)}"
|
||||
|
||||
|
||||
def test_validate_folder_name_accepts_unicode_and_punctuation():
|
||||
"""Common real-world folder names must pass through unchanged. The
|
||||
guard is meant to block escape shapes, not normal naming."""
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user