search for: nbdkit_stdio_saf

On Mon, Apr 13, 2020 at 07:28:59PM -0500, Eric Blake wrote: [...] This patch is fine and can be pushed if you want, but I've got some small comments. > +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). I wonder if we want to say "returns 1" rather than true, to give ourselves wiggle room in future in case we suddenly...

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

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

...if the script needs to store state it has to store it @@ -131,6 +138,15 @@ the script. This directory persists for the lifetime of nbdkit and is deleted when nbdkit exits. The name of the directory is passed to each script invocation in the C<$tmpdir> environment variable. +=item C<$nbdkit_stdio_safe> + +This is set to C<1> or C<0> depending on whether or not the script can +safely use stdin/stdout (eg. for reading passwords from the user). +See also the discussion of C<nbdkit_stdio_safe> in +L<nbdkit-plugin(3)>. + +=back + =head2 Handles Handles are arbitrary st...

On 4/14/20 2:47 AM, Richard W.M. Jones wrote: > On Mon, Apr 13, 2020 at 07:28:59PM -0500, Eric Blake wrote: > [...] > > This patch is fine and can be pushed if you want, but I've got some > small comments. > >> +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). > > I wonder if we want to say "returns 1" rather than true, to give > ourselves wiggle room in fut...

In v2: - use int instead of bool in the public header - split the tests from the code - don't overload test-layers; instead, add new tests - add a missing fflush exposed by the new tests - other minor cleanups Eric Blake (3): server: Add nbdkit_stdio_safe server: Sanitize stdin/out before running plugin code server: More tests of stdin/out handling docs/nbdkit-plugin.pod | 23 ++++- plugins/sh/nbdkit-sh-plugin.pod | 4 +- include/nbdkit-common.h | 1 + tests/Makefile.am | 23 +++++ server/internal.h...

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

...t -s sh -' (https://www.redhat.com/archives/libguestfs/2020-April/msg00032.html) I'm still open to ideas on a better name, and/or whether adding <stdbool.h> to our public include files is a good idea (if not, returning int instead of bool is tolerable). Eric Blake (2): server: Add nbdkit_stdio_safe server: Sanitize stdin/out before running plugin code docs/nbdkit-plugin.pod | 23 +++++++++++++++++++- plugins/sh/nbdkit-sh-plugin.pod | 4 +++- include/nbdkit-common.h | 2 ++ server/internal.h | 2 ++ server/background.c | 12 ++++------- server...

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

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

This was a rather longer trip around the houses than I anticipated! The basic purpose of the patch series is to set $nbdkit_stdio_safe to "0" or "1" in sh and eval plugin scripts. To do that, I ended up adding a nicer way to manipulate environ lists, and to do that, I ended up adding a whole generic vector implementation which is applicable in a lot of different places. Rich.

...tr, - uint32_t *r); -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 int 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); -extern int nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen); -extern void nbdkit_shutdown (void)...

...code. We also test this -- see tests/Makefile.am test-ansi-c-plugin.la -- however it may be that the test doesn't work properly if it didn't fail after your change. The solution is quite simple - return an int instead. Cf. nbdkit_parse_bool Rich. > Eric Blake (2): > server: Add nbdkit_stdio_safe > server: Sanitize stdin/out before running plugin code > > docs/nbdkit-plugin.pod | 23 +++++++++++++++++++- > plugins/sh/nbdkit-sh-plugin.pod | 4 +++- > include/nbdkit-common.h | 2 ++ > server/internal.h | 2 ++ > server/background.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. > +...

...o read from or write to stdin/stdout, for example --dump-plugin or "password=-" (Eric Blake). API New "nbdkit_shutdown" call which allows plugins to ask for nbdkit to exit. This is used to implement the new "exitlast" filter. New "nbdkit_stdio_safe" call allows plugins to check if reading from stdin or writing to stdout is safe, eg. if it is safe to read passwords interactively (Eric Blake). "can_*" callbacks which return booleans can return any value ≥ 1 to mean true. Previous versions of nbdkit...

...e state it has to store it > @@ -131,6 +138,15 @@ the script. This directory persists for the lifetime of nbdkit and is > deleted when nbdkit exits. The name of the directory is passed to > each script invocation in the C<$tmpdir> environment variable. > > +=item C<$nbdkit_stdio_safe> ...this name makes more sense (matching the public API), but disagrees with the commit title. Side thought: Both the eval and sh plugins already pass on all unrecognized key=value pairs through to the .config callback, and error out if the config callback returns missing. But right now,...

.../* Note: Don't exit here, otherwise this won't work when compiled * for libFuzzer. diff --git a/server/nbdkit.syms b/server/nbdkit.syms index a67669b7..9e293444 100644 --- a/server/nbdkit.syms +++ b/server/nbdkit.syms @@ -73,6 +73,7 @@ nbdkit_set_error; nbdkit_shutdown; nbdkit_stdio_safe; + nbdkit_string_intern; nbdkit_vdebug; nbdkit_verror; diff --git a/server/plugins.c b/server/plugins.c index 924533cb..3bc50bc7 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -329,17 +329,13 @@ plugin_open (struct backend *b, int readonly, const char *exportname, * will...

.../* Note: Don't exit here, otherwise this won't work when compiled * for libFuzzer. diff --git a/server/nbdkit.syms b/server/nbdkit.syms index 212e36aa..d17878b7 100644 --- a/server/nbdkit.syms +++ b/server/nbdkit.syms @@ -74,6 +74,8 @@ nbdkit_set_error; nbdkit_shutdown; nbdkit_stdio_safe; + nbdkit_strdup_intern; + nbdkit_strndup_intern; nbdkit_vdebug; nbdkit_verror; diff --git a/server/plugins.c b/server/plugins.c index da28b2f1..7425857e 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -328,17 +328,13 @@ plugin_open (struct backend *b, int readonly, const...

...sg, buf); + free (buf); +} + +static int +stdio_config (const char *key, const char *value) +{ + bool check = stdio_check (); + assert (check == false); + msg = key; + return 0; +} + +static int +stdio_config_complete (void) +{ + bool check = stdio_check (); + assert (check == false); + if (nbdkit_stdio_safe ()) { + char *buf = NULL; + size_t len = 0; + + /* Reading from stdin during .config_complete is safe except under -s */ + if (getline (&buf, &len, stdin) == -1) + assert (false); + /* Output during .config_complete is unusual, but not forbidden */ + printf ("%s...

...| 5 ++- tests/test-layers-filter.c | 5 ++- 25 files changed, 171 insertions(+), 80 deletions(-) diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h index 47288050..671cd4a4 100644 --- a/include/nbdkit-common.h +++ b/include/nbdkit-common.h @@ -110,7 +110,6 @@ extern int 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); -extern const char *nbdkit_export_name (void); extern int nbdkit_peer_name (struct sockaddr *addr, socklen_t...