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

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

 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

On 6/26/19 11:53 AM, 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.
On the other hand, allowing: prog='isoprog --parameter' may be
intentional, and I just broke that.  Maybe I need to revert this?
> 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@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.
> 

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

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

On Wed, Jun 26, 2019 at 12:16:02PM -0500, Eric Blake
wrote:
> On 6/26/19 11:53 AM, 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.
> 
> On the other hand, allowing: prog='isoprog --parameter' may be
> intentional, and I just broke that.  Maybe I need to revert this?
This is fine, because they can use params for that.  I think this
patch makes sense, so let's leave it.

Rich.
> > 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@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.
> > 
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 


> _______________________________________________
> Libguestfs mailing list
> Libguestfs@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
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html