[PATCH] Simplify permission checking logic in user.c - Mailing list pgsql-hackers

From Paul Martinez
Subject [PATCH] Simplify permission checking logic in user.c
Date
Msg-id CACqFVBZz4Oh2TMFCYPc7yfS3W19K=Ly9i8ANhZp2yQ6tiSu5Pg@mail.gmail.com
Whole thread Raw
Responses Re: [PATCH] Simplify permission checking logic in user.c
Re: [PATCH] Simplify permission checking logic in user.c
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Andrey Borodin
Date:
Subject: Re: [HACKERS] Custom compression methods
Next
From: Thomas Munro
Date:
Subject: Re: Let's start using setenv()