sadpandajoe commented on code in PR #44036:
URL: https://github.com/apache/superset/pull/44036#discussion_r3981887124
##########
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:
Changing only `masked_encrypted_extra.oauth2_client_info.token_request_uri`
to an attacker-controlled public host still reuses the stored client secret:
`UpdateDatabaseCommand` only guards URI/`engine_params` destination changes,
while `unmask_encrypted_extra` restores this masked secret before saving. The
next OAuth exchange then posts the real `client_secret` and code to the new
host, and this safe-host check allows it because the host is public. Could
endpoint changes require a freshly supplied secret as well?
##########
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:
`raise_for_access(dashboard=...)` is weaker than the guest token it
authorizes: for dashboards without explicit viewers, it returns when the caller
can access any one member datasource, but a dashboard-scoped guest token
without a `datasets` claim can read every member datasource. A service role
with `grant_guest_token` plus access to one chart can therefore mint a token
that exposes other charts it could not read directly. Could the minting check
require access to every datasource the token will grant, or constrain the
token's `datasets` claim?
##########
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:
`Session.trust_env` remains enabled, so an `HTTP_PROXY`/`HTTPS_PROXY`
request uses Requests' separately created `ProxyManager`; this assignment only
changes the direct `poolmanager`. Those proxy connections use the default pool
classes and never call `_raise_for_unsafe_peer`, leaving the DNS-rebinding
check inactive whenever a deployment has an environment proxy. Could the
protected session disable environment proxies or install the validating
connection classes on the proxy manager too?
##########
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:
This still returns 500 because Flask-AppBuilder 5.2.2's `response_403` takes
no `message` argument; the added integration test fails with that `TypeError`
on this head. Could this use `self.response(403, message=error.message)`
instead?
--
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]
