[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
From 27a0f9b2036680564ef6dfa54f3961af221cbc50 Mon Sep 17 00:00:00 2001
From: Matthew Stickney
Date: Mon, 21 May 2018 23:24:34 -0400
Subject: [PATCH] Disable input echo on the console, not stdin.
When data is piped to psql, a handle to stdin will no longer be a handle
to the console; SetConsoleMode will fail, and prompt input will be echoed
to the screen.
Instead, use CreateFile to get a handle to the console's input buffer by
opening CONIN$, and set the console mode there. Prompt input is already
being read from CONIN$, so now prompt-related I/O modifications alter the
same buffer.
the same buffer.
consistently.
---
src/port/sprompt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/port/sprompt.c b/src/port/sprompt.c
index 70dfa69d7b..f57b49bd3a 100644
--- a/src/port/sprompt.c
+++ b/src/port/sprompt.c
@@ -112,7 +112,7 @@ simple_prompt(const char *prompt, char *destination, size_t
destlen, bool echo)
if (!echo)
{
/* get a new handle to turn echo off */
- t = GetStdHandle(STD_INPUT_HANDLE);
+ t = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL);
/* save the old configuration first */
GetConsoleMode(t, &t_orig);
@@ -164,6 +164,7 @@ simple_prompt(const char *prompt, char *destination, size_t
destlen, bool echo)
{
/* reset to the original console mode */
SetConsoleMode(t, t_orig);
+ CloseHandle(t);
fputs("\n", termout);
fflush(termout);
}
--
2.16.2.windows.1
Re: [PATCH] (Windows) psql echoes password when reading from pipe
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 wrote:
> Matthew Stickney 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
Re: [PATCH] (Windows) psql echoes password when reading from pipe
On Tue, May 22, 2018 at 4:09 PM, Tom Lane 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
Re: [PATCH] (Windows) psql echoes password when reading from pipe
On Tue, May 22, 2018 at 6:14 PM, Tom Lane 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.
Re: [PATCH] (Windows) psql echoes password when reading from pipe
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
From 734b979e64947e1b820f5c1a40454bb35c03 Mon Sep 17 00:00:00 2001
From: Matthew Stickney
Date: Mon, 21 May 2018 23:24:34 -0400
Subject: [PATCH] Disable input echo on the console, not stdin.
When data is piped to psql, a handle to stdin will no longer be a handle
to the console; SetConsoleMode will fail, and prompt input will be echoed
to the screen.
Instead, retrieve a handle to the console's input buffer fron termin, and
set the console mode there (this requires write permissions). The code is
now setting the console mode on the same handle it reads from, which should
always be consistent.
---
src/port/sprompt.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/port/sprompt.c b/src/port/sprompt.c
index 70dfa69d7b..340232a7a2 100644
--- a/src/port/sprompt.c
+++ b/src/port/sprompt.c
@@ -66,8 +66,12 @@ simple_prompt(const char *prompt, char *destination, size_t
destlen, bool echo)
*
* XXX fgets() still receives text in the console's input code page.
This
* makes non-ASCII credentials unportable.
+*
+* Note that termin must also be opened with write permissions in order
for
+* SetConsoleMode() to succeed, even though it's only used for reading
here.
+*
*/
- termin = fopen("CONIN$", "r");
+ termin = fopen("CONIN$", "w+");
termout = fopen("CONOUT$", "w+");
#else
@@ -112,7 +116,7 @@ simple_prompt(const char *prompt, char *destination, size_t
destlen, bool echo)
if (!echo)
{
/* get a new handle to turn echo off */
- t = GetStdHandle(STD_INPUT_HANDLE);
+ t = (HANDLE)_get_osfhandle(_fileno(termin));
/* save the old configuration first */
GetConsoleMode(t, &t_orig);
--
2.16.2.windows.1
