TASK-020: Fix log_target security vulnerability (defense in depth)
**Issue:** - log_target accepted arbitrary paths, allowing authenticated users to write files as root via fail2ban (e.g., /etc/cron.d/bangui-pwned) - fail2ban runs as root and opens files specified in log_target **Solution:** 1. **Model layer validation:** Already existed in GlobalConfigUpdate, prevents invalid paths before reaching service 2. **Service layer validation:** Added defensive check in update_global_config() that validates log_target even if model validation is bypassed 3. **New validation helper:** Added validate_log_target() utility that accepts special values (STDOUT, STDERR, SYSLOG) or paths within allowed directories **Changes:** - app/utils/path_utils.py: Added validate_log_target() helper - app/services/config_service.py: Added service-layer validation before sending command to fail2ban - backend/tests: Fixed session_secret length issues in fixtures (min 32 chars) - backend/tests: Added tests for valid special log targets - Docs/Backend-Development.md: Documented log_target security requirements **Test Coverage:** - Model validation rejects /etc/passwd (existing test) - Model validation accepts STDOUT, STDERR, SYSLOG special values - Model validation accepts paths in allowed directories - Service layer validation tested with special values Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -59,6 +59,7 @@ from app.utils.fail2ban_response import (
|
||||
ok,
|
||||
to_dict,
|
||||
)
|
||||
from app.utils.path_utils import validate_log_target
|
||||
|
||||
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
||||
|
||||
@@ -421,8 +422,15 @@ async def update_global_config(socket_path: str, update: GlobalConfigUpdate) ->
|
||||
|
||||
Raises:
|
||||
ConfigOperationError: If a ``set`` command is rejected.
|
||||
ConfigValidationError: If log_target validation fails.
|
||||
~app.utils.fail2ban_client.Fail2BanConnectionError: Socket unreachable.
|
||||
"""
|
||||
if update.log_target is not None:
|
||||
try:
|
||||
validate_log_target(update.log_target)
|
||||
except ValueError as e:
|
||||
raise ConfigValidationError(str(e)) from e
|
||||
|
||||
client = Fail2BanClient(socket_path=socket_path, timeout=FAIL2BAN_SOCKET_TIMEOUT)
|
||||
|
||||
async def _set_global(key: str, value: Fail2BanToken) -> None:
|
||||
|
||||
@@ -38,3 +38,25 @@ def validate_log_path(log_path: str) -> str:
|
||||
raise ValueError(
|
||||
f"Log path {log_path!r} is outside allowed directories: {allowed_dirs_str}"
|
||||
)
|
||||
|
||||
|
||||
def validate_log_target(log_target: str) -> str:
|
||||
"""Validate that a log target is either a special value or a valid file path.
|
||||
|
||||
Accepts special values (STDOUT, STDERR, SYSLOG) and file paths that resolve
|
||||
to one of the configured allowed log directories.
|
||||
|
||||
Args:
|
||||
log_target: The log target to validate.
|
||||
|
||||
Returns:
|
||||
The validated log target (unchanged).
|
||||
|
||||
Raises:
|
||||
ValueError: If the target is not a special value and not in allowed directories.
|
||||
"""
|
||||
if log_target.upper() in ("STDOUT", "STDERR", "SYSLOG"):
|
||||
return log_target
|
||||
|
||||
return validate_log_path(log_target)
|
||||
|
||||
|
||||
@@ -18,7 +18,7 @@ def _mock_allowed_dirs(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
database_path=":memory:",
|
||||
fail2ban_socket="/tmp/fake.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use",
|
||||
session_secret="a" * 32,
|
||||
)
|
||||
|
||||
monkeypatch.setattr("app.models.config.get_settings", mock_get_settings)
|
||||
|
||||
@@ -29,7 +29,7 @@ from app.models.config import (
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_SETUP_PAYLOAD = {
|
||||
"master_password": "testpassword1",
|
||||
"master_password": "Testpass1!",
|
||||
"database_path": "bangui.db",
|
||||
"fail2ban_socket": "/var/run/fail2ban/fail2ban.sock",
|
||||
"timezone": "UTC",
|
||||
@@ -43,7 +43,7 @@ async def config_client(tmp_path: Path) -> AsyncClient: # type: ignore[misc]
|
||||
settings = Settings(
|
||||
database_path=str(tmp_path / "config_test.db"),
|
||||
fail2ban_socket="/tmp/fake.sock",
|
||||
session_secret="test-config-secret",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
session_duration_minutes=60,
|
||||
timezone="UTC",
|
||||
log_level="debug",
|
||||
|
||||
@@ -36,10 +36,11 @@ def _mock_settings(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
database_path=":memory:",
|
||||
fail2ban_socket="/tmp/fake.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
)
|
||||
|
||||
monkeypatch.setattr("app.models.config.get_settings", mock_get_settings)
|
||||
monkeypatch.setattr("app.utils.path_utils.get_settings", mock_get_settings)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -519,6 +520,34 @@ class TestUpdateGlobalConfig:
|
||||
cmd = next(c for c in sent if len(c) >= 3 and c[1] == "loglevel")
|
||||
assert cmd[2] == "DEBUG"
|
||||
|
||||
async def test_invalid_log_target_raises_config_validation_error(self) -> None:
|
||||
"""update_global_config rejects invalid log_target from model validation."""
|
||||
from pydantic import ValidationError
|
||||
|
||||
with pytest.raises(ValidationError, match="outside allowed directories"):
|
||||
GlobalConfigUpdate(log_target="/etc/passwd")
|
||||
|
||||
async def test_valid_special_log_target(self) -> None:
|
||||
"""update_global_config accepts special log_target values."""
|
||||
sent: list[list[Any]] = []
|
||||
|
||||
async def _send(command: list[Any]) -> Any:
|
||||
sent.append(command)
|
||||
return (0, "OK")
|
||||
|
||||
class _FakeClient:
|
||||
def __init__(self, **_kw: Any) -> None:
|
||||
self.send = AsyncMock(side_effect=_send)
|
||||
|
||||
for target in ["STDOUT", "STDERR", "SYSLOG"]:
|
||||
sent.clear()
|
||||
update = GlobalConfigUpdate(log_target=target)
|
||||
with patch("app.services.config_service.Fail2BanClient", _FakeClient):
|
||||
await config_service.update_global_config(_SOCKET, update)
|
||||
|
||||
cmd = next(c for c in sent if len(c) >= 3 and c[1] == "logtarget")
|
||||
assert cmd[2] == target
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# test_regex (synchronous)
|
||||
|
||||
Reference in New Issue
Block a user