fix(security): apply global guest RLS rules to sub-queries in virtual datasets

UPDATING.md

@@ -29,8 +29,21 @@ assists people when migrating to a new version.
 A guest token RLS rule with no `dataset` key applies to every dataset. Such
 rules are now also injected into sub-queries of custom SQL expressions (with
 `ALLOW_ADHOC_SUBQUERY` enabled) and into SQL Lab queries, for every table that
-resolves to a dataset, instead of only into the chart's outer query. A virtual
-dataset's inner SQL is unchanged, since its outer query already applies them.
+resolves to a dataset, instead of only into the chart's outer query. In a
+virtual dataset's SQL they are injected into the tables read inside an
+uncorrelated sub-query (scalar, `IN` or `EXISTS`, including CTEs such a sub-query
+reads). Tables whose rows reach the virtual dataset's output (`FROM`, joins,
+derived tables, `LATERAL`) or are keyed to them by a correlated sub-query (one
+that itself references an outer table or CTE by name or alias, such as
+`lookup.id = a.lid`; a sub-query nested in it doesn't count) are still left to
+the outer query, which already applies the rules. An
+unqualified outer reference (`WHERE id = lid`) can't be told apart from a local
+column, so that sub-query gets the rules; qualify it to keep a lookup without the
+column working. Like a join, a correlated sub-query over another multi-tenant
+table is not scoped by these rules; give such rules a `dataset` if needed. The
+virtual dataset's own RLS rules, which its outer query applies too, are
+injected into those uncorrelated sub-queries the same way, for every user, when
+a sub-query reads the table the virtual dataset is named after.
 
 If a sub-query reads a dataset that lacks a column the rule references, the
 query now fails with a column-not-found error instead of reading rows the rule

superset/models/helpers.py

@@ -3799,8 +3799,13 @@ def get_from_clause(
                             ),
                             self.database,
                             self.database.get_default_catalog(),
-                            exclude_dataset_id=self_id,
-                            include_global_guest_rls=False,
+                            # at least as strict as apply_rls(), which injects
+                            # this dataset's own RLS and the global guest rules
+                            # into the inner SQL's sub-queries
+                            exclude_dataset_id=(
+                                None if statement.has_subquery() else self_id
+                            ),
+                            include_global_guest_rls=statement.has_subquery(),
                         )
                         for statement in parsed_script.statements
                         for table in statement.tables

superset/sql/parse.py

@@ -47,6 +47,7 @@
     Scope,
     ScopeType,
     traverse_scope,
+    walk_in_scope,
 )
 
 from superset.exceptions import QueryClauseValidationException, SupersetParseError
@@ -807,13 +808,18 @@ def apply_rls(
         schema: str | None,
         predicates: dict[Table, list[InternalRepresentation]],
         method: RLSMethod,
-    ) -> None:
+        subquery_predicates: dict[Table, list[InternalRepresentation]] | None = None,
+    ) -> bool:
         """
         Apply relevant RLS rules to the statement inplace.
 
         :param catalog: The default catalog for non-qualified table names
         :param schema: The default schema for non-qualified table names
         :param method: The method to use for applying the rules.
+        :param subquery_predicates: The rules for tables read inside a sub-query
+            (scalar, ``IN`` or ``EXISTS``), whose rows don't reach the statement's
+            output. Defaults to ``predicates``.
+        :returns: True if any rule was applied, False otherwise.
         """
         raise NotImplementedError()
 
@@ -1982,16 +1988,21 @@ def apply_rls(
         schema: str | None,
         predicates: dict[Table, list[exp.Expression]],
         method: RLSMethod,
-    ) -> None:
+        subquery_predicates: dict[Table, list[exp.Expression]] | None = None,
+    ) -> bool:
         """
         Apply relevant RLS rules to the statement inplace.
 
         :param catalog: The default catalog for non-qualified table names
         :param schema: The default schema for non-qualified table names
         :param method: The method to use for applying the rules.
+        :param subquery_predicates: The rules for tables read inside a sub-query
+            (scalar, ``IN`` or ``EXISTS``), whose rows don't reach the statement's
+            output. Defaults to ``predicates``.
+        :returns: True if any rule was applied, False otherwise.
         """
