## TASK-013 — nginx missing security response headers **Severity:** High ### Where found `Docker/nginx.conf` — the server block has no `Content-Security-Policy`, `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy`, or `Strict-Transport-Security` headers. ### Why this is needed Without these headers: - **No CSP** — injected scripts can run freely (XSS). - **No X-Frame-Options** — the app can be embedded in an iframe on an attacker-controlled page (clickjacking). - **No X-Content-Type-Options** — browsers may MIME-sniff responses and execute text/plain as JavaScript. - **No Referrer-Policy** — internal URLs are leaked in the `Referer` header to third-party resources. - **No HSTS** — even with HTTPS configured, browsers will still attempt HTTP first unless told otherwise. ### Goal Add all OWASP-recommended security headers to the nginx server block. ### What to do Add to the `server` block in `nginx.conf`: ```nginx add_header Content-Security-Policy "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; connect-src 'self'; frame-ancestors 'none';" always; add_header X-Frame-Options "DENY" always; add_header X-Content-Type-Options "nosniff" always; add_header Referrer-Policy "no-referrer" always; add_header Permissions-Policy "geolocation=(), microphone=(), camera=()" always; # Only add HSTS when HTTPS is confirmed: # add_header Strict-Transport-Security "max-age=63072000; includeSubDomains; preload" always; ``` ### Possible traps and issues - Fluent UI v9 uses inline `style` attributes — `style-src 'self' 'unsafe-inline'` is required for now. A stricter CSP using nonces would require server-side rendering of the HTML shell. - HSTS must only be added when HTTPS is fully configured and working — it is irreversible for the configured `max-age`. - Use `always` on `add_header` so headers are also included in error responses (4xx, 5xx). ### Docs changes needed - `Architekture.md` — document the nginx security header configuration. ### Doc references - [Architekture.md](Architekture.md) — nginx configuration --- ## TASK-014 — `add_log_path` passes arbitrary paths to fail2ban — no allowlist **Severity:** High ### Where found `backend/app/services/config_service.py` — `add_log_path()`. `backend/app/models/config.py` — `AddLogPathRequest.log_path: str`. ### Why this is needed An authenticated user can instruct fail2ban to monitor any file path on the system (e.g., `log_path: "/etc/shadow"`). fail2ban runs as root and opens the file for reading. Even if fail2ban cannot meaningfully parse it, repeated log monitoring of sensitive files can leak their contents via fail2ban's own logging, and the feature represents an unintended read primitive into arbitrary root-readable files. ### Goal Restrict monitored log paths to a configurable set of safe directories. ### What to do 1. Add a `@field_validator("log_path")` to `AddLogPathRequest` that: - Calls `Path(log_path).resolve()` to canonicalize. - Checks `resolved.is_relative_to(Path("/var/log"))` or any path in `settings.allowed_log_dirs` (a new configurable list). - Raises `ValueError` if the path is outside allowed prefixes. 2. Add `BANGUI_ALLOWED_LOG_DIRS` to `Settings` as `list[str]` defaulting to `["/var/log", "/config/log"]`. 3. Note: use `is_relative_to()`, not `startswith()` — the latter is bypassable with `/var/log_evil`. ### Possible traps and issues - The validator runs on the Pydantic model before the service is called — the resolved path check happens at request time, not at the OS level. The allowed list must match the actual Docker volume mount paths. - Custom log file locations (e.g., `/home/app/logs`) need to be added to `BANGUI_ALLOWED_LOG_DIRS`. ### Docs changes needed - `Features.md` — document the log path restrictions. - `Backend-Development.md` — input validation for path parameters. ### Doc references - [Features.md](Features.md) — jail log monitoring - [Backend-Development.md](Backend-Development.md) — path validation --- ## TASK-015 — `GlobalConfigUpdate.log_target`/`log_level` have no validation **Severity:** High ### Where found `backend/app/models/config.py` — `GlobalConfigUpdate`. `backend/app/services/config_service.py` — `update_global_config()`. ### Why this is needed `log_target` is forwarded raw to fail2ban via the Unix socket. fail2ban (running as root) creates or opens the file at that path if it does not exist. `log_level` is forwarded raw without checking it is a valid fail2ban log level. Both fields represent an injection path into fail2ban's internal state from an authenticated but potentially compromised account. ### Goal Validate both fields before forwarding to fail2ban. ### What to do 1. Change `log_target` in `GlobalConfigUpdate` to accept only: - `Literal["STDOUT", "STDERR", "SYSLOG"]`, or - A path validated the same way as `AddLogPathRequest.log_path` (see TASK-014). 2. Change `log_level` to `Literal["CRITICAL", "ERROR", "WARNING", "NOTICE", "INFO", "DEBUG"]`. 3. Apply the same restrictions in `get_global_config` responses for consistency. ### Possible traps and issues - The allowlist for `log_target` paths must be consistent with TASK-014 (`BANGUI_ALLOWED_LOG_DIRS`). - Existing deployments using non-standard `log_target` values (e.g., `/var/log/fail2ban.log`) must still work — ensure `/var/log` is in the default allowlist. ### Docs changes needed - `Features.md` — document valid values for `log_target` and `log_level`. - `Backend-Development.md` — Pydantic Literal types for constrained string fields. ### Doc references - [Features.md](Features.md) — global fail2ban configuration - [Backend-Development.md](Backend-Development.md) — model validation --- ## TASK-016 — `delete_log_path` query parameter unvalidated **Severity:** Medium ### Where found `backend/app/routers/jail_config.py` — `DELETE /api/config/jails/{name}/logpath` — `log_path: str = Query(...)`. ### Why this is needed The `log_path` query parameter is passed directly to the fail2ban socket command `["set", name, "dellogpath", log_path]` without any path validation. An attacker could pass traversal strings or paths to sensitive files, instructing fail2ban to stop monitoring them and potentially confusing fail2ban's internal state. ### Goal Apply the same allowlist validation as `add_log_path` (TASK-014) to `delete_log_path`. ### What to do 1. Extract the log path validation logic from TASK-014 into a shared helper function in `backend/app/utils/path_utils.py` (e.g., `validate_log_path(path: str, allowed_dirs: list[str]) -> str`). 2. Call the helper from both `AddLogPathRequest` validator and the `delete_log_path` route handler. 3. Return 422 with a descriptive error if validation fails. ### Possible traps and issues - Query parameters cannot have Pydantic field validators directly in FastAPI — use a `Depends` dependency that validates and returns the resolved path, or validate explicitly at the start of the route handler. ### Docs changes needed - `Backend-Development.md` — path validation helper usage. ### Doc references - [Backend-Development.md](Backend-Development.md) — input validation patterns --- ## TASK-017 — `ip LIKE ?` without escaping `%` and `_` wildcards **Severity:** Medium ### Where found `backend/app/repositories/fail2ban_db_repo.py` and `backend/app/repositories/history_archive_repo.py` — SQL queries using `ip LIKE ?` with `f"{ip_filter}%"` interpolation. ### Why this is needed SQLite's `LIKE` operator treats `%` (any sequence of characters) and `_` (any single character) as wildcards. If an IP filter value contains these characters — unusual for well-formed IPs, but possible via crafted input — the query matches unintended rows. For example, `ip_filter = "10.0.0_"` would match `10.0.0.1` through `10.0.0.9`. ### Goal Escape LIKE metacharacters in all `LIKE` query parameters. ### What to do 1. Escape `%` → `\%` and `_` → `\_` in the filter string before use. 2. Add `ESCAPE '\'` to the SQL: `ip LIKE ? ESCAPE '\'`. 3. Extract this into a helper: `def escape_like(s: str) -> str: return s.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")`. ### Possible traps and issues - The backslash escape character itself must also be escaped first to avoid double-escaping. Process in the order: `\` → `\\`, then `%` → `\%`, then `_` → `\_`. - Test with IPs that contain dots — dots are not LIKE wildcards in SQLite, so they do not need escaping. ### Docs changes needed - `Backend-Development.md` — database query conventions (LIKE escaping). ### Doc references - [Backend-Development.md](Backend-Development.md) — database access patterns --- ## TASK-018 — `_write_conf_file` and `_create_conf_file` not atomic **Severity:** Medium ### Where found `backend/app/services/config_file_helpers.py` lines ~190 (`_write_conf_file`) and ~226 (`_create_conf_file`) — both use `target.write_text(content, encoding="utf-8")` directly. ### Why this is needed `Path.write_text()` overwrites the file in place. If the process is killed mid-write, the config file is left in a truncated or corrupt state. fail2ban config files are critical — a corrupt `jail.d/sshd.conf` prevents fail2ban from reloading and may disable active protection. ### Goal Make all config file writes atomic using write-to-temp + rename. ### What to do 1. Replace `target.write_text(content)` with: ```python import tempfile, os tmp_fd, tmp_path = tempfile.mkstemp(dir=target.parent, suffix=".tmp") try: with os.fdopen(tmp_fd, "w", encoding="utf-8") as f: f.write(content) os.replace(tmp_path, target) except Exception: os.unlink(tmp_path) raise ``` 2. Apply to both `_write_conf_file` and `_create_conf_file`. 3. This pattern is already used correctly in `jail_config_service.py` — follow that exact implementation. ### Possible traps and issues - The temp file must be in the same directory as the target (`dir=target.parent`) so `os.replace()` is atomic (same filesystem, single rename syscall). - On Windows, `os.replace()` may fail if the target is open — not relevant for Linux containers, but worth noting. ### Docs changes needed - `Backend-Development.md` — atomic file write conventions. ### Doc references - [Backend-Development.md](Backend-Development.md) — file I/O patterns --- ## TASK-019 — `session_secret` has no minimum-length enforcement **Severity:** Medium ### Where found `backend/app/config.py` — `session_secret: str = Field(..., description="...")`. No `min_length` constraint. ### Why this is needed `session_secret` is the HMAC key used to sign all session tokens. A secret shorter than 32 characters (256 bits) significantly weakens HMAC-SHA256. The app currently accepts any non-empty string, including a single character. ### Goal Enforce a minimum secret length of 32 characters at startup. ### What to do 1. Add `min_length=32` to the `session_secret` Field definition. 2. Update the error message to explain: `"session_secret must be at least 32 characters. Generate one with: python -c \"import secrets; print(secrets.token_hex(32))\""`. ### Possible traps and issues - This is a breaking change for any existing deployment where the secret is shorter than 32 characters. Include a migration note in the changelog. - The debug compose file uses `dev-secret-do-not-use-in-production` (42 chars) — this already passes the 32-char check, so dev environments are unaffected. ### Docs changes needed - `Backend-Development.md` — document the `session_secret` constraint. ### Doc references - [Backend-Development.md](Backend-Development.md) — configuration reference --- ## TASK-020 — `log_target` accepts arbitrary paths — root file write via fail2ban (CRITICAL) **Severity:** Critical ### Where found `backend/app/models/config.py` — `GlobalConfigUpdate.log_target: str | None`. `backend/app/services/config_service.py` — `update_global_config()` forwards the value to fail2ban without validation. ### Why this is needed fail2ban runs as root. When `log_target` is set to a path, fail2ban opens (and if necessary creates) that file for writing. An authenticated user can send `PUT /api/config/global` with `{"log_target": "/etc/cron.d/bangui-pwned"}`, causing fail2ban to create that file as root. With crafted content appended via fail2ban's own logging, this escalates to a root write primitive and potentially to Remote Code Execution. ### Goal Block all `log_target` values that are not `"STDOUT"`, `"STDERR"`, `"SYSLOG"`, or a path under the configured allowed log directories. ### What to do 1. **Immediate:** Add a strict `@field_validator("log_target")` to `GlobalConfigUpdate` that enforces the allowlist (see TASK-015 — this task and TASK-015 share the same fix). 2. **Defense in depth:** Before sending the command to fail2ban in `update_global_config()`, validate again at the service layer (not just the model layer). 3. Add a regression test: `POST /api/config/global` with `log_target="/etc/passwd"` must return 422. ### Possible traps and issues - This must be fixed before TASK-015 since it is the more severe variant. The fixes are identical — implement them together. - Pydantic model validators run before the service receives the value, but an integration test confirming the full request path is essential. ### Docs changes needed - `Features.md` — document valid log_target values. - `Backend-Development.md` — critical input validation requirement for config endpoints. ### Doc references - [Features.md](Features.md) — fail2ban global configuration - [Backend-Development.md](Backend-Development.md) — input validation --- ## TASK-021 — `set_jail_config_enabled` and `write_jail_config_file` not atomic **Severity:** Medium ### Where found `backend/app/services/raw_config_io_service.py` lines ~268 (`set_jail_config_enabled`) and ~344 (`write_jail_config_file`) — both use `path.write_text(updated)` directly. ### Why this is needed Same root cause as TASK-018. A process kill mid-write leaves the jail config file corrupted, disabling that jail on next fail2ban reload. ### Goal Atomic writes for `set_jail_config_enabled` and `write_jail_config_file`. ### What to do Same as TASK-018: replace `path.write_text(content)` with the `NamedTemporaryFile` + `os.replace()` pattern in both functions. This is most efficiently done as part of TASK-018 by extracting a shared `atomic_write(path, content)` helper in `config_file_helpers.py`. ### Possible traps and issues - Same as TASK-018. - Extracting the helper makes TASK-018 and TASK-021 a single coordinated change. ### Docs changes needed - `Backend-Development.md` — atomic write helper documentation. ### Doc references - [Backend-Development.md](Backend-Development.md) — file I/O conventions --- ## TASK-022 — Session tokens stored in plaintext in SQLite **Severity:** High ### Where found `backend/app/db.py` — `sessions` table schema: `token TEXT NOT NULL UNIQUE`. `backend/app/repositories/session_repo.py` — `INSERT INTO sessions (token, ...)` and `SELECT ... WHERE token = ?` both use the raw token value. ### Why this is needed If the BanGUI SQLite database file is exposed (volume mount misconfiguration, backup leak, path traversal via another vulnerability), all active session tokens are immediately usable — no cracking required. The attacker can directly use the token in the `bangui_session` cookie or `Authorization: Bearer` header. ### Goal Store a one-way hash of the session token in the database so that the DB file alone is not sufficient to hijack a session. ### What to do 1. In `session_repo.create_session()`, store `hashlib.sha256(token.encode()).hexdigest()` instead of `token` in the `token` column. 2. In `session_repo.get_session()` and `delete_session()`, hash the supplied token before the SQL lookup. 3. The `Session` model's `token` field returned to the service layer still contains the raw token (for use in signing and response) — only the DB column changes. 4. Add a migration (`_MIGRATIONS[2]`) that renames the existing `sessions` table to `sessions_old`, creates a new one, and drops `sessions_old` (or simply truncates all sessions on upgrade, since they are all compromised anyway once the DB was readable in plaintext). ### Possible traps and issues - Coordinate with TASK-025 (HMAC bypass) — both fixes invalidate all existing sessions. Do them in the same release. - The migration must be atomic (see TASK-023). - The `Session.token` field name is slightly misleading once it stores a hash — consider renaming the DB column to `token_hash`. ### Docs changes needed - `Architekture.md` — update session data model description. - `Backend-Development.md` — document the session token hashing pattern. ### Doc references - [Architekture.md](Architekture.md) — authentication and session model - [Backend-Development.md](Backend-Development.md) — security patterns --- ## TASK-023 — Database migration is non-atomic **Severity:** Medium ### Where found `backend/app/db.py` — `_apply_migration()`: calls `db.executescript(migration_script)` (which auto-commits per SQLite Python driver behavior) and then separately `db.execute("INSERT INTO schema_migrations ...")` + `db.commit()`. ### Why this is needed `executescript()` issues an implicit `COMMIT` before executing the script, so the schema change and the migration record insertion are in two separate transactions. A process crash between them leaves the database in a migrated-but-unrecorded state. On next startup, the migration is re-applied. For a migration that is not idempotent (e.g., `INSERT` without `OR IGNORE`, `ALTER TABLE ADD COLUMN` without `IF NOT EXISTS`), this causes a runtime error or data duplication. ### Goal Wrap each migration's DDL and its `schema_migrations` record in a single atomic transaction. ### What to do 1. Replace `db.executescript(migration_script)` with individual `await db.execute(stmt)` calls for each DDL statement in the migration (split on `;`). 2. Wrap the entire migration (all DDL statements + the `INSERT INTO schema_migrations`) in an explicit `BEGIN IMMEDIATE` ... `COMMIT` transaction. 3. Test: verify that a simulated crash mid-migration (mocked `execute` that raises on the second statement) leaves the DB at its prior version. ### Possible traps and issues - SQLite DDL in WAL mode: `CREATE TABLE IF NOT EXISTS` and `CREATE INDEX IF NOT EXISTS` are safe to re-run. `ALTER TABLE ADD COLUMN` is not — it must be guarded with a `PRAGMA table_info` check if used in future migrations. - Splitting a migration script on `;` must handle semicolons inside string literals and comments. Consider storing each migration as a `list[str]` of individual statements instead of a single script string. ### Docs changes needed - `Backend-Development.md` — migration authoring guidelines. ### Doc references - [Backend-Development.md](Backend-Development.md) — database schema and migrations --- ## TASK-024 — No CSRF protection on state-mutating endpoints **Severity:** High ### Where found All `POST`, `PUT`, `DELETE` routes in `backend/app/routers/`. Only `SameSite=Lax` on the session cookie provides any CSRF protection. ### Why this is needed `SameSite=Lax` blocks cross-site `