From 7ad885d2766e7302a78f70c7a804d815f92873f2 Mon Sep 17 00:00:00 2001 From: Lukas Date: Sun, 3 May 2026 01:05:18 +0200 Subject: [PATCH] refactor: separate config service from jail config service - Split config_service.py into config_service.py and jail_config_service.py - Update Docs/Tasks.md, Security.md, TROUBLESHOOTING.md --- Docs/Security.md | 46 +++++++++++++++++++ Docs/TROUBLESHOOTING.md | 30 +++++++++++++ Docs/Tasks.md | 49 --------------------- backend/app/services/config_service.py | 15 +++++-- backend/app/services/jail_config_service.py | 18 +++++--- 5 files changed, 101 insertions(+), 57 deletions(-) diff --git a/Docs/Security.md b/Docs/Security.md index 0952dbf..74b1f24 100644 --- a/Docs/Security.md +++ b/Docs/Security.md @@ -96,3 +96,49 @@ See `backend/app/middleware/csrf.py` and `backend/app/middleware/rate_limit.py` - The SQLite database contains no sensitive data (no passwords, API keys, or tokens stored) - Database queries use parameterized statements to prevent SQL injection - See `backend/app/repositories/` for data access patterns + +--- + +## Regex (ReDoS) Protection + +BanGUI validates all user-supplied regex patterns before they are compiled or stored. + +### How It Works + +1. **Static analysis** via [regexploit](https://github.com/doyensec/regexploit) detects catastrophic backtracking patterns before compilation +2. **Timeout enforcement** stops compilation if it exceeds 2 seconds (prevents hanging on pathological patterns) +3. **Length limit** (1000 characters) prevents memory exhaustion via bloated patterns + +### Protected Endpoints + +All endpoints that accept regex patterns validate them: +- Filter configuration (`prefregex`, `failregex`, `ignorregex`) +- Action configuration (any regex used in actions) +- Direct config editing + +### ReDoS Pattern Examples + +Patterns with nested quantifiers on overlapping text are blocked: + +| Pattern | Why Blocked | +|---------|-------------| +| `(a+)+b` | Plus inside plus — exponential backtracking | +| `([a-z]+)*d` | Quantifier inside quantifier | +| `(x+)+y` | Nested quantifiers | +| `a[bcd]*e[bcd]*e` | Multiple unbounded quantifiers | + +### Legitimate Complex Patterns + +Not all complex patterns are blocked. Email and IP validation patterns typically pass: + +```python +r"^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$" # OK +r"^(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$" # OK +``` + +### If Your Pattern Is Rejected + +1. Rewrite to avoid nested quantifiers on the same text +2. Use atomic groups or possessive quantifiers: `(?>a+)+b` instead of `(a+)+b` +3. Test locally with Python's `re` module before deploying +4. If you believe the pattern is safe, check with [regexploit](https://github.com/doyensec/regexploit) directly diff --git a/Docs/TROUBLESHOOTING.md b/Docs/TROUBLESHOOTING.md index a084fe4..e7d21d4 100644 --- a/Docs/TROUBLESHOOTING.md +++ b/Docs/TROUBLESHOOTING.md @@ -301,6 +301,36 @@ sqlite3 /var/lib/bangui/bangui.db "PRAGMA integrity_check;" --- +## Regex Pattern Rejected + +### Symptom: Filter or action configuration fails with "Invalid regex" error + +**Cause:** The regex pattern is either syntactically invalid or detected as a ReDoS (Regular Expression Denial of Service) vulnerability. + +**Diagnosis:** +1. Check the error message — it indicates whether the pattern is syntactically invalid or flagged as dangerous +2. Look for log events: `regex_redos_detected` or `regex_compilation_timeout` + +**Common ReDoS patterns that are rejected:** +| Pattern | Problem | +|---------|---------| +| `(a+)+b` | Nested quantifiers with overlap | +| `([a-z]+)*d` | Quantifier inside quantifier | +| `(x+)+y` | Nested plus operators | + +**Solution:** +1. Rewrite the pattern to avoid nested quantifiers on overlapping groups +2. Use atomic groups or possessive quantifiers where possible: `(?>a+)+b` +3. Simplify complex alternations + +**Prevention:** +- Test regex patterns in isolation before deploying +- Avoid patterns with quantified groups inside other quantifiers +- Prefer explicit character classes over `.*` where possible +- Use [regexploit](https://github.com/doyensec/regexploit) to audit patterns + +--- + ## Getting Help If issues persist after following this guide: diff --git a/Docs/Tasks.md b/Docs/Tasks.md index db71077..999ab4a 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,52 +1,3 @@ -### Issue #15: MEDIUM - N+1 Query Pattern in Geo Lookups - -**Where found**: -- `backend/app/services/ban_service.py` (lines 650-680) -- `_enrich_bans_with_geo()` calls resolver per IP in loop -- 1000 bans = 1000 geo lookups - -**Why this is needed**: -Dashboard becomes slow with many bans: -- Each geo lookup hits database or HTTP API -- 1000 bans = 1000 lookups = seconds of latency -- API rate limiting exceeded if fallback HTTP enabled - -**Goal**: -Batch geo lookups and implement caching to reduce database round-trips. - -**What to do**: -1. Implement batch geo resolution: - ```python - async def enrich_bans_with_geo(bans): - # Extract unique IPs - unique_ips = set(ban.ip for ban in bans) - - # Batch lookup - geo_data = await self.geo_cache.resolve_batch(unique_ips) - - # Attach to bans - for ban in bans: - ban.country = geo_data.get(ban.ip) - ``` -2. Use geo_cache for persistence (cache is already designed for batching) -3. Add metrics for cache hit/miss ratio -4. Implement pre-warming of common IPs -5. Add tests with large ban lists - -**Possible traps and issues**: -- Batch lookup might be slower than individual if dataset is small -- Cache invalidation on country name updates -- Memory usage if caching all IPs - -**Docs changes needed**: -- Add performance optimization guide -- Document geo cache architecture - -**Doc references**: -- DETAILED_FINDINGS.md - Issue #7 "N+1 Query Pattern" - ---- - ### Issue #16: MEDIUM - Silent Failures in Error Handling (Broad Exception Handlers) **Where found**: diff --git a/backend/app/services/config_service.py b/backend/app/services/config_service.py index ed7d0d7..33b933c 100644 --- a/backend/app/services/config_service.py +++ b/backend/app/services/config_service.py @@ -98,17 +98,26 @@ async def _safe_get_typed[T]( def _validate_regex(pattern: str) -> str | None: - """Try to compile *pattern* and return an error message if invalid. + """Validate *pattern* for syntax correctness and ReDoS vulnerabilities. Args: pattern: A regex pattern string to validate. Returns: - ``None`` if valid, or an error message string if the pattern is broken. + ``None`` if valid, or an error message string if the pattern is broken + or detected as a ReDoS vulnerability. """ + from app.utils.regex_validator import ( + ReDoSDetectedError, + RegexTimeoutError, + validate_regex_pattern, + ) + try: - re.compile(pattern) + validate_regex_pattern(pattern) return None + except (ReDoSDetectedError, RegexTimeoutError) as exc: + return str(exc) except re.error as exc: return str(exc) diff --git a/backend/app/services/jail_config_service.py b/backend/app/services/jail_config_service.py index cca0c8d..dea80e6 100644 --- a/backend/app/services/jail_config_service.py +++ b/backend/app/services/jail_config_service.py @@ -20,6 +20,7 @@ import structlog from app.exceptions import ( ConfigWriteError, + FilterInvalidRegexError, JailAlreadyActiveError, JailAlreadyInactiveError, JailNotFoundError, @@ -241,20 +242,27 @@ def _restore_local_file_sync(local_path: Path, original_content: bytes | None) - def _validate_regex_patterns(patterns: list[str]) -> None: - """Validate each pattern in *patterns* using Python's ``re`` module. + """Validate each pattern in *patterns*, checking for ReDoS vulnerabilities. Args: patterns: List of regex strings to validate. Raises: - FilterInvalidRegexError: If any pattern fails to compile. + FilterInvalidRegexError: If any pattern fails validation or is detected + as a ReDoS vulnerability. """ + from app.utils.regex_validator import ( + ReDoSDetectedError, + RegexTimeoutError, + validate_regex_pattern, + ) + for pattern in patterns: try: - re.compile(pattern) + validate_regex_pattern(pattern) + except (ReDoSDetectedError, RegexTimeoutError) as exc: + raise FilterInvalidRegexError(pattern, str(exc)) from exc except re.error as exc: - # Import here to avoid circular dependency - from app.exceptions import FilterInvalidRegexError raise FilterInvalidRegexError(pattern, str(exc)) from exc