diff --git a/docs/en/operations/query-cache.md b/docs/en/operations/query-cache.md index 50c5ff4457f..fbff622ae38 100644 --- a/docs/en/operations/query-cache.md +++ b/docs/en/operations/query-cache.md @@ -29,10 +29,6 @@ Transactionally inconsistent caching is traditionally provided by client tools o the same caching logic and configuration is often duplicated. With ClickHouse's query cache, the caching logic moves to the server side. This reduces maintenance effort and avoids redundancy. -:::note -Security consideration: The cached query result is tied to the user executing it. Authorization checks are performed when the query is executed. This means that if there are any alterations to the user's role or permissions between the time the query is cached and when the cache is accessed, the result will not reflect these changes. We recommend using different users to distinguish between different levels of access, instead of actively toggling roles for a single user between queries, as this practice may lead to unexpected query results. -::: - ## Configuration Settings and Usage Setting [use_query_cache](settings/settings.md#use-query-cache) can be used to control whether a specific query or all queries of the diff --git a/src/Interpreters/Cache/QueryCache.cpp b/src/Interpreters/Cache/QueryCache.cpp index 8347d32bd3c..1e8fdeb1b5d 100644 --- a/src/Interpreters/Cache/QueryCache.cpp +++ b/src/Interpreters/Cache/QueryCache.cpp @@ -129,12 +129,15 @@ String queryStringFromAST(ASTPtr ast) QueryCache::Key::Key( ASTPtr ast_, Block header_, - const String & user_name_, bool is_shared_, + const String & user_name_, std::optional user_id_, const std::vector & current_user_roles_, + bool is_shared_, std::chrono::time_point expires_at_, bool is_compressed_) : ast(removeQueryCacheSettings(ast_)) , header(header_) , user_name(user_name_) + , user_id(user_id_) + , current_user_roles(current_user_roles_) , is_shared(is_shared_) , expires_at(expires_at_) , is_compressed(is_compressed_) @@ -142,8 +145,8 @@ QueryCache::Key::Key( { } -QueryCache::Key::Key(ASTPtr ast_, const String & user_name_) - : QueryCache::Key(ast_, {}, user_name_, false, std::chrono::system_clock::from_time_t(1), false) /// dummy values for everything != AST or user name +QueryCache::Key::Key(ASTPtr ast_, const String & user_name_, std::optional user_id_, const std::vector & current_user_roles_) + : QueryCache::Key(ast_, {}, user_name_, user_id_, current_user_roles_, false, std::chrono::system_clock::from_time_t(1), false) /// dummy values for everything != AST or user name { } @@ -401,7 +404,10 @@ QueryCache::Reader::Reader(Cache & cache_, const Key & key, const std::lock_guar const auto & entry_key = entry->key; const auto & entry_mapped = entry->mapped; - if (!entry_key.is_shared && entry_key.user_name != key.user_name) + const bool is_same_user_name = (entry_key.user_name == key.user_name); + const bool is_same_user_id = ((!entry_key.user_id.has_value() && !key.user_id.has_value()) || (entry_key.user_id.has_value() && key.user_id.has_value() && *entry_key.user_id == *key.user_id)); + const bool is_same_current_user_roles = (entry_key.current_user_roles == key.current_user_roles); + if (!entry_key.is_shared && (!is_same_user_name || !is_same_user_id || !is_same_current_user_roles)) { LOG_TRACE(logger, "Inaccessible query result found for query {}", doubleQuoteString(key.query_string)); return; diff --git a/src/Interpreters/Cache/QueryCache.h b/src/Interpreters/Cache/QueryCache.h index d3c98dbd97a..dff949bee27 100644 --- a/src/Interpreters/Cache/QueryCache.h +++ b/src/Interpreters/Cache/QueryCache.h @@ -51,8 +51,16 @@ public: /// Result metadata for constructing the pipe. const Block header; - /// The user who executed the query. + /// The name, id and current roles of the user who executed the query. + /// These members are necessary to ensure that a (non-shared, see below) entry can only be written and read by the same user with + /// the same roles. Example attack scenarios: + /// - after DROP USER, it must not be possible to create a new user with with the dropped user name and access the dropped user's + /// query cache entries + /// - different roles of the same user may be tied to different row-level policies. It must not be possible to switch role and + /// access another role's cache entries const String user_name; + std::optional user_id; + std::vector current_user_roles; /// If the associated entry can be read by other users. In general, sharing is a bad idea: First, it is unlikely that different /// users pose the same queries. Second, sharing potentially breaches security. E.g. User A should not be able to bypass row @@ -74,12 +82,13 @@ public: /// Ctor to construct a Key for writing into query cache. Key(ASTPtr ast_, Block header_, - const String & user_name_, bool is_shared_, + const String & user_name_, std::optional user_id_, const std::vector & current_user_roles_, + bool is_shared_, std::chrono::time_point expires_at_, bool is_compressed); /// Ctor to construct a Key for reading from query cache (this operation only needs the AST + user name). - Key(ASTPtr ast_, const String & user_name_); + Key(ASTPtr ast_, const String & user_name_, std::optional user_id_, const std::vector & current_user_roles_); bool operator==(const Key & other) const; }; diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 63804d2d86f..fe119662c91 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -1010,7 +1010,7 @@ static std::tuple executeQueryImpl( { if (can_use_query_cache && settings.enable_reads_from_query_cache) { - QueryCache::Key key(ast, context->getUserName()); + QueryCache::Key key(ast, context->getUserName(), context->getUserID(), context->getCurrentRoles()); QueryCache::Reader reader = query_cache->createReader(key); if (reader.hasCacheEntryForKey()) { @@ -1123,7 +1123,8 @@ static std::tuple executeQueryImpl( { QueryCache::Key key( ast, res.pipeline.getHeader(), - context->getUserName(), settings.query_cache_share_between_users, + context->getUserName(), context->getUserID(), context->getCurrentRoles(), + settings.query_cache_share_between_users, std::chrono::system_clock::now() + std::chrono::seconds(settings.query_cache_ttl), settings.query_cache_compress_entries); diff --git a/src/Storages/System/StorageSystemQueryCache.cpp b/src/Storages/System/StorageSystemQueryCache.cpp index 8538820cf41..b909b57c638 100644 --- a/src/Storages/System/StorageSystemQueryCache.cpp +++ b/src/Storages/System/StorageSystemQueryCache.cpp @@ -37,11 +37,16 @@ void StorageSystemQueryCache::fillData(MutableColumns & res_columns, ContextPtr std::vector content = query_cache->dump(); const String & user_name = context->getUserName(); + std::optional user_id = context->getUserID(); + std::vector current_user_roles = context->getCurrentRoles(); for (const auto & [key, query_result] : content) { /// Showing other user's queries is considered a security risk - if (!key.is_shared && key.user_name != user_name) + const bool is_same_user_name = (key.user_name == user_name); + const bool is_same_user_id = ((!key.user_id.has_value() && !user_id.has_value()) || (key.user_id.has_value() && user_id.has_value() && *key.user_id == *user_id)); + const bool is_same_current_user_roles = (key.current_user_roles == current_user_roles); + if (!key.is_shared && (!is_same_user_name || !is_same_user_id || !is_same_current_user_roles)) continue; res_columns[0]->insert(key.query_string); /// approximates the original query string diff --git a/tests/queries/0_stateless/02494_query_cache_user_isolation.reference b/tests/queries/0_stateless/02494_query_cache_user_isolation.reference new file mode 100644 index 00000000000..f8c4b31b22a --- /dev/null +++ b/tests/queries/0_stateless/02494_query_cache_user_isolation.reference @@ -0,0 +1,28 @@ +Attack 1 +0 +system.query_cache with old user 1 +0 +0 1 +1 0 +system.query_cache with new user 0 +0 +0 1 +1 0 +0 1 +Attack 2 +-- policy_1 test +1 1 +3 1 +6 1 +-- policy_2 test +2 2 +5 2 +8 2 +-- policy_1 with query cache test +1 1 +3 1 +6 1 +-- policy_2 with query cache test +2 2 +5 2 +8 2 diff --git a/tests/queries/0_stateless/02494_query_cache_user_isolation.sh b/tests/queries/0_stateless/02494_query_cache_user_isolation.sh new file mode 100755 index 00000000000..1c53bf55af6 --- /dev/null +++ b/tests/queries/0_stateless/02494_query_cache_user_isolation.sh @@ -0,0 +1,109 @@ +#!/usr/bin/env bash +# Tags: no-parallel, no-fasttest +# Tag no-parallel: Messes with internal cache +# no-fasttest: Produces wrong results in fasttest, unclear why, didn't reproduce locally. + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CURDIR"/../shell_config.sh + +# -- Attack 1: +# - create a user, +# - run a query whose result is stored in the query cache, +# - drop the user, recreate it with the same name +# - test that the cache entry is inaccessible + +${CLICKHOUSE_CLIENT} --query "SELECT 'Attack 1'" + +rnd=`tr -dc 1-9