rusackas commented on code in PR #44036:
URL: https://github.com/apache/superset/pull/44036#discussion_r3983425495
##########
superset/security/manager.py:
##########
@@ -5349,11 +5349,24 @@ def validate_guest_token_resources(resources:
GuestTokenResources) -> None:
embedded =
EmbeddedDashboardDAO.find_by_id(str(resource["id"]))
if not embedded:
raise EmbeddedDashboardNotFoundError()
+ dashboard = embedded.dashboard
elif not dashboard.embedded:
# A raw dashboard id must still reference an embedded
dashboard;
# otherwise a guest token could be scoped to a
non-embedded one.
raise EmbeddedDashboardNotFoundError()
+ # The caller minting the token must themselves be entitled to
+ # the dashboard being scoped. `grant_guest_token` is a
+ # coarse, instance-wide permission -- without this check, an
+ # operator who narrows it to a non-Admin role (a realistic
+ # "embedding backend service" grant) would let that
+ # principal mint a fully valid guest token for *any*
+ # embedded dashboard, not just ones they have access to.
+ try:
+ self.raise_for_access(dashboard=dashboard)
+ except SupersetSecurityException as ex:
+ raise EmbeddedDashboardAccessDeniedError() from ex
Review Comment:
You're right, and thanks for running it. FAB 5's response_403 takes no
message, so my fix raised before it could answer. Now built with
self.response(403, ...) in 9c7d15b, and the integration test that caught it is
the one pinning it.
##########
superset/db_engine_specs/base.py:
##########
@@ -901,6 +905,47 @@ def get_oauth2_config(cls) -> OAuth2ClientConfig | None:
return config
+ @staticmethod
+ def _validate_oauth2_endpoint_host(uri: str) -> None:
+ """
+ Validate an OAuth2 authorization/token endpoint URI before it's used.
+
+ ``config["authorization_request_uri"]``/``config["token_request_uri"]``
+ can come from a database's own ``encrypted_extra.oauth2_client_info``
+ (editable by anyone with ``can_write`` on Database, not just the
+ deployment operator). The authorization URI is handed to the user's
+ browser as a redirect target; the token URI is POSTed to directly by
+ this server, carrying the connection's ``client_secret`` in the
+ request body. Neither is otherwise validated, so an attacker with
+ write access to one database's config could point either at an
+ internal host, exfiltrating the client secret (token URI) or using
+ Superset as an open redirect into the internal network (authorization
+ URI) -- and since the connection is typically shared, this is
+ exercised by every user who goes through that database's OAuth2 flow,
+ not just the one who configured it.
+
+ Operators with a legitimately internal IdP can opt out via
+ ``DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS`` -- but that flag only
+ widens which *hosts* are acceptable, not which URI *schemes* are;
+ a non-http(s) scheme is refused unconditionally.
+ """
+ try:
+ parsed = urlparse(uri)
+ except ValueError as ex:
+ # e.g. an unmatched IPv6 bracket -- urlparse raises rather than
+ # returning an unusable result.
+ raise OAuth2Error("Invalid OAuth2 endpoint URI") from ex
+
+ if parsed.scheme not in ("http", "https"):
+ raise OAuth2Error("Invalid OAuth2 endpoint URI")
+
+ if app.config["DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS"]:
+ return
+
+ if not parsed.hostname or not is_safe_host(parsed.hostname):
Review Comment:
Good catch, fixed in 9c7d15b. Changing any of the OAuth2 endpoint URIs
inside masked_encrypted_extra while the client secret is still the mask is now
refused the same way a host or SSH endpoint move is, with a fresh secret
required to confirm it. Tests cover the repoint, the fresh-secret case, and the
plain masked round-trip.
##########
superset/security/manager.py:
##########
@@ -5349,11 +5349,24 @@ def validate_guest_token_resources(resources:
GuestTokenResources) -> None:
embedded =
EmbeddedDashboardDAO.find_by_id(str(resource["id"]))
if not embedded:
raise EmbeddedDashboardNotFoundError()
+ dashboard = embedded.dashboard
elif not dashboard.embedded:
# A raw dashboard id must still reference an embedded
dashboard;
# otherwise a guest token could be scoped to a
non-embedded one.
raise EmbeddedDashboardNotFoundError()
+ # The caller minting the token must themselves be entitled to
+ # the dashboard being scoped. `grant_guest_token` is a
+ # coarse, instance-wide permission -- without this check, an
+ # operator who narrows it to a non-Admin role (a realistic
+ # "embedding backend service" grant) would let that
+ # principal mint a fully valid guest token for *any*
+ # embedded dashboard, not just ones they have access to.
+ try:
+ self.raise_for_access(dashboard=dashboard)
Review Comment:
Agreed, the dashboard check alone was too coarse. As of 9c7d15b minting also
requires access to every datasource the token will grant, scoped to the
datasets allowlist when the request carries one. Admin, editor, and viewers of
a published RBAC dashboard are covered by the dashboard entitlement itself and
skip the per-datasource pass. Three unit tests for those paths.
##########
superset/utils/network.py:
##########
@@ -44,6 +50,107 @@
PING_TIMEOUT = 5
+class SSRFProtectionError(ConnectionError):
+ """
+ Raised when an outbound request's connected peer is not a public,
+ globally-routable address. Subclasses ``ConnectionError`` so it's
+ already covered by any caller that catches connection failures broadly
+ (and, when raised from within an active ``requests``/``urllib3``
+ connection attempt, surfaces to callers as a
+ ``requests.exceptions.ConnectionError``, since ``requests`` wraps
+ whatever a connection class raises during ``connect()``).
+ """
+
+
+def _raise_for_unsafe_peer(conn: HTTPConnection) -> None:
+ """
+ Validate that a connection's actual peer is publicly routable.
+
+ An upfront ``is_safe_host`` check resolves and validates the hostname
+ once, ahead of time; the connection opened here is resolved
+ independently and may reach a different address (DNS rebinding via a
+ low-TTL record), so the check has to be repeated against the address
+ actually connected to.
+ """
+ sock = conn.sock
+ if sock is None:
+ return
+ peer = sock.getpeername()[0]
+ if not is_safe_ip(ipaddress.ip_address(peer)):
+ raise SSRFProtectionError("Request target host is not allowed.")
+
+
+class _PeerValidatingHTTPConnection(HTTPConnection):
+ """HTTP connection that validates the peer address on connect."""
+
+ def connect(self) -> None:
+ super().connect()
+ _raise_for_unsafe_peer(self)
+
+
+class _PeerValidatingHTTPSConnection(HTTPSConnection):
+ """HTTPS connection that validates the peer address after the handshake."""
+
+ def connect(self) -> None:
+ super().connect()
+ _raise_for_unsafe_peer(self)
+
+
+class _PeerValidatingHTTPConnectionPool(HTTPConnectionPool):
+ ConnectionCls = _PeerValidatingHTTPConnection
+
+
+class _PeerValidatingHTTPSConnectionPool(HTTPSConnectionPool):
+ ConnectionCls = _PeerValidatingHTTPSConnection
+
+
+class PeerValidatingHTTPAdapter(HTTPAdapter):
+ """
+ Transport adapter that routes requests through connection classes which
+ validate the connected peer address, closing the TOCTOU window between
+ an upfront ``is_safe_host`` check and the connection ``send()`` actually
+ opens.
+
+ Mirrors the peer-validation approach already used for webhook alert/
+ report dispatch (``superset.reports.notifications.webhook``) and
+ dataset-import data URIs
+ (``superset.commands.dataset.importers.v1.utils``); factored out here so
+ other outbound-request call sites (e.g. OAuth2 token/authorization
+ endpoints) can reuse it instead of re-implementing it.
+ """
+
+ def init_poolmanager(self, *args: Any, **kwargs: Any) -> None:
+ super().init_poolmanager(*args, **kwargs)
+ # Assign a new dict rather than mutating the manager's dict in
+ # place -- the attribute otherwise aliases urllib3's module-global
+ # default scheme-to-pool-class mapping.
+ self.poolmanager.pool_classes_by_scheme = {
Review Comment:
Good catch. Rather than validate the proxy hop (the peer there is the proxy,
not the target), the protected session now sets trust_env=False in 9c7d15b, so
environment proxies are ignored for these calls. Deployments that need one keep
the existing allow_unsafe_hosts opt-in, which returns plain requests. Test
added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