-        if not predicates:
-            return
+        if not predicates and not subquery_predicates:
+            return False
 
         transformers = {
             RLSMethod.AS_PREDICATE: RLSAsPredicateTransformer,
@@ -2001,13 +2012,24 @@ def apply_rls(
             raise ValueError(f"Invalid RLS method: {method}")
 
         transformer = transformers[method](catalog, schema, predicates)
+        subquery_transformer = transformer
+        scopes = traverse_scope(self._parsed)
+        subquery_scopes: set[int] = set()
+        if subquery_predicates is not None:
+            subquery_transformer = transformers[method](
+                catalog, schema, subquery_predicates
+            )
+            subquery_scopes = _find_subquery_scopes(scopes)
 
         # Rewrite the real table reads -- the same set ``extract_tables_from_statement``
         # authorizes -- so the filtered set equals the authorized set. (A CTE reference
         # sharing a rule's table name is not a read here.)
         seen: set[int] = set()
-        reads: list[exp.Table] = []
-        for scope in traverse_scope(self._parsed):
+        reads: list[tuple[exp.Table, RLSTransformer]] = []
+        for scope in scopes:
+            scope_transformer = (
+                subquery_transformer if id(scope) in subquery_scopes else transformer
+            )
             for source in scope.sources.values():
                 # dedupe by identity: a correlated LATERAL reaches one node twice
                 if (
@@ -2016,16 +2038,22 @@ def apply_rls(
                     and id(source) not in seen
                 ):
                     seen.add(id(source))
-                    reads.append(source)
+                    reads.append((source, scope_transformer))
 
         # Wrap the deepest reads first: a parenthesised-join head carries its join in
         # its args, so wrapping an ancestor before its descendant would strand the
         # descendant read's replacement off the live tree.
-        for node in sorted(reads, key=lambda read: read.depth, reverse=True):
-            replacement = transformer(node)
+        applied = False
+        for node, node_transformer in sorted(
+            reads, key=lambda read: read[0].depth, reverse=True
+        ):
+            applied = applied or node_transformer.get_predicate(node) is not None
+            replacement = node_transformer(node)
             if replacement is not node:
                 node.replace(replacement)
 
+        return applied
+
 
 class KQLSplitState(enum.Enum):
     """
@@ -2802,6 +2830,87 @@ def resolve(scope: Scope, seen: frozenset[int]) -> list[exp.Table]:
     )
 
 
+def _find_subquery_scopes(scopes: list[Scope]) -> set[int]:
+    """
+    Find the scopes whose rows only reach a statement through a sub-query.
+
+    That is every uncorrelated ``SUBQUERY`` scope (a scalar, ``IN`` or ``EXISTS``
+    sub-query), every scope nested inside one, and every CTE one of them reads from,
+    including the scopes nested inside that CTE. Only a CTE named in the sub-query's
+    own ``FROM`` or joins counts, not every CTE in lexical scope (which
+    ``Scope.sources`` holds), so a CTE only joined in the main ``FROM`` keeps the
+    outer query's rules. A CTE read both from a sub-query and
+    from the statement's ``FROM`` counts as a sub-query, so its reads get the stricter
+    rules. The body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output like a join,
+    so it is left out. A correlated sub-query is left out too: it is typically a
+    lookup keyed to the enclosing rows, often over a table without the rule's
+    columns, which the rules would break the same way they would break a join. The
+    outer query doesn't scope such a sub-query's tables either (UPDATING.md).
+
+    :param scopes: The scopes of the statement, as returned by ``traverse_scope``
+    :returns: The ``id`` of each scope found
+    """
+    found: set[int] = set()
+    pending = [
+        scope
+        for scope in scopes
+        if scope.scope_type == ScopeType.SUBQUERY
+        and not (scope.parent and scope.parent.scope_type == ScopeType.UDTF)
+        and not _is_correlated(scope)
+    ]
+    while pending:
+        scope = pending.pop()
+        if id(scope) in found:
+            continue
+        found.add(id(scope))
+        pending.extend(child for child in scopes if child.parent is scope)
+        pending.extend(
+            source
+            for _, source in scope.selected_sources.values()
+            if isinstance(source, Scope) and source.scope_type == ScopeType.CTE
+        )
+    return found
+
+
+def _is_correlated(scope: Scope) -> bool:
+    """
+    Does a sub-query reference a table of an enclosing query?
+
+    Only a column qualified with an enclosing table's name or alias counts, when the
+    sub-query has no table of its own under that name. An unqualified column can't
+    be told apart from one of the sub-query's own, so it is treated as local, which
+    errs toward the sub-query getting the stricter rules. (``Scope``'s own
+    ``is_correlated_subquery`` treats every unqualified column as external.)
+
+    Only the sub-query's own columns count, not those of a sub-query nested in it
+    (which ``Scope.columns`` includes): a nested correlated sub-query doesn't key the
+    wrapping sub-query's tables to the enclosing rows.
+
+    Names are the ones each query reads in its ``FROM`` and joins
+    (``Scope.selected_sources``), not every CTE in lexical scope, so a reference to
+    a CTE the enclosing query reads counts as external. They are compared ignoring
+    letter-case, since most engines fold unquoted names. On one that doesn't, a
+    qualifier matching only when case is ignored either names one of the
+    sub-query's own tables, which errs toward the stricter rules, or names no table
+    at all and the engine rejects the query.
+
+    :param scope: A ``SUBQUERY`` scope
+    :returns: True if the sub-query is correlated
+    """
+    enclosing: set[str] = set()
+    parent = scope.parent
+    while parent:
+        enclosing.update(name.lower() for name in parent.selected_sources)
+        parent = parent.parent
+    local = {name.lower() for name in scope.selected_sources}
+    return any(
+        isinstance(node, exp.Column)
+        and node.table.lower() in enclosing
+        and node.table.lower() not in local
+        for node in walk_in_scope(scope.expression)
+    )
+
+
 def is_cte(source: exp.Table, scope: Scope) -> bool:
     """
     Does this reference resolve to a CTE rather than to a real table?

superset/utils/rls.py

@@ -69,11 +69,16 @@ def apply_rls(
         pass the virtual dataset's id here so its own RLS isn't injected again
         on top of the outer-WHERE application (avoids double-apply when the
         virtual dataset's table_name collides with a table in its own SQL — for
-        example, after converting a physical dataset with RLS to virtual).
+        example, after converting a physical dataset with RLS to virtual). Like
+        global guest rules, its RLS is still injected into tables read inside an
+        uncorrelated sub-query, which the outer query does not constrain.
     :param include_global_guest_rls: Also inject global (unscoped) guest RLS
         rules. Pass False only for a virtual dataset's inner SQL, whose outer
-        query already applies them. Any other statement, such as a SQL Lab query
-        or an adhoc sub-query, is not constrained by such an outer query.
+        query already applies them to the rows the inner SQL returns. They are
+        still injected into tables read inside an uncorrelated sub-query there,
+        since the outer query does not constrain those. Any other statement, such
+        as a SQL Lab query or an adhoc sub-query, is not constrained by such an
+        outer query.
     :returns: True if any RLS predicates were actually applied, False otherwise.
     """
     # There are two ways to insert RLS: either replacing the table with a subquery
@@ -83,24 +88,58 @@ def apply_rls(
 
     # collect all RLS predicates for all tables in the query
     default_catalog = database.get_default_catalog()
-    predicates: dict[Table, list[Any]] = {}
-    for table in parsed_statement.tables:
-        table = table.qualify(catalog=catalog, schema=schema)
-        predicates[table] = [
-            parsed_statement.parse_predicate(predicate)
-            for predicate in get_predicates_for_table(
-                table,
-                database,
-                default_catalog,
-                exclude_dataset_id=exclude_dataset_id,
-                include_global_guest_rls=include_global_guest_rls,
-            )
-            if predicate
-        ]
-
-    has_predicates = any(predicates.values())
-    parsed_statement.apply_rls(catalog, schema, predicates, method)
-    return has_predicates
+
+    def collect_predicates(
+        include_global: bool, exclude_id: int | None
+    ) -> dict[Table, list[Any]]:
+        predicates: dict[Table, list[Any]] = {}
+        for table in parsed_statement.tables:
+            table = table.qualify(catalog=catalog, schema=schema)
+            predicates[table] = [
+                parsed_statement.parse_predicate(predicate)
+                for predicate in get_predicates_for_table(
+                    table,
+                    database,
+                    default_catalog,
+                    exclude_dataset_id=exclude_id,
+                    include_global_guest_rls=include_global,
+                )
+                if predicate
+            ]
+        return predicates
+
+    predicates = collect_predicates(include_global_guest_rls, exclude_dataset_id)
+    # The outer query only constrains the rows that reach it, so a table read
+    # inside an uncorrelated sub-query still gets the rules left to the outer
+    # query: the global guest rules and the excluded dataset's own RLS. The
+    # second lookup is skipped when neither applies: only a guest token carries
+    # global guest rules, and most virtual datasets have no RLS of their own.
+    needs_subquery_predicates = parsed_statement.has_subquery() and (
+        (
+            not include_global_guest_rls
+            and security_manager.get_current_guest_user_if_guest() is not None
+        )
+        or (exclude_dataset_id is not None and _dataset_has_rls(exclude_dataset_id))
+    )
+    subquery_predicates = (
+        collect_predicates(True, None) if needs_subquery_predicates else None
+    )
+
+    return parsed_statement.apply_rls(
+        catalog, schema, predicates, method, subquery_predicates
+    )
+
+
+def _dataset_has_rls(dataset_id: int) -> bool:
+    """
+    Does the dataset have RLS predicates for the current user?
+
+    :param dataset_id: The dataset's id
+    """
+    from superset.connectors.sqla.models import SqlaTable
+
+    dataset = db.session.get(SqlaTable, dataset_id)
+    return dataset is not None and bool(dataset.get_sqla_row_level_filters())
 
 
 def _identifier_predicate(column: Any, value: str | None, fold: bool) -> Any:
@@ -228,8 +267,9 @@ def get_predicates_for_table(
     # For a virtual dataset's inner SQL, callers exclude global (unscoped) guest
     # RLS to prevent double application. Global guest rules will be applied to
     # the outer query via get_sqla_row_level_filters() on the virtual dataset
-    # itself. Dataset-scoped guest rules are still included here because they
-    # target this specific physical dataset and won't match on the outer query.
+    # itself; apply_rls() still includes them for tables read in sub-queries.
+    # Dataset-scoped guest rules are still included here because they target
+    # this specific physical dataset and won't match on the outer query.
     # A folded match can resolve to several datasets naming the same physical
     # table, in which case every one's predicates apply; deduplicated because a
     # single RLS rule can be attached to more than one of them.