Re: Prevent pg_basebackup running as root

2020年1月30日(木) 14:57 Michael Paquier <michael(at)paquier(dot)xyz>:
>
> On Thu, Jan 30, 2020 at 02:29:06PM +0900, Ian Barwick wrote:
> > I can't think of any practical reason why pg_basebackup would ever need to
> > be run as root; we disallow that for initdb, pg_ctl and pg_upgrade, so it
> > seems reasonable to do the same for pg_basebackup. Trivial patch attached,
> > which as with the other cases will allow only the --help/--version options
> > to be executed as root, otherwise nothing else.
>
> My take on the matter is that we should prevent anything creating or
> modifying the data directory to run as root if we finish with
> permissions incompatible with what a postmaster expects. So +1.
>
> > The patch doesn't update the pg_basebackup documentation page; we don't
> > mention it in the pg_ctl and pg_upgrade pages either and it doesn't seem
> > particularly important to mention it explicitly.
>
> We don't mention that in the docs of pg_rewind either. Note also that
> before 5d5aedd pg_rewind printed an error without exiting :)

Ouch.

> > + /*
> > + * Disallow running as root, as PostgreSQL will be unable to start
> > + * with root-owned files.
> > + */
>
> Here is a suggestion:
> /*
> * Don't allow pg_basebackup to be run as root, to avoid creating
> * files in the data directory with ownership rights incompatible
> * with the postmaster. We need only check for root -- any other user
> * won't have sufficient permissions to modify files in the data
> * directory.
> */

I think we can skip the second sentence altogether. It'd be theoretically
easy enough to up with some combination of group permissions,
sticky bits, umask, ACL settings etc/ which would allow one user to
modify the files owned by another user,

> > + #ifndef WIN32
>
> Indentation here.

Whoops, that's what comes from typing on the train ;)

> > + if (geteuid() == 0) /* 0 is root's uid */
> > + {
> > + pg_log_error("cannot be run as root");
> > + fprintf(stderr,
> > + _("Please log in (using, e.g., \"su\") as the (unprivileged) user that will\n"
> > + "own the server process.\n"));
> > + exit(1);
> > + }
> > +#endif
>
> I would recommend to map with the existing message of pg_rewind for
> consistency:
> pg_log_error("cannot be executed by \"root\"");
> fprintf(stderr, _("You must run %s as the PostgreSQL superuser.\n"),
> progname);

Hmm, I was using the existing message from initdb and pg_ctl for consistency:

src/bin/initdb/initdb.c:

if (geteuid() == 0) /* 0 is root's uid */
{
pg_log_error("cannot be run as root");
fprintf(stderr,
_("Please log in (using, e.g., \"su\") as the
(unprivileged) user that will\n"
"own the server process.\n"));
exit(1);
}

src/bin/pg_ctl/pg_ctl.c:

if (geteuid() == 0)
{
write_stderr(_("%s: cannot be run as root\n"
"Please log in (using, e.g., \"su\") as the "
"(unprivileged) user that will\n"
"own the server process.\n"),
progname);
exit(1);
}

src/bin/pg_upgrade/option.c:

if (os_user_effective_id == 0)
pg_fatal("%s: cannot be run as root\n", os_info.progname);

I wonder if it would be worth settling on a common message and way of emitting
it, each utility does it slightly differently.

> A backpatch could be surprising for some users as that's a behavior
> change, so I would recommend not to do a backpatch.

Agreed.

Regards

Ian Barwick