diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 73e8945..41ade79 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -6,3 +6,7 @@ **Vulnerability:** Unhandled exceptions (e.g., `OSError` from missing `ping` binary) in `subprocess.call` within `ThreadPoolExecutor` workers bubbled up, crashing the aggregator process and leaking internal system details/stack traces. Additionally, a lack of timeout limits posed a Denial of Service (DoS) risk through resource exhaustion, tying up worker threads. **Learning:** `ThreadPoolExecutor` does not inherently catch unhandled exceptions in its worker tasks; calling `.result()` or iterating over `as_completed()` will re-raise them. External subprocess calls are a high-risk vector for unpredictable `OSErrors`. **Prevention:** Always wrap external system calls (`subprocess.call`, file operations, network requests) inside worker functions with secure `try...except` blocks. Log generic errors to avoid leaking stack traces, and implement upper bounds on input values that govern resource allocation (e.g., thread duration via timeout limits). +## 2024-05-23 - [Server-Side Request Forgery Prevention in Utils] +**Vulnerability:** The `is_reachable` utility function validated if an IP was correctly formatted, but did not restrict the semantic destination. This allowed Server-Side Request Forgery (SSRF) where the scanner could be manipulated into pinging loopback (`127.0.0.1`), link-local (e.g., AWS metadata `169.254.169.254`), or multicast addresses. +**Learning:** Input validation is not just about syntax (like `ipaddress.ip_address`), but also about semantics and intended use. Even a simple `ping` utility can become an SSRF vector if it allows querying internal or restricted network ranges. +**Prevention:** Always implement explicit allow-lists or block-lists for network destinations based on business logic. Use built-in library properties (like `ip_obj.is_loopback`, `ip_obj.is_link_local`) to robustly filter out non-routable or restricted administrative IP ranges before attempting network requests. diff --git a/test_testping1.py b/test_testping1.py index 9cff379..2557596 100644 --- a/test_testping1.py +++ b/test_testping1.py @@ -11,7 +11,7 @@ def test_is_reachable_success(self, mock_call): # Simulate a successful ping response by returning 0 mock_call.return_value = 0 - self.assertTrue(is_reachable('127.0.0.1')) + self.assertTrue(is_reachable('192.168.1.1')) @patch('testping1.subprocess.call') def test_is_reachable_failure(self, mock_call): @@ -65,7 +65,7 @@ def test_is_reachable_secure_error_handling(self, mock_call): """Test is_reachable handles OSError securely without leaking exceptions.""" mock_call.side_effect = FileNotFoundError("No such file or directory: 'ping'") with self.assertLogs(level='ERROR') as log: - self.assertFalse(is_reachable('127.0.0.1')) + self.assertFalse(is_reachable('192.168.1.1')) self.assertIn("Failed to execute ping command safely.", log.output[0]) self.assertNotIn("FileNotFoundError", log.output[0]) @@ -91,10 +91,20 @@ def test_is_reachable_subprocess_timeout(self, mock_call): """Test is_reachable handles subprocess.TimeoutExpired securely.""" mock_call.side_effect = subprocess.TimeoutExpired(cmd='ping', timeout=7) with self.assertLogs(level='ERROR') as log: - self.assertFalse(is_reachable('127.0.0.1', timeout=5)) + self.assertFalse(is_reachable('192.168.1.1', timeout=5)) self.assertIn("Ping command timed out unexpectedly.", log.output[0]) self.assertNotIn("TimeoutExpired", log.output[0]) + @patch('testping1.subprocess.call') + def test_is_reachable_ssrf_prevention(self, mock_call): + """Test is_reachable prevents SSRF by rejecting loopback, multicast, etc.""" + ssrf_ips = ['127.0.0.1', '169.254.169.254', '224.0.0.1', '0.0.0.0'] + for ip in ssrf_ips: + with self.assertLogs(level='ERROR') as log: + self.assertFalse(is_reachable(ip)) + self.assertIn("IP address not allowed for scanning", log.output[0]) + mock_call.assert_not_called() + @patch('testping1.subprocess.call') def test_is_reachable_calls_ping_correctly(self, mock_call): """Test is_reachable calls the ping command with correct arguments.""" diff --git a/testping1.py b/testping1.py index 12f7c75..eda9b2a 100644 --- a/testping1.py +++ b/testping1.py @@ -29,6 +29,12 @@ def is_reachable(ip, timeout=1): logging.error(f"Invalid IP address format: {repr(ip)}") return False + # 🛡️ Sentinel: Prevent Server-Side Request Forgery (SSRF) + # Block loopback, link-local, multicast, and unspecified addresses from being pinged. + if ip_obj.is_loopback or ip_obj.is_link_local or ip_obj.is_multicast or ip_obj.is_unspecified: + logging.error(f"IP address not allowed for scanning: {ip}") + return False + # 🛡️ Sentinel: Validate timeout to prevent argument injection or errors try: timeout_val = int(timeout)