Thread: [PATCH] (Windows) psql echoes password when reading from pipe

This is my first time submitting a patch here; apologies in advance if I 
flub the process.

On windows, if you pipe data to psql, the password prompt correctly 
reads from and writes to the console, but the password text is echoed to 
the console. This is because echoing is disabled on the handle for 
stdin, but as part of a pipeline stdin doesn't refer to the console. 
I've attached a patch that gets a handle to the console's input buffer 
by opening CONIN$ instead, which corrects the problem.

I think the change is straightforward enough to apply directly, but 
there's another concern that might bear discussion: SetConsoleMode can 
fail, and when it does prompt input will be echoed (i.e. it's 
fail-open). Is it worth check for and reporting for an error there? 
Given that this is meant to be used interactively, I think the risk of 
someone not noticing the echo is low.

-Matt Stickney

Matthew Stickney <mtstickney@gmail.com> writes:
> On windows, if you pipe data to psql, the password prompt correctly
> reads from and writes to the console, but the password text is echoed to
> the console. This is because echoing is disabled on the handle for
> stdin, but as part of a pipeline stdin doesn't refer to the console.
> I've attached a patch that gets a handle to the console's input buffer
> by opening CONIN$ instead, which corrects the problem.

Thanks for the report and patch!

I know zip about Windows coding, but I can't help comparing this:

-        t = GetStdHandle(STD_INPUT_HANDLE);
+        t = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
OPEN_EXISTING,0, NULL); 

to the code a little bit above:

    termin = fopen("CONIN$", "r");

Is it possible to operate on "termin" instead of doing a second open
(which might fail, which we are failing to cope with :-()?

            regards, tom lane

It is possible, at the cost of two extra function calls, which could
theoretically fail (and require a little extra munging to work on
Windows CE -- is that a target platform for postgres?).

Similar to using CreateFile, I think the cases in which those calls
could fail are so extraordinary that psql probably wouldn't run at all
(i.e. there's no console for the process, in which case psql would
crash as soon as it attempted to do IO). It also requires opening
termin with "w+", because SetConsoleMode needs write permissions. You
could avoid that by doing a DuplicateHandle on the underlying handle
that you retrieve from termin, but I doubt it's worth it.

I'll do up a modified patch tonight, if that sounds like it'd be better.

-Matt Stickney

On Tue, May 22, 2018 at 2:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Matthew Stickney <mtstickney@gmail.com> writes:
>> On windows, if you pipe data to psql, the password prompt correctly
>> reads from and writes to the console, but the password text is echoed to
>> the console. This is because echoing is disabled on the handle for
>> stdin, but as part of a pipeline stdin doesn't refer to the console.
>> I've attached a patch that gets a handle to the console's input buffer
>> by opening CONIN$ instead, which corrects the problem.
>
> Thanks for the report and patch!
>
> I know zip about Windows coding, but I can't help comparing this:
>
> -               t = GetStdHandle(STD_INPUT_HANDLE);
> +               t = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
OPEN_EXISTING,0, NULL);
 
