[Libguestfs] [PATCH nbdkit 9/9] eval, sh: Define $nbdkit_safe_stdio = 0|1 in scripts.

[Eric Blake]

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