Re: Prevent pg_basebackup running as root

2020年1月30日(木) 14:57 Michael Paquier <michael@paquier.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