Trying to read a password or inline script from stdin when the -s option was used to connect to our client over stdin is not going to work. It would be nice to fail such usage as soon as possible, by giving plugins a means to decide if it is safe to use stdio during .config. Update nbdkit_read_password and the sh plugin to take advantage of the new entry point. Also, treat 'nbdkit -s --dump-plugin' as an error, as .dump_plugin is supposed to interact with stdout. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/nbdkit-plugin.pod | 23 ++++++++++++++++++++++- plugins/sh/nbdkit-sh-plugin.pod | 4 +++- include/nbdkit-common.h | 2 ++ server/main.c | 5 +++-- server/nbdkit.syms | 1 + server/public.c | 18 +++++++++++++++++- server/test-public.c | 23 +++++++++++++++++++++-- plugins/sh/sh.c | 7 ++++++- 8 files changed, 75 insertions(+), 8 deletions(-) diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 6c1311eb..822e19df 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -388,6 +388,10 @@ should probably look at other plugins and follow the same conventions. If the value is a relative path, then note that the server changes directory when it starts up. See L</FILENAMES AND PATHS> above. +If C<nbdkit_stdio_safe> returns true, the value of the configuration +parameter may be used to trigger reading additional data through stdin +(such as a password or inline script). + If the C<.config> callback is not provided by the plugin, and the user tries to specify any C<key=value> arguments, then nbdkit will exit with an error. @@ -1148,6 +1152,23 @@ C<str> can be a string containing a case-insensitive form of various common toggle values. The function returns 0 or 1 if the parse was successful. If there was an error, it returns C<-1>. +=head2 Interacting with stdin and stdout + +Use the C<nbdkit_stdio_safe> utility function to determine if it is +safe to interact with stdin and stdout during C<.config>. When nbdkit +is started under the single connection mode (C<-s>), the plugin must +not directly interact with stdin, because that would interfere with +the client. The result of this function only matters prior to +C<.config_complete>; once nbdkit reaches C<.get_ready>, the plugin +should assume that nbdkit may have closed the original stdin and +stdout in order to become a daemon. + + bool nbdkit_stdio_safe (void); + +As an example, the L<nbdkit-sh-plugin(3)> uses this function to +determine whether it is safe to support C<script=-> to read a script +from stdin. + =head2 Reading passwords The C<nbdkit_read_password> utility function can be used to read @@ -1180,7 +1201,7 @@ used directly on the command line, eg: nbdkit myplugin password=mostsecret But more securely this function can also read a password -interactively: +interactively (if C<nbdkit_stdio_safe> returns true): nbdkit myplugin password=- diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod index ffd0310f..20a2b785 100644 --- a/plugins/sh/nbdkit-sh-plugin.pod +++ b/plugins/sh/nbdkit-sh-plugin.pod @@ -48,7 +48,9 @@ as the name of the script, like this: EOF By default the inline script runs under F</bin/sh>. You can add a -shebang (C<#!>) to use other scripting languages. +shebang (C<#!>) to use other scripting languages. Of course, reading +an inline script from stdin is incompatible with the C<-s> +(C<--single>) mode of nbdkit that connects a client on stdin. =head1 WRITING AN NBDKIT SH PLUGIN diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h index 9d1d89d0..9b10500a 100644 --- a/include/nbdkit-common.h +++ b/include/nbdkit-common.h @@ -38,6 +38,7 @@ #endif #include <stdarg.h> +#include <stdbool.h> #include <stdint.h> #include <errno.h> #include <sys/socket.h> @@ -106,6 +107,7 @@ extern int nbdkit_parse_int64_t (const char *what, const char *str, int64_t *r); extern int nbdkit_parse_uint64_t (const char *what, const char *str, uint64_t *r); +extern bool nbdkit_stdio_safe (void); extern int nbdkit_read_password (const char *value, char **password); extern char *nbdkit_realpath (const char *path); extern int nbdkit_nanosleep (unsigned sec, unsigned nsec); diff --git a/server/main.c b/server/main.c index 62598b81..748122fc 100644 --- a/server/main.c +++ b/server/main.c @@ -529,12 +529,13 @@ main (int argc, char *argv[]) (port && listen_stdin) || (unixsocket && listen_stdin) || (listen_stdin && run) || + (listen_stdin && dump_plugin) || (vsock && unixsocket) || (vsock && listen_stdin) || (vsock && run)) { fprintf (stderr, - "%s: -p, --run, -s, -U or --vsock options cannot be used " - "in this combination\n", + "%s: --dump-plugin, -p, --run, -s, -U or --vsock options " + "cannot be used in this combination\n", program_name); exit (EXIT_FAILURE); } diff --git a/server/nbdkit.syms b/server/nbdkit.syms index 111223f2..20c390a9 100644 --- a/server/nbdkit.syms +++ b/server/nbdkit.syms @@ -65,6 +65,7 @@ nbdkit_realpath; nbdkit_set_error; nbdkit_shutdown; + nbdkit_stdio_safe; nbdkit_vdebug; nbdkit_verror; diff --git a/server/public.c b/server/public.c index 3fd11253..f19791bc 100644 --- a/server/public.c +++ b/server/public.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -404,6 +404,13 @@ nbdkit_parse_bool (const char *str) return -1; } +/* Return true if it is safe to read from stdin during configuration. */ +bool +nbdkit_stdio_safe (void) +{ + return !listen_stdin; +} + /* Read a password from configuration value. */ static int read_password_from_fd (const char *what, int fd, char **password); @@ -419,6 +426,11 @@ nbdkit_read_password (const char *value, char **password) /* Read from stdin. */ if (strcmp (value, "-") == 0) { + if (!nbdkit_stdio_safe ()) { + nbdkit_error ("stdin is not available for reading password"); + return -1; + } + printf ("password: "); /* Set no echo. */ @@ -455,6 +467,10 @@ nbdkit_read_password (const char *value, char **password) if (nbdkit_parse_int ("password file descriptor", &value[1], &fd) == -1) return -1; + if (!nbdkit_stdio_safe () && fd < STDERR_FILENO) { + nbdkit_error ("stdin/out is not available for reading password"); + return -1; + } if (read_password_from_fd (&value[1], fd, password) == -1) return -1; } diff --git a/server/test-public.c b/server/test-public.c index fe347d44..d4903420 100644 --- a/server/test-public.c +++ b/server/test-public.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2019 Red Hat Inc. + * Copyright (C) 2018-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -53,6 +53,8 @@ nbdkit_error (const char *fs, ...) error_flagged = true; } +bool listen_stdin; + volatile int quit; int quit_fd = -1; @@ -427,7 +429,24 @@ test_nbdkit_read_password (void) pass = false; } - /* XXX Testing reading from stdin would require setting up a pty */ + /* XXX Testing reading from stdin would require setting up a pty. But + * we can test that it is forbidden with -s. + */ + listen_stdin = true; + if (nbdkit_read_password ("-", &pw) != -1) { + fprintf (stderr, "Failed to diagnose failed password from stdin with -s\n"); + pass = false; + } + else if (pw != NULL) { + fprintf (stderr, "Failed to set password to NULL on failure\n"); + pass = false; + } + else if (!error_flagged) { + fprintf (stderr, "Wrong error message handling\n"); + pass = false; + } + error_flagged = false; + return pass; } diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c index 736b8ef0..c8a321f1 100644 --- a/plugins/sh/sh.c +++ b/plugins/sh/sh.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2019 Red Hat Inc. + * Copyright (C) 2018-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -116,6 +116,11 @@ inline_script (void) char *filename = NULL; CLEANUP_FREE char *cmd = NULL; + if (!nbdkit_stdio_safe ()) { + nbdkit_error ("inline script is incompatible with -s"); + return NULL; + } + if (asprintf (&filename, "%s/%s", tmpdir, scriptname) == -1) { nbdkit_error ("asprintf: %m"); goto err; -- 2.26.0.rc2

As shown in the previous patch, plugins may choose to use stdin or stdout during .config. But from .get_ready onwards, well-written plugins shouldn't be needing any further use of stdin/out. We already swapped stdin/out to /dev/null while daemonizing, but did not do do during -f or --run, which leads to some surprising inconsistency when trying to debug a plugin that works in the foreground but fails when daemonized. What's more, while the task executed by --run should use the original stdin/out (and we even have tests that would fail without this), we don't want the plugin to interfere with whatever --run is doing. So it's time to move the swap over to /dev/null into a central location, right before .get_ready. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/internal.h | 2 ++ server/background.c | 12 ++++-------- server/captive.c | 10 ++++++++-- server/connections.c | 12 ------------ server/main.c | 33 ++++++++++++++++++++++++++++++++- tests/test-layers-plugin.c | 12 +++++++++++- tests/test-layers.c | 4 +++- 7 files changed, 60 insertions(+), 25 deletions(-) diff --git a/server/internal.h b/server/internal.h index 67eb6a32..79e1906c 100644 --- a/server/internal.h +++ b/server/internal.h @@ -132,6 +132,8 @@ extern bool tls_verify_peer; extern char *unixsocket; extern const char *user, *group; extern bool verbose; +extern int orig_in; +extern int orig_out; /* Linked list of backends. Each backend struct is followed by either * a filter or plugin struct. "top" points to the first one. They diff --git a/server/background.c b/server/background.c index 531ba470..8388d9c8 100644 --- a/server/background.c +++ b/server/background.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2019 Red Hat Inc. + * Copyright (C) 2019-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -70,13 +70,9 @@ fork_into_background (void) chdir ("/"); #pragma GCC diagnostic pop - /* Close stdin/stdout and redirect to /dev/null. */ - close (0); - close (1); - open ("/dev/null", O_RDONLY); - open ("/dev/null", O_WRONLY); - - /* If not verbose, set stderr to the same as stdout as well. */ + /* By this point, stdin/out have been redirected to /dev/null. + * If not verbose, set stderr to the same as stdout as well. + */ if (!verbose) dup2 (1, 2); diff --git a/server/captive.c b/server/captive.c index 0a243d2b..1ff10192 100644 --- a/server/captive.c +++ b/server/captive.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2019 Red Hat Inc. + * Copyright (C) 2019-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -151,7 +151,13 @@ run_command (void) } if (pid > 0) { /* Parent process is the run command. */ - r = system (cmd); + /* Restore original stdin/out */ + if (dup2 (orig_in, STDIN_FILENO) == -1 || + dup2 (orig_out, STDOUT_FILENO) == -1) { + r = -1; + } + else + r = system (cmd); if (r == -1) { nbdkit_error ("failure to execute external command: %m"); r = EXIT_FAILURE; diff --git a/server/connections.c b/server/connections.c index c54c71c1..c7b55ca1 100644 --- a/server/connections.c +++ b/server/connections.c @@ -335,18 +335,6 @@ free_connection (struct connection *conn) return; conn->close (); - if (listen_stdin) { - int fd; - - /* Restore something to stdin/out so the rest of our code can - * continue to assume that all new fds will be above stderr. - * Swap directions to get EBADF on improper use of stdin/out. - */ - fd = open ("/dev/null", O_WRONLY | O_CLOEXEC); - assert (fd == 0); - fd = open ("/dev/null", O_RDONLY | O_CLOEXEC); - assert (fd == 1); - } /* Don't call the plugin again if quit has been set because the main * thread will be in the process of unloading it. The plugin.unload diff --git a/server/main.c b/server/main.c index 748122fc..596dd306 100644 --- a/server/main.c +++ b/server/main.c @@ -101,6 +101,8 @@ const char *user, *group; /* -u & -g */ bool verbose; /* -v */ bool vsock; /* --vsock */ unsigned int socket_activation /* $LISTEN_FDS and $LISTEN_PID set */; +int orig_in = -1; /* dup'd stdin during -s/--run */ +int orig_out = -1; /* dup'd stdout during -s/--run */ /* The linked list of zero or more filters, and one plugin. */ struct backend *top; @@ -694,6 +696,35 @@ main (int argc, char *argv[]) top->config_complete (top); + /* Sanitize stdin/stdout to /dev/null, after saving the originals + * when needed. We are still single-threaded at this point, and + * already checked that stdin/out were open, so we don't have to + * worry about other threads accidentally grabbing our intended fds, + * or races on FD_CLOEXEC. + */ + if (listen_stdin || run) { +#ifndef F_DUPFD_CLOEXEC +#define F_DUPFD_CLOEXEC F_DUPFD +#endif + orig_in = fcntl (STDIN_FILENO, F_DUPFD_CLOEXEC, STDERR_FILENO + 1); + orig_out = fcntl (STDOUT_FILENO, F_DUPFD_CLOEXEC, STDERR_FILENO + 1); +#if F_DUPFD == F_DUPFD_CLOEXEC + orig_in = set_cloexec (orig_in); + orig_out = set_cloexec (orig_out); +#endif + if (orig_in == -1 || orig_out == -1) { + perror ("fcntl"); + exit (EXIT_FAILURE); + } + } + close (STDIN_FILENO); + close (STDOUT_FILENO); + if (open ("/dev/null", O_RDONLY) != STDIN_FILENO || + open ("/dev/null", O_WRONLY) != STDOUT_FILENO) { + perror ("open"); + exit (EXIT_FAILURE); + } + /* Select the correct thread model based on config. */ lock_init_thread_model (); @@ -918,7 +949,7 @@ start_serving (void) change_user (); write_pidfile (); threadlocal_new_server_thread (); - handle_single_connection (0, 1); + handle_single_connection (orig_in, orig_out); return; } diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c index 8858bede..5a6b3020 100644 --- a/tests/test-layers-plugin.c +++ b/tests/test-layers-plugin.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2019 Red Hat Inc. + * Copyright (C) 2018-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -36,6 +36,7 @@ #include <stdlib.h> #include <stdint.h> #include <string.h> +#include <unistd.h> #define NBDKIT_API_VERSION 2 #include <nbdkit-plugin.h> @@ -75,7 +76,16 @@ test_layers_plugin_config_complete (void) static int test_layers_plugin_get_ready (void) { + char c = 'X'; DEBUG_FUNCTION; + + /* Test that stdin/stdout have been sanitized */ + if (write (STDOUT_FILENO, &c, 1) != 1 || + read (STDIN_FILENO, &c, 1) != 0) { + nbdkit_error ("unusual stdio behavior"); + return -1; + } + return 0; } diff --git a/tests/test-layers.c b/tests/test-layers.c index 33ae5a75..5bdce54e 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2019 Red Hat Inc. + * Copyright (C) 2018-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -125,6 +125,8 @@ main (int argc, char *argv[]) if (pid == 0) { /* Child. */ dup2 (sfd[1], 0); dup2 (sfd[1], 1); + close (sfd[0]); + close (sfd[1]); close (pfd[0]); dup2 (pfd[1], 2); close (pfd[1]); -- 2.26.0.rc2