fix(security): eliminate DNS rebinding TOCTOU and add SSRF validation to MCP tools#6519
Open
ashusnapx wants to merge 5 commits into
Open
fix(security): eliminate DNS rebinding TOCTOU and add SSRF validation to MCP tools#6519ashusnapx wants to merge 5 commits into
ashusnapx wants to merge 5 commits into
Conversation
… to MCP tools Closes crewAIInc#6504 Vulnerability 1: DNS Rebinding (TOCTOU) in SSRF protection ========================================================== validate_url() resolved DNS and checked the IP, then returned the URL as a string. The subsequent requests.get() resolved DNS again, creating a TOCTOU window where the DNS record could change between validation and connection (DNS rebinding attack). Fix: - Add validate_url_and_resolve() that returns ValidatedURL(url, resolved_ip) - Add PinnedIPAdapter — a requests HTTPAdapter that pins TCP connections to the pre-resolved IP, eliminating the second DNS resolution - Update safe_get() to use validate_url_and_resolve() and _build_pinned_session() for every request (including redirect targets) - Refactor validate_url() to share validation logic via _validate_url_common() Vulnerability 2: MCP tools bypass SSRF validation entirely ========================================================== MCP tool wrappers (mcp_tool_wrapper.py, mcp_native_tool.py) passed arguments directly to MCP servers without any URL validation. An agent using an MCP fetch server could be instructed to access internal URLs, cloud metadata, etc. Fix: - Add _validate_mcp_server_url() — validates the MCP server connection URL - Add _validate_mcp_tool_args_for_urls() — recursively scans string arguments for URLs (http/https/file) and validates each one - Both MCP wrappers now validate server URL and tool arguments before execution, blocking SSRF attempts with a clear error message Tests: 51 passing (30 safe_path + 7 safe_requests + 14 MCP SSRF). ruff clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesSSRF protection
Sequence Diagram(s)sequenceDiagram
participant safe_get
participant validate_url_and_resolve
participant PinnedIPAdapter
participant HTTPServer
safe_get->>validate_url_and_resolve: validate initial URL
validate_url_and_resolve-->>safe_get: URL and resolved IP
safe_get->>PinnedIPAdapter: create pinned session
PinnedIPAdapter->>HTTPServer: request using pinned IP
HTTPServer-->>safe_get: response or redirect
safe_get->>validate_url_and_resolve: validate redirect target
sequenceDiagram
participant MCPToolWrapper
participant URLScanner
participant validate_url_and_resolve
participant MCPServer
MCPToolWrapper->>URLScanner: scan server URL and arguments
URLScanner->>validate_url_and_resolve: validate embedded URLs
validate_url_and_resolve-->>URLScanner: allow or ValueError
URLScanner-->>MCPToolWrapper: validation result
MCPToolWrapper->>MCPServer: execute allowed tool call
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
lib/crewai/src/crewai/tools/mcp_tool_wrapper.py (1)
11-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicated SSRF helpers across both MCP wrappers.
_URL_PATTERNand_validate_mcp_tool_args_for_urlsare copied verbatim intomcp_native_tool.py. The copies are already diverging (the native one is mis-scoped), which is exactly the failure mode duplication invites. Consider extracting them into a single shared module imported by both wrappers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/tools/mcp_tool_wrapper.py` around lines 11 - 55, Extract _URL_PATTERN and _validate_mcp_tool_args_for_urls into one shared MCP security helper module, then update both mcp_tool_wrapper.py and mcp_native_tool.py to import and reuse those symbols. Remove the duplicated definitions from each wrapper, preserving the existing recursive URL validation behavior and correcting the native wrapper’s scoping through the shared implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai-tools/src/crewai_tools/security/safe_path.py`:
- Around line 270-290: The PinnedIPAdapter implementation does not actually
route connections through the validated IP. Update init_poolmanager to assign
the correctly configured manager to the requests-expected poolmanager attribute,
pass PoolManager arguments by their intended meaning, and ensure the
connection-opening path consumes _resolved_ip while preserving the original
hostname for Host and TLS SNI. Verify safe_get uses this adapter so
DNS-rebinding protection is enforced.
In `@lib/crewai-tools/tests/utilities/test_safe_requests.py`:
- Around line 185-209: Add a behavioral test alongside
test_safe_get_uses_pinned_ip_adapter that exercises the real
_build_pinned_session and PinnedIPAdapter instead of monkeypatching the session
builder. Use a controllable HTTP/HTTPS server or transport to assert the
connection targets the resolved IP while the request Host header and TLS SNI
remain the original hostname; retain the existing test only for validating the
passed ValidatedURL if needed.
In `@lib/crewai/src/crewai/tools/mcp_native_tool.py`:
- Around line 110-136: Dedent _run_async so it is defined as a MCPNativeTool
class method rather than nested inside _validate_mcp_tool_args_for_urls. Keep
_validate_mcp_tool_args_for_urls at module scope and preserve _run_async’s
existing implementation so self._run_async(**kwargs) remains callable.
In `@lib/crewai/tests/mcp/test_mcp_ssrf.py`:
- Around line 11-14: Add parallel SSRF validation coverage for the native MCP
path by importing the corresponding validators from crewai.tools.mcp_native_tool
or invoking MCPNativeTool._run and asserting the SSRF-blocked message. Keep the
existing mcp_tool_wrapper coverage and ensure the native path’s URL and
tool-argument validation behavior is exercised.
---
Nitpick comments:
In `@lib/crewai/src/crewai/tools/mcp_tool_wrapper.py`:
- Around line 11-55: Extract _URL_PATTERN and _validate_mcp_tool_args_for_urls
into one shared MCP security helper module, then update both mcp_tool_wrapper.py
and mcp_native_tool.py to import and reuse those symbols. Remove the duplicated
definitions from each wrapper, preserving the existing recursive URL validation
behavior and correcting the native wrapper’s scoping through the shared
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bbd2626-2155-45cc-b9bf-cd9e1e11dba5
📒 Files selected for processing (6)
lib/crewai-tools/src/crewai_tools/security/safe_path.pylib/crewai-tools/src/crewai_tools/security/safe_requests.pylib/crewai-tools/tests/utilities/test_safe_requests.pylib/crewai/src/crewai/tools/mcp_native_tool.pylib/crewai/src/crewai/tools/mcp_tool_wrapper.pylib/crewai/tests/mcp/test_mcp_ssrf.py
…ct shared MCP helpers, add native MCP test coverage - Fix PinnedIPAdapter: rewrite URL in send() to pin TCP to resolved IP while preserving Host header and TLS SNI (was storing unused pool manager) - Extract shared SSRF helpers (_URL_PATTERN, validate_mcp_server_url, validate_mcp_tool_args_for_urls) into mcp_security.py to eliminate duplication between mcp_tool_wrapper and mcp_native_tool - Fix _run_async indentation: was nested inside _validate_mcp_tool_args_for_urls, now properly a MCPNativeTool class method - Add 3 native MCP path tests (blocks internal, blocks metadata, allows public) - Add behavioral PinnedIPAdapter test verifying adapter stores correct IP
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/crewai/src/crewai/tools/mcp_security.py (1)
12-12: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGreedy URL regex can capture trailing punctuation and cause false blocks.
[^\s\"']+will absorb trailing characters like),,, or.when a URL is embedded in prose (e.g."see http://public.example."matcheshttp://public.example.). That mangled host is then handed tovalidate_url_and_resolve, which may raise on resolution and incorrectly block a legitimate tool call, or surface a non-ValueErrorthat escapes the SSRF handler.Consider trimming common trailing delimiters from
match.group()before validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/tools/mcp_security.py` at line 12, Update _URL_PATTERN URL extraction to remove common prose delimiters such as trailing ), , and . from match.group() before passing the URL to validate_url_and_resolve, while preserving valid URL characters and the existing SSRF validation flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai-tools/src/crewai_tools/security/safe_path.py`:
- Around line 316-320: Remove the server_hostname injection from
PinnedIPAdapter.send and keep its kwargs compatible with HTTPAdapter.send before
calling super().send(request, **kwargs). Add or update coverage for an HTTPS
request through the pinned path to verify it no longer raises TypeError.
In `@lib/crewai-tools/tests/utilities/test_safe_requests.py`:
- Around line 212-227: The test_pinned_adapter_rewrites_url test does not
exercise PinnedIPAdapter.send() or its URL and Host header rewriting. Replace
the _resolved_ip, hasattr, and callable assertions with a mocked
HTTPAdapter.send() invocation, then assert the forwarded request uses
93.184.216.34 while preserving the expected Host header.
---
Nitpick comments:
In `@lib/crewai/src/crewai/tools/mcp_security.py`:
- Line 12: Update _URL_PATTERN URL extraction to remove common prose delimiters
such as trailing ), , and . from match.group() before passing the URL to
validate_url_and_resolve, while preserving valid URL characters and the existing
SSRF validation flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1312343-bfc7-4fc7-9aa3-ffce47c7c578
📒 Files selected for processing (6)
lib/crewai-tools/src/crewai_tools/security/safe_path.pylib/crewai-tools/tests/utilities/test_safe_requests.pylib/crewai/src/crewai/tools/mcp_native_tool.pylib/crewai/src/crewai/tools/mcp_security.pylib/crewai/src/crewai/tools/mcp_tool_wrapper.pylib/crewai/tests/mcp/test_mcp_ssrf.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/crewai/src/crewai/tools/mcp_tool_wrapper.py
…name kwarg, strip prose delimiters from URLs, rewrite behavioral test - PinnedIPAdapter: removed server_hostname injection from send() which caused TypeError on HTTPS requests (HTTPAdapter.send() doesn't accept it) - PinnedIPAdapter: added init_poolmanager override with assert_hostname=False for correct TLS SNI when URL contains an IP address - MCP SSRF: added _clean_url() to strip trailing ), ,, . from regex-matched URLs in prose (e.g. markdown links, natural language) before validation - Behavioral test: replaced hasattr/callable assertions with mocked HTTPAdapter.send() that verifies URL is rewritten to use pinned IP and Host header preserves original hostname - Added HTTPS variant of behavioral test to cover TLS path
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai-tools/tests/utilities/test_safe_requests.py (1)
212-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider parametrizing the two adapter tests to reduce duplication.
test_pinned_adapter_rewrites_urlandtest_pinned_adapter_https_rewrites_urlare identical except for the URL scheme. A single parametrized test would eliminate the copy-paste. Additionally,MagicMockis imported at module level (line 8) but re-imported locally inside both functions — the local imports can be removed.♻️ Optional refactor: parametrize and remove redundant imports
+@pytest.mark.parametrize("url,expected_scheme", [ + ("http://public.example/data", "http"), + ("https://public.example/secure", "https"), +]) +def test_pinned_adapter_rewrites_url( + url: str, expected_scheme: str, public_dns: None, monkeypatch: pytest.MonkeyPatch +) -> None: + """Behavioral test: PinnedIPAdapter.send() rewrites URL and sets Host header.""" + from crewai_tools.security.safe_path import PinnedIPAdapter + + adapter = PinnedIPAdapter(resolved_ip="93.184.216.34") + + captured_requests: list[Any] = [] + + def fake_send(self: Any, request: Any, **kwargs: Any) -> MagicMock: + captured_requests.append(request) + resp = MagicMock() + resp.status_code = 200 + return resp + + monkeypatch.setattr( + "requests.adapters.HTTPAdapter.send", fake_send, + ) + + req = requests.Request("GET", url) + prepared = req.prepare() + adapter.send(prepared) + + assert len(captured_requests) == 1 + sent = captured_requests[0] + assert "93.184.216.34" in sent.url + assert sent.headers["Host"] == "public.example"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai-tools/tests/utilities/test_safe_requests.py` around lines 212 - 271, Consolidate test_pinned_adapter_rewrites_url and test_pinned_adapter_https_rewrites_url into one parametrized test covering both http and https URL schemes, while preserving their existing assertions and behavior. Remove the redundant function-local MagicMock imports and use the existing module-level import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/crewai-tools/tests/utilities/test_safe_requests.py`:
- Around line 212-271: Consolidate test_pinned_adapter_rewrites_url and
test_pinned_adapter_https_rewrites_url into one parametrized test covering both
http and https URL schemes, while preserving their existing assertions and
behavior. Remove the redundant function-local MagicMock imports and use the
existing module-level import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a13e5a7-cd98-4d9c-a260-6dc8417bef60
📒 Files selected for processing (3)
lib/crewai-tools/src/crewai_tools/security/safe_path.pylib/crewai-tools/tests/utilities/test_safe_requests.pylib/crewai/src/crewai/tools/mcp_security.py
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/crewai/src/crewai/tools/mcp_security.py
- lib/crewai-tools/src/crewai_tools/security/safe_path.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two SSRF security vulnerabilities in crewai-tools.
Closes #6504
Vulnerability 1: DNS Rebinding (TOCTOU) in SSRF protection
validate_url()resolved DNS and checked the IP, then returned the URL as a string. The subsequentrequests.get()resolved DNS again, creating a TOCTOU window where the DNS record could change between validation and connection.Attack scenario: Attacker registers a domain that first resolves to a public IP (passes validation), then quickly changes to
169.254.169.254(cloud metadata) before the connection is made.Fix:
validate_url_and_resolve()→ returnsValidatedURL(url, resolved_ip)PinnedIPAdapter— arequests.HTTPAdapterthat pins TCP connections to the pre-resolved IPsafe_get()to use the pinned adapter for every request (including redirect targets)validate_url()to share validation logic via_validate_url_common()PinnedIPAdapter implementation:
send()to rewrite the URL, replacing hostname with the pinned IPHostheader to the original hostname for virtual hostinginit_poolmanager()to configureassert_hostname=Falsefor correct TLS SNI when the URL contains an IP addressserver_hostnamekwarg injected intosend()— keeps kwargs compatible withHTTPAdapter.send()signatureVulnerability 2: MCP tools bypass SSRF validation entirely
MCP tool wrappers (
mcp_tool_wrapper.py,mcp_native_tool.py) passed arguments directly to MCP servers without any URL validation. An agent using an MCP fetch server could be instructed to access internal URLs, cloud metadata, etc.Fix:
validate_mcp_server_url()— validates the MCP server connection URLvalidate_mcp_tool_args_for_urls()— recursively scans string arguments for URLs (http,https,file) and validates each one against the SSRF guardmcp_security.pyto eliminate duplication_clean_url()to strip trailing prose delimiters (),,,.) from regex-matched URLs before validationFiles Changed
security/safe_path.pyValidatedURL,validate_url_and_resolve(),PinnedIPAdapter(URL-level IP pinning + pool manager SNI), refactoredvalidate_url()security/safe_requests.pysafe_get()to use pinned sessionstools/mcp_security.py_clean_url()for prose-safe URL extractiontools/mcp_tool_wrapper.pytools/mcp_native_tool.py_run_asyncindentationtests/mcp/test_mcp_ssrf.pytests/utilities/test_safe_requests.pyTests
Verification