Re: BUG #15371: a user who not a member of pg_read_server_files role can create a new user into pg_read_server_files

From: "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>
To: "110876189(at)qq(dot)com" <110876189(at)qq(dot)com>, "pgsql-bugs(at)lists(dot)postgresql(dot)org" <pgsql-bugs(at)lists(dot)postgresql(dot)org>
Subject: Re: BUG #15371: a user who not a member of pg_read_server_files role can create a new user into pg_read_server_files
Date: 2018-09-08 07:37:01
Message-ID: CAKFQuwZtXU8QD6kZ4BBNgq0F1xrwq+mrStUkTRt+-W3vSgeuaw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

On Saturday, September 8, 2018, PG Bug reporting form <
noreply(at)postgresql(dot)org> wrote:

>
> see the code src/backend/commands/user.c, the check privillige code is :
> static void
> AddRoleMems(const char *rolename, Oid roleid,
> List *memberSpecs, List *memberIds,
> Oid grantorId, bool admin_opt)
> {
> else
> {
> if (!have_createrole_privilege() &&
> !is_admin_of_role(grantorId, roleid))
> ereport(ERROR,
> (errcode(ERRCODE_INSUFFICIENT_
> PRIVILEGE),
> errmsg("must have admin option on
> role \"%s\"",
> rolename)));
> }
>
> I think the line "if (!have_createrole_privilege() &&
> !is_admin_of_role(grantorId, roleid))" should been modifed to "if
> (!have_createrole_privilege() || !is_admin_of_role(grantorId, roleid))" .
>

Security code would ideally be written so not erroring requires an actual
matched condition while leaving the default to be an error. So, I think,
the && is ok, remove both negations, put a commented no-op there, and move
the error to an else block.

Whether that is sufficient for this bug I do not know, but it would be a
more secure format.

David J.

In response to

Browse pgsql-bugs by date

  From Date Subject
Next Message David G. Johnston 2018-09-08 07:41:45 Re: BUG #15371: a user who not a member of pg_read_server_files role can create a new user into pg_read_server_files
Previous Message PG Bug reporting form 2018-09-08 07:08:22 BUG #15372: pg_stat_statements extension ignore stats_temp_directory setting and always write into pg_stat_tmp