Skip to content

Commit

Permalink
Restrict privileges from Unauthorised TSQL logins (#2174)
Browse files Browse the repository at this point in the history
An unprivileged T-SQL login should not be allowed to create roles and users in Babelfish.

Avoid granting CREATEROLE and CREATEDB privilege to non-sysadmins logins Manage CREATEDB/CREATEROLE privileges as part of grant/revoke membership to/from sysadmin via TDS Port only. Though the grant sysadmin to user works from psql endpoint for superuser, it will not add CREATEDB/CREATEROLE privileges. If a TSQL user wants to have the sysadmin membership and CREATEDB /CREATEROLE privileges, it should alter the server role via TDS port. Issues Resolved
Any unprivileged Babelfish role should not grant/revoke sysadmin role or non-Babelfish roles to itself and to others from the PG port. Any unprivileged Babelfish role should not drop any role via PG port. Any unprivileged Babelfish role should not alter any role via PG port. Any unprivileged Babelfish role should not create any role via PG port. Restrict PG user to "grant sysadmin to user" to any user via PG port.

Task: BABEL-4573, BABEL-4574, BABEL-4646

Signed-off-by: Shalini Lohia [email protected]
  • Loading branch information
shalinilohia50 authored Dec 25, 2023
1 parent f52753a commit 41724c2
Show file tree
Hide file tree
Showing 30 changed files with 670 additions and 18 deletions.
2 changes: 1 addition & 1 deletion contrib/babelfishpg_tds/src/backend/tds/tdsutils.c
Original file line number Diff line number Diff line change
Expand Up @@ -1094,7 +1094,7 @@ handle_alter_role(AlterRoleStmt* alter_role_stmt)
if ((authForm && authForm->rolcanlogin) && (strcmp(defel->defname, "password") == 0 ||
strcmp(defel->defname, "connectionlimit") == 0 ||
strcmp(defel->defname, "validUntil") == 0))
allow_alter_role_operation = true;
allow_alter_role_operation = true;
else
{
allow_alter_role_operation = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4575,6 +4575,12 @@ and has_schema_privilege(sch.schema_id, 'USAGE')
and has_table_privilege(t.oid, 'SELECT,INSERT,UPDATE,DELETE,TRUNCATE,TRIGGER');
GRANT SELECT ON sys.views TO PUBLIC;

-- Update existing logins to remove createrole privilege
CREATE OR REPLACE PROCEDURE sys.bbf_remove_createrole_from_logins()
LANGUAGE C
AS 'babelfishpg_tsql', 'remove_createrole_from_logins';
CALL sys.bbf_remove_createrole_from_logins();

-- Drops the temporary procedure used by the upgrade script.
-- Please have this be one of the last statements executed in this upgrade script.
DROP PROCEDURE sys.babelfish_drop_deprecated_object(varchar, varchar, varchar);
Expand Down
8 changes: 4 additions & 4 deletions contrib/babelfishpg_tsql/src/backend_parser/gram-tsql-rule.y
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ tsql_CreateLoginStmt:
@1)); /* Must be first */
n->options = lappend(n->options,
makeDefElem("createdb",
(Node *)makeBoolean(true),
(Node *)makeBoolean(false),
@1));
n->options = lappend(n->options,
makeDefElem("createrole",
(Node *)makeBoolean(true),
(Node *)makeBoolean(false),
@1));
n->options = lappend(n->options,
makeDefElem("inherit",
Expand All @@ -64,11 +64,11 @@ tsql_CreateLoginStmt:
@1)); /* Must be first */
n->options = lappend(n->options,
makeDefElem("createdb",
(Node *)makeBoolean(true),
(Node *)makeBoolean(false),
@1));
n->options = lappend(n->options,
makeDefElem("createrole",
(Node *)makeBoolean(true),
(Node *)makeBoolean(false),
@1));
n->options = lappend(n->options,
makeDefElem("inherit",
Expand Down
18 changes: 18 additions & 0 deletions contrib/babelfishpg_tsql/src/pl_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -2653,6 +2653,13 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
}
else if (isuser || isrole)
{
const char *db_owner_name;

db_owner_name = get_db_owner_name(get_cur_db_name());
if (!has_privs_of_role(GetUserId(),get_role_oid(db_owner_name, false)))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("User does not have permission to perform this action.")));
/*
* check whether sql user name and role name contains
* '\' or not
Expand Down Expand Up @@ -3355,10 +3362,18 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
{
const char *prev_current_user;
const char *session_user_name;
StringInfoData query;
RoleSpec *spec;

check_alter_server_stmt(grant_role);
prev_current_user = GetUserNameFromId(GetUserId(), false);
session_user_name = GetUserNameFromId(GetSessionUserId(), false);
spec = (RoleSpec *) linitial(grant_role->grantee_roles);
initStringInfo(&query);
if (grant_role->is_grant)
appendStringInfo(&query, "ALTER ROLE dummy WITH createrole createdb; ");
else
appendStringInfo(&query, "ALTER ROLE dummy WITH nocreaterole nocreatedb; ");

bbf_set_current_user(session_user_name);
PG_TRY();
Expand All @@ -3370,17 +3385,20 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
else
standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
queryEnv, dest, qc);
exec_alter_role_cmd(query.data, spec);

}
PG_CATCH();
{
/* Clean up. Restore previous state. */
bbf_set_current_user(prev_current_user);
pfree(query.data);
PG_RE_THROW();
}
PG_END_TRY();
/* Clean up. Restore previous state. */
bbf_set_current_user(prev_current_user);
pfree(query.data);
return;
}
else if (is_alter_role_stmt(grant_role))
Expand Down
1 change: 1 addition & 0 deletions contrib/babelfishpg_tsql/src/pltsql.h
Original file line number Diff line number Diff line change
Expand Up @@ -2190,5 +2190,6 @@ extern int64 last_scope_identity_value(void);
*/
void GetOpenqueryTupdescFromMetadata(char *linked_server, char *query, TupleDesc *tupdesc);
extern void exec_utility_cmd_helper(char *query_str);
extern void exec_alter_role_cmd(char *query_str, RoleSpec *role);

