diff --git a/.jules/sentinel.md b/.jules/sentinel.md index fa35a57f..4ad0579f 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -1,8 +1,4 @@ -## 2024-05-24 - SSRF in WebDAV connection test -**Vulnerability:** The `_test_webdav_connection` function had a custom SSRF check that failed to resolve DNS names, allowing attackers to bypass the check by providing a domain that resolves to an internal IP (e.g., `127.0.0.1`). -**Learning:** DNS resolution is required for robust SSRF protection when validating URLs provided by users. -**Prevention:** Use a centralized `is_private_ip` function (now in `app/utils/network.py`) that resolves the hostname to its IPs and checks if any are private. -## 2026-03-22 - B310: urllib.request.urlopen replaced with httpx -**Vulnerability:** The `_test_webdav_connection` function used `urllib.request.urlopen`, which natively supports dangerous schemes like `file://` or `ftp://` and follows redirects by default, potentially allowing SSRF bypasses or Local File Inclusion. -**Learning:** `urllib.request` should be avoided for user-supplied URLs. Even when URL schemes are manually validated, `urllib`'s default redirect following behavior can bypass SSRF protections (e.g. redirecting to `127.0.0.1`). -**Prevention:** Use a modern, safer HTTP client like `httpx` with `follow_redirects=False` when testing user-provided URLs. +## 2025-05-18 - [SSRF Bypass via DNS Resolution Failure] +**Vulnerability:** The `is_private_ip` function in `app/utils/network.py` failed open (returned `False`) when a hostname could not be resolved (`socket.gaierror`). +**Learning:** This fail-open pattern was originally added to allow external domains in tests, but in production, it created a severe SSRF risk. An attacker could bypass SSRF protections by providing a URL that fails to resolve during the security check but resolves later (DNS rebinding), or by exploiting internal routing behaviors via unresolvable addresses. +**Prevention:** Always fail securely in network authorization functions. If a domain cannot be resolved to verify its safety, the request must be blocked (`return True` / default-deny). Tests should mock DNS resolution correctly instead of compromising production security logic. \ No newline at end of file diff --git a/app/utils/network.py b/app/utils/network.py index 7ea6d88b..ef775ba0 100644 --- a/app/utils/network.py +++ b/app/utils/network.py @@ -27,8 +27,8 @@ def is_private_ip(hostname: str) -> bool: return True return False except (socket.gaierror, socket.error): - # Cannot resolve - allow for testing/development - # In production, DNS should work properly - # Log this for debugging - logger.warning(f"Could not resolve hostname: {hostname}") - return False # Changed from True to False to allow external domains in tests + # Cannot resolve. + # Fail securely: block unresolved domains to prevent DNS rebinding + # and SSRF bypasses via unresolvable addresses. + logger.warning(f"Could not resolve hostname (blocking securely): {hostname}") + return True diff --git a/tests/test_coverage_polish.py b/tests/test_coverage_polish.py index c5b7c4de..1f196ba3 100644 --- a/tests/test_coverage_polish.py +++ b/tests/test_coverage_polish.py @@ -520,14 +520,14 @@ class TestURLUploadAdditionalCoverage: assert exc_info.value.status_code == 400 def test_is_private_ip_unresolvable_hostname(self): - """Cover DNS resolution failure branch (lines 67-72).""" + """Cover DNS resolution failure branch blocking unresolvable domains.""" import socket as _socket from app.utils.network import is_private_ip with patch("socket.getaddrinfo", side_effect=_socket.gaierror("nope")): result = is_private_ip("nonexistent.invalid.hostname.test") - assert result is False + assert result is True # Fail securely by returning True def test_is_private_ip_hostname_resolves_to_private(self): """Cover branch where hostname resolves to a private IP (line 64-65).""" diff --git a/tests/test_url_upload.py b/tests/test_url_upload.py index 7fead0ae..5dff00ac 100644 --- a/tests/test_url_upload.py +++ b/tests/test_url_upload.py @@ -698,6 +698,18 @@ class TestURLUploadCoverageGaps: assert result is False mock_getaddrinfo.assert_called_once() + @patch("app.utils.network.socket.getaddrinfo") + def test_is_private_ip_unresolvable_hostname_fails_securely(self, mock_getaddrinfo): + """Test that unresolvable hostnames fail securely by blocking access.""" + import socket + + from app.utils.network import is_private_ip + + mock_getaddrinfo.side_effect = socket.gaierror("Name or service not known") + + result = is_private_ip("unresolvable.example.internal") + assert result is True # Fails securely + @patch("socket.getaddrinfo") def test_is_private_ip_hostname_resolves_multiple_ips_all_public(self, mock_getaddrinfo): """Test hostname with multiple public IPs returns False (covers 65->61 loop branch)"""