Libguestfs - Jun 2020 - Re: [PATCH nbdkit 1/3] server: Disallow password=- from non-tty and fix error message (RHBZ#1842440).

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

$ ./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

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

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

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

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