fix(security): harden origin middleware against DNS rebinding attacks (#12860)#14085
Open
jiangyj545 wants to merge 1 commit into
Open
fix(security): harden origin middleware against DNS rebinding attacks (#12860)#14085jiangyj545 wants to merge 1 commit into
jiangyj545 wants to merge 1 commit into
Conversation
…Comfy-Org#12860) - Add CSRF token generation (per-process, sent as cookie + response header) - Require CSRF token on state-changing requests (POST/PUT/DELETE/PATCH) from loopback origins — prevents cross-site request forgery via DNS rebinding - Add double-DNS-resolution check (_dns_rebind_check): resolves the Host hostname twice with 200ms delay; rejects if addresses differ (rebind signal) or if any resolved IP is non-loopback - Preserve all existing Sec-Fetch-Site and Host/Origin mismatch checks CVE reference: huntr.com/bounties/f1458e43-64a7-4df2-b71c-9ca453755dc7
jiangyj545
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
📝 WalkthroughWalkthroughThis PR adds CSRF token and DNS rebinding defense mechanisms to the loopback-origin middleware in 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (1)
server.py (1)
167-169: 💤 Low valueDNS rebinding check only covers IPv4.
The function only resolves
AF_INETaddresses. An attacker could potentially use an IPv6-only DNS rebinding attack (rebinding to::1) which wouldn't be detected by this check.Consider adding
AF_INET6resolution to the check, or document why IPv6 is excluded.🤖 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 `@server.py` around lines 167 - 169, The DNS rebinding check currently calls socket.getaddrinfo(hostname, None, socket.AF_INET, ...) into r1/r2 which only resolves IPv4; update the check to also resolve IPv6 (AF_INET6) and include those results in the comparison (or call getaddrinfo with family=0 to return both families), then compare the combined address sets from the two lookups for changes; locate where r1, r2 and hostname are used in the function and ensure the comparison logic handles both IPv4 and IPv6 entries (normalize/address-only comparison) to detect rebinding to ::1 as well as 127.0.0.1.
🤖 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 `@server.py`:
- Around line 166-171: The DNS resolution and sleep are blocking: replace the
synchronous socket.getaddrinfo and time.sleep calls with non-blocking
equivalents by running getaddrinfo in an executor (e.g.,
asyncio.get_running_loop().run_in_executor or asyncio.to_thread) and use
asyncio.sleep(0.2) instead of time.sleep; ensure the surrounding function is
async (or the call site invokes an async helper) and preserve the
socket.gaierror handling and the logic producing r1 and r2 so the rest of the
code that uses r1/r2 remains unchanged.
---
Nitpick comments:
In `@server.py`:
- Around line 167-169: The DNS rebinding check currently calls
socket.getaddrinfo(hostname, None, socket.AF_INET, ...) into r1/r2 which only
resolves IPv4; update the check to also resolve IPv6 (AF_INET6) and include
those results in the comparison (or call getaddrinfo with family=0 to return
both families), then compare the combined address sets from the two lookups for
changes; locate where r1, r2 and hostname are used in the function and ensure
the comparison logic handles both IPv4 and IPv6 entries (normalize/address-only
comparison) to detect rebinding to ::1 as well as 127.0.0.1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+166
to
+171
| try: | ||
| r1 = socket.getaddrinfo(hostname, None, socket.AF_INET, socket.SOCK_STREAM) | ||
| time.sleep(0.2) | ||
| r2 = socket.getaddrinfo(hostname, None, socket.AF_INET, socket.SOCK_STREAM) | ||
| except socket.gaierror: | ||
| return False # unresolvable → reject |
There was a problem hiding this comment.
Blocking calls in async context will stall the event loop.
time.sleep(0.2) and socket.getaddrinfo() are synchronous blocking operations. When called from the async middleware, they block the entire aiohttp event loop for the duration, causing latency spikes and reducing throughput for all concurrent requests.
Run the blocking DNS work in a thread pool executor to avoid stalling the event loop:
Suggested fix using asyncio executor
+import asyncio
+
-def _dns_rebind_check(hostname):
+async def _dns_rebind_check(hostname):
"""Resolve *hostname* twice with a 200ms delay.
Returns True if both resolutions yield the **same** set of loopback addresses.
Returns False when the two results differ (classic DNS rebinding signal)
or when any result contains a non-loopback IP.
"""
+ loop = asyncio.get_running_loop()
+
+ def _resolve():
+ try:
+ r1 = socket.getaddrinfo(hostname, None, socket.AF_INET, socket.SOCK_STREAM)
+ time.sleep(0.2)
+ r2 = socket.getaddrinfo(hostname, None, socket.AF_INET, socket.SOCK_STREAM)
+ return r1, r2
+ except socket.gaierror:
+ return None, None
+
try:
- r1 = socket.getaddrinfo(hostname, None, socket.AF_INET, socket.SOCK_STREAM)
- time.sleep(0.2)
- r2 = socket.getaddrinfo(hostname, None, socket.AF_INET, socket.SOCK_STREAM)
- except socket.gaierror:
- return False # unresolvable → reject
+ r1, r2 = await loop.run_in_executor(None, _resolve)
+ if r1 is None:
+ return False # unresolvable → reject
+ except Exception:
+ return False
ips1 = sorted({addr[4][0] for addr in r1})
...Then update the call site at line 218:
- if not _dns_rebind_check(host_domain_parsed.hostname or host_domain):
+ if not await _dns_rebind_check(host_domain_parsed.hostname or host_domain):🤖 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 `@server.py` around lines 166 - 171, The DNS resolution and sleep are blocking:
replace the synchronous socket.getaddrinfo and time.sleep calls with
non-blocking equivalents by running getaddrinfo in an executor (e.g.,
asyncio.get_running_loop().run_in_executor or asyncio.to_thread) and use
asyncio.sleep(0.2) instead of time.sleep; ensure the surrounding function is
async (or the call site invokes an async helper) and preserve the
socket.gaierror handling and the logic producing r1 and r2 so the rest of the
code that uses r1/r2 remains unchanged.
Member
|
Can you tell me which browser is vulnerable to this? |
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
2 participants
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 #12860 — The
create_origin_only_middlewareinserver.pyis vulnerable to DNS rebinding attacks. An attacker-controlled domain that rebinds to127.0.0.1can bypass the existing Host/Origin check because both headers will contain the attacker's domain (which matches), while the actual TCP connection goes to localhost.Root Cause
The current middleware only checks that Host == Origin for loopback addresses. In a DNS rebinding scenario:
Reference: huntr.com/bounties/f1458e43-64a7-4df2-b71c-9ca453755dc7 (NCC Group PoC)
Changes
1. CSRF Token (per-process, auto-generated)
2. Double-DNS-Resolution Check (_dns_rebind_check)
3. CSRF Validation for State-Changing Requests
4. Token Delivery
Backward Compatibility
Testing Scenarios