server: Fix reading passwords interactively.
https://bugzilla.redhat.com/show_bug.cgi?id=1842440 Patches 1 and 2 address fairly obvious bugs in how we handle reading passwords from stdin. There are other ways we may consider fixing these bugs: - Should password=- always open /dev/tty and ignore stdin entirely? - Should we make password=-0/-1/-2 work by skipping the close? Or perhaps reopen the file descriptors on /dev/null after reading the password? (This seems over-complicated to me, I think -0/-1/-2 is always likely to be a user error). Rich.
This command fails with an incorrect error message: $ nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' </dev/null password: nbdkit: error: could not read password from stdin: Inappropriate ioctl for device The error (ENOTTY Inappropriate ioctl for device) is actually a leftover errno from the previous isatty call. This happens because getline(3) can return -1 both for error and EOF. In the EOF case it does not set errno so we get the previous random value. Also: $ echo -n '' | nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' password: nbdkit: error: could not read password from stdin: Inappropriate ioctl for device $ echo -n '1' | nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' password: [password is read with no error] This raises the question of what password=- actually means. It's documented as "read a password interactively", with the word "interactively" going back to at least nbdkit 1.2, and therefore I think we should reject attempts to use password=- from non-ttys. Since at least nbdkit 1.2 we have allowed passwords to be read from files (password=+FILENAME), and since nbdkit 1.16 you can read passwords from arbitrary file descriptors (password=-FD). Another justification for the interactive-only nature of password=- is that it prints a “password: ” prompt. So I believe it is fair to ban password=- unless the input is a tty. This commit also fixes the error message by handling the case where getline returns -1 without setting errno. After this change: $ ./nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' password: <--- press ^D nbdkit: error: could not read password from stdin: end of file $ echo -n '' | ./nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' nbdkit: error: stdin is not a tty, cannot read password interactively $ echo -n '1' | ./nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' nbdkit: error: stdin is not a tty, cannot read password interactively $ ./nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' password: <--- press return key [zero length password is read] Thanks: Ming Xie, Pino Toscano. --- docs/nbdkit-plugin.pod | 8 ++- server/public.c | 107 ++++++++++++++++++++++++++--------------- 2 files changed, 74 insertions(+), 41 deletions(-) diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index f5e6dd01..612688ab 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -1242,8 +1242,12 @@ or from a file descriptor inherited by nbdkit: nbdkit myplugin password=-99 -(If the password begins with a C<-> or C<+> character then it must be -passed in a file). +=head3 Notes on reading passwords + +If the password begins with a C<-> or C<+> character then it must be +passed in a file. + +C<password=-> can only be used when stdin is a terminal. =head2 Safely interacting with stdin and stdout diff --git a/server/public.c b/server/public.c index bcf1a3a2..dafdfbae 100644 --- a/server/public.c +++ b/server/public.c @@ -413,53 +413,18 @@ nbdkit_stdio_safe (void) } /* Read a password from configuration value. */ +static int read_password_interactive (char **password); static int read_password_from_fd (const char *what, int fd, char **password); int nbdkit_read_password (const char *value, char **password) { - int tty, err; - struct termios orig, temp; - ssize_t r; - size_t n; - *password = NULL; - /* Read from stdin. */ + /* Read from stdin interactively. */ if (strcmp (value, "-") == 0) { - if (!nbdkit_stdio_safe ()) { - nbdkit_error ("stdin is not available for reading password"); + if (read_password_interactive (password) == -1) return -1; - } - - printf ("password: "); - - /* Set no echo. */ - tty = isatty (0); - if (tty) { - tcgetattr (0, &orig); - temp = orig; - temp.c_lflag &= ~ECHO; - tcsetattr (0, TCSAFLUSH, &temp); - } - - r = getline (password, &n, stdin); - err = errno; - - /* Restore echo. */ - if (tty) - tcsetattr (0, TCSAFLUSH, &orig); - - /* Complete the printf above. */ - printf ("\n"); - - if (r == -1) { - errno = err; - nbdkit_error ("could not read password from stdin: %m"); - return -1; - } - if (*password && r > 0 && (*password)[r-1] == '\n') - (*password)[r-1] = '\0'; } /* Read from numbered file descriptor. */ @@ -501,6 +466,62 @@ nbdkit_read_password (const char *value, char **password) return 0; } +static int +read_password_interactive (char **password) +{ + int err; + struct termios orig, temp; + ssize_t r; + size_t n; + + if (!nbdkit_stdio_safe ()) { + nbdkit_error ("stdin is not available for reading password"); + return -1; + } + + if (!isatty (0)) { + nbdkit_error ("stdin is not a tty, cannot read password interactively"); + return -1; + } + + printf ("password: "); + + /* Set no echo. This is best-effort, ignore errors. */ + tcgetattr (0, &orig); + temp = orig; + temp.c_lflag &= ~ECHO; + tcsetattr (0, TCSAFLUSH, &temp); + + /* To distinguish between error and EOF we have to check errno. + * getline can return -1 and errno = 0 which means we got end of + * file. + */ + errno = 0; + r = getline (password, &n, stdin); + err = errno; + + /* Restore echo. */ + tcsetattr (0, TCSAFLUSH, &orig); + + /* Complete the printf above. */ + printf ("\n"); + + if (r == -1) { + if (err == 0) + nbdkit_error ("could not read password from stdin: end of file"); + else { + errno = err; + nbdkit_error ("could not read password from stdin: %m"); + } + return -1; + } + + if (*password && r > 0 && (*password)[r-1] == '\n') + (*password)[r-1] = '\0'; + + return 0; +} + static int read_password_from_fd (const char *what, int fd, char **password) { @@ -515,10 +536,18 @@ read_password_from_fd (const char *what, int fd, char **password) close (fd); return -1; } + + /* To distinguish between error and EOF we have to check errno. + * getline can return -1 and errno = 0 which means we got end of + * file, which is simply a zero length password. + */ + errno = 0; r = getline (password, &n, fp); err = errno; + fclose (fp); - if (r == -1) { + + if (r == -1 && err != 0) { errno = err; nbdkit_error ("could not read password from %s: %m", what); return -1; -- 2.25.0
On 6/1/20 5:31 AM, Richard W.M. Jones wrote:
This command fails with an incorrect error message:
$ nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' </dev/null password: nbdkit: error: could not read password from stdin: Inappropriate ioctl for device
The error (ENOTTY Inappropriate ioctl for device) is actually a leftover errno from the previous isatty call. This happens because getline(3) can return -1 both for error and EOF. In the EOF case it does not set errno so we get the previous random value.
Also:
$ echo -n '' | nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' password: nbdkit: error: could not read password from stdin: Inappropriate ioctl for device
$ echo -n '1' | nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' password: [password is read with no error]
This raises the question of what password=- actually means. It's documented as "read a password interactively", with the word "interactively" going back to at least nbdkit 1.2, and therefore I think we should reject attempts to use password=- from non-ttys.
Makes sense.
Since at least nbdkit 1.2 we have allowed passwords to be read from files (password=+FILENAME), and since nbdkit 1.16 you can read passwords from arbitrary file descriptors (password=-FD).
Another justification for the interactive-only nature of password=- is that it prints a “password: ” prompt.
So I believe it is fair to ban password=- unless the input is a tty.
I agree with that decision.
This commit also fixes the error message by handling the case where getline returns -1 without setting errno.
After this change:
$ ./nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' password: <--- press ^D nbdkit: error: could not read password from stdin: end of file $ echo -n '' | ./nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' nbdkit: error: stdin is not a tty, cannot read password interactively $ echo -n '1' | ./nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' nbdkit: error: stdin is not a tty, cannot read password interactively $ ./nbdkit ssh host=localhost /nosuchfile password=- --run 'qemu-img info $nbd' password: <--- press return key [zero length password is read]
Thanks: Ming Xie, Pino Toscano. --- docs/nbdkit-plugin.pod | 8 ++- server/public.c | 107 ++++++++++++++++++++++++++--------------- 2 files changed, 74 insertions(+), 41 deletions(-)
+static int +read_password_interactive (char **password) +{ + int err; + struct termios orig, temp; + ssize_t r; + size_t n; + + if (!nbdkit_stdio_safe ()) { + nbdkit_error ("stdin is not available for reading password"); + return -1; + } + + if (!isatty (0)) {
Could spell it STDIN_FILENO if desired, but that's trivial.
+ /* To distinguish between error and EOF we have to check errno. + * getline can return -1 and errno = 0 which means we got end of + * file. + */ + errno = 0; + r = getline (password, &n, stdin); + err = errno; + + /* Restore echo. */ + tcsetattr (0, TCSAFLUSH, &orig); + + /* Complete the printf above. */ + printf ("\n"); + + if (r == -1) { + if (err == 0) + nbdkit_error ("could not read password from stdin: end of file");
Is this actually an error? EOF on a tty merely means that the password is the empty string (perhaps because the user intentionally pressed ^D instead of enter to stop further input)...
+ else { + errno = err; + nbdkit_error ("could not read password from stdin: %m"); + } + return -1; + } + + if (*password && r > 0 && (*password)[r-1] == '\n') + (*password)[r-1] = '\0';
...especially since you already allow an interactive user to pass an empty password by pressing solely enter. The rest of the patch looks fine; I'll leave it up to you what you want to do about handling an empty password interactively. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 6/1/20 12:18 PM, Eric Blake wrote:
This raises the question of what password=- actually means. It's documented as "read a password interactively", with the word "interactively" going back to at least nbdkit 1.2, and therefore I think we should reject attempts to use password=- from non-ttys.
Makes sense.
Since at least nbdkit 1.2 we have allowed passwords to be read from files (password=+FILENAME), and since nbdkit 1.16 you can read passwords from arbitrary file descriptors (password=-FD).
Another justification for the interactive-only nature of password=- is that it prints a “password: ” prompt.
So I believe it is fair to ban password=- unless the input is a tty.
I agree with that decision.
And now that I read the code in server/public.c, I'm worried that password=- and password=-N have the same problem across re-exec: we blindly call fdopen() on the fd, then blindly call fclose() on the resulting FILE* - which DOES close the fd. That means that vddk passing in an fd has the same problem: the passed-in fd will be closed by the first pass through nbdkit_read_password(), and unavailable for the second pass after re-exec. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
$ ./nbdkit ssh host=localhost /nosuchfile password=-0 --run 'qemu-img info $nbd' abc fcntl: Bad file descriptor The reason for this is that we close the file descriptor after reading the password. Closing stdin causes bad stuff to happen. --- docs/nbdkit-plugin.pod | 5 +++++ server/public.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 612688ab..7b8a5393 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -1249,6 +1249,11 @@ passed in a file. C<password=-> can only be used when stdin is a terminal. +C<password=-FD> cannot be used with stdin, stdout or stderr +(ie. C<-0>, C<-1> or C<-2>). The reason is that after reading the +password the file descriptor is closed, which causes bad stuff to +happen. + =head2 Safely interacting with stdin and stdout int nbdkit_stdio_safe (void); diff --git a/server/public.c b/server/public.c index dafdfbae..2e36e43a 100644 --- a/server/public.c +++ b/server/public.c @@ -433,8 +433,8 @@ nbdkit_read_password (const char *value, char **password) if (nbdkit_parse_int ("password file descriptor", &value[1], &fd) == -1) return -1; - if (fd == STDIN_FILENO && !nbdkit_stdio_safe ()) { - nbdkit_error ("stdin is not available for reading password"); + if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO) { + nbdkit_error ("cannot use password -FD for stdin/stdout/stderr"); return -1; } if (read_password_from_fd (&value[1], fd, password) == -1) -- 2.25.0
On 6/1/20 5:31 AM, Richard W.M. Jones wrote:
$ ./nbdkit ssh host=localhost /nosuchfile password=-0 --run 'qemu-img info $nbd' abc fcntl: Bad file descriptor
The reason for this is that we close the file descriptor after reading the password. Closing stdin causes bad stuff to happen. --- docs/nbdkit-plugin.pod | 5 +++++ server/public.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-)
Makes sense.
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 612688ab..7b8a5393 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -1249,6 +1249,11 @@ passed in a file.
C<password=-> can only be used when stdin is a terminal.
+C<password=-FD> cannot be used with stdin, stdout or stderr +(ie. C<-0>, C<-1> or C<-2>). The reason is that after reading the +password the file descriptor is closed, which causes bad stuff to +happen.
Sure, we could always skip closing on those three, but it's just as easy to not permit it in the first place :)
+ =head2 Safely interacting with stdin and stdout
int nbdkit_stdio_safe (void); diff --git a/server/public.c b/server/public.c index dafdfbae..2e36e43a 100644 --- a/server/public.c +++ b/server/public.c @@ -433,8 +433,8 @@ nbdkit_read_password (const char *value, char **password)
if (nbdkit_parse_int ("password file descriptor", &value[1], &fd) == -1) return -1; - if (fd == STDIN_FILENO && !nbdkit_stdio_safe ()) { - nbdkit_error ("stdin is not available for reading password"); + if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO) {
Could shorten to if (fd <= STDERR_FILENO) if desired, but I'm fine either way. ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
password=+/dev/tty works already, but it does not print a “password: ” prompt or suppress echo. --- TODO | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TODO b/TODO index 02627069..c1a6ec0a 100644 --- a/TODO +++ b/TODO @@ -103,6 +103,9 @@ General ideas for improvements realloc is suspect) - add more iterators, map function, etc, as required. +* Add password=-tty to mean read a password interactively from /dev/tty. + This is slightly different from password=+/dev/tty + Suggestions for plugins ----------------------- -- 2.25.0
On 6/1/20 5:31 AM, Richard W.M. Jones wrote:
password=+/dev/tty works already, but it does not print a “password: ” prompt or suppress echo. --- TODO | 3 +++ 1 file changed, 3 insertions(+)
ACK.
diff --git a/TODO b/TODO index 02627069..c1a6ec0a 100644 --- a/TODO +++ b/TODO @@ -103,6 +103,9 @@ General ideas for improvements realloc is suspect) - add more iterators, map function, etc, as required.
+* Add password=-tty to mean read a password interactively from /dev/tty. + This is slightly different from password=+/dev/tty + Suggestions for plugins -----------------------
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
I had an idea that I would make an alternate version of this patch where password=- always reads from /dev/tty. But looking at the source code of sudo that seems really complicated to do across all platforms (and even if we only made it work on Linux). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (2)
-
Eric Blake -
Richard W.M. Jones