forked from claw/flaskpaste
security: implement pentest remediation (RATE-002, CLI-001)
RATE-002: Proactive rate limit cleanup when entries exceed threshold - Add RATE_LIMIT_CLEANUP_THRESHOLD config (default 0.8) - Trigger cleanup before hitting hard limit - Prevents memory exhaustion under sustained load CLI-001: Validate clipboard tool paths against trusted directories - Add TRUSTED_CLIPBOARD_DIRS for Unix system paths - Add TRUSTED_WINDOWS_PATTERNS for Windows validation - Reject tools in user-writable locations (PATH hijack prevention) - Use absolute paths in subprocess calls
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.)
|
||||
|
||||
59
fpaste
59
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
|
||||
|
||||
|
||||
|
||||
96
tests/test_cli_security.py
Normal file
96
tests/test_cli_security.py
Normal file
@@ -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
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user