>
> to the code a little bit above:
>
>         termin = fopen("CONIN$", "r");
>
> Is it possible to operate on "termin" instead of doing a second open
> (which might fail, which we are failing to cope with :-()?
>
>                         regards, tom lane

[ please don't top-post ]

Matthew Stickney <mtstickney@gmail.com> writes:
> On Tue, May 22, 2018 at 2:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Is it possible to operate on "termin" instead of doing a second open
>> (which might fail, which we are failing to cope with :-()?

> It is possible, at the cost of two extra function calls, which could
> theoretically fail (and require a little extra munging to work on
> Windows CE -- is that a target platform for postgres?).

AFAIK we don't claim to support CE.

> Similar to using CreateFile, I think the cases in which those calls
> could fail are so extraordinary that psql probably wouldn't run at all
> (i.e. there's no console for the process, in which case psql would
> crash as soon as it attempted to do IO). It also requires opening
> termin with "w+", because SetConsoleMode needs write permissions. You
> could avoid that by doing a DuplicateHandle on the underlying handle
> that you retrieve from termin, but I doubt it's worth it.

Hm.  The failure mode I was thinking about was insufficient resources
to allocate another handle; so it seems like all of these are more or
less equivalent on that basis, and we might as well go with whatever's
shortest.  But perhaps it's worth adding logic to deal with failure
of the call?  Or can we expect the additional calls to fall through
silently anyway if given an invalid handle?  (If so, maybe a comment
about that is sufficient.)

            regards, tom lane

On Tue, May 22, 2018 at 4:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> [ please don't top-post ]

Sorry, I'm used to using a better mail client than this.

> Hm.  The failure mode I was thinking about was insufficient resources
> to allocate another handle

You have a point here; CreateFile does create a new handle (as would
DuplicateHandle), while _fileno and _get_osfhandle just retrieve the
existing file descriptor and handle, respectively. I checked the docs
more carefully, and both calls should never fail unless the input
stream is invalid.

> But perhaps it's worth adding logic to deal with failure of the call?

I think it would be sufficient to check whether the SetConsoleMode
call fails, because that can fail even on a valid handle (e.g. if you
don't have a handle with write access). That would catch the case
where opening termin fails, too, although that might deserve it's own
check to get better error information.

simple_prompt seems to be used in a lot of different utilities for
different reasons; there seem to be a number of conventions for
reporting errors in src/port/ code, but it looks like other
interactive utilities generally print a message to stderr, and return
a basic success/failure value. Does that sound like the right
approach? I'm not sure if it's obvious how to handle errors in the
other utilities, but I can take a look.

-Matt Stickney

Matthew Stickney <mtstickney@gmail.com> writes:
> On Tue, May 22, 2018 at 4:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> But perhaps it's worth adding logic to deal with failure of the call?

> I think it would be sufficient to check whether the SetConsoleMode
> call fails, because that can fail even on a valid handle (e.g. if you
> don't have a handle with write access). That would catch the case
> where opening termin fails, too, although that might deserve it's own
> check to get better error information.

> simple_prompt seems to be used in a lot of different utilities for
> different reasons; there seem to be a number of conventions for
> reporting errors in src/port/ code, but it looks like other
> interactive utilities generally print a message to stderr, and return
> a basic success/failure value. Does that sound like the right
> approach? I'm not sure if it's obvious how to handle errors in the
> other utilities, but I can take a look.

Well, the question that ought to be answered first is whether to do
anything at all, beyond not-crashing.  It doesn't seem to me that
refusing to accept a password if we can't disable echo is a net win,
so I'm inclined to think it's okay to silently ignore failure to
turn off echo.

The other aspect of this code that maybe needs consideration is the
silent fallback to use stdin/stderr if we can't open the console.
It seems like that could be a really bad idea if stdin is a pipe.
But on the other hand, maybe somebody is depending on it somewhere?
I'm not sure I'd want to back-patch a change in that behavior, anyway.

            regards, tom lane

On Tue, May 22, 2018 at 6:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Well, the question that ought to be answered first is whether to do
> anything at all, beyond not-crashing.  It doesn't seem to me that
> refusing to accept a password if we can't disable echo is a net win,
> so I'm inclined to think it's okay to silently ignore failure to
> turn off echo.

As a middle ground, I suppose you could print a warning message but
not bail out of simple_prompt; that doesn't mean much for users, since
echo only happens when you're actually typing something at a console,
where echoing is obvious. The chief benefit is for developers: once
somebody notices echoing hasn't been disabled, somebody's going to
have to figure out why, and a good error message might make that
easier.

> The other aspect of this code that maybe needs consideration is the
> silent fallback to use stdin/stderr if we can't open the console.
> It seems like that could be a really bad idea if stdin is a pipe.
> But on the other hand, maybe somebody is depending on it somewhere?
> I'm not sure I'd want to back-patch a change in that behavior, anyway.

In terms of the echoing, this isn't an issue: if there's no pipe,
stdin is a wrapper around CONIN$ already, and if it's a pipe (or other
redirect) echoing isn't an issue. It seems to be possible to trigger
the fallback by doing hijinks with the console in a parent process and
having psql inherit the now-restricted console. The MSDN docs say
parent processes /should/ use sharing modes that would avoid this
problem, but it's not actually enforced.

Based on the current code, the fallback is necessary for things to
work with terminal emulators like mintty on windows, because they
operate by hooking child processes up to a pipe, since the terminal
emulator isn't an actual console (windows doesn't have ptys). I don't
think echo disabling works there, but psql doesn't seem to run in that
environment at all.

The attached is a patch that uses the _fileno/_get_osfhandle approach. 
This doesn't address the stdin fallback, or error handling if opening 
termin fails; however, it should be no worse than what's there now, and 
it fixes the immediate problem.

I'm still thinking about the fallback in terms of a mintty-type 
environment, but I wanted to get this patch out there in the meantime.

-Matt Stickney

Matthew Stickney <mtstickney@gmail.com> writes:
> The attached is a patch that uses the _fileno/_get_osfhandle approach. 
> This doesn't address the stdin fallback, or error handling if opening 
> termin fails; however, it should be no worse than what's there now, and 
> it fixes the immediate problem.

LGTM; pushed with some cosmetic changes.

> I'm still thinking about the fallback in terms of a mintty-type 
> environment, but I wanted to get this patch out there in the meantime.

Yeah.  I'm not absolutely sure that we need to do anything there,
given the lack of field complaints; but if we can think of a simple
way to make it more bulletproof, all the better.

            regards, tom lane