Hey, hackers,
As part of building Postgres for a managed environment in Google Cloud SQL,
we've made various small changes to permission checks to carefully limit what
customers have access to.
We were making some changes in src/backend/commands/user.c and noticed some
clunky logic related to verifying that the current user has sufficient
permissions to perform an action:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/user.c;h=0e6800bf3e4f3f584bbb0b2f6f8ae999f5d94bf1;hb=HEAD#l288
The checks for whether the current user can create a user with the SUPERUSER,
REPLICATION, or BYPASSRLS attributes are chained together using if/else-if,
before finally checking whether the user has CREATEROLE privileges in a
final else. This construction is error-prone, since once one branch is visited,
later ones are skipped, and it implicitly assumes that the permissions needed
for each subsequent action are subsets of the permissions needed for the
previous action. Since each branch requires SUPERUSER this is fine, but
changing one of the checks could inadvertently allow users without the
CREATEROLE permission to create users.
A similar construction is used later in the file in the AlterRole function:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/user.c;h=0e6800bf3e4f3f584bbb0b2f6f8ae999f5d94bf1;hb=HEAD#l717
This small patch simply modifies the code to replace the if/else-if chain with
separate if statements, and always checks whether the user has the CREATEROLE
privilege. (The have_createrole_privilege function includes another superuser
check.) Given the current checks in each branch, this code is equivalent, but
easier to modify in the future.
- Paul
