diff --git a/.jules/sentinel.md b/.jules/sentinel.md index a1ed0d5..9d9f391 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -60,3 +60,10 @@ **Prevention:** 1. Implement strict validation on all data items from external lists (`is_valid_rule`). 2. Filter out items containing dangerous characters (`<`, `>`, `"` etc.) or control codes. + +## 2026-05-24 - [Weak Blacklist Validation for Rules] +**Vulnerability:** The `is_valid_rule` function used a blacklist of dangerous characters (`<>\"'`();{}[]`) to sanitize rules. This missed several dangerous characters like `&`, `|`, and ` ` which could allow command injection or other attacks if the backend processed rules unsafely. +**Learning:** Blacklists are inherently fragile because they require knowing every possible dangerous character. Whitelists are robust because they define exactly what is allowed. +**Prevention:** +1. Replace blacklists with strict whitelists whenever possible. +2. Use regex to enforce allowable character sets (e.g., `^[a-zA-Z0-9.\-_:*\/]+$`). diff --git a/main.py b/main.py index c781058..2f9351e 100644 --- a/main.py +++ b/main.py @@ -259,16 +259,15 @@ def validate_profile_id(profile_id: str) -> bool: def is_valid_rule(rule: str) -> bool: """ Validates that a rule is safe to use. - Rejects potential XSS payloads or control characters. + Enforces a strict whitelist of allowed characters. + Allowed: Alphanumeric, hyphen, dot, underscore, asterisk, colon (IPv6), slash (CIDR) """ - if not rule or not rule.isprintable(): + if not rule: return False - # Block characters common in XSS and injection attacks - # Allowed: Alphanumeric, hyphen, dot, underscore, asterisk, colon (IPv6), slash (CIDR) - # Block: < > " ' ` ( ) ; { } [ ] - dangerous_chars = set("<>\"'`();{}[]") - if any(c in dangerous_chars for c in rule): + # Strict whitelist to prevent injection + # ^[a-zA-Z0-9.\-_:*\/]+$ + if not re.match(r"^[a-zA-Z0-9.\-_:*\/]+$", rule): return False return True diff --git a/tests/test_security.py b/tests/test_security.py index 579c60d..f9091b3 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -1,6 +1,8 @@ + import pytest from unittest.mock import MagicMock import main +from main import is_valid_rule def test_push_rules_filters_xss_payloads(): """ @@ -85,3 +87,35 @@ def test_push_rules_filters_xss_payloads(): finally: main._api_post_form = original_post_form main.log = original_log + +@pytest.mark.parametrize("rule,expected_validity", [ + # Valid Inputs (should pass) + ("example.com", True), + ("1.2.3.4", True), + ("2001:db8::1", True), + ("192.168.1.0/24", True), + ("*.example.com", True), + ("_sip._tcp.example.com", True), + ("valid-domain-with-hyphens.com", True), + ("sub.domain.example.com", True), + + # Invalid Inputs (should fail) + # These contain characters NOT in the whitelist (and some not in the old blacklist) + ("example.com && rm -rf /", False), # Contains spaces and & + ("$(whoami).example.com", False), # Contains $ and ( ) + ("example.com|nc 1.2.3.4 80", False),# Contains | and space + ("example.com; ls", False), # Contains ; and space + ("", False),# Contains < > ( ) + ("invalid@email.com", False), # Contains @ (not allowed in whitelist) + ("http://example.com", True), # Safe from injection (contains only allowed chars) + ("example.com/path", True), # Allowed (CIDR uses /) + ("foo bar", False), # Space not allowed + ("foo\tbar", False), # Tab not allowed + ("foo\nbar", False), # Newline not allowed + ("`touch hacked`", False), # Backtick not allowed +]) +def test_is_valid_rule_strict(rule, expected_validity): + """ + Tests the is_valid_rule function against a strict whitelist of inputs. + """ + assert is_valid_rule(rule) == expected_validity, f"Failed for rule: {rule}"