From a481b38b6f068587ecc0623981709ffbbad5ceb5 Mon Sep 17 00:00:00 2001 From: Anju Bharti <66729219+anju15bharti@users.noreply.github.com> Date: Mon, 19 Aug 2024 19:16:05 +0530 Subject: [PATCH] Restrict DROP USER/ROLE from non-dbo user (#2859) Earlier, any user was able to drop user/role, irrespective of whether that user has required privileges or not. With this commit, Only dbo and members of db_owner will have the permission to drop user/role. Additionally, this restricts dropping internal database principal such as dbo and db_owner, it restricts dropping non-Babelfish roles from TDS endpoint. Task: BABEL-5173 Authored-by: ANJU BHARTI Co-authored-by: Harsh Lunagariya Signed-off-by: Harsh Lunagariya --- contrib/babelfishpg_tsql/src/pl_handler.c | 30 +- test/JDBC/expected/BABEL-3844-vu-cleanup.out | 2 +- .../JDBC/expected/restrict_drop_user_role.out | 302 ++++++++++++++++++ test/JDBC/input/restrict_drop_user_role.mix | 203 ++++++++++++ 4 files changed, 535 insertions(+), 2 deletions(-) create mode 100644 test/JDBC/expected/restrict_drop_user_role.out create mode 100644 test/JDBC/input/restrict_drop_user_role.mix diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index 3265c05f6a..49b1323f15 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3171,15 +3171,43 @@ bbf_ProcessUtility(PlannedStmt *pstmt, { RoleSpec *rolspec = lfirst(item); char *user_name; + const char *db_principal_type = drop_user ? "user" : "role"; + const char *db_owner_name; + int role_oid; + int rolename_len; + bool is_tsql_db_principal = false; user_name = get_physical_user_name(db_name, rolspec->rolename, false); + db_owner_name = get_db_owner_name(db_name); + role_oid = get_role_oid(user_name, true); + rolename_len = strlen(rolspec->rolename); + is_tsql_db_principal = OidIsValid(role_oid) && + ((drop_user && is_user(role_oid)) || + (drop_role && is_role(role_oid))); + + /* If user is dbo or role is db_owner, restrict dropping */ + if ((drop_user && rolename_len == 3 && strncmp(rolspec->rolename, "dbo", 3) == 0) || + (drop_role && rolename_len == 8 && strncmp(rolspec->rolename, "db_owner", 8) == 0)) + ereport(ERROR, + (errcode(ERRCODE_CHECK_VIOLATION), + errmsg("Cannot drop the %s '%s'.", db_principal_type, rolspec->rolename))); + /* + * Check for current_user's privileges + * must be database owner to drop user/role + */ + if ((!stmt->missing_ok && !is_tsql_db_principal) || + !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal_type, rolspec->rolename))); + /* * If a role has members, do not drop it. * Note that here we don't handle invalid * roles. */ - if (drop_role && !is_empty_role(get_role_oid(user_name, true))) + if (drop_role && !is_empty_role(role_oid)) ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), errmsg("The role has members. It must be empty before it can be dropped."))); diff --git a/test/JDBC/expected/BABEL-3844-vu-cleanup.out b/test/JDBC/expected/BABEL-3844-vu-cleanup.out index 2f51052245..9dd45e6462 100644 --- a/test/JDBC/expected/BABEL-3844-vu-cleanup.out +++ b/test/JDBC/expected/BABEL-3844-vu-cleanup.out @@ -6,7 +6,7 @@ drop user test_user; GO ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: role "master_test_user" does not exist)~~ +~~ERROR (Message: Cannot drop the user 'test_user', because it does not exist or you do not have permission.)~~ drop login [babel\aduser1]; diff --git a/test/JDBC/expected/restrict_drop_user_role.out b/test/JDBC/expected/restrict_drop_user_role.out new file mode 100644 index 0000000000..fa3bcc0426 --- /dev/null +++ b/test/JDBC/expected/restrict_drop_user_role.out @@ -0,0 +1,302 @@ +-- tsql +create login no_priv_login1 with password = '12345678' +go + +create role dont_drop_role +go + +create login no_priv_login2 with password = '12345678' +go + +create user no_priv_user2 for login no_priv_login2 +go + +create login no_priv_login3 with password = '12345678' +go + +create user no_priv_user3 for login no_priv_login3 +go + +create role no_priv_role3; +go + +create database restrict_user_db1 +go + +use restrict_user_db1 +go + +create role dont_drop_role +go + +create user no_priv_user1 for login no_priv_login1 +go + +use master +go + +-- tsql user=no_priv_login1 password=12345678 +-- case-1 - when current user is guest +-- permission denied +select current_user,db_name() +go +~~START~~ +varchar#!#nvarchar +guest#!#master +~~END~~ + + +drop role dont_drop_role +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'dont_drop_role', because it does not exist or you do not have permission.)~~ + + +drop user no_priv_user2 +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'no_priv_user2', because it does not exist or you do not have permission.)~~ + + +-- tsql +select current_user,db_name() +go +~~START~~ +varchar#!#nvarchar +dbo#!#master +~~END~~ + + +create user no_priv_user1 for login no_priv_login1 +go + +-- tsql user=no_priv_login1 password=12345678 +-- case-2 - when current user has no privilege +-- permission denied +use master +go + +select current_user +go +~~START~~ +varchar +no_priv_user1 +~~END~~ + + +drop role dont_drop_role +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'dont_drop_role', because it does not exist or you do not have permission.)~~ + + +drop user no_priv_user2 +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'no_priv_user2', because it does not exist or you do not have permission.)~~ + + +-- tsql + +-- case-3 - when current login has privilege +-- 3.1 - when login is database owner +Alter authorization on database::restrict_user_db1 to no_priv_login2 +go + +-- tsql user=no_priv_login2 password=12345678 database=restrict_user_db1 +-- allowed +select current_user +go +~~START~~ +varchar +dbo +~~END~~ + + +drop role dont_drop_role +go + +drop user no_priv_user1 +go + +-- error +-- try to drop non existing role/user +drop role fake_role +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'fake_role', because it does not exist or you do not have permission.)~~ + + +drop user fake_user; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'fake_user', because it does not exist or you do not have permission.)~~ + + +drop user db +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'db', because it does not exist or you do not have permission.)~~ + + +drop role db_own +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'db_own', because it does not exist or you do not have permission.)~~ + + +drop user dbo_u1 +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'dbo_u1', because it does not exist or you do not have permission.)~~ + + +drop role db_owner_r1 +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'db_owner_r1', because it does not exist or you do not have permission.)~~ + + +-- should deny +-- try to drop dbo user, db_owner role +drop role db_owner +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'db_owner'.)~~ + + +drop user dbo +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'dbo'.)~~ + + + +create role dont_drop_role +go + +create user no_priv_user1 for login no_priv_login1 +go + +-- tsql +-- 3.2 - when login is sysadmin +-- allowed +drop user no_priv_user1 +go + +drop user no_priv_user2 +go + +drop role dont_drop_role +go + +-- 3.3 - when drop user/role has IF EXISTS clause +-- Should not throw error for the case where IF EXISTS clause is included +drop role if exists fake_role; +go + +drop user if exists fake_user; +go + +-- both of the following statements should be executed successfully since user/role exists +drop user if exists no_priv_user3; +go + +drop user if exists no_priv_role3; +go + +select count(*) from sys.database_principals where name like '%no_priv_%3'; +go +~~START~~ +int +0 +~~END~~ + + +-- psql +-- 3.4 - Try dropping role created from PG role +create role master_pguser1; +go + +-- tsql +drop user pguser1; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'pguser1', because it does not exist or you do not have permission.)~~ + + +drop role pguser1; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'pguser1', because it does not exist or you do not have permission.)~~ + + +-- it will still try to drop it, but permission denied will be generated since +-- bbf_role_admin won't have privilege to drop non-babelfish roles +drop user if exists pguser1; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: permission denied to drop role)~~ + + +drop role if exists pguser1; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: permission denied to drop role)~~ + + +-- psql +drop role master_pguser1; +go + +-- Need to terminate active session before cleaning up the login +SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) +WHERE sys.suser_name(usesysid) = 'no_priv_login1' AND backend_type = 'client backend' AND usesysid IS NOT NULL; +go +~~START~~ +bool +t +~~END~~ + + + +-- tsql +drop login no_priv_login1 +go + +-- psql +-- Need to terminate active session before cleaning up the login +SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) +WHERE sys.suser_name(usesysid) = 'no_priv_login2' AND backend_type = 'client backend' AND usesysid IS NOT NULL; +go +~~START~~ +bool +t +~~END~~ + + +-- tsql +drop login no_priv_login2 +go + +drop login no_priv_login3 +go + +drop database restrict_user_db1 +go diff --git a/test/JDBC/input/restrict_drop_user_role.mix b/test/JDBC/input/restrict_drop_user_role.mix new file mode 100644 index 0000000000..6bac815143 --- /dev/null +++ b/test/JDBC/input/restrict_drop_user_role.mix @@ -0,0 +1,203 @@ +-- tsql +create login no_priv_login1 with password = '12345678' +go + +create role dont_drop_role +go + +create login no_priv_login2 with password = '12345678' +go + +create user no_priv_user2 for login no_priv_login2 +go + +create login no_priv_login3 with password = '12345678' +go + +create user no_priv_user3 for login no_priv_login3 +go + +create role no_priv_role3; +go + +create database restrict_user_db1 +go + +use restrict_user_db1 +go + +create role dont_drop_role +go + +create user no_priv_user1 for login no_priv_login1 +go + +use master +go + +-- case-1 - when current user is guest +-- permission denied +-- tsql user=no_priv_login1 password=12345678 +select current_user,db_name() +go + +drop role dont_drop_role +go + +drop user no_priv_user2 +go + +-- tsql +select current_user,db_name() +go + +create user no_priv_user1 for login no_priv_login1 +go + +-- case-2 - when current user has no privilege +-- permission denied +-- tsql user=no_priv_login1 password=12345678 +use master +go + +select current_user +go + +drop role dont_drop_role +go + +drop user no_priv_user2 +go + +-- case-3 - when current login has privilege +-- tsql + +-- 3.1 - when login is database owner +Alter authorization on database::restrict_user_db1 to no_priv_login2 +go + +-- allowed +-- tsql user=no_priv_login2 password=12345678 database=restrict_user_db1 +select current_user +go + +drop role dont_drop_role +go + +drop user no_priv_user1 +go + +-- error +-- try to drop non existing role/user +drop role fake_role +go + +drop user fake_user; +go + +drop user db +go + +drop role db_own +go + +drop user dbo_u1 +go + +drop role db_owner_r1 +go + +-- should deny +-- try to drop dbo user, db_owner role +drop role db_owner +go + +drop user dbo +go + + +create role dont_drop_role +go + +create user no_priv_user1 for login no_priv_login1 +go + +-- 3.2 - when login is sysadmin +-- allowed +-- tsql +drop user no_priv_user1 +go + +drop user no_priv_user2 +go + +drop role dont_drop_role +go + +-- 3.3 - when drop user/role has IF EXISTS clause +-- Should not throw error for the case where IF EXISTS clause is included +drop role if exists fake_role; +go + +drop user if exists fake_user; +go + +-- both of the following statements should be executed successfully since user/role exists +drop user if exists no_priv_user3; +go + +drop user if exists no_priv_role3; +go + +select count(*) from sys.database_principals where name like '%no_priv_%3'; +go + +-- 3.4 - Try dropping role created from PG role +-- psql +create role master_pguser1; +go + +-- tsql +drop user pguser1; +go + +drop role pguser1; +go + +-- it will still try to drop it, but permission denied will be generated since +-- bbf_role_admin won't have privilege to drop non-babelfish roles +drop user if exists pguser1; +go + +drop role if exists pguser1; +go + +-- psql +drop role master_pguser1; +go + +-- Need to terminate active session before cleaning up the login +SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) +WHERE sys.suser_name(usesysid) = 'no_priv_login1' AND backend_type = 'client backend' AND usesysid IS NOT NULL; +go + + +-- tsql +drop login no_priv_login1 +go + +-- psql +-- Need to terminate active session before cleaning up the login +SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) +WHERE sys.suser_name(usesysid) = 'no_priv_login2' AND backend_type = 'client backend' AND usesysid IS NOT NULL; +go + +-- tsql +drop login no_priv_login2 +go + +drop login no_priv_login3 +go + +drop database restrict_user_db1 +go \ No newline at end of file