Fix pg_rewind which can be run as root user
Hi all,
I was just going through pg_rewind's code, and noticed the following
pearl:
/*
* Don't allow pg_rewind to be run as root, to avoid overwriting the
* ownership of files in the data directory. We need only check for root
* -- any other user won't have sufficient permissions to modify files in
* the data directory.
*/
#ifndef WIN32
if (geteuid() == 0)
{
fprintf(stderr, _("cannot be executed by \"root\"\n"));
fprintf(stderr, _("You must run %s as the PostgreSQL superuser.\n"),
progname);
}
#endif
While that's nice to inform the user about the problem, that actually
does not prevent pg_rewind to run as root. Attached is a patch, which
needs a back-patch down to 9.5.
Thanks,
--
Michael
Attachments:
rewind-root-run.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index b9ea6a4c21..a1ab13963a 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -208,6 +208,7 @@ main(int argc, char **argv)
fprintf(stderr, _("cannot be executed by \"root\"\n"));
fprintf(stderr, _("You must run %s as the PostgreSQL superuser.\n"),
progname);
+ exit(1);
}
#endif
On Mon, Apr 9, 2018 at 7:11 AM, Michael Paquier <michael@paquier.xyz> wrote:
Hi all,
I was just going through pg_rewind's code, and noticed the following
pearl:
/*
* Don't allow pg_rewind to be run as root, to avoid overwriting the
* ownership of files in the data directory. We need only check for
root
* -- any other user won't have sufficient permissions to modify files
in
* the data directory.
*/
#ifndef WIN32
if (geteuid() == 0)
{
fprintf(stderr, _("cannot be executed by \"root\"\n"));
fprintf(stderr, _("You must run %s as the PostgreSQL
superuser.\n"),
progname);
}
#endifWhile that's nice to inform the user about the problem, that actually
does not prevent pg_rewind to run as root. Attached is a patch, which
needs a back-patch down to 9.5.
Seems simple enough and the right hting to do, but I wonder if we should
really backpatch it. Yes, the behaviour is not great now, but there is also
a non-zero risk of breaking peoples automated failover scripts of we
backpatch it, isn't it?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
On Mon, Apr 9, 2018 at 12:23 PM, Magnus Hagander <magnus@hagander.net> wrote:
Seems simple enough and the right hting to do, but I wonder if we should
really backpatch it. Yes, the behaviour is not great now, but there is also
a non-zero risk of breaking peoples automated failover scripts of we
backpatch it, isn't it?
+1 to committing to master as a documented behavioral change.
--
Peter Geoghegan
Magnus Hagander <magnus@hagander.net> writes:
Seems simple enough and the right hting to do, but I wonder if we should
really backpatch it. Yes, the behaviour is not great now, but there is also
a non-zero risk of breaking peoples automated failover scripts of we
backpatch it, isn't it?
Yeah, I'd vote against backpatching. This doesn't seem like an essential
fix.
regards, tom lane
On Mon, Apr 9, 2018 at 9:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
Seems simple enough and the right hting to do, but I wonder if we should
really backpatch it. Yes, the behaviour is not great now, but there isalso
a non-zero risk of breaking peoples automated failover scripts of we
backpatch it, isn't it?Yeah, I'd vote against backpatching. This doesn't seem like an essential
fix.
Applied, and pushed this way.
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>