mirror of
https://github.com/comfyanonymous/ComfyUI.git
synced 2026-07-03 21:20:49 +08:00
revert(security): drop CVE-2026-56674 Origin: null CSRF change
Per maintainer review, the reported CSRF is already mitigated by the pre-existing Sec-Fetch-Site: cross-site check for current browsers, and the null-origin rejection risked breaking legitimate sandboxed-iframe embeds. Restores origin_only_middleware and is_loopback in server.py to their prior state (the Sec-Fetch-Site check is retained) and removes utils/origin_check.py and its regression test. The other four GHSA-779p fixes are unaffected.
This commit is contained in:
parent
5611d1c9b6
commit
c68789b0fb
54
server.py
54
server.py
@ -23,6 +23,8 @@ import json
|
|||||||
import glob
|
import glob
|
||||||
import struct
|
import struct
|
||||||
import ssl
|
import ssl
|
||||||
|
import socket
|
||||||
|
import ipaddress
|
||||||
from PIL import Image, ImageOps
|
from PIL import Image, ImageOps
|
||||||
from PIL.PngImagePlugin import PngInfo
|
from PIL.PngImagePlugin import PngInfo
|
||||||
from io import BytesIO
|
from io import BytesIO
|
||||||
@ -38,7 +40,6 @@ import comfy.utils
|
|||||||
import comfy.model_management
|
import comfy.model_management
|
||||||
from comfy_api import feature_flags
|
from comfy_api import feature_flags
|
||||||
import node_helpers
|
import node_helpers
|
||||||
from utils.origin_check import is_cross_origin_forbidden
|
|
||||||
from comfyui_version import __version__
|
from comfyui_version import __version__
|
||||||
from app.frontend_management import FrontendManager, parse_version
|
from app.frontend_management import FrontendManager, parse_version
|
||||||
from comfy_api.internal import _ComfyNodeInternal
|
from comfy_api.internal import _ComfyNodeInternal
|
||||||
@ -126,6 +127,33 @@ def create_cors_middleware(allowed_origin: str):
|
|||||||
|
|
||||||
return cors_middleware
|
return cors_middleware
|
||||||
|
|
||||||
|
|
||||||
|
def is_loopback(host):
|
||||||
|
if host is None:
|
||||||
|
return False
|
||||||
|
try:
|
||||||
|
if ipaddress.ip_address(host).is_loopback:
|
||||||
|
return True
|
||||||
|
else:
|
||||||
|
return False
|
||||||
|
except:
|
||||||
|
pass
|
||||||
|
|
||||||
|
loopback = False
|
||||||
|
for family in (socket.AF_INET, socket.AF_INET6):
|
||||||
|
try:
|
||||||
|
r = socket.getaddrinfo(host, None, family, socket.SOCK_STREAM)
|
||||||
|
for family, _, _, _, sockaddr in r:
|
||||||
|
if not ipaddress.ip_address(sockaddr[0]).is_loopback:
|
||||||
|
return loopback
|
||||||
|
else:
|
||||||
|
loopback = True
|
||||||
|
except socket.gaierror:
|
||||||
|
pass
|
||||||
|
|
||||||
|
return loopback
|
||||||
|
|
||||||
|
|
||||||
def create_origin_only_middleware():
|
def create_origin_only_middleware():
|
||||||
@web.middleware
|
@web.middleware
|
||||||
async def origin_only_middleware(request: web.Request, handler):
|
async def origin_only_middleware(request: web.Request, handler):
|
||||||
@ -139,13 +167,23 @@ def create_origin_only_middleware():
|
|||||||
if 'Host' in request.headers and 'Origin' in request.headers:
|
if 'Host' in request.headers and 'Origin' in request.headers:
|
||||||
host = request.headers['Host']
|
host = request.headers['Host']
|
||||||
origin = request.headers['Origin']
|
origin = request.headers['Origin']
|
||||||
# The host/origin CSRF decision (incl. the Origin: null bypass fix
|
host_domain = host.lower()
|
||||||
# for GHSA-779p-m5rp-r4h4 #1) lives in utils.origin_check so it can
|
parsed = urllib.parse.urlparse(origin)
|
||||||
# be unit-tested without standing up the full server. See
|
origin_domain = parsed.netloc.lower()
|
||||||
# tests-unit/security_test/test_ghsa_779p_01_origin_csrf.py.
|
host_domain_parsed = urllib.parse.urlsplit('//' + host_domain)
|
||||||
if is_cross_origin_forbidden(host, origin):
|
|
||||||
logging.warning("WARNING: request with non matching host and origin, returning 403 (host={!r} origin={!r})".format(host, origin))
|
#limit the check to when the host domain is localhost, this makes it slightly less safe but should still prevent the exploit
|
||||||
return web.Response(status=403)
|
loopback = is_loopback(host_domain_parsed.hostname)
|
||||||
|
|
||||||
|
if parsed.port is None: #if origin doesn't have a port strip it from the host to handle weird browsers, same for host
|
||||||
|
host_domain = host_domain_parsed.hostname
|
||||||
|
if host_domain_parsed.port is None:
|
||||||
|
origin_domain = parsed.hostname
|
||||||
|
|
||||||
|
if loopback and host_domain is not None and origin_domain is not None and len(host_domain) > 0 and len(origin_domain) > 0:
|
||||||
|
if host_domain != origin_domain:
|
||||||
|
logging.warning("WARNING: request with non matching host and origin {} != {}, returning 403".format(host_domain, origin_domain))
|
||||||
|
return web.Response(status=403)
|
||||||
|
|
||||||
if request.method == "OPTIONS":
|
if request.method == "OPTIONS":
|
||||||
response = web.Response()
|
response = web.Response()
|
||||||
|
|||||||
@ -1,114 +0,0 @@
|
|||||||
"""CI unit guard for FIX #1 of GHSA-779p-m5rp-r4h4 — the Origin: null CSRF bypass.
|
|
||||||
|
|
||||||
Vuln #1 was a CSRF bypass: the loopback host/origin guard in
|
|
||||||
``server.create_origin_only_middleware`` skipped the comparison entirely whenever
|
|
||||||
the origin host parsed to an empty string. An opaque ``Origin: null`` (sent by a
|
|
||||||
sandboxed iframe or a ``data:``/``file:`` document) parses to exactly that, so an
|
|
||||||
attacker could forge cross-origin state-mutating requests against a victim's
|
|
||||||
loopback ComfyUI — the entry primitive for the [CISA-1] CSRF -> stored-XSS ->
|
|
||||||
client-side RCE chain.
|
|
||||||
|
|
||||||
The decision logic was extracted verbatim from the middleware closure into
|
|
||||||
``utils.origin_check`` precisely so it can be exercised here: ``server.py`` cannot
|
|
||||||
be imported in a unit test (importing it spins up the full PromptServer/aiohttp
|
|
||||||
app and its global side effects), which is why finding #1 previously had no
|
|
||||||
server-free CI coverage and only a live-server POC
|
|
||||||
(``.security/pocs/test_security_ghsa_779p.py::TestOriginNullCsrf``, skipped
|
|
||||||
unless a server is running on 127.0.0.1:8188). This file is the fast, hermetic
|
|
||||||
guard so the Origin: null bypass cannot silently reopen.
|
|
||||||
|
|
||||||
Cases use IP literals so the loopback determination does not depend on DNS; the
|
|
||||||
one name-based case ("localhost") relies only on standard loopback resolution.
|
|
||||||
"""
|
|
||||||
|
|
||||||
from utils.origin_check import is_cross_origin_forbidden, is_loopback
|
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
# The regression: an opaque/empty Origin against a loopback Host MUST be a 403.
|
|
||||||
# Each of these returned False (allowed) before the fix — that is the bug.
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
OPAQUE_ORIGIN_ON_LOOPBACK = [
|
|
||||||
("127.0.0.1:8188", "null"), # the exact reported bypass
|
|
||||||
("127.0.0.1:8188", ""), # empty Origin header, same empty-host path
|
|
||||||
("127.0.0.1", "null"), # host without an explicit port
|
|
||||||
("[::1]:8188", "null"), # IPv6 loopback host
|
|
||||||
]
|
|
||||||
|
|
||||||
|
|
||||||
def test_origin_null_is_forbidden():
|
|
||||||
"""Origin: null against a loopback host must be rejected (the #1 fix)."""
|
|
||||||
assert is_cross_origin_forbidden("127.0.0.1:8188", "null") is True, (
|
|
||||||
"Origin: null was treated as allowed — this is exactly the "
|
|
||||||
"GHSA-779p-m5rp-r4h4 #1 CSRF bypass reopening."
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
def test_all_opaque_origins_on_loopback_forbidden():
|
|
||||||
for host, origin in OPAQUE_ORIGIN_ON_LOOPBACK:
|
|
||||||
assert is_cross_origin_forbidden(host, origin) is True, (host, origin)
|
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
# False-positive guards: legitimate same-origin requests must stay allowed, or
|
|
||||||
# the fix would break the dev server. The port-stripping cases preserve the
|
|
||||||
# original "handle weird browsers" behaviour (origin without a port matches a
|
|
||||||
# host with one, and vice versa).
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
MATCHING_ORIGINS = [
|
|
||||||
("127.0.0.1:8188", "http://127.0.0.1:8188"),
|
|
||||||
("127.0.0.1:8188", "http://127.0.0.1"), # origin has no port -> host port stripped for compare
|
|
||||||
("127.0.0.1", "http://127.0.0.1"),
|
|
||||||
("localhost:8188", "http://localhost:8188"),
|
|
||||||
]
|
|
||||||
|
|
||||||
|
|
||||||
def test_matching_origins_allowed():
|
|
||||||
for host, origin in MATCHING_ORIGINS:
|
|
||||||
assert is_cross_origin_forbidden(host, origin) is False, (host, origin)
|
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
# Genuine cross-origin requests against a loopback host must be forbidden.
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
MISMATCHED_ORIGINS_ON_LOOPBACK = [
|
|
||||||
("127.0.0.1:8188", "http://evil.com"),
|
|
||||||
("127.0.0.1:8188", "https://127.0.0.1:9999"), # same host, different port
|
|
||||||
("127.0.0.1:8188", "http://localhost.evil.com"),
|
|
||||||
]
|
|
||||||
|
|
||||||
|
|
||||||
def test_mismatched_origins_on_loopback_forbidden():
|
|
||||||
for host, origin in MISMATCHED_ORIGINS_ON_LOOPBACK:
|
|
||||||
assert is_cross_origin_forbidden(host, origin) is True, (host, origin)
|
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
# Scope guard: the check is deliberately limited to loopback hosts. A
|
|
||||||
# non-loopback Host must NOT trip the guard (even on a mismatch / opaque
|
|
||||||
# origin) — this preserves the original behaviour and documents that the
|
|
||||||
# mitigation is localhost-only by design.
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
NON_LOOPBACK_HOSTS = [
|
|
||||||
("203.0.113.5:8188", "http://evil.com"), # public IP literal -> not loopback
|
|
||||||
("203.0.113.5:8188", "null"),
|
|
||||||
("10.0.0.5:8188", "null"), # private but not loopback
|
|
||||||
]
|
|
||||||
|
|
||||||
|
|
||||||
def test_non_loopback_host_not_subject_to_check():
|
|
||||||
for host, origin in NON_LOOPBACK_HOSTS:
|
|
||||||
assert is_cross_origin_forbidden(host, origin) is False, (host, origin)
|
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
# is_loopback — the predicate the scoping above depends on.
|
|
||||||
# ---------------------------------------------------------------------------
|
|
||||||
def test_is_loopback_true_for_loopback_addresses():
|
|
||||||
for host in ("127.0.0.1", "::1", "localhost"):
|
|
||||||
assert is_loopback(host) is True, host
|
|
||||||
|
|
||||||
|
|
||||||
def test_is_loopback_false_for_non_loopback_and_none():
|
|
||||||
for host in ("203.0.113.5", "10.0.0.5", None):
|
|
||||||
assert is_loopback(host) is False, host
|
|
||||||
@ -1,91 +0,0 @@
|
|||||||
"""Host/Origin CSRF check for the loopback dev server.
|
|
||||||
|
|
||||||
Extracted verbatim from ``server.create_origin_only_middleware`` so the decision
|
|
||||||
logic is importable and unit-testable without standing up the full
|
|
||||||
PromptServer/aiohttp app (importing ``server`` pulls in ``nodes``/``execution``/
|
|
||||||
torch and has global side effects). The wiring lives in ``server.py``; the
|
|
||||||
regression guard for GHSA-779p-m5rp-r4h4 finding #1 (CSRF bypass via
|
|
||||||
``Origin: null``) lives in
|
|
||||||
``tests-unit/security_test/test_ghsa_779p_01_origin_csrf.py``.
|
|
||||||
|
|
||||||
Only ``urllib.parse``/``ipaddress``/``socket`` (stdlib) are imported here, so the
|
|
||||||
module stays cheap to import from a unit test.
|
|
||||||
"""
|
|
||||||
|
|
||||||
import ipaddress
|
|
||||||
import socket
|
|
||||||
import urllib.parse
|
|
||||||
|
|
||||||
|
|
||||||
def is_loopback(host):
|
|
||||||
if host is None:
|
|
||||||
return False
|
|
||||||
try:
|
|
||||||
if ipaddress.ip_address(host).is_loopback:
|
|
||||||
return True
|
|
||||||
else:
|
|
||||||
return False
|
|
||||||
except ValueError:
|
|
||||||
# Not an IP literal (ip_address raises ValueError); fall through to DNS
|
|
||||||
# resolution below. Narrowed from a bare except so genuine interrupts
|
|
||||||
# (KeyboardInterrupt/SystemExit) aren't swallowed.
|
|
||||||
pass
|
|
||||||
|
|
||||||
loopback = False
|
|
||||||
for family in (socket.AF_INET, socket.AF_INET6):
|
|
||||||
try:
|
|
||||||
r = socket.getaddrinfo(host, None, family, socket.SOCK_STREAM)
|
|
||||||
for family, _, _, _, sockaddr in r:
|
|
||||||
if not ipaddress.ip_address(sockaddr[0]).is_loopback:
|
|
||||||
return loopback
|
|
||||||
else:
|
|
||||||
loopback = True
|
|
||||||
except socket.gaierror:
|
|
||||||
pass
|
|
||||||
|
|
||||||
return loopback
|
|
||||||
|
|
||||||
|
|
||||||
def is_cross_origin_forbidden(host, origin):
|
|
||||||
"""Return True if a request with these ``Host``/``Origin`` headers must be rejected (403).
|
|
||||||
|
|
||||||
This prevents the case where a random website can queue Comfy workflows by
|
|
||||||
making a POST to 127.0.0.1, which browsers don't prevent. In that case the
|
|
||||||
Host and Origin hostnames won't match. The check is intentionally limited to
|
|
||||||
when the Host resolves to a loopback address; for non-loopback hosts it
|
|
||||||
returns False (it is a localhost-CSRF mitigation, not a general same-origin
|
|
||||||
enforcer).
|
|
||||||
|
|
||||||
GHSA-779p-m5rp-r4h4 #1 fix: an opaque origin (e.g. ``"null"`` sent by a
|
|
||||||
sandboxed iframe or a ``data:``/``file:`` document) parses to an empty/None
|
|
||||||
host. Previously such requests skipped the comparison entirely, which let an
|
|
||||||
attacker bypass the host/origin CSRF check with ``Origin: null``. A missing
|
|
||||||
or empty origin host is now treated as a mismatch and rejected.
|
|
||||||
"""
|
|
||||||
host_domain = host.lower()
|
|
||||||
parsed = urllib.parse.urlparse(origin)
|
|
||||||
origin_domain = parsed.netloc.lower()
|
|
||||||
host_domain_parsed = urllib.parse.urlsplit('//' + host_domain)
|
|
||||||
|
|
||||||
# A non-numeric or out-of-range port (e.g. Origin: http://127.0.0.1:99999)
|
|
||||||
# makes urllib raise ValueError on .port access. Treat a malformed port as a
|
|
||||||
# rejected request rather than letting it surface as an uncaught 500 in the
|
|
||||||
# middleware — it fails closed, consistent with the CSRF stance.
|
|
||||||
try:
|
|
||||||
origin_port = parsed.port
|
|
||||||
host_port = host_domain_parsed.port
|
|
||||||
except ValueError:
|
|
||||||
return True
|
|
||||||
|
|
||||||
loopback = is_loopback(host_domain_parsed.hostname)
|
|
||||||
|
|
||||||
if origin_port is None: # if origin doesn't have a port strip it from the host to handle weird browsers, same for host
|
|
||||||
host_domain = host_domain_parsed.hostname
|
|
||||||
if host_port is None:
|
|
||||||
origin_domain = parsed.hostname
|
|
||||||
|
|
||||||
if loopback and host_domain is not None and len(host_domain) > 0:
|
|
||||||
if origin_domain is None or len(origin_domain) == 0 or host_domain != origin_domain:
|
|
||||||
return True
|
|
||||||
|
|
||||||
return False
|
|
||||||
Loading…
Reference in New Issue
Block a user