diff --git a/PENTEST_PLAN.md b/PENTEST_PLAN.md index ed78c92..32ed2c6 100644 --- a/PENTEST_PLAN.md +++ b/PENTEST_PLAN.md @@ -245,8 +245,8 @@ Testing uses specialized Claude subagents for different security domains, with f - [x] **BURN-001**: Track HEAD requests as paste access for burn-after-read - [x] **BURN-002**: Add test for HEAD-then-GET race condition - [x] **RATE-001**: Add maximum entries limit to rate limit storage -- [ ] **RATE-002**: Add automatic cleanup trigger when threshold exceeded -- [ ] **CLI-001**: Validate clipboard tool paths against allow-list +- [x] **RATE-002**: Add automatic cleanup trigger when threshold exceeded +- [x] **CLI-001**: Validate clipboard tool paths against allow-list ### Medium-term (Medium) diff --git a/app/api/routes.py b/app/api/routes.py index 4197f21..f78fa4b 100644 --- a/app/api/routes.py +++ b/app/api/routes.py @@ -166,6 +166,7 @@ def check_rate_limit(client_ip: str, authenticated: bool = False) -> tuple[bool, window = current_app.config["RATE_LIMIT_WINDOW"] max_requests = current_app.config["RATE_LIMIT_MAX"] max_entries = current_app.config.get("RATE_LIMIT_MAX_ENTRIES", 10000) + cleanup_threshold = current_app.config.get("RATE_LIMIT_CLEANUP_THRESHOLD", 0.8) if authenticated: max_requests *= current_app.config.get("RATE_LIMIT_AUTH_MULTIPLIER", 5) @@ -174,7 +175,15 @@ def check_rate_limit(client_ip: str, authenticated: bool = False) -> tuple[bool, cutoff = now - window with _rate_limit_lock: - # RATE-001: Enforce maximum entries to prevent memory exhaustion + entry_count = len(_rate_limit_requests) + + # RATE-002: Proactive cleanup when exceeding threshold + threshold_count = int(max_entries * cleanup_threshold) + if entry_count >= threshold_count: + # Clean up expired entries first + _prune_rate_limit_entries(threshold_count // 2, cutoff) + + # RATE-001: Hard limit enforcement (fallback if threshold cleanup wasn't enough) if len(_rate_limit_requests) >= max_entries and client_ip not in _rate_limit_requests: # Evict oldest entries (those with oldest last request time) _prune_rate_limit_entries(max_entries // 2, cutoff) diff --git a/app/config.py b/app/config.py index 6c86f09..0fdc91f 100644 --- a/app/config.py +++ b/app/config.py @@ -101,6 +101,10 @@ class Config: RATE_LIMIT_AUTH_MULTIPLIER = int(os.environ.get("FLASKPASTE_RATE_AUTH_MULT", "5")) # Maximum unique IPs tracked in rate limit storage (RATE-001: memory DoS protection) RATE_LIMIT_MAX_ENTRIES = int(os.environ.get("FLASKPASTE_RATE_MAX_ENTRIES", "10000")) + # RATE-002: Cleanup threshold (0.0-1.0) - trigger cleanup when entries exceed this ratio + RATE_LIMIT_CLEANUP_THRESHOLD = float( + os.environ.get("FLASKPASTE_RATE_CLEANUP_THRESHOLD", "0.8") + ) # Audit Logging # Track security-relevant events (paste creation, deletion, rate limits, etc.) diff --git a/fpaste b/fpaste index ba89258..d7ac0e6 100755 --- a/fpaste +++ b/fpaste @@ -455,12 +455,65 @@ CLIPBOARD_WRITE_COMMANDS = [ ("wl-copy", ["wl-copy"]), ] +# CLI-001: Trusted directories for clipboard tools (system paths only) +# Prevents command injection via malicious PATH manipulation +TRUSTED_CLIPBOARD_DIRS = frozenset( + { + "/usr/bin", + "/usr/local/bin", + "/bin", + "/opt/homebrew/bin", # macOS Homebrew + "/usr/X11/bin", + "/usr/X11R6/bin", + } +) + +# Windows system directories (checked case-insensitively) +TRUSTED_WINDOWS_PATTERNS = ( + "\\windows\\", + "\\system32\\", + "\\syswow64\\", + "\\windowsapps\\", +) + + +def is_trusted_clipboard_path(path: str) -> bool: + """Check if clipboard tool path is in a trusted system directory. + + CLI-001: Validates that resolved clipboard tool paths are in expected + system locations to prevent command injection via PATH manipulation. + """ + if not path: + return False + + resolved = Path(path).resolve() + parent = str(resolved.parent) + + # Check Unix trusted directories + if parent in TRUSTED_CLIPBOARD_DIRS: + return True + + # Check Windows paths (case-insensitive) + parent_lower = parent.lower() + for pattern in TRUSTED_WINDOWS_PATTERNS: + if pattern in parent_lower: + return True + + # Also allow Windows Program Files paths + return "\\program files" in parent_lower + def find_clipboard_command(commands: list[tuple[str, list[str]]]) -> list[str] | None: - """Find first available clipboard command.""" + """Find first available clipboard command in trusted directories. + + CLI-001: Validates that found commands are in trusted system directories + to prevent command injection via PATH manipulation. + """ for tool_name, cmd in commands: - if shutil.which(tool_name): - return cmd + tool_path = shutil.which(tool_name) + if tool_path and is_trusted_clipboard_path(tool_path): + # Use absolute path for security + return [tool_path, *cmd[1:]] return None diff --git a/tests/test_cli_security.py b/tests/test_cli_security.py new file mode 100644 index 0000000..b3cbaaf --- /dev/null +++ b/tests/test_cli_security.py @@ -0,0 +1,96 @@ +"""Tests for CLI security features (CLI-001: clipboard tool validation).""" + +import importlib.machinery +import importlib.util +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + + +def load_fpaste_module(): + """Load the fpaste script as a module.""" + fpaste_path = Path(__file__).parent.parent / "fpaste" + spec = importlib.util.spec_from_loader( + "fpaste", + importlib.machinery.SourceFileLoader("fpaste", str(fpaste_path)), + ) + if spec is None or spec.loader is None: + pytest.skip("Could not load fpaste module") + + fpaste = importlib.util.module_from_spec(spec) + sys.modules["fpaste"] = fpaste + spec.loader.exec_module(fpaste) + return fpaste + + +class TestClipboardPathValidation: + """Tests for CLI-001: clipboard tool path validation.""" + + @pytest.fixture + def fpaste(self): + """Load fpaste module.""" + return load_fpaste_module() + + def test_trusted_unix_paths(self, fpaste): + """Paths in trusted Unix directories should be accepted.""" + assert fpaste.is_trusted_clipboard_path("/usr/bin/xclip") is True + assert fpaste.is_trusted_clipboard_path("/usr/local/bin/xsel") is True + assert fpaste.is_trusted_clipboard_path("/bin/cat") is True + assert fpaste.is_trusted_clipboard_path("/opt/homebrew/bin/pbcopy") is True + + def test_untrusted_unix_paths(self, fpaste): + """Paths in user-writable directories should be rejected.""" + assert fpaste.is_trusted_clipboard_path("/home/user/bin/xclip") is False + assert fpaste.is_trusted_clipboard_path("/tmp/xclip") is False + assert fpaste.is_trusted_clipboard_path("/var/tmp/malicious") is False + assert fpaste.is_trusted_clipboard_path("./xclip") is False + assert fpaste.is_trusted_clipboard_path("") is False + + def test_none_path_rejected(self, fpaste): + """None path should be rejected.""" + # is_trusted_clipboard_path should handle None gracefully + assert fpaste.is_trusted_clipboard_path(None) is False + + @pytest.mark.skipif(sys.platform != "win32", reason="Windows path tests only run on Windows") + def test_trusted_windows_paths(self, fpaste): + """Paths in trusted Windows directories should be accepted.""" + # Test Windows paths (case-insensitive) + assert fpaste.is_trusted_clipboard_path("C:\\Windows\\System32\\clip.exe") is True + assert fpaste.is_trusted_clipboard_path("C:\\WINDOWS\\SYSTEM32\\clip.exe") is True + assert fpaste.is_trusted_clipboard_path("C:\\Program Files\\tool.exe") is True + + def test_untrusted_windows_paths(self, fpaste): + """Paths in user-writable Windows directories should be rejected.""" + assert fpaste.is_trusted_clipboard_path("C:\\Users\\user\\xclip.exe") is False + assert fpaste.is_trusted_clipboard_path("C:\\Temp\\malicious.exe") is False + + def test_find_clipboard_command_rejects_untrusted(self, fpaste): + """find_clipboard_command should reject tools in untrusted paths.""" + with patch("shutil.which") as mock_which: + # Untrusted path should be rejected + mock_which.return_value = "/tmp/malicious/xclip" + result = fpaste.find_clipboard_command(fpaste.CLIPBOARD_READ_COMMANDS) + assert result is None + + def test_find_clipboard_command_accepts_trusted(self, fpaste): + """find_clipboard_command should accept tools in trusted paths.""" + with patch("shutil.which") as mock_which: + # Trusted path should be accepted + mock_which.return_value = "/usr/bin/xclip" + result = fpaste.find_clipboard_command(fpaste.CLIPBOARD_READ_COMMANDS) + assert result is not None + assert result[0] == "/usr/bin/xclip" + + def test_find_clipboard_uses_absolute_path(self, fpaste): + """find_clipboard_command should use absolute paths.""" + with patch("shutil.which") as mock_which: + mock_which.return_value = "/usr/bin/xclip" + result = fpaste.find_clipboard_command(fpaste.CLIPBOARD_READ_COMMANDS) + + # Should use absolute path, not just tool name + assert result[0] == "/usr/bin/xclip" + # Rest of command args should be preserved + assert "-selection" in result + assert "clipboard" in result diff --git a/tests/test_rate_limiting.py b/tests/test_rate_limiting.py index 4b1bb1c..eae9471 100644 --- a/tests/test_rate_limiting.py +++ b/tests/test_rate_limiting.py @@ -267,3 +267,75 @@ class TestRateLimitMaxEntries: finally: app.config["RATE_LIMIT_MAX_ENTRIES"] = original_max reset_rate_limits() + + +class TestRateLimitCleanupThreshold: + """Tests for RATE-002: automatic cleanup trigger when threshold exceeded.""" + + def test_cleanup_threshold_config_exists(self, app): + """RATE_LIMIT_CLEANUP_THRESHOLD config should exist.""" + assert "RATE_LIMIT_CLEANUP_THRESHOLD" in app.config + threshold = app.config["RATE_LIMIT_CLEANUP_THRESHOLD"] + assert isinstance(threshold, float) + assert 0.0 < threshold <= 1.0 + + def test_proactive_cleanup_at_threshold(self, app): + """Cleanup triggers proactively when threshold exceeded.""" + from app.api.routes import ( + _rate_limit_requests, + check_rate_limit, + reset_rate_limits, + ) + + reset_rate_limits() + + original_max = app.config.get("RATE_LIMIT_MAX_ENTRIES", 10000) + original_threshold = app.config.get("RATE_LIMIT_CLEANUP_THRESHOLD", 0.8) + + # Set max to 10, threshold to 0.5 (cleanup at 5 entries) + app.config["RATE_LIMIT_MAX_ENTRIES"] = 10 + app.config["RATE_LIMIT_CLEANUP_THRESHOLD"] = 0.5 + + try: + with app.app_context(): + # Add 6 entries (exceeds 50% threshold of 10) + for i in range(6): + check_rate_limit(f"10.0.0.{i}", authenticated=False) + + # Threshold cleanup should have triggered, reducing count + # Cleanup prunes to threshold/2 = 2-3 entries + assert len(_rate_limit_requests) < 6 + finally: + app.config["RATE_LIMIT_MAX_ENTRIES"] = original_max + app.config["RATE_LIMIT_CLEANUP_THRESHOLD"] = original_threshold + reset_rate_limits() + + def test_no_cleanup_below_threshold(self, app): + """No cleanup when below threshold.""" + from app.api.routes import ( + _rate_limit_requests, + check_rate_limit, + reset_rate_limits, + ) + + reset_rate_limits() + + original_max = app.config.get("RATE_LIMIT_MAX_ENTRIES", 10000) + original_threshold = app.config.get("RATE_LIMIT_CLEANUP_THRESHOLD", 0.8) + + # Set max to 20, threshold to 0.8 (cleanup at 16 entries) + app.config["RATE_LIMIT_MAX_ENTRIES"] = 20 + app.config["RATE_LIMIT_CLEANUP_THRESHOLD"] = 0.8 + + try: + with app.app_context(): + # Add 5 entries (below 80% threshold of 20 = 16) + for i in range(5): + check_rate_limit(f"10.0.0.{i}", authenticated=False) + + # All 5 entries should remain (no cleanup triggered) + assert len(_rate_limit_requests) == 5 + finally: + app.config["RATE_LIMIT_MAX_ENTRIES"] = original_max + app.config["RATE_LIMIT_CLEANUP_THRESHOLD"] = original_threshold + reset_rate_limits()