Thread: Prevent pg_basebackup running as root

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 :)

> +    /*
> +     * 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.
 */

> +    #ifndef WIN32

Indentation here.

> +    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);

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

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

On Thu, Jan 30, 2020 at 03:38:54PM +0900, Ian Barwick wrote:
> 2020年1月30日(木) 14:57 Michael Paquier <michael@paquier.xyz>:

I have never noticed that your client was configured in Japanese :)

> 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,

Okay, fine by me.

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

Ahh, indeed.  pg_rewind has inherited its message from pg_resetwal.

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

Not sure that's a good idea.  Each tool has its own properties, so it
is good to keep some flexibility in the error message produced.

Anyway, your patch looks like a good idea to me, so let's see if
others have opinions or objections about it.
--
Michael

On Thu, Jan 30, 2020 at 04:00:40PM +0900, Michael Paquier wrote:
> Anyway, your patch looks like a good idea to me, so let's see if
> others have opinions or objections about it.

Seeing nothing, committed v2.
--
Michael

On 2020/02/01 18:34, Michael Paquier wrote:
> On Thu, Jan 30, 2020 at 04:00:40PM +0900, Michael Paquier wrote:
>> Anyway, your patch looks like a good idea to me, so let's see if
>> others have opinions or objections about it.
> 
> Seeing nothing, committed v2.

Thanks!


Regards

Ian Barwick


-- 
Ian Barwick                   https://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

Hi,

On 2020-02-01 18:34:33 +0900, Michael Paquier wrote:
> Seeing nothing, committed v2.

For reference: As a consequence of the discussion starting at
https://www.postgresql.org/message-id/20200205172259.GW3195%40tamriel.snowman.net
this patch has been reverted, at least for now.

Greetings,

Andres Freund