#endif /* PLTSQL_H */
50 changes: 50 additions & 0 deletions contrib/babelfishpg_tsql/src/pltsql_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -1900,4 +1900,54 @@ char
else
appendStringInfo(&query, "\"%s\".\"%s\"", schema_name, index_name);
return query.data;

}

/*
* Helper function to execute ALTER ROLE command using
* ProcessUtility(). Caller should make sure their
* inputs are sanitized to prevent unexpected behaviour.
*/
void
exec_alter_role_cmd(char *query_str, RoleSpec *role)
{
List *parsetree_list;
Node *stmt;
PlannedStmt *wrapper;

parsetree_list = raw_parser(query_str, RAW_PARSE_DEFAULT);

if (list_length(parsetree_list) != 1)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("Expected 1 statement but get %d statements after parsing",
list_length(parsetree_list))));

/* Update the dummy statement with real values */
stmt = parsetree_nth_stmt(parsetree_list, 0);

/* Update dummy statement with real values */
update_AlterRoleStmt(stmt, role);

/* Run the built query */
/* need to make a wrapper PlannedStmt */
wrapper = makeNode(PlannedStmt);
wrapper->commandType = CMD_UTILITY;
wrapper->canSetTag = false;
wrapper->utilityStmt = stmt;
wrapper->stmt_location = 0;
wrapper->stmt_len = strlen(query_str);

/* do this step */
ProcessUtility(wrapper,
query_str,
false,
PROCESS_UTILITY_SUBCOMMAND,
NULL,
NULL,
None_Receiver,
NULL);

/* make sure later steps can see the object created here */
CommandCounterIncrement();
}
52 changes: 49 additions & 3 deletions contrib/babelfishpg_tsql/src/rolecmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -1562,15 +1562,15 @@ is_alter_server_stmt(GrantRoleStmt *stmt)
{
RoleSpec *spec = (RoleSpec *) linitial(stmt->granted_roles);

if (strcmp(spec->rolename, "sysadmin") != 0) /* only supported server
if (strcmp(spec->rolename, "sysadmin") == 0) /* only supported server
* role */
return false;
return true;
}
/* has one and only one grantee */
if (list_length(stmt->grantee_roles) != 1)
return false;

return true;
return false;
}

void
Expand Down Expand Up @@ -2342,3 +2342,49 @@ check_windows_logon_length(char *input)
else
return false;
}

PG_FUNCTION_INFO_V1(remove_createrole_from_logins);
Datum
remove_createrole_from_logins(PG_FUNCTION_ARGS)
{
Relation rel;
TableScanDesc scan;
HeapTuple tuple;

rel = table_open(get_authid_login_ext_oid(), AccessShareLock);
scan = table_beginscan_catalog(rel, 0, NULL);
tuple = heap_getnext(scan, ForwardScanDirection);

while (HeapTupleIsValid(tuple))
{
Form_authid_login_ext loginform;
char *rolname;
loginform = (Form_authid_login_ext) GETSTRUCT(tuple);
rolname = pstrdup(NameStr(loginform->rolname));

/*
* For each login (except sysadmin and the member of sysadmin), remove
* createrole and createdb privileges from the logins.
*/
if ((strcmp(rolname, "sysadmin") != 0) && !has_privs_of_role(get_role_oid(rolname, false), get_role_oid("sysadmin", false)))
{
StringInfoData query;
RoleSpec *role;

role = makeNode(RoleSpec);
role->roletype = ROLESPEC_CSTRING;
role->location = -1;
role->rolename = rolname;
initStringInfo(&query);

appendStringInfo(&query, "ALTER ROLE dummy WITH nocreaterole nocreatedb; ");
exec_alter_role_cmd(query.data, role);
pfree(query.data);
}
pfree(rolname);
tuple = heap_getnext(scan, ForwardScanDirection);
}
table_endscan(scan);
table_close(rel, AccessShareLock);
PG_RETURN_INT32(0);
}
89 changes: 84 additions & 5 deletions test/JDBC/expected/ownership_restrictions_from_pg.out
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,72 @@
CREATE LOGIN ownership_restrictions_from_pg_login1 WITH password = '12345678';
GO

