Eric Blake
2020-Apr-15 18:09 UTC
Re: [Libguestfs] [PATCH nbdkit 9/9] eval, sh: Define $nbdkit_safe_stdio = 0|1 in scripts.
On 4/15/20 11:16 AM, Richard W.M. Jones wrote: In the subject, you describe $nbdkit_safe_stdio, but in the patch body...> --- > plugins/eval/nbdkit-eval-plugin.pod | 11 +++-------- > plugins/sh/nbdkit-sh-plugin.pod | 18 +++++++++++++++++- > plugins/sh/call.c | 8 ++++++-- > tests/test-single-sh.sh | 4 ++++ > 4 files changed, 30 insertions(+), 11 deletions(-) >The patch does not work as intended as currently written: when we invoke /path/to/script config $key $value, we have already set up stdin/stdout to our own pipes [1]. For .config and .config_complete, reading will always see EOF (no other callback needs to interact with the original stdin, and callbacks like .pwrite actually use the pipe for data). If we want to allow a script to read a password from stdin, we need to preserve the original fd to .config and .config_complete rather than passing in an empty pipe (but those are the only two callbacks where it makes sense, and even then, only when we did not use script=- or when -s is in effect). [1] ./nbdkit eval config='ls -l /proc/$$/fd >/dev/tty' a=b total 0 lr-x------. 1 eblake eblake 64 Apr 15 13:03 0 -> 'pipe:[1669308]' l-wx------. 1 eblake eblake 64 Apr 15 13:03 1 -> 'pipe:[1669309]' l-wx------. 1 eblake eblake 64 Apr 15 13:03 2 -> 'pipe:[1669310]' lr-x------. 1 eblake eblake 64 Apr 15 13:03 255 -> /tmp/nbdkitevalVUNZ1F/config> +++ b/plugins/sh/nbdkit-sh-plugin.pod > @@ -119,7 +119,14 @@ These exit codes are reserved for future use. > > =back > > -=head2 Temporary directory > +=head2 Environment variables passed to the script > + > +When running the script, certain environment variables are set by the > +plugin: > + > +=over 4 > + > +=item C<$tmpdir> > > A fresh script is invoked for each method call (ie. scripts are > stateless), so 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 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, if a script wants to set up an environment variable that will still be present to affect later calls, it has to track things itself (perhaps by having the .config callback append to $tmpdir/vars, then all other callbacks start by 'source $tmpdir/vars'). Would it make sense to reserve a special exit value from the .config callback for the case where the script wants the current key=value passed to config to be preserved to all future callbacks? Or even state that the config callback exiting with status 2 (for missing) is NOT an error, but does the key=value preservation automatically? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2020-Apr-15 18:18 UTC
Re: [Libguestfs] [PATCH nbdkit 9/9] eval, sh: Define $nbdkit_safe_stdio = 0|1 in scripts.
On Wed, Apr 15, 2020 at 01:09:21PM -0500, Eric Blake wrote:> On 4/15/20 11:16 AM, Richard W.M. Jones wrote: > > In the subject, you describe $nbdkit_safe_stdio, but in the patch body... > > >--- > > plugins/eval/nbdkit-eval-plugin.pod | 11 +++-------- > > plugins/sh/nbdkit-sh-plugin.pod | 18 +++++++++++++++++- > > plugins/sh/call.c | 8 ++++++-- > > tests/test-single-sh.sh | 4 ++++ > > 4 files changed, 30 insertions(+), 11 deletions(-) > > > > The patch does not work as intended as currently written: when we > invoke /path/to/script config $key $value, we have already set up > stdin/stdout to our own pipes [1].Urgghh ... very true. I think patches 1-8 are still worthwhile though :-) And with the same mechanism we can pass other environment variables in if we think of any in future ($nbdkit_peer_name?).> For .config and > .config_complete, reading will always see EOF (no other callback > needs to interact with the original stdin, and callbacks like > .pwrite actually use the pipe for data). If we want to allow a > script to read a password from stdin, we need to preserve the > original fd to .config and .config_complete rather than passing in > an empty pipe (but those are the only two callbacks where it makes > sense, and even then, only when we did not use script=- or when -s > is in effect). > > [1] ./nbdkit eval config='ls -l /proc/$$/fd >/dev/tty' a=b > total 0 > lr-x------. 1 eblake eblake 64 Apr 15 13:03 0 -> 'pipe:[1669308]' > l-wx------. 1 eblake eblake 64 Apr 15 13:03 1 -> 'pipe:[1669309]' > l-wx------. 1 eblake eblake 64 Apr 15 13:03 2 -> 'pipe:[1669310]' > lr-x------. 1 eblake eblake 64 Apr 15 13:03 255 -> > /tmp/nbdkitevalVUNZ1F/configYes we might I suppose duplicate original stdin to another file descriptor and pass the FD in via an environment variable. [...]> 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, if > a script wants to set up an environment variable that will still be > present to affect later calls, it has to track things itself > (perhaps by having the .config callback append to $tmpdir/vars, then > all other callbacks start by 'source $tmpdir/vars'). Would it make > sense to reserve a special exit value from the .config callback for > the case where the script wants the current key=value passed to > config to be preserved to all future callbacks? Or even state that > the config callback exiting with status 2 (for missing) is NOT an > error, but does the key=value preservation automatically?Would it be secure, given that plausibly the command line could be supplied from a different place than the script. eg. if an untrusted user sets $PATH ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Eric Blake
2020-Apr-15 19:11 UTC
Re: [Libguestfs] [PATCH nbdkit 9/9] eval, sh: Define $nbdkit_safe_stdio = 0|1 in scripts.
On 4/15/20 1:18 PM, Richard W.M. Jones wrote:> On Wed, Apr 15, 2020 at 01:09:21PM -0500, Eric Blake wrote: >> On 4/15/20 11:16 AM, Richard W.M. Jones wrote: >> >> In the subject, you describe $nbdkit_safe_stdio, but in the patch body... >> >>> --- >>> plugins/eval/nbdkit-eval-plugin.pod | 11 +++-------- >>> plugins/sh/nbdkit-sh-plugin.pod | 18 +++++++++++++++++- >>> plugins/sh/call.c | 8 ++++++-- >>> tests/test-single-sh.sh | 4 ++++ >>> 4 files changed, 30 insertions(+), 11 deletions(-) >>> >> >> The patch does not work as intended as currently written: when we >> invoke /path/to/script config $key $value, we have already set up >> stdin/stdout to our own pipes [1]. > > Urgghh ... very true. > > I think patches 1-8 are still worthwhile though :-) And with the same > mechanism we can pass other environment variables in if we think of > any in future ($nbdkit_peer_name?).Indeed, and I plan to review them (a quick glance made it seem like they are nice, but I may find something in a closer look)> >> For .config and >> .config_complete, reading will always see EOF (no other callback >> needs to interact with the original stdin, and callbacks like >> .pwrite actually use the pipe for data). If we want to allow a >> script to read a password from stdin, we need to preserve the >> original fd to .config and .config_complete rather than passing in >> an empty pipe (but those are the only two callbacks where it makes >> sense, and even then, only when we did not use script=- or when -s >> is in effect). >> >> [1] ./nbdkit eval config='ls -l /proc/$$/fd >/dev/tty' a=b >> total 0 >> lr-x------. 1 eblake eblake 64 Apr 15 13:03 0 -> 'pipe:[1669308]' >> l-wx------. 1 eblake eblake 64 Apr 15 13:03 1 -> 'pipe:[1669309]' >> l-wx------. 1 eblake eblake 64 Apr 15 13:03 2 -> 'pipe:[1669310]' >> lr-x------. 1 eblake eblake 64 Apr 15 13:03 255 -> >> /tmp/nbdkitevalVUNZ1F/config > > Yes we might I suppose duplicate original stdin to another file > descriptor and pass the FD in via an environment variable.I think we can get away with just not overriding stdin/out with a pipe in the first place (no need to complicate things to tell the script about which alternat fd to use). The input pipe is important to .pwrite, but not .config.> > [...] > >> 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, if >> a script wants to set up an environment variable that will still be >> present to affect later calls, it has to track things itself >> (perhaps by having the .config callback append to $tmpdir/vars, then >> all other callbacks start by 'source $tmpdir/vars'). Would it make >> sense to reserve a special exit value from the .config callback for >> the case where the script wants the current key=value passed to >> config to be preserved to all future callbacks? Or even state that >> the config callback exiting with status 2 (for missing) is NOT an >> error, but does the key=value preservation automatically? > > Would it be secure, given that plausibly the command line could be > supplied from a different place than the script. eg. if an untrusted > user sets $PATH ...Interesting thought. That makes me lean more towards a new exit value (for intentional opt-in) rather than reusing status 2 (missing) as the reason to drive the new behavior. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Reasonably Related Threads
- Re: [PATCH nbdkit 9/9] eval, sh: Define $nbdkit_safe_stdio = 0|1 in scripts.
- Re: [PATCH nbdkit 9/9] eval, sh: Define $nbdkit_safe_stdio = 0|1 in scripts.
- [PATCH nbdkit 9/9] eval, sh: Define $nbdkit_safe_stdio = 0|1 in scripts.
- Re: [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
- [nbdkit PATCH 5/5] sh, eval: Implement .default_export