Fix check_access_control_on_utilized_columns_only for CTE #24647

rschlussel · 2025-02-27T21:20:35Z

Description

Previously we weren't associating the with query with the columns used from it in the main part of the query, so we wouldn't consider any columns as used. When the usage within the cte or outside of it was just an identifier, we would have the utilized column collected anyway because the column collected in the main query kept the source table information. However, if there was an expression in between, we would lose that information, and we wouldn't check access control for the column that was used in an expression in the cte.

Example:

For the following query

with cte as (select  x as c1, z + 1 as c2 from t13) select c1, c2 from (select * from cte)

Previously we would only check access permissions on t13.x, but not t13.z. With this change we will check column access on both 13.x and t13.z.

Motivation and Context

Fix a bug

Impact

Fixes a security hole where we were not checking access for some columns used in a query when the reference is within a with clause.

Test Plan

unit test

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Fix a security bug when ``check_access_control_for_utlized_columns`` is true for queries that uses a ``WITH`` clause. Previously we would sometime not check permissions for certain columns that were used in the query.  Now we will always check permissions for all columns used in the query. There are some corner cases for CTEs with the same name where we may check more columns than are used or fall back to checking all columns referenced in the query.

rschlussel · 2025-02-27T23:06:36Z

I'm realizing i didn't test for the following cases, and not sure they will work properly

2 with clauses and one refers to the other e.g. with cte1 as (SELECT x as c1, z+1 as c2 from t13), cte2 as (SELECT c1+1 as a c2 as b) SELECT * from cte2. Not sure what order the with clauses show up in the list, but they may actually need to be evaluated in reverse order
2 ctes with the same name (e.g. nested/within different scopes). I think to handle this we'd need a different way to identify the source table that takes into account the scope. If it's too complex IMO it's okay that we don't handle this and just fall back to regular, but failing the checkState is probably not the clearest way to do that. And ideally we'd add a query warning or something so people know that happened.

rschlussel · 2025-02-28T00:45:03Z

I've updated this to handle multiple ctes that reference each other, and add more explicit handling for falling back to checking all columns when multiple ctes have the same name. Also added a warning for queries that fall back to checking all columns so that it's easier to see when it's happening. Since this is a security vulnerability, i'd rather get the fix in quickly than invest in handling that corner case.

rschlussel · 2025-02-28T12:30:40Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/UtilizedColumnsAnalyzer.java

@@ -526,7 +526,10 @@ public void copyFieldIdsToExploreForWithQuery(WithQuery withQuery)
            List<RelationId> relationIds = fieldsToExplore.keySet().stream()
                    .filter(key -> key.getSourceNode() instanceof Table && ((Table) key.getSourceNode()).getName().equals(name))
                    .collect(toImmutableList());
-            if (relationIds.size() != 1) {
+            if (relationIds.isEmpty()) {


In what cases would the relationId would be empty? Is that if the CTE doesn't actually get used in the query or are there other cases as well? Can you add tests?

facebook-github-bot · 2025-02-28T18:22:45Z

@rschlussel has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-02-28T18:53:37Z

@rschlussel has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-02-28T20:02:35Z

@rschlussel has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kevintang2022 · 2025-03-06T17:56:03Z

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java

+        // TODO(kevintang2022): Fix visitWithQuery so that it looks up relation properly.
+        // Currently, it just looks the relations using the name (string) of the CTE


Added some todo for followup. The name conflict is caused by just using name for the look up. We might need to lookup with more info or add a unique identifier to each column

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/RelationId.java

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java

Previously we weren't associating the with query with the columns used from it in the main part of the query, so we wouldn't consider any columns as used. When the usage within the cte or outside of it was just an identifier, we would have the utilized column collected anyway because the column collected in the main query kept the source table information. However, if there was an expression in between, we would lose that information, and we wouldn't check access control for the column that was used in an expression in the cte. Example: For the following query ``` with cte as (select x as c1, z + 1 as c2 from t13) select c1, c2 from (select * from cte) ``` Previously we would only check access permissions on t13.x, but not t13.z. With this change we will check column access on both 13.x and t13.z. Fix empty relations case Add error message to utilized columns warning. handle ctes used multiple times fixup >1 reference support Add tests for multiple cte with same name Co-authored-by: Kevin Tang <tangk@meta.com> Add comments clarification and test for multiple same name for multiple columns

rschlussel

looks good, thanks! let's find someone else to review. (i can't approve because it's my PR)

jaystarshot

Please add a Release note

rschlussel requested review from jaystarshot, feilong-liu, ClarenceThreepwood and a team as code owners February 27, 2025 21:20

rschlussel requested a review from presto-oss February 27, 2025 21:20

prestodb-ci added the from:Meta PR from Meta label Feb 27, 2025

rschlussel requested a review from kevintang2022 February 27, 2025 21:20

rschlussel force-pushed the fix-utilized-columns branch from 2dc6fb5 to ae19df1 Compare February 27, 2025 21:38

rschlussel force-pushed the fix-utilized-columns branch 2 times, most recently from facc558 to 3879ae5 Compare February 28, 2025 00:41

rschlussel force-pushed the fix-utilized-columns branch from 3879ae5 to f1f6703 Compare February 28, 2025 00:49

rschlussel commented Feb 28, 2025

View reviewed changes

kevintang2022 previously approved these changes Mar 6, 2025

View reviewed changes

kevintang2022 force-pushed the fix-utilized-columns branch from 72b6a4b to bc4566d Compare March 6, 2025 18:14

rschlussel commented Mar 6, 2025

View reviewed changes

kevintang2022 dismissed their stale review via c4cceec March 6, 2025 19:11

kevintang2022 force-pushed the fix-utilized-columns branch from c4cceec to 30bc51f Compare March 6, 2025 19:26

rschlussel commented Mar 6, 2025

View reviewed changes

jaystarshot approved these changes Mar 7, 2025

View reviewed changes

rschlussel merged commit 95845fb into prestodb:master Mar 7, 2025
53 checks passed

This was referenced Mar 10, 2025

Add release notes for 0.292 unix280/presto#5

Closed

Add release notes for 0.292 unix280/presto#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix check_access_control_on_utilized_columns_only for CTE #24647

Fix check_access_control_on_utilized_columns_only for CTE #24647

rschlussel commented Feb 27, 2025 •

edited

Loading

rschlussel commented Feb 27, 2025

rschlussel commented Feb 28, 2025

rschlussel Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

kevintang2022 Mar 6, 2025

rschlussel left a comment •

edited

Loading

jaystarshot left a comment

		// TODO(kevintang2022): Fix visitWithQuery so that it looks up relation properly.
		// Currently, it just looks the relations using the name (string) of the CTE

Fix check_access_control_on_utilized_columns_only for CTE #24647

Fix check_access_control_on_utilized_columns_only for CTE #24647

Conversation

rschlussel commented Feb 27, 2025 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Release Notes

rschlussel commented Feb 27, 2025

rschlussel commented Feb 28, 2025

rschlussel Feb 28, 2025

Choose a reason for hiding this comment

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

kevintang2022 Mar 6, 2025

Choose a reason for hiding this comment

rschlussel left a comment • edited Loading

Choose a reason for hiding this comment

jaystarshot left a comment

Choose a reason for hiding this comment

rschlussel commented Feb 27, 2025 •

edited

Loading

rschlussel left a comment •

edited

Loading