[Libguestfs] [nbdkit PATCH] iso: Shell-quote an alternative isoprog

[Richard W.M. Jones]

On Wed, Jun 26, 2019 at 11:53:04AM -0500, Eric Blake wrote:
> Otherwise, a user can do things like "nbdkit iso . prog='date;prog'"
> to run unintended commands in addition to their alternative isoprog.
> This is not a CVE (since nbdkit isn't running with any more privileges
> than the user running those commands themselves), but shows the
> frailty of relying on the shell to parse subsidiary commands rather
> than exec()ing them directly.  This patch also doesn't resolve the
> fact that we are also passing params= through shell parsing (if we
> don't like that, we should consider changing the interface to make the
> user write param='-V' param='My Disk Image' and use shell_quote() over
> each param, rather than the current params='-V "My Disk Image"'), but
> does try to enhance the docs to point it out with more clarity.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> I'm pushing this now, but we may want to reconsider the iso plugin
> exposing params= that is intentionally designed for another round of
> shell parsing, as a followup patch.  Ideally, we want to avoid ever
> passing user-supplied data through another shell invocation without
> first re-quoting it.

This is fine.  I think users can use params if they want to add
extra parameters.

ACK

Thanks, Rich.

>  plugins/iso/nbdkit-iso-plugin.pod | 4 +++-
>  plugins/iso/iso.c                 | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/iso/nbdkit-iso-plugin.pod b/plugins/iso/nbdkit-iso-plugin.pod
> index 90e26f01..597c3c50 100644
> --- a/plugins/iso/nbdkit-iso-plugin.pod
> +++ b/plugins/iso/nbdkit-iso-plugin.pod
> @@ -62,7 +62,9 @@ For example:
>  would specify Joliet (I<-J>), Rock Ridge (I<-r>) and TRANS.TBL (I<-T>)
>  extensions, and specify the volume ID (I<-V>) as C<My Disk Image>.
> 
> -Take care when quoting this parameter.
> +Take care when quoting this parameter; nbdkit passes the resulting
> +string through another layer of shell interpretation without any
> +sanity checks for unquoted shell metacharacters.
> 
>  =item B<prog=>mkisofs
> 
> diff --git a/plugins/iso/iso.c b/plugins/iso/iso.c
> index 4728ff32..5634bac9 100644
> --- a/plugins/iso/iso.c
> +++ b/plugins/iso/iso.c
> @@ -94,7 +94,8 @@ make_iso (void)
>      return -1;
>    }
> 
> -  fprintf (fp, "%s -quiet", isoprog);
> +  shell_quote (isoprog, fp);
> +  fprintf (fp, " -quiet");
>    if (params)
>      fprintf (fp, " %s", params);
>    for (i = 0; i < nr_dirs; ++i) {
> -- 
> 2.20.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

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