[Libguestfs] [PATCH nbdkit v2] tmpdisk: Pass any parameters as shell variables to the command.

[Eric Blake]

On 4/7/20 11:24 AM, Richard W.M. Jones wrote:
> This allows us to be much more flexible about what commands can be
> used.  It also means we do not need to encode any special behaviour
> for type or label parameters.
> ---
>   plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod |  91 +++++++++-----
>   plugins/tmpdisk/tmpdisk.c                 | 147 ++++++++++++++--------
>   plugins/tmpdisk/default-command.sh.in     |   6 +
>   3 files changed, 164 insertions(+), 80 deletions(-)
> 
> diff --git a/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod b/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod
> index 490bcf6c..f9e3296a 100644
> --- a/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod
> +++ b/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod
> @@ -4,9 +4,9 @@ nbdkit-tmpdisk-plugin - create a fresh temporary filesystem for each client
>   
>   =head1 SYNOPSIS
>   
> - nbdkit tmpdisk [size=]SIZE
> -                [type=ext4|xfs|vfat|...] [label=LABEL]
> -                [command=COMMAND]
> + nbdkit tmpdisk [size=]SIZE [type=ext4|xfs|vfat|...] [label=LABEL]
> +
> + nbdkit tmpdisk [size=]SIZE command=COMMAND
>   

Is it worth spelling this:

nbdkit tmpdisk [size=]SIZE command=COMMAND [VAR=VALUE...]

> +=item C<$size>
> +
> +The virtual size in bytes.  This is the C<size> parameter, converted
> +to bytes.

Is it worth mentioning that if the command ends up rounding the passed 
in size, the size passed in to nbdkit may differ from the size 
advertised to the client?

> +=head2 Serve a fresh operating system to each client
> +
> + nbdkit tmpdisk 16G \
> +     command=' virt-builder -o "$disk" --size ${size}b "$os" ' \
> +     os=fedora-31

Cool, even if it is time-consuming setup :)

> @@ -96,6 +119,10 @@ Select the filesystem label.  The default is not set.
>   
>   Specify the virtual size of the disk image.
>   
> +If using C<command>, this is only a suggested size.  The actual size
> +of the resulting disk will be the size of the disk created by
> +C<command>.

Ah, this addresses my comment above.

> +++ b/plugins/tmpdisk/tmpdisk.c

> +/* Shell variables. */
> +static struct var {
> +  struct var *next;
> +  const char *key, *value;
> +} *vars;

Linked-list, where...

