From df2cfe5bb65d84a2f365236a865c4847f01e0493 Mon Sep 17 00:00:00 2001 From: Aditya Chhabra Date: Mon, 10 Nov 2025 19:42:17 +0530 Subject: [PATCH 1/2] fix: add SSRF protection to HTTP request tool with IP blocking and response limits --- src/cleveragents/agents/tool.py | 88 +++++++++++++++++++++++++++------ tests/unit/agents/test_tool.py | 28 ++++++++++- 2 files changed, 99 insertions(+), 17 deletions(-) diff --git a/src/cleveragents/agents/tool.py b/src/cleveragents/agents/tool.py index ac56c6ef9..04a5cea25 100644 --- a/src/cleveragents/agents/tool.py +++ b/src/cleveragents/agents/tool.py @@ -6,14 +6,18 @@ capabilities within the RxPy-based reactive architecture. """ import asyncio +import ipaddress import json import logging import os import re +import socket import subprocess from typing import Any from typing import List from typing import Optional, Union, Literal +from urllib.parse import urlparse +import ast import aiohttp @@ -22,6 +26,7 @@ from cleveragents.core.exceptions import AgentCreationError from cleveragents.core.exceptions import ExecutionError from cleveragents.templates.renderer import TemplateRenderer + logger = logging.getLogger(__name__) @@ -290,7 +295,7 @@ class ToolAgent(Agent): "float": float, "__builtins__": {}, } - result = eval(expression, allowed_names) # pylint: disable=eval-used + result = ast.literal_eval(expression, allowed_names) # pylint: disable=eval-used return str(result) except Exception as e: raise ExecutionError(f"Math evaluation failed: {e}") from e @@ -309,10 +314,53 @@ class ToolAgent(Agent): except json.JSONDecodeError as e: raise ExecutionError(f"JSON parsing failed: {e}") from e + def _validate_url_safety(self, url: str) -> None: + """Validate URL to prevent SSRF attacks.""" + try: + parsed = urlparse(url) + except Exception as e: + raise ExecutionError(f"Invalid URL: {e}") from e + + if not parsed.scheme or not parsed.hostname: + raise ExecutionError("Invalid URL format") + + if parsed.scheme not in ['http', 'https']: + raise ExecutionError(f"Protocol '{parsed.scheme}' not allowed") + + try: + ip_addresses = socket.getaddrinfo(parsed.hostname, None, socket.AF_UNSPEC, socket.SOCK_STREAM) + except socket.gaierror as e: + raise ExecutionError(f"Cannot resolve hostname '{parsed.hostname}': {e}") from e + + blocked_ranges = [ + ipaddress.ip_network('127.0.0.0/8'), + ipaddress.ip_network('10.0.0.0/8'), + ipaddress.ip_network('172.16.0.0/12'), + ipaddress.ip_network('192.168.0.0/16'), + ipaddress.ip_network('169.254.0.0/16'), + ipaddress.ip_network('::1/128'), + ipaddress.ip_network('fc00::/7'), + ipaddress.ip_network('fe80::/10'), + ] + + for addr_info in ip_addresses: + ip_str = addr_info[4][0] + try: + ip_obj = ipaddress.ip_address(ip_str) + except ValueError: + continue + + for blocked_range in blocked_ranges: + if ip_obj in blocked_range: + raise ExecutionError( + f"Access to {parsed.hostname} ({ip_str}) blocked: " + f"resolves to restricted IP range {blocked_range}" + ) + async def _http_request_tool( self, args: dict[str, Any], context: Optional[dict[str, Any]] # pylint: disable=unused-argument ) -> str: - """HTTP request tool.""" + """HTTP request tool with SSRF protection.""" url = args.get("url", "") method = args.get("method", "GET").upper() headers = args.get("headers", {}) @@ -321,20 +369,34 @@ class ToolAgent(Agent): if not url: raise ExecutionError("HTTP tool requires a URL") + if method not in ['GET', 'POST', 'PUT', 'DELETE', 'PATCH', 'HEAD']: + raise ExecutionError(f"HTTP method '{method}' not allowed") + + self._validate_url_safety(url) + + max_size = 10 * 1024 * 1024 try: - async with aiohttp.ClientSession() as session: + connector = aiohttp.TCPConnector(limit=10) + timeout = aiohttp.ClientTimeout(total=self.timeout, connect=5) + async with aiohttp.ClientSession(connector=connector, timeout=timeout) as session: async with session.request( - method, url, headers=headers, json=data, timeout=self.timeout + method, url, headers=headers, json=data, allow_redirects=False ) as response: content = await response.text() + if len(content) > max_size: + raise ExecutionError(f"Response too large: {len(content)} bytes (max: {max_size})") return f"Status: {response.status}\n\n{content}" + except aiohttp.ClientError as e: + raise ExecutionError(f"HTTP request failed: {e}") from e + except asyncio.TimeoutError as e: + raise ExecutionError(f"HTTP request timed out") from e except Exception as e: raise ExecutionError(f"HTTP request failed: {e}") from e async def _file_read_tool( self, args: dict[str, Any], context: Optional[dict[str, Any]] ) -> str: - """File reading tool.""" + """File reading tool with comprehensive path validation.""" filepath = args.get("file", "") if "args" in args and args["args"]: filepath = args["args"][0] @@ -342,15 +404,9 @@ class ToolAgent(Agent): if not filepath: raise ExecutionError("File read tool requires a file path") - if self.safe_mode: - # Always block directory traversal attempts - if ".." in filepath: - raise ExecutionError("Unsafe file path blocked in safe mode") - # Block absolute paths unless in unsafe mode - if filepath.startswith("/") and not ( - context and context.get("_unsafe_mode", False) - ): - raise ExecutionError("Unsafe file path blocked in safe mode") + # Use comprehensive validation instead of simple checks + unsafe_mode = context and context.get("_unsafe_mode", False) + self._validate_file_path_safety(filepath, unsafe_mode) try: with open(filepath, "r", encoding="utf-8") as f: @@ -363,7 +419,7 @@ class ToolAgent(Agent): # Format: Special marker + metadata + full content # The special marker helps identify this for clean display return ( - f"[FILE_READ_SUCCESS]📄 File: {filepath} | " + f"[FILE_READ_SUCCESS]:page_facing_up: File: {filepath} | " f"Lines: {line_count} | Size: {char_count} chars\n" f"[FILE_CONTENT_START]\n{content}\n[FILE_CONTENT_END]" ) @@ -612,4 +668,4 @@ class ToolAgent(Agent): "timeout": self.timeout, } ) - return metadata + return metadata \ No newline at end of file diff --git a/tests/unit/agents/test_tool.py b/tests/unit/agents/test_tool.py index 9c944a16d..e97830167 100644 --- a/tests/unit/agents/test_tool.py +++ b/tests/unit/agents/test_tool.py @@ -257,7 +257,13 @@ class TestToolAgent: async def __aexit__(self, *args): return None - with patch('aiohttp.ClientSession', return_value=AsyncSessionMock()): + # Mock socket.getaddrinfo to return a public IP + mock_getaddrinfo = Mock(return_value=[ + (2, 1, 6, '', ('93.184.216.34', 0)) + ]) + + with patch('aiohttp.ClientSession', return_value=AsyncSessionMock()), \ + patch('socket.getaddrinfo', mock_getaddrinfo): result = await agent._http_request_tool({"url": "http://example.com"}, None) assert "Status: 200" in result @@ -274,6 +280,26 @@ class TestToolAgent: assert "requires a URL" in str(exc_info.value) + @pytest.mark.asyncio + async def test_http_request_tool_blocks_ssrf(self, template_renderer): + """Test HTTP request tool blocks SSRF attacks.""" + config = {"type": "tool", "tools": ["http_request"]} + agent = ToolAgent("test_tool", config, template_renderer) + + ssrf_urls = [ + ("http://localhost/admin", [("127.0.0.1", 0)]), + ("http://169.254.169.254/latest/meta-data/", [("169.254.169.254", 0)]), + ("http://192.168.1.1/config", [("192.168.1.1", 0)]), + ("http://10.0.0.1/internal", [("10.0.0.1", 0)]), + ] + + for url, mock_ip in ssrf_urls: + mock_getaddrinfo = Mock(return_value=[(2, 1, 6, '', mock_ip)]) + with patch('socket.getaddrinfo', mock_getaddrinfo): + with pytest.raises(ExecutionError) as exc_info: + await agent._http_request_tool({"url": url}, None) + assert "blocked" in str(exc_info.value).lower() + @pytest.mark.asyncio async def test_file_read_tool(self, template_renderer): """Test file read tool.""" -- 2.52.0 From e62030066088e40c0be430e9fb13773e330422dd Mon Sep 17 00:00:00 2001 From: Aditya Chhabra Date: Mon, 17 Nov 2025 19:18:56 +0530 Subject: [PATCH 2/2] fix: set builtins to an empty dict to prevent dangerous access; fix linting issues --- src/cleveragents/agents/tool.py | 24 +++++++++++++++++------- tests/unit/agents/test_tool.py | 8 ++++---- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/cleveragents/agents/tool.py b/src/cleveragents/agents/tool.py index 04a5cea25..46bdc40fe 100644 --- a/src/cleveragents/agents/tool.py +++ b/src/cleveragents/agents/tool.py @@ -17,7 +17,6 @@ from typing import Any from typing import List from typing import Optional, Union, Literal from urllib.parse import urlparse -import ast import aiohttp @@ -295,7 +294,13 @@ class ToolAgent(Agent): "float": float, "__builtins__": {}, } - result = ast.literal_eval(expression, allowed_names) # pylint: disable=eval-used + # setting builtins to an empty dict prevents access to + # dangerous functions like __import__ and allows only + # the allowed functions to be used + result = eval( # pylint: disable=eval-used + expression, {"__builtins__": {}}, allowed_names + ) + return str(result) except Exception as e: raise ExecutionError(f"Math evaluation failed: {e}") from e @@ -328,7 +333,9 @@ class ToolAgent(Agent): raise ExecutionError(f"Protocol '{parsed.scheme}' not allowed") try: - ip_addresses = socket.getaddrinfo(parsed.hostname, None, socket.AF_UNSPEC, socket.SOCK_STREAM) + ip_addresses = socket.getaddrinfo( + parsed.hostname, None, socket.AF_UNSPEC, socket.SOCK_STREAM + ) except socket.gaierror as e: raise ExecutionError(f"Cannot resolve hostname '{parsed.hostname}': {e}") from e @@ -384,12 +391,15 @@ class ToolAgent(Agent): ) as response: content = await response.text() if len(content) > max_size: - raise ExecutionError(f"Response too large: {len(content)} bytes (max: {max_size})") + raise ExecutionError( + f"Response too large: {len(content)} bytes " + f"(max: {max_size})" + ) return f"Status: {response.status}\n\n{content}" except aiohttp.ClientError as e: raise ExecutionError(f"HTTP request failed: {e}") from e except asyncio.TimeoutError as e: - raise ExecutionError(f"HTTP request timed out") from e + raise ExecutionError("HTTP request timed out") from e except Exception as e: raise ExecutionError(f"HTTP request failed: {e}") from e @@ -405,7 +415,7 @@ class ToolAgent(Agent): raise ExecutionError("File read tool requires a file path") # Use comprehensive validation instead of simple checks - unsafe_mode = context and context.get("_unsafe_mode", False) + unsafe_mode = bool(context and context.get("_unsafe_mode", False)) self._validate_file_path_safety(filepath, unsafe_mode) try: @@ -593,7 +603,7 @@ class ToolAgent(Agent): # Validate inputs self._validate_file_write_args(filepath, content) # Check unsafe mode requirement - unsafe_mode = context and context.get("_unsafe_mode", False) + unsafe_mode = bool(context and context.get("_unsafe_mode", False)) if not unsafe_mode: logger.error("File writing requires unsafe mode") raise ExecutionError("File writing requires unsafe mode") diff --git a/tests/unit/agents/test_tool.py b/tests/unit/agents/test_tool.py index e97830167..167fe5211 100644 --- a/tests/unit/agents/test_tool.py +++ b/tests/unit/agents/test_tool.py @@ -287,10 +287,10 @@ class TestToolAgent: agent = ToolAgent("test_tool", config, template_renderer) ssrf_urls = [ - ("http://localhost/admin", [("127.0.0.1", 0)]), - ("http://169.254.169.254/latest/meta-data/", [("169.254.169.254", 0)]), - ("http://192.168.1.1/config", [("192.168.1.1", 0)]), - ("http://10.0.0.1/internal", [("10.0.0.1", 0)]), + ("http://localhost/admin", ("127.0.0.1", 0)), + ("http://169.254.169.254/latest/meta-data/", ("169.254.169.254", 0)), + ("http://192.168.1.1/config", ("192.168.1.1", 0)), + ("http://10.0.0.1/internal", ("10.0.0.1", 0)), ] for url, mock_ip in ssrf_urls: -- 2.52.0