From d3730a4f6ac89612d749d10843def181f7830635 Mon Sep 17 00:00:00 2001 From: Mats Kindahl Date: Tue, 25 Apr 2023 18:01:48 +0200 Subject: [PATCH] Add permission checks to run_job() There were no permission checks when calling run_job(), so it was possible to execute any job regardless of who owned it. This commit adds such checks. --- CHANGELOG.md | 1 + src/bgw/job.c | 13 ++++++++-- src/bgw/job.h | 2 +- tsl/src/bgw_policy/job_api.c | 7 ++++-- tsl/test/expected/bgw_security.out | 32 +++++++++++++++++++++++++ tsl/test/sql/CMakeLists.txt | 1 + tsl/test/sql/bgw_security.sql | 38 ++++++++++++++++++++++++++++++ 7 files changed, 89 insertions(+), 5 deletions(-) create mode 100644 tsl/test/expected/bgw_security.out create mode 100644 tsl/test/sql/bgw_security.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 503b35ff1..c7bdc3a82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ accidentally triggering the load of a previous DB version.** * #5570 Improve interpolate error message on datatype mismatch * #5583 Fix parameterization in DecompressChunk path generation * #5602 Fix broken CAgg with JOIN repair function +* #5615 Add permission checks to run_job() **Thanks** * @kovetskiy and @DZDomi for reporting peformance regression in Realtime Continuous Aggregates diff --git a/src/bgw/job.c b/src/bgw/job.c index a960a8aa6..3de45de09 100644 --- a/src/bgw/job.c +++ b/src/bgw/job.c @@ -974,12 +974,21 @@ ts_bgw_job_check_max_retries(BgwJob *job) } void -ts_bgw_job_permission_check(BgwJob *job) +ts_bgw_job_permission_check(BgwJob *job, const char *cmd) { if (!has_privs_of_role(GetUserId(), job->fd.owner)) + { + const char *owner_name = GetUserNameFromId(job->fd.owner, false); + const char *user_name = GetUserNameFromId(GetUserId(), false); ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("insufficient permissions to alter job %d", job->fd.id))); + errmsg("insufficient permissions to %s job %d", cmd, job->fd.id), + errdetail("Job %d is owned by role \"%s\" but user \"%s\" does not belong to that " + "role.", + job->fd.id, + owner_name, + user_name))); + } } void diff --git a/src/bgw/job.h b/src/bgw/job.h index 9fd3137bb..cf7721a51 100644 --- a/src/bgw/job.h +++ b/src/bgw/job.h @@ -47,7 +47,7 @@ extern TSDLLEXPORT int32 ts_bgw_job_insert_relation( Interval *retry_period, Name proc_schema, Name proc_name, Name check_schema, Name check_name, Oid owner, bool scheduled, bool fixed_schedule, int32 hypertable_id, Jsonb *config, TimestampTz initial_start, const char *timezone); -extern TSDLLEXPORT void ts_bgw_job_permission_check(BgwJob *job); +extern TSDLLEXPORT void ts_bgw_job_permission_check(BgwJob *job, const char *cmd); extern TSDLLEXPORT void ts_bgw_job_validate_job_owner(Oid owner); diff --git a/tsl/src/bgw_policy/job_api.c b/tsl/src/bgw_policy/job_api.c index 422840a60..8d87e9583 100644 --- a/tsl/src/bgw_policy/job_api.c +++ b/tsl/src/bgw_policy/job_api.c @@ -263,6 +263,8 @@ job_run(PG_FUNCTION_ARGS) int32 job_id = PG_GETARG_INT32(0); BgwJob *job = find_job(job_id, PG_ARGISNULL(0), false); + ts_bgw_job_permission_check(job, "run"); + job_execute(job); PG_RETURN_VOID(); @@ -326,7 +328,7 @@ job_alter(PG_FUNCTION_ARGS) if (job == NULL) PG_RETURN_NULL(); - ts_bgw_job_permission_check(job); + ts_bgw_job_permission_check(job, "alter"); if (!PG_ARGISNULL(1)) job->fd.schedule_interval = *PG_GETARG_INTERVAL_P(1); @@ -463,7 +465,8 @@ job_alter_set_hypertable_id(PG_FUNCTION_ARGS) BgwJob *job = find_job(job_id, PG_ARGISNULL(0), false /* missing_ok */); if (job == NULL) PG_RETURN_NULL(); - ts_bgw_job_permission_check(job); + + ts_bgw_job_permission_check(job, "alter"); if (!PG_ARGISNULL(1)) { diff --git a/tsl/test/expected/bgw_security.out b/tsl/test/expected/bgw_security.out new file mode 100644 index 000000000..7a73f58b8 --- /dev/null +++ b/tsl/test/expected/bgw_security.out @@ -0,0 +1,32 @@ +-- This file and its contents are licensed under the Timescale License. +-- Please see the included NOTICE for copyright information and +-- LICENSE-TIMESCALE for a copy of the license. +\set ROLE_ADMIN :TEST_DBNAME _admin +\c :TEST_DBNAME :ROLE_SUPERUSER +CREATE ROLE :ROLE_ADMIN; +GRANT :ROLE_ADMIN TO :ROLE_DEFAULT_PERM_USER; +\c :TEST_DBNAME :ROLE_SUPERUSER +CREATE TABLE custom_log (ts integer, msg text); +GRANT ALL ON custom_log TO PUBLIC; +CREATE PROCEDURE custom_job(integer, jsonb) AS $$ + INSERT INTO custom_log values($1, 'custom_job'); +$$ LANGUAGE SQL; +SELECT add_job('custom_job', '1h') AS job_id \gset +-- Set the owner of the job to the admin role +UPDATE _timescaledb_config.bgw_job SET owner = :'ROLE_ADMIN' WHERE id = :job_id; +SELECT id, proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id; + id | proc_name | owner +------+------------+----------------------- + 1000 | custom_job | db_bgw_security_admin +(1 row) + +\c :TEST_DBNAME :ROLE_DEFAULT_PERM_USER_2 +-- We should fail to execute the job since we do not own it or belong +-- to the group that owns it. +\set ON_ERROR_STOP 0 +CALL run_job(:job_id); +ERROR: insufficient permissions to run job 1000 +\set ON_ERROR_STOP 1 +\c :TEST_DBNAME :ROLE_DEFAULT_PERM_USER +-- This should succeed since the role belongs to the job owner group. +CALL run_job(:job_id); diff --git a/tsl/test/sql/CMakeLists.txt b/tsl/test/sql/CMakeLists.txt index 49c749887..89a441653 100644 --- a/tsl/test/sql/CMakeLists.txt +++ b/tsl/test/sql/CMakeLists.txt @@ -4,6 +4,7 @@ include(GenerateTestSchedule) # so unless you have a good reason, add new test files here. set(TEST_FILES bgw_custom.sql + bgw_security.sql bgw_policy.sql cagg_errors.sql cagg_invalidation.sql diff --git a/tsl/test/sql/bgw_security.sql b/tsl/test/sql/bgw_security.sql new file mode 100644 index 000000000..7ec19c29b --- /dev/null +++ b/tsl/test/sql/bgw_security.sql @@ -0,0 +1,38 @@ +-- This file and its contents are licensed under the Timescale License. +-- Please see the included NOTICE for copyright information and +-- LICENSE-TIMESCALE for a copy of the license. + +\set ROLE_ADMIN :TEST_DBNAME _admin +\c :TEST_DBNAME :ROLE_SUPERUSER + +CREATE ROLE :ROLE_ADMIN; +GRANT :ROLE_ADMIN TO :ROLE_DEFAULT_PERM_USER; + +\c :TEST_DBNAME :ROLE_SUPERUSER + +CREATE TABLE custom_log (ts integer, msg text); +GRANT ALL ON custom_log TO PUBLIC; + +CREATE PROCEDURE custom_job(integer, jsonb) AS $$ + INSERT INTO custom_log values($1, 'custom_job'); +$$ LANGUAGE SQL; + +SELECT add_job('custom_job', '1h') AS job_id \gset + +-- Set the owner of the job to the admin role +UPDATE _timescaledb_config.bgw_job SET owner = :'ROLE_ADMIN' WHERE id = :job_id; + +SELECT id, proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id; + +\c :TEST_DBNAME :ROLE_DEFAULT_PERM_USER_2 + +-- We should fail to execute the job since we do not own it or belong +-- to the group that owns it. +\set ON_ERROR_STOP 0 +CALL run_job(:job_id); +\set ON_ERROR_STOP 1 + +\c :TEST_DBNAME :ROLE_DEFAULT_PERM_USER + +-- This should succeed since the role belongs to the job owner group. +CALL run_job(:job_id);