>   static int
>   tmpdisk_config (const char *key, const char *value)
>   {

> +  /* Any other parameter will be forwarded to a shell variable. */
>     else {
> -    nbdkit_error ("unknown parameter '%s'", key);
> -    return -1;
> +    struct var *v_next = vars;
> +
> +    vars = malloc (sizeof *vars);
> +    if (vars == NULL) {
> +      perror ("malloc");
> +      exit (EXIT_FAILURE);
> +    }
> +    vars->next = v_next;
> +    vars->key = key;
> +    vars->value = value;

...the list is populated with later entries first, and...

> @@ -163,6 +191,7 @@ run_command (const char *disk)
>     CLEANUP_FREE char *cmd = NULL;
>     size_t len = 0;
>     int r;
> +  struct var *v;
>   
>     fp = open_memstream (&cmd, &len);
>     if (fp == NULL) {
> @@ -173,21 +202,26 @@ run_command (const char *disk)
>     /* Avoid stdin/stdout leaking (because of nbdkit -s). */
>     fprintf (fp, "exec </dev/null >/dev/null\n");
>   
> -  /* Set the shell variables. */
> +  /* Set the standard shell variables. */
>     fprintf (fp, "disk=");
>     shell_quote (disk, fp);
>     putc ('\n', fp);
> -  if (label) {
> -    fprintf (fp, "label=");
> -    shell_quote (label, fp);
> +  fprintf (fp, "size=%" PRIi64 "\n", requested_size);
> +  putc ('\n', fp);
> +
> +  /* The other parameters/shell variables. */
> +  for (v = vars; v != NULL; v = v->next) {
> +    /* Keys probably can never contain shell-unsafe chars (because of
> +     * nbdkit's own restrictions), but quoting it makes it safe.
> +     */
> +    shell_quote (v->key, fp);

You are correct that nbdkit itself prevents non-safe names from being 
passed as a key to config.  But if it did not, (for example, if '1=a' 
were allowed through instead of an error), quoting the key would end up 
trying to invoke a command rather than setting a shell variable.  If 
anything, I think it might be a bit cleaner to assert() that key is a 
valid shell name, instead of trying to shell_quote() it.  On the other 
hand, I don't see it as a security bug: even if 'nbdkit tmpdisk size=... 
command=... 1=a' sneaks through and caused the shell to attempt 
execution of a command named '1=a', it's merely user error rather than a 
CVE because nbdkit isn't running with any additional privileges compared 
to the user just running '1=a' himself in the first place.

But it's all theoretical (shell_quote() is a no-op on safe names, and as 
the comment says, we are currently guaranteed a safe name), so I can 
live with what you have.

> +    putc ('=', fp);
> +    shell_quote (v->value, fp);
>       putc ('\n', fp);
>     }

...then processed front-to-back in passing to the shell.  This means that:

nbdkit tmpdisk command=... a=1 a=2

results in command seeing $a as 1, which is a bit confusing (most 
command lines allow later parameters to override earlier ones).  Fixing 
it would mean maintaining two pointers: the head of the list 
(unchanging, used when reading the list), and the tail (updated with 
each call to .config when building the list).

> @@ -228,36 +264,25 @@ tmpdisk_open (int readonly)
>       goto error;
>     }
>     h->fd = -1;
> +  h->size = -1;
>     h->can_punch_hole = true;
>   
> -  /* Create the new disk image for this connection. */
> -  if (asprintf (&disk, "%s/tmpdiskXXXXXX", tmpdir) == -1) {
> +  /* For security reasons we have to create a temporary directory
> +   * under tmpdir that only the current user can access.  If we
> +   * created it in a shared directory then another user might be able
> +   * to see the temporary file being created and interfere with it
> +   * before we reopen it in the plugin below.
> +   */
> +  if (asprintf (&dir, "%s/tmpdiskXXXXXX", tmpdir) == -1) {
>       nbdkit_error ("asprintf: %m");
>       goto error;
>     }
> -
> -#ifdef HAVE_MKOSTEMP
> -  h->fd = mkostemp (disk, O_CLOEXEC);
> -#else
> -  /* Racy, fix your libc. */
> -  h->fd = mkstemp (disk);
> -  if (h->fd >= 0) {
> -    h->fd = set_cloexec (h->fd);
> -    if (h->fd == -1) {
> -      int e = errno;
> -      unlink (disk);
> -      errno = e;
> -    }
> -  }
> -#endif
> -  if (h->fd == -1) {
> -    nbdkit_error ("mkstemp: %m");
> +  if (mkdtemp (dir) == NULL) {

Hmm - even though we know Haiku still lacks mkostemp, 
https://github.com/haiku/haiku/blob/master/src/system/libroot/posix/stdlib/mktemp.c 
says that it at least has mkdtemp.  Blindly using it for now is fine; if 
we later need to add a fallback for whatever platform lacks it, we can 
deal with it then.  The use of mkdtemp is indeed safe.

> +    nbdkit_error ("%s: %m", dir);
>       goto error;
>     }
> -
> -  /* Truncate the disk to a sparse file of the right size. */
> -  if (ftruncate (h->fd, size) == -1) {
> -    nbdkit_error ("ftruncate: %s: %m", disk);
> +  if (asprintf (&disk, "%s/disk", dir) == -1) {
> +    nbdkit_error ("asprintf: %m");
>       goto error;
>     }
>   
> @@ -265,11 +290,34 @@ tmpdisk_open (int readonly)
>     if (run_command (disk) == -1)
>       goto error;
>   
> +  /* The external command must have created the disk, and then we must
> +   * find the true size.
> +   */
> +  if (readonly)
> +    flags = O_RDONLY | O_CLOEXEC;
> +  else
> +    flags = O_RDWR | O_CLOEXEC | O_NOCTTY;

It looks odd to use O_NOCTTY for write but not read.  O_NOCTTY is 
important if we are worried that the user may have created a pty at 
$disk (in which case, we do not want that pty becoming the controlling 
terminal for nbdkit itself), but such a user is crazy ($command should 
normally create a regular file).  And if we really are worried about 
O_NOCTTY, why are we not worried about O_NONBLOCK to avoid hanging if 
the user creates a FIFO?  Are we also worried enough about the user 
trying to trick us into a symlink attack to use O_NOFOLLOW?  I could 
live with just 'O_RDRW | O_CLOEXEC'.

I suspect Haiku might need a followup for handling the fact that it 
lacks O_CLOEXEC, but saving it for a separate patch is fine.

> +  h->fd = open (disk, flags);
> +  if (h->fd == -1) {
> +    nbdkit_error ("open: %s: %m", disk);
> +    goto error;
> +  }
> +
> +  if (fstat (h->fd, &statbuf) == -1) {
> +    nbdkit_error ("fstat: %s: %m", disk);
> +    goto error;
> +  }
> +
> +  h->size = statbuf.st_size;

Is this going to be the right size if command created a block device 
(either directly at $disk via mknod, or more likely elsewhere with $disk 
being a symlink to that alternate location)?  Should we check fstat() 
results for S_ISREG/S_ISBLK to decide whether we need to lseek() to 
determine the actual size?

But as to your question about whether the security looks reasonable, 
yes, I think your use of mkdtemp ensures that no one else is trampling 
on the image between $command creating it and when we finally open() our fd.

Overall this looks like a sensible direction to head in.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org