From f32612d8e7828c4e4419000ff2028121c1d35101 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 23 Mar 2026 02:16:39 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITICAL/H?= =?UTF-8?q?IGH]=20Fix=20SSRF=20vulnerability=20in=20network=20scanner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add semantic destination validation to the `is_reachable` utility function to prevent Server-Side Request Forgery (SSRF). Block loopback, link-local, multicast, and unspecified IP addresses to prevent the scanner from being manipulated into querying internal metadata services or local host resources. Update test suite to use standard private IP (192.168.1.1) instead of loopback (127.0.0.1) and add explicit tests for SSRF prevention. Append CRITICAL security learning to Sentinel journal. Co-authored-by: ManupaKDU <95234271+ManupaKDU@users.noreply.github.com> --- .jules/sentinel.md | 4 ++++ test_testping1.py | 16 +++++++++++++--- testping1.py | 6 ++++++ 3 files changed, 23 insertions(+), 3 deletions(-) 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)