[PATCH] Simplify permission checking logic in user.c

From: Paul Martinez <paulmtz(at)google(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: [PATCH] Simplify permission checking logic in user.c
Date: 2020-12-29 20:26:19
Message-ID: CACqFVBZz4Oh2TMFCYPc7yfS3W19K=Ly9i8ANhZp2yQ6tiSu5Pg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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 Content-Type Size
user-c-if-else-if.patch application/octet-stream 2.0 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2020-12-29 20:34:59 Re: Let's start using setenv()
Previous Message Andrey Borodin 2020-12-29 17:48:49 Re: [HACKERS] Custom compression methods