From 8698b89f6a38f65e8a5c1ea296138897401cb12c Mon Sep 17 00:00:00 2001 From: Lukas Date: Sun, 26 Apr 2026 13:04:14 +0200 Subject: [PATCH] TASK-010: Replace .split() with shlex.split() for fail2ban_start_command - Add @field_validator for fail2ban_start_command to validate with shlex.split() at startup, catching misconfigured commands with mismatched quotes - Replace .split() with shlex.split() in jail_config.py line 450 - Replace .split() with shlex.split() in config_misc.py line 154 - Update Backend-Development.md with configuration documentation explaining quoted path handling and common pitfalls - Add comprehensive test suite (8 tests) covering valid commands, quoted paths, and mismatched quote errors This fix ensures commands like '/opt/my tools/fail2ban-client' start are correctly parsed as two tokens instead of three, preventing execution failures when the path contains spaces. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Backend-Development.md | 22 ++++++ Docs/Tasks.md | 36 ---------- backend/app/config.py | 27 ++++++++ backend/app/routers/config_misc.py | 3 +- backend/app/routers/jail_config.py | 3 +- backend/tests/test_config.py | 107 +++++++++++++++++++++++++++++ 6 files changed, 160 insertions(+), 38 deletions(-) create mode 100644 backend/tests/test_config.py diff --git a/Docs/Backend-Development.md b/Docs/Backend-Development.md index 21870bf..18430de 100644 --- a/Docs/Backend-Development.md +++ b/Docs/Backend-Development.md @@ -522,6 +522,28 @@ environment: **Important:** If `Secure=true` is set, browsers will reject the session cookie when the backend is served over HTTP. Ensure your nginx/reverse proxy terminates TLS and passes `X-Forwarded-Proto: https` so FastAPI knows the connection is secure. +### fail2ban_start_command Configuration + +The `fail2ban_start_command` setting specifies the shell command used to start the fail2ban daemon during recovery operations (e.g., after a rollback). + +**Format & Parsing:** +- The command is split into arguments using `shlex.split()`, which respects shell quoting rules. +- Paths with spaces must be quoted. Example: `"/opt/my tools/fail2ban-client" start`. +- The command is **not** executed through a shell — no shell variables or globbing are interpreted. + +**Validation:** +- The command is validated at startup using `shlex.split()`. Mismatched quotes will raise a `ValueError` with the problematic command in the error message. + +**Environment Variables:** +```bash +BANGUI_FAIL2BAN_START_COMMAND="fail2ban-client start" # Default +BANGUI_FAIL2BAN_START_COMMAND="systemctl start fail2ban" # systemd +BANGUI_FAIL2BAN_START_COMMAND='"/opt/my tools/fail2ban" start' # Quoted path +``` + +**Common Pitfall:** +Using `.split()` instead of `shlex.split()` would break commands with spaces in paths. Always use quoted strings for paths that contain whitespace. + ### Login Rate Limiting The login endpoint (`POST /api/auth/login`) is protected against brute-force attacks using an in-memory rate limiter. diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 39059a1..aac0597 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,39 +1,3 @@ -## TASK-009 — Blocklist URL has no scheme/host validation — SSRF risk - -**Severity:** High - -### Where found -`backend/app/models/blocklist.py` — `BlocklistSourceCreate.url: str = Field(..., min_length=1)`. `backend/app/services/blocklist_service.py` — `preview_source()` and `_download_text_with_retries()`. - -### Why this is needed -An authenticated user can supply `file:///etc/passwd`, `http://169.254.169.254/latest/meta-data/` (AWS metadata service), `http://10.0.0.1/admin`, or `http://localhost:8000/api/setup` as a blocklist URL. The backend fetches it and either returns its contents in the preview response or attempts to parse it as an IP list. This is a Server-Side Request Forgery (SSRF) vulnerability. - -### Goal -Restrict blocklist URLs to safe, public HTTP/HTTPS endpoints only. - -### What to do -1. Change `url: str` to `url: AnyHttpUrl` in `BlocklistSourceCreate` — this rejects `file://`, `ftp://`, and other non-http schemes. -2. Add a `@field_validator("url")` that: - - Parses the hostname. - - Resolves it via `socket.getaddrinfo()` (or uses `ipaddress.ip_address()` if it is a bare IP). - - Rejects RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), loopback (`127.0.0.0/8`), link-local (`169.254.0.0/16`), and IPv6 equivalents. -3. Add a validator for `http://` URLs — consider requiring `https://` only, or adding a configuration flag. - -### Possible traps and issues -- DNS rebinding: the hostname resolves to a public IP at validation time but to a private IP at fetch time. Mitigate by re-validating the final connection IP in aiohttp (custom `TCPConnector` or response callback). -- `AnyHttpUrl` allows ports — ensure `http://evil.com:22/` is also blocked or at least safe. -- `socket.getaddrinfo()` is blocking — use `asyncio.get_event_loop().getaddrinfo()` or run in executor. - -### Docs changes needed -- `Features.md` — document URL validation constraints for blocklist sources. -- `Backend-Development.md` — SSRF prevention pattern. - -### Doc references -- [Features.md](Features.md) — blocklist management -- [Backend-Development.md](Backend-Development.md) — input validation - ---- - ## TASK-010 — `fail2ban_start_command` split with `.split()` instead of `shlex.split()` **Severity:** Low diff --git a/backend/app/config.py b/backend/app/config.py index 7dc1b37..dbdd917 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -4,6 +4,7 @@ Follows pydantic-settings patterns: all values are prefixed with BANGUI_ and validated at startup via the Settings singleton. """ +import shlex from typing import Literal from pydantic import Field, field_validator @@ -151,6 +152,32 @@ class Settings(BaseSettings): ), ) + @field_validator("fail2ban_start_command", mode="after") + @classmethod + def _validate_fail2ban_start_command(cls, value: str) -> str: + """Validate fail2ban_start_command by attempting to parse it with shlex. + + Ensures the command can be split into arguments without shell interpretation. + Raises ValueError if the command contains mismatched quotes. + + Args: + value: The fail2ban start command string. + + Returns: + The validated command string. + + Raises: + ValueError: If the command contains mismatched quotes. + """ + try: + shlex.split(value) + except ValueError as e: + raise ValueError( + f"fail2ban_start_command contains mismatched quotes or is otherwise " + f"unparseable: {value!r} — {e}" + ) from e + return value + model_config = SettingsConfigDict( env_prefix="BANGUI_", env_file=".env", diff --git a/backend/app/routers/config_misc.py b/backend/app/routers/config_misc.py index db9c107..2c9dbd7 100644 --- a/backend/app/routers/config_misc.py +++ b/backend/app/routers/config_misc.py @@ -1,5 +1,6 @@ from __future__ import annotations +import shlex from typing import Annotated import structlog @@ -149,7 +150,7 @@ async def restart_fail2ban( ``POST /api/config/jails/{name}/rollback`` if a specific jail is suspect. """ - start_cmd_parts: list[str] = start_cmd.split() + start_cmd_parts: list[str] = shlex.split(start_cmd) restarted = await jail_service.restart_daemon( socket_path, diff --git a/backend/app/routers/jail_config.py b/backend/app/routers/jail_config.py index b32b245..817ce8c 100644 --- a/backend/app/routers/jail_config.py +++ b/backend/app/routers/jail_config.py @@ -1,5 +1,6 @@ from __future__ import annotations +import shlex from typing import Annotated from fastapi import APIRouter, Path, Query, Request, status @@ -445,7 +446,7 @@ async def rollback_jail( HTTPException: 400 if *name* contains invalid characters. HTTPException: 500 if writing the .local override file fails. """ - start_cmd_parts: list[str] = start_cmd.split() + start_cmd_parts: list[str] = shlex.split(start_cmd) result = await jail_config_service.rollback_jail(config_dir, socket_path, name, start_cmd_parts) diff --git a/backend/tests/test_config.py b/backend/tests/test_config.py new file mode 100644 index 0000000..5391d30 --- /dev/null +++ b/backend/tests/test_config.py @@ -0,0 +1,107 @@ +"""Unit tests for application configuration and validation.""" + +import pytest +from pydantic import ValidationError + +from app.config import Settings + + +def test_fail2ban_start_command_validates_simple_command() -> None: + """Simple fail2ban commands without special characters are accepted.""" + settings = Settings( + database_path="/tmp/test.db", + fail2ban_socket="/tmp/fake_fail2ban.sock", + fail2ban_config_dir="/tmp/fail2ban", + session_secret="test-secret-key-do-not-use-in-production", + fail2ban_start_command="fail2ban-client start", + ) + assert settings.fail2ban_start_command == "fail2ban-client start" + + +def test_fail2ban_start_command_validates_systemctl_command() -> None: + """systemctl commands are accepted.""" + settings = Settings( + database_path="/tmp/test.db", + fail2ban_socket="/tmp/fake_fail2ban.sock", + fail2ban_config_dir="/tmp/fail2ban", + session_secret="test-secret-key-do-not-use-in-production", + fail2ban_start_command="systemctl start fail2ban", + ) + assert settings.fail2ban_start_command == "systemctl start fail2ban" + + +def test_fail2ban_start_command_accepts_quoted_paths() -> None: + """Commands with quoted paths containing spaces are accepted.""" + settings = Settings( + database_path="/tmp/test.db", + fail2ban_socket="/tmp/fake_fail2ban.sock", + fail2ban_config_dir="/tmp/fail2ban", + session_secret="test-secret-key-do-not-use-in-production", + fail2ban_start_command='"/opt/my tools/fail2ban-client" start', + ) + assert settings.fail2ban_start_command == '"/opt/my tools/fail2ban-client" start' + + +def test_fail2ban_start_command_rejects_mismatched_quotes() -> None: + """Commands with mismatched quotes raise ValidationError.""" + with pytest.raises(ValidationError) as exc_info: + Settings( + database_path="/tmp/test.db", + fail2ban_socket="/tmp/fake_fail2ban.sock", + fail2ban_config_dir="/tmp/fail2ban", + session_secret="test-secret-key-do-not-use-in-production", + fail2ban_start_command='"/opt/my tools/fail2ban-client start', + ) + error_msg = str(exc_info.value) + assert "fail2ban_start_command" in error_msg + assert "mismatched quotes" in error_msg or "No closing quotation" in error_msg + + +def test_fail2ban_start_command_error_includes_problematic_value() -> None: + """Validation errors include the problematic command value.""" + problematic_command = '"/opt/broken start' + with pytest.raises(ValidationError) as exc_info: + Settings( + database_path="/tmp/test.db", + fail2ban_socket="/tmp/fake_fail2ban.sock", + fail2ban_config_dir="/tmp/fail2ban", + session_secret="test-secret-key-do-not-use-in-production", + fail2ban_start_command=problematic_command, + ) + error_msg = str(exc_info.value) + assert problematic_command in error_msg + + +def test_fail2ban_start_command_default_value_is_valid() -> None: + """The default fail2ban_start_command value is valid and parseable.""" + settings = Settings( + database_path="/tmp/test.db", + fail2ban_socket="/tmp/fake_fail2ban.sock", + fail2ban_config_dir="/tmp/fail2ban", + session_secret="test-secret-key-do-not-use-in-production", + ) + assert settings.fail2ban_start_command == "fail2ban-client start" + + +def test_fail2ban_start_command_single_quoted() -> None: + """Commands with single quotes are accepted.""" + settings = Settings( + database_path="/tmp/test.db", + fail2ban_socket="/tmp/fake_fail2ban.sock", + fail2ban_config_dir="/tmp/fail2ban", + session_secret="test-secret-key-do-not-use-in-production", + fail2ban_start_command="'/usr/bin/fail2ban-client' start", + ) + assert settings.fail2ban_start_command == "'/usr/bin/fail2ban-client' start" + + +def test_fail2ban_start_command_multiple_arguments() -> None: + """Commands with multiple arguments are accepted.""" + settings = Settings( + database_path="/tmp/test.db", + fail2ban_socket="/tmp/fake_fail2ban.sock", + fail2ban_config_dir="/tmp/fail2ban", + session_secret="test-secret-key-do-not-use-in-production", + fail2ban_start_command="fail2ban-client -c /etc/fail2ban start", + ) + assert settings.fail2ban_start_command == "fail2ban-client -c /etc/fail2ban start"