Horton

Thread

Commits

  1. On Windows, use COMSPEC to find the location of cmd.exe.

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

    The Post Office <noreply@postgresql.org> — 2019-10-26T04:30:28Z

    The following bug has been logged on the website:

Bug reference:      16080
Logged by:          cili
Email address:      cilizili@protonmail.com
PostgreSQL version: 12.0
Operating system:   Microsoft Windows [Version 10.0.18362.418]
Description:        

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

Instructions:
# cd %TEMP%
# "c:\Program Files\PostgreSQL\12\bin\pg_ctl.exe" initdb -D test
# copy %windir%\system32\calc.exe cmd.exe
# "c:\Program Files\PostgreSQL\12\bin\pg_ctl.exe" start -D test
waiting for server to start.... stopped waiting
pg_ctl: could not start server
Examine the log output.

Expected:
The PostgreSQL is started.

Actual:
The Windows calculator instead of PostgreSQL is started.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-26T13:36:41Z

    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

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

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2019-10-26T14:15:04Z

    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].

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

Regards,

Juan José Santamaría Flecha

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-26T15:20:04Z

    =?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

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

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2019-10-26T17:33:02Z

    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.


> 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-26T17:44:00Z

    =?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

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

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2019-10-27T10:07:31Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-27T15:42:46Z

    =?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

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

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2019-10-27T20:55:45Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-28T18:16:21Z

    =?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