BUG #16080: pg_ctl is failed if a fake cmd.exe exist in the current directory.

PG Bug reporting form <noreply@postgresql.org> writes:

If a fake cmd.exe exits in the current directory, pg_ctl is failed to
start.

I do not think this is a bug. There are probably thousands of ways to
break Postgres by misconfiguring your system, and this is one of 'em.

(Likewise for your identical complaint about pg_upgrade.)

regards, tom lane

On Sat, Oct 26, 2019 at 3:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I do not think this is a bug. There are probably thousands of ways to
break Postgres by misconfiguring your system, and this is one of 'em.

There is a difference in the behaviour for the WIN32 port. In other
platforms execl() is called from "src/bin/pg_ctl/pc_ctl" with the full path
("/bin/sh"), but for WIN32 the function CreateProcessAsUser() calls the CMD
command without a path, and this function has a search logic of its own [1]https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessasusera.

If even this difference out is of any value I can propose a patch.

[1]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessasusera
https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessasusera

Regards,

Juan José Santamaría Flecha

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:

There is a difference in the behaviour for the WIN32 port. In other
platforms execl() is called from "src/bin/pg_ctl/pc_ctl" with the full path
("/bin/sh"), but for WIN32 the function CreateProcessAsUser() calls the CMD
command without a path, and this function has a search logic of its own [1].

Right, but does cmd.exe have a well-defined location in Windows?
I don't think we can know which drive it's on, for starters.

Ultimately this seems like a problem of insecure search path,
which is not our responsibility to fix (and people would not
appreciate us trying, in many cases).

regards, tom lane

On Sat, Oct 26, 2019 at 5:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Right, but does cmd.exe have a well-defined location in Windows?
I don't think we can know which drive it's on, for starters.

The environment variable COMSPEC [1]https://en.wikipedia.org/wiki/COMSPEC should point to the right location.

Ultimately this seems like a problem of insecure search path,
which is not our responsibility to fix (and people would not
appreciate us trying, in many cases).

Totally agree, if this not is an improvement there is nothing to fix.

[1]: https://en.wikipedia.org/wiki/COMSPEC

Regards,

Juan José Santamaría Flecha

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:

On Sat, Oct 26, 2019 at 5:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Right, but does cmd.exe have a well-defined location in Windows?
I don't think we can know which drive it's on, for starters.

The environment variable COMSPEC [1] should point to the right location.

Hm. I don't have any objection to using COMSPEC if it's set, but
of course that changes nothing from a security perspective. It's
just a different route by which pg_ctl, pg_upgrade, etc can be
misled.

regards, tom lane

On Sat, Oct 26, 2019 at 7:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <
juanjo.santamaria@gmail.com> writes:

On Sat, Oct 26, 2019 at 5:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Right, but does cmd.exe have a well-defined location in Windows?
I don't think we can know which drive it's on, for starters.

The environment variable COMSPEC [1] should point to the right location.

Hm. I don't have any objection to using COMSPEC if it's set, but
of course that changes nothing from a security perspective. It's
just a different route by which pg_ctl, pg_upgrade, etc can be
misled.

The only impact this will have is finding the CMD executable directly,
without having to rely on CreateProcessAsUser() logic.

Please find attached a patch with this simple modification.

Regards,

Juan José Santamaría Flecha

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:

On Sat, Oct 26, 2019 at 7:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hm. I don't have any objection to using COMSPEC if it's set, but
of course that changes nothing from a security perspective. It's
just a different route by which pg_ctl, pg_upgrade, etc can be
misled.

Please find attached a patch with this simple modification.

I poked around a bit for other references to cmd.exe. It looks
like psql's do_shell() is handling this correctly already, but should
we not also fix spawn_process() in src/test/regress/pg_regress.c ?

There are also a couple of references in pg_upgrade's test.sh,
but I don't feel a need to change those.

Another point that could be raised here: seeing that psql honors the
SHELL variable to substitute for /bin/sh, should these other programs
do likewise? I'm inclined to think not, because what psql is doing is
launching an interactive shell, so the user's shell preference should be
honored. In these other cases we want plain old Bourne shell thank you,
so ignoring SHELL seems correct. But it's worth thinking about, and
perhaps adding a comment about.

regards, tom lane

On Sun, Oct 27, 2019 at 4:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <
juanjo.santamaria@gmail.com> writes:

On Sat, Oct 26, 2019 at 7:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hm. I don't have any objection to using COMSPEC if it's set, but
of course that changes nothing from a security perspective. It's
just a different route by which pg_ctl, pg_upgrade, etc can be
misled.

Please find attached a patch with this simple modification.

I poked around a bit for other references to cmd.exe. It looks
like psql's do_shell() is handling this correctly already, but should
we not also fix spawn_process() in src/test/regress/pg_regress.c ?

Agreed, so please find attached an updated patch.

There are also a couple of references in pg_upgrade's test.sh,
but I don't feel a need to change those.

Agreed, this will honor PATH since is called from a shell,

Another point that could be raised here: seeing that psql honors the
SHELL variable to substitute for /bin/sh, should these other programs
do likewise? I'm inclined to think not, because what psql is doing is
launching an interactive shell, so the user's shell preference should be
honored. In these other cases we want plain old Bourne shell thank you,
so ignoring SHELL seems correct. But it's worth thinking about, and
perhaps adding a comment about.

Also agree on this: honoring SHELL makes sense only if there is client
interaction.

Regards,

Juan José Santamaría Flecha

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:

On Sun, Oct 27, 2019 at 4:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I poked around a bit for other references to cmd.exe. It looks
like psql's do_shell() is handling this correctly already, but should
we not also fix spawn_process() in src/test/regress/pg_regress.c ?

Agreed, so please find attached an updated patch.

Looks OK now, so pushed. (I don't have the ability to test this
locally, but the buildfarm will soon tell us if it's wrong.)

regards, tom lane