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

[Richard W.M. Jones]

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