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

[Eric Blake]

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