5:31 a.m.
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.
5:31 a.m.
New subject: [PATCH nbdkit 1/3] server: Disallow password=- from non-tty and fix error message (RHBZ#1842440).
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
12:18 p.m.
New subject: [PATCH nbdkit 1/3] server: Disallow password=- from non-tty and fix error message (RHBZ#1842440).
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
12:34 p.m.
New subject: [PATCH nbdkit 1/3] server: Disallow password=- from non-tty and fix error message (RHBZ#1842440).
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
5:31 a.m.
New subject: [PATCH nbdkit 2/3] server: Disallow -FD for stdin/stdout/stderr.
$ ./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
12:20 p.m.
New subject: [PATCH nbdkit 2/3] server: Disallow -FD for stdin/stdout/stderr.
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
5:31 a.m.
New subject: [PATCH nbdkit 3/3] todo: Suggest password=-tty as future enhancement.
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
12:20 p.m.
New subject: [PATCH nbdkit 3/3] todo: Suggest password=-tty as future enhancement.
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
8:09 a.m.
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
1634
days inactive
1634
days old
8 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones