TASK-014: Add log path validation to prevent arbitrary file access
Restrict monitored log paths to a configurable allowlist of safe directories to prevent authenticated users from instructing fail2ban to monitor arbitrary files on the system, which could leak contents via fail2ban logging. Changes: - Add 'allowed_log_dirs' setting to Settings (defaults to /var/log, /config/log) - Add @field_validator to AddLogPathRequest to validate log paths at request time - Validator resolves paths to canonical form and checks against allowed prefixes - Use Path.is_relative_to() to prevent prefix bypass attacks like /var/log_evil - Add comprehensive tests for valid/invalid paths and symlink handling - Update Features.md and Backend-Development.md with security documentation Security improvements: - Blocks access to sensitive files (/etc/shadow, /etc/passwd, etc.) - Resolves symlinks before validation to prevent escape routes - Uses proper path comparison instead of string prefix matching - Configurable via BANGUI_ALLOWED_LOG_DIRS environment variable Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -553,6 +553,35 @@ BANGUI_FAIL2BAN_START_COMMAND='"/opt/my tools/fail2ban" start' # Quoted path
|
|||||||
**Common Pitfall:**
|
**Common Pitfall:**
|
||||||
Using `.split()` instead of `shlex.split()` would break commands with spaces in paths. Always use quoted strings for paths that contain whitespace.
|
Using `.split()` instead of `shlex.split()` would break commands with spaces in paths. Always use quoted strings for paths that contain whitespace.
|
||||||
|
|
||||||
|
### Log Path Validation & Allowlisting
|
||||||
|
|
||||||
|
Authenticated users can instruct fail2ban to monitor additional log files through the API endpoint `POST /api/config/jails/{name}/logpath`. To prevent path-traversal attacks and unauthorized reads of sensitive system files, all requested log paths must resolve to locations within a configurable allowlist of safe directories.
|
||||||
|
|
||||||
|
**Allowed Directories:**
|
||||||
|
- Configured via the `BANGUI_ALLOWED_LOG_DIRS` environment variable (comma-separated list).
|
||||||
|
- Defaults to: `["/var/log", "/config/log"]`.
|
||||||
|
|
||||||
|
**Path Validation Rules:**
|
||||||
|
1. The requested path is resolved to its canonical form using `Path(log_path).resolve()`, which:
|
||||||
|
- Expands relative paths to absolute paths.
|
||||||
|
- Resolves symbolic links to their real targets.
|
||||||
|
- Normalizes `.` and `..` components.
|
||||||
|
2. The resolved path is checked using `Path.is_relative_to()` against each allowed directory prefix.
|
||||||
|
3. If the resolved path is not relative to any allowed directory, a `ValueError` is raised with a descriptive error message.
|
||||||
|
|
||||||
|
**Implementation:**
|
||||||
|
- Validation occurs in the Pydantic model `AddLogPathRequest` using a `@field_validator`.
|
||||||
|
- The validator runs at request time, before the service layer is invoked.
|
||||||
|
- Symlinks that escape allowed directories are rejected (see [symlink bypass tests](../../backend/tests/test_models.py)).
|
||||||
|
|
||||||
|
**Important:** Use `is_relative_to()`, not `startswith()` or string prefix matching. The latter is bypassable with paths like `/var/log_evil/file.log`.
|
||||||
|
|
||||||
|
**Environment Variables:**
|
||||||
|
```bash
|
||||||
|
BANGUI_ALLOWED_LOG_DIRS="/var/log,/config/log" # Default
|
||||||
|
BANGUI_ALLOWED_LOG_DIRS="/var/log,/config/log,/home/app/logs" # Custom directory
|
||||||
|
```
|
||||||
|
|
||||||
### Login Rate Limiting
|
### Login Rate Limiting
|
||||||
|
|
||||||
The login endpoint (`POST /api/auth/login`) is protected against brute-force attacks using an in-memory rate limiter.
|
The login endpoint (`POST /api/auth/login`) is protected against brute-force attacks using an in-memory rate limiter.
|
||||||
|
|||||||
@@ -212,11 +212,12 @@ A page to inspect and modify the fail2ban configuration without leaving the web
|
|||||||
|
|
||||||
- Option to register additional log files that fail2ban should monitor.
|
- Option to register additional log files that fail2ban should monitor.
|
||||||
- For each new log, specify:
|
- For each new log, specify:
|
||||||
- The path to the log file.
|
- The path to the log file (must be within allowed directories to prevent unauthorized access to sensitive files).
|
||||||
- One or more regex patterns that define what constitutes a failure.
|
- One or more regex patterns that define what constitutes a failure.
|
||||||
- The jail name and basic jail settings (ban time, retries, etc.).
|
- The jail name and basic jail settings (ban time, retries, etc.).
|
||||||
- Choose whether the file should be read from the beginning or only new lines (head vs. tail).
|
- Choose whether the file should be read from the beginning or only new lines (head vs. tail).
|
||||||
- Preview matching lines from the log against the provided regex before saving, so the user can verify the pattern works.
|
- Preview matching lines from the log against the provided regex before saving, so the user can verify the pattern works.
|
||||||
|
- **Log Path Security:** Added log paths must resolve to locations within a configured allowlist of safe directories (default: `/var/log` and `/config/log`). This prevents authenticated users from instructing fail2ban to monitor sensitive system files. Paths containing symlinks are resolved to their canonical targets before validation.
|
||||||
|
|
||||||
### Regex Tester
|
### Regex Tester
|
||||||
|
|
||||||
@@ -264,7 +265,7 @@ A page to inspect and modify the fail2ban configuration without leaving the web
|
|||||||
- Truncation notice when the total log file line count exceeds the requested tail limit.
|
- Truncation notice when the total log file line count exceeds the requested tail limit.
|
||||||
- Container automatically scrolls to the bottom after each data update.
|
- Container automatically scrolls to the bottom after each data update.
|
||||||
- When fail2ban is configured to log to a non-file target (STDOUT, STDERR, SYSLOG, SYSTEMD-JOURNAL), an informational banner explains that file-based log viewing is unavailable.
|
- When fail2ban is configured to log to a non-file target (STDOUT, STDERR, SYSLOG, SYSTEMD-JOURNAL), an informational banner explains that file-based log viewing is unavailable.
|
||||||
- The log file path is validated against a safe prefix allowlist on the backend to prevent path-traversal reads.
|
- Log file paths are validated against a configurable allowlist of safe directories on the backend to prevent unauthorized reads of sensitive system files.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -142,6 +142,14 @@ class Settings(BaseSettings):
|
|||||||
"Used for listing, viewing, and editing configuration files through the web UI."
|
"Used for listing, viewing, and editing configuration files through the web UI."
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
|
allowed_log_dirs: list[str] = Field(
|
||||||
|
default_factory=lambda: ["/var/log", "/config/log"],
|
||||||
|
description=(
|
||||||
|
"List of allowed directory prefixes for jail log paths. "
|
||||||
|
"Any log path added must resolve to a path within one of these directories. "
|
||||||
|
"Use absolute paths. Symlinks are resolved before validation."
|
||||||
|
),
|
||||||
|
)
|
||||||
fail2ban_start_command: str = Field(
|
fail2ban_start_command: str = Field(
|
||||||
default="fail2ban-client start",
|
default="fail2ban-client start",
|
||||||
description=(
|
description=(
|
||||||
@@ -193,4 +201,4 @@ def get_settings() -> Settings:
|
|||||||
A validated :class:`Settings` object. Raises :class:`pydantic.ValidationError`
|
A validated :class:`Settings` object. Raises :class:`pydantic.ValidationError`
|
||||||
if required keys are absent or values fail validation.
|
if required keys are absent or values fail validation.
|
||||||
"""
|
"""
|
||||||
return Settings() # type: ignore[call-arg] # pydantic-settings populates required fields from env vars
|
return Settings() # pydantic-settings populates required fields from env vars
|
||||||
|
|||||||
@@ -4,9 +4,12 @@ Request, response, and domain models for the config router and service.
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
import datetime
|
import datetime
|
||||||
|
from pathlib import Path
|
||||||
from typing import Literal
|
from typing import Literal
|
||||||
|
|
||||||
from pydantic import BaseModel, ConfigDict, Field
|
from pydantic import BaseModel, ConfigDict, Field, field_validator
|
||||||
|
|
||||||
|
from app.config import get_settings
|
||||||
|
|
||||||
DNSMode = Literal["yes", "warn", "no", "raw"]
|
DNSMode = Literal["yes", "warn", "no", "raw"]
|
||||||
LogEncoding = Literal["auto", "ascii", "utf-8", "UTF-8", "latin-1"]
|
LogEncoding = Literal["auto", "ascii", "utf-8", "UTF-8", "latin-1"]
|
||||||
@@ -213,6 +216,43 @@ class AddLogPathRequest(BaseModel):
|
|||||||
description="If true, monitor from current end of file (tail). If false, read from the beginning.",
|
description="If true, monitor from current end of file (tail). If false, read from the beginning.",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
@field_validator("log_path", mode="after")
|
||||||
|
@classmethod
|
||||||
|
def validate_log_path(cls, value: str) -> str:
|
||||||
|
"""Validate that the log path is within allowed directories.
|
||||||
|
|
||||||
|
Resolves the path to its canonical form (resolving symlinks) and checks
|
||||||
|
that it is relative to one of the allowed log directories from settings.
|
||||||
|
|
||||||
|
Args:
|
||||||
|
value: The log path to validate.
|
||||||
|
|
||||||
|
Returns:
|
||||||
|
The validated log path.
|
||||||
|
|
||||||
|
Raises:
|
||||||
|
ValueError: If the path is outside allowed log directories.
|
||||||
|
"""
|
||||||
|
settings = get_settings()
|
||||||
|
try:
|
||||||
|
resolved_path = Path(value).resolve()
|
||||||
|
except (OSError, RuntimeError) as e:
|
||||||
|
raise ValueError(f"Cannot resolve path {value!r}: {e}") from e
|
||||||
|
|
||||||
|
for allowed_dir in settings.allowed_log_dirs:
|
||||||
|
allowed_path = Path(allowed_dir).resolve()
|
||||||
|
try:
|
||||||
|
resolved_path.relative_to(allowed_path)
|
||||||
|
return value
|
||||||
|
except ValueError:
|
||||||
|
continue
|
||||||
|
|
||||||
|
allowed_dirs_str = ", ".join(settings.allowed_log_dirs)
|
||||||
|
raise ValueError(
|
||||||
|
f"Log path {value!r} is outside allowed directories: {allowed_dirs_str}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
class LogPreviewRequest(BaseModel):
|
class LogPreviewRequest(BaseModel):
|
||||||
"""Payload for ``POST /api/config/preview-log``."""
|
"""Payload for ``POST /api/config/preview-log``."""
|
||||||
|
|||||||
141
backend/tests/test_models.py
Normal file
141
backend/tests/test_models.py
Normal file
@@ -0,0 +1,141 @@
|
|||||||
|
"""Unit tests for Pydantic models and their validators."""
|
||||||
|
|
||||||
|
import tempfile
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from pydantic import ValidationError
|
||||||
|
|
||||||
|
from app.config import Settings
|
||||||
|
from app.models.config import AddLogPathRequest
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def _mock_allowed_dirs(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||||
|
"""Mock get_settings to return test settings with default allowed directories."""
|
||||||
|
def mock_get_settings() -> Settings:
|
||||||
|
return Settings(
|
||||||
|
database_path=":memory:",
|
||||||
|
fail2ban_socket="/tmp/fake.sock",
|
||||||
|
fail2ban_config_dir="/tmp/fail2ban",
|
||||||
|
session_secret="test-secret-key-do-not-use",
|
||||||
|
)
|
||||||
|
|
||||||
|
monkeypatch.setattr("app.models.config.get_settings", mock_get_settings)
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_valid_in_var_log(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Valid log paths in /var/log are accepted."""
|
||||||
|
req = AddLogPathRequest(log_path="/var/log/auth.log", tail=True)
|
||||||
|
assert req.log_path == "/var/log/auth.log"
|
||||||
|
assert req.tail is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_valid_in_config_log(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Valid log paths in /config/log are accepted."""
|
||||||
|
req = AddLogPathRequest(log_path="/config/log/app.log", tail=False)
|
||||||
|
assert req.log_path == "/config/log/app.log"
|
||||||
|
assert req.tail is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_valid_with_subdirectory(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Log paths in subdirectories of allowed paths are accepted."""
|
||||||
|
req = AddLogPathRequest(log_path="/var/log/syslog/auth.log", tail=True)
|
||||||
|
assert req.log_path == "/var/log/syslog/auth.log"
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_rejects_path_outside_allowed(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Paths outside allowed directories are rejected."""
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
AddLogPathRequest(log_path="/etc/passwd", tail=True)
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "outside allowed directories" in error_msg
|
||||||
|
assert "/etc/passwd" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_rejects_home_directory(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Paths in home directories are rejected."""
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
AddLogPathRequest(log_path="/home/user/app.log", tail=True)
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "outside allowed directories" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_rejects_shadow_file(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Paths to sensitive files like /etc/shadow are rejected."""
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
AddLogPathRequest(log_path="/etc/shadow", tail=True)
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "outside allowed directories" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_rejects_symlink_escape(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||||
|
"""Symlinks that escape allowed directories are rejected."""
|
||||||
|
with tempfile.TemporaryDirectory() as tmpdir:
|
||||||
|
allowed_dir = Path(tmpdir) / "allowed"
|
||||||
|
escape_dir = Path(tmpdir) / "escape"
|
||||||
|
allowed_dir.mkdir()
|
||||||
|
escape_dir.mkdir()
|
||||||
|
|
||||||
|
symlink = allowed_dir / "escape_link"
|
||||||
|
symlink.symlink_to(escape_dir)
|
||||||
|
|
||||||
|
def mock_get_settings() -> Settings:
|
||||||
|
return Settings(
|
||||||
|
database_path=":memory:",
|
||||||
|
fail2ban_socket="/tmp/fake.sock",
|
||||||
|
fail2ban_config_dir="/tmp/fail2ban",
|
||||||
|
session_secret="test-secret-key-do-not-use",
|
||||||
|
allowed_log_dirs=[str(allowed_dir)],
|
||||||
|
)
|
||||||
|
|
||||||
|
monkeypatch.setattr("app.models.config.get_settings", mock_get_settings)
|
||||||
|
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
AddLogPathRequest(log_path=str(symlink / "evil.log"), tail=True)
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "outside allowed directories" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_validates_startswith_bypass(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Paths like /var/log_evil that bypass startswith() are rejected."""
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
AddLogPathRequest(log_path="/var/log_evil/somefile.log", tail=True)
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "outside allowed directories" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_default_tail_is_true(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Tail defaults to True."""
|
||||||
|
req = AddLogPathRequest(log_path="/var/log/app.log")
|
||||||
|
assert req.tail is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_error_message_lists_allowed_dirs(_mock_allowed_dirs: None) -> None:
|
||||||
|
"""Error message includes the list of allowed directories."""
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
AddLogPathRequest(log_path="/root/secret.log", tail=True)
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "/var/log" in error_msg
|
||||||
|
assert "/config/log" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_add_log_path_request_custom_allowed_dirs(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||||
|
"""Custom allowed directories from settings are respected."""
|
||||||
|
def mock_get_settings() -> Settings:
|
||||||
|
return Settings(
|
||||||
|
database_path=":memory:",
|
||||||
|
fail2ban_socket="/tmp/fake.sock",
|
||||||
|
fail2ban_config_dir="/tmp/fail2ban",
|
||||||
|
session_secret="test-secret-key-do-not-use",
|
||||||
|
allowed_log_dirs=["/custom/logs", "/another/path"],
|
||||||
|
)
|
||||||
|
|
||||||
|
monkeypatch.setattr("app.models.config.get_settings", mock_get_settings)
|
||||||
|
|
||||||
|
req = AddLogPathRequest(log_path="/custom/logs/app.log", tail=True)
|
||||||
|
assert req.log_path == "/custom/logs/app.log"
|
||||||
|
|
||||||
|
with pytest.raises(ValidationError):
|
||||||
|
AddLogPathRequest(log_path="/var/log/app.log", tail=True)
|
||||||
|
|
||||||
Reference in New Issue
Block a user