Re: Introduce psystem() to replace system() - Mailing list pgsql-hackers

Hello!!

For some time I've been wondering why PostgreSQL needs system() calls,
which use a shell that can lead to many problems, and also why it
requires a shell to run a command.

I first started thinking about this when I was trying to run a full
distroless PostgreSQL container. It turns out that isn't possible since
the shell is a requirement, and distroless containers are secure exactly
_because_ there's no shell to execute any command other than the ones
that are meant to be executed.

After some research I found out that using system() has other problems,
like issues related to quoting that are really painful to solve [0][1],
and also the exit codes control[2]. Both topics have already been
discussed on the list.

But the main argument now for me is security. Not having a shell avoids
any possible PATH injection, missing quoting to escape a command, or new
lines that the shell interprets differently from what you'd expect.

After some thinking I came up with a small interface, which only purpose
is to replace system() calls in a more smooth way using execv() under
the hood. I suppose you could use execl() but I've decided to keep it
simple, leaving the opportunity to expand in the future. I already
implemented one call with `pg_ctl initdb` as an example.

There's an important topic related to using shell versus not a shell. In
some places like `archive_command` people may use `&&`, but this idea
aims to avoid this kind of behavior since it's not secure. Probably we
can implement a way to run commands in sequence, or simply tell the
users that this isn't allowed anymore, but it's possible to trigger
commands in sequence since the interface allows to manipulate the STDIN
and STDOUT.

I would like to open the discussion here if this is the right direction.
There's a lot to do and this still a work in progress, the current patch
is small and simple, but already provides building blocks in this direction.

Hi Jonathan

Thanks for working on this. I think avoiding the shell for cases where PostgreSQL is just
invoking a known executable makes sense.

There is some related work here which may be worth looking at:

https://www.postgresql.org/message-id/flat/CAGECzQQh6VSy3KG4pN1d%3Dh9J%3DD1rStFCMR%2Bt7yh_Kwj-g87aLQ%40mail.gmail.com

That effort ran into some similar issues around replacing system()/popen() and handling the
fork/exec boundary.

A few things I noticed in this patch:

1. system() and execv() do not have the same semantics. For internal commands such as pg_ctl
invoking initdb, that is probably fine and preferable. But for things like archive_command,
shell features such as PATH lookup, &&, pipes and redirection are part of the existing
interface, so changing those would be a compatibility decision rather than just an
implementation change.
2. For pipelines, pcommand_exec() only knows the stdin/stdout/stderr FDs. After fork(),
however, a child may inherit other pipe endpoints as well. Those need to be closed before
exec(), otherwise an unused write end can keep a pipe alive and prevent EOF from being seen.
3. I also wonder about returning errno from pcommand_exec(). An execv() failure should
probably remain distinguishable from a program that successfully execs and later exits with
the same numeric status. An error pipe from child to parent may be useful here.

Overall I like the direction, but I think it would be useful to reuse some of the lessons
from the earlier pg_system() / pg_popen() work before this grows into a more general process
/pipeline API.

Regards,

[0] https://www.postgresql.org/message-id/7606.1153326421%40sss.pgh.pa.us
[1] https://www.postgresql.org/message-id/CA%2BTgmobBmWWCgPUd04NGoQ%3D_XvcidV%2BsE2F7KChEXfs8KBPg6w%40mail.gmail.com
[2] https://www.postgresql.org/message-id/21292.1358698487%40sss.pgh.pa.us

--
Jonathan Gonzalez V.
EDB https://enterprisedb.com