CREATE LOGIN ownership_restrictions_from_pg_login2 WITH password = '12345678';
GO

-- tsql user=ownership_restrictions_from_pg_login2 password=12345678
CREATE ROLE ownership_restrictions_from_pg_role_by_pg_login2;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: User does not have permission to perform this action.)~~


CREATE USER ownership_restrictions_from_pg_user_by_pg_login2;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: User does not have permission to perform this action.)~~


-- tsql
ALTER SERVER ROLE sysadmin ADD MEMBER ownership_restrictions_from_pg_login2;
GO

-- tsql user=ownership_restrictions_from_pg_login2 password=12345678
CREATE DATABASE ownership_restrictions_from_pg_login2_db1;
GO

-- tsql
ALTER SERVER ROLE sysadmin DROP MEMBER ownership_restrictions_from_pg_login2;
GO

-- tsql user=ownership_restrictions_from_pg_login2 password=12345678
USE ownership_restrictions_from_pg_login2_db1;
GO

SELECT current_user;
GO
~~START~~
varchar
dbo
~~END~~


CREATE ROLE ownership_restrictions_from_pg_role_by_pg_login2;
GO

DROP ROLE ownership_restrictions_from_pg_role_by_pg_login2;
GO

-- This is a temporary failure, it will be fixed with BABEL-4652.
CREATE USER ownership_restrictions_from_pg_user_by_pg_login2;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: errstart was not called)~~



-- DROP USER ownership_restrictions_from_pg_user_by_pg_login2;
-- GO
USE master;
go

-- tsql
DROP DATABASE ownership_restrictions_from_pg_login2_db1;
go

CREATE ROLE ownership_restrictions_from_pg_role1;
GO

Expand Down Expand Up @@ -238,6 +304,11 @@ GO

ALTER ROLE ownership_restrictions_from_pg_login1 VALID UNTIL 'infinity';
GO
~~ERROR (Code: 0)~~

~~ERROR (Message: ERROR: permission denied
Server SQLState: 42501)~~


ALTER ROLE ownership_restrictions_from_pg_login1 rename to master_ownership_restrictions_from_pg_role5;
GO
Expand Down Expand Up @@ -368,14 +439,12 @@ GO
Server SQLState: 42501)~~


-- after connection limit set to 1, shouldn't be able to connect to multiple sessions
ALTER ROLE ownership_restrictions_from_pg_login1 WITH CONNECTION LIMIT 1;
GO
~~ERROR (Code: 0)~~

-- tsql user=ownership_restrictions_from_pg_login1 password=12345678
~~ERROR (Code: 33557097)~~

~~ERROR (Message: too many connections for role "ownership_restrictions_from_pg_login1" )~~
~~ERROR (Message: ERROR: permission denied
Server SQLState: 42501)~~


-- psql user=ownership_restrictions_from_pg_login1 password=12345678
Expand Down Expand Up @@ -840,8 +909,18 @@ t
~~END~~


SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL)
WHERE sys.suser_name(usesysid) = 'ownership_restrictions_from_pg_login2' AND backend_type = 'client backend' AND usesysid IS NOT NULL;
GO
~~START~~
bool
t
~~END~~


-- tsql
DROP DATABASE ownership_restrictions_from_pg_db;
DROP ROLE ownership_restrictions_from_pg_role1;
DROP LOGIN ownership_restrictions_from_pg_login1;
DROP LOGIN ownership_restrictions_from_pg_login2;
GO
11 changes: 11 additions & 0 deletions test/JDBC/expected/permission_restrictions_from_pg-vu-prepare.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-- tsql
create login permission_restrictions_tsql_login with password = '123';
go

select rolname, rolcreaterole, rolcreatedb from pg_roles where rolname = 'permission_restrictions_tsql_login';
go
~~START~~
varchar#!#bit#!#bit
permission_restrictions_tsql_login#!#1#!#1
~~END~~

Loading

0 comments on commit 41724c2

Please sign in to comment.