Libguestfs - Nov 2015 - Re: [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)

When running commands in the mounted guest (using the "command" API,
and
APIs based on it), provide the /dev/null from the appliance as open fd
for stdin.  Commands usually assume stdin is open if they didn't close
it explicitly, so this should avoid crashes or misbehavings due to that.
---
 daemon/command.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/daemon/command.c b/daemon/command.c
index 1593de9..b2d84ca 100644
--- a/daemon/command.c
+++ b/daemon/command.c
@@ -23,6 +23,8 @@
 #include <stdbool.h>
 #include <string.h>
 #include <sys/stat.h>
+#include <sys/types.h>
+#include <fcntl.h>
 
 #include "guestfs_protocol.h"
 #include "daemon.h"
@@ -242,7 +244,7 @@ do_command (char *const *argv)
 {
   char *out;
   CLEANUP_FREE char *err = NULL;
-  int r;
+  int r, fd, flags;
   CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted = false };
   CLEANUP_RESOLVER_STATE struct resolver_state resolver_state      { .mounted =
false };
@@ -259,6 +261,17 @@ do_command (char *const *argv)
     return NULL;
   }
 
+  /* Provide /dev/null as stdin for the command, since we want
+   * to make sure processes have an open stdin, and it is not
+   * possible to rely on the guest to provide it (Linux guests
+   * get /dev dynamically populated at runtime by udev).
+   */
+  fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
+  if (fd == -1) {
+    reply_with_perror ("/dev/null");
+    return NULL;
+  }
+
   if (bind_mount (&bind_state) == -1)
     return NULL;
   if (enable_network) {
@@ -266,8 +279,10 @@ do_command (char *const *argv)
       return NULL;
   }
 
+  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd;
+
   CHROOT_IN;
-  r = commandv (&out, &err, (const char * const *) argv);
+  r = commandvf (&out, &err, flags, (const char * const *) argv);
   CHROOT_OUT;
 
   free_bind_state (&bind_state);
-- 
2.1.0

On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano
wrote:
> When running commands in the mounted guest (using the "command"
API, and
> APIs based on it), provide the /dev/null from the appliance as open fd
> for stdin.  Commands usually assume stdin is open if they didn't close
> it explicitly, so this should avoid crashes or misbehavings due to that.
> ---
>  daemon/command.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/daemon/command.c b/daemon/command.c
> index 1593de9..b2d84ca 100644
> --- a/daemon/command.c
> +++ b/daemon/command.c
> @@ -23,6 +23,8 @@
>  #include <stdbool.h>
>  #include <string.h>
>  #include <sys/stat.h>
> +#include <sys/types.h>
> +#include <fcntl.h>
>  
>  #include "guestfs_protocol.h"
>  #include "daemon.h"
> @@ -242,7 +244,7 @@ do_command (char *const *argv)
>  {
>    char *out;
>    CLEANUP_FREE char *err = NULL;
> -  int r;
> +  int r, fd, flags;
>    CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted = false };
>    CLEANUP_RESOLVER_STATE struct resolver_state resolver_state >      {
.mounted = false };
> @@ -259,6 +261,17 @@ do_command (char *const *argv)
>      return NULL;
>    }
>  
> +  /* Provide /dev/null as stdin for the command, since we want
> +   * to make sure processes have an open stdin, and it is not
> +   * possible to rely on the guest to provide it (Linux guests
> +   * get /dev dynamically populated at runtime by udev).
> +   */
> +  fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
> +  if (fd == -1) {
> +    reply_with_perror ("/dev/null");
> +    return NULL;
> +  }
> +
>    if (bind_mount (&bind_state) == -1)
>      return NULL;
>    if (enable_network) {
> @@ -266,8 +279,10 @@ do_command (char *const *argv)
>        return NULL;
>    }
>  
> +  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd;
> +
>    CHROOT_IN;
> -  r = commandv (&out, &err, (const char * const *) argv);
> +  r = commandvf (&out, &err, flags, (const char * const *) argv);
>    CHROOT_OUT;
>  
>    free_bind_state (&bind_state);
Looks good.  How about naming the variable 'dev_null_fd' or something,
so we know it's not just a temporary file descriptor variable?

Anyway, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Friday 20 November 2015 11:58:25 Richard W.M. Jones
wrote:
> On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano wrote:
> > When running commands in the mounted guest (using the
"command" API, and
> > APIs based on it), provide the /dev/null from the appliance as open fd
> > for stdin.  Commands usually assume stdin is open if they didn't
close
> > it explicitly, so this should avoid crashes or misbehavings due to
that.
> > ---
> >  daemon/command.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/daemon/command.c b/daemon/command.c
> > index 1593de9..b2d84ca 100644
> > --- a/daemon/command.c
> > +++ b/daemon/command.c
> > @@ -23,6 +23,8 @@
> >  #include <stdbool.h>
> >  #include <string.h>
> >  #include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <fcntl.h>
> >  
> >  #include "guestfs_protocol.h"
> >  #include "daemon.h"
> > @@ -242,7 +244,7 @@ do_command (char *const *argv)
> >  {
> >    char *out;
> >    CLEANUP_FREE char *err = NULL;
> > -  int r;
> > +  int r, fd, flags;
> >    CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted =
false };
> >    CLEANUP_RESOLVER_STATE struct resolver_state resolver_state >
>      { .mounted = false };
> > @@ -259,6 +261,17 @@ do_command (char *const *argv)
> >      return NULL;
> >    }
> >  
> > +  /* Provide /dev/null as stdin for the command, since we want
> > +   * to make sure processes have an open stdin, and it is not
> > +   * possible to rely on the guest to provide it (Linux guests
> > +   * get /dev dynamically populated at runtime by udev).
> > +   */
> > +  fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
> > +  if (fd == -1) {
> > +    reply_with_perror ("/dev/null");
> > +    return NULL;
> > +  }
> > +
> >    if (bind_mount (&bind_state) == -1)
> >      return NULL;
> >    if (enable_network) {
> > @@ -266,8 +279,10 @@ do_command (char *const *argv)
> >        return NULL;
> >    }
> >  
> > +  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd;
> > +
> >    CHROOT_IN;
> > -  r = commandv (&out, &err, (const char * const *) argv);
> > +  r = commandvf (&out, &err, flags, (const char * const *)
argv);
> >    CHROOT_OUT;
> >  
> >    free_bind_state (&bind_state);
> 
> Looks good.  How about naming the variable 'dev_null_fd' or
something,
> so we know it's not just a temporary file descriptor variable?
Makes sense, pushed with the suggested correction.

Thanks,
-- 
Pino Toscano

On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano
wrote:
> When running commands in the mounted guest (using the "command"
API, and
> APIs based on it), provide the /dev/null from the appliance as open fd
> for stdin.  Commands usually assume stdin is open if they didn't close
> it explicitly, so this should avoid crashes or misbehavings due to that.
This does not look right.
> +  /* Provide /dev/null as stdin for the command, since we want
> +   * to make sure processes have an open stdin, and it is not
> +   * possible to rely on the guest to provide it (Linux guests
> +   * get /dev dynamically populated at runtime by udev).
> +   */
> +  fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
> +  if (fd == -1) {
> +    reply_with_perror ("/dev/null");
> +    return NULL;
> +  }
> +
I disagree with this (see below).
>    if (bind_mount (&bind_state) == -1)
>      return NULL;
nit: this leaks the fd on error, but it may not matter much.
>    if (enable_network) {
> @@ -266,8 +279,10 @@ do_command (char *const *argv)
>        return NULL;
>    }
>  
nit: same.
> +  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd;
> +
>    CHROOT_IN;
> -  r = commandv (&out, &err, (const char * const *) argv);
> +  r = commandvf (&out, &err, flags, (const char * const *) argv);
>    CHROOT_OUT;
>  
>    free_bind_state (&bind_state);
According to the analysis in
https://bugzilla.redhat.com/show_bug.cgi?id=1280029 the problem was that
the target program was being executed in a chroot which did not have
/dev/null populated, hence the open in commandrvf failed and the process
was left without fd 0.

commandrvf does the following in the child:
    close (0);
    if (flag_copy_stdin) {
      dup2 (flag_copy_fd, STDIN_FILENO);
    } else {
      /* Set stdin to /dev/null (ignore failure) */
      ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC));
    }
    [..]
    execvp (argv[0], (void *) argv);

First observation is that regardless of whether this open("/dev/null",
..)
succeeds, there is no fd 0 in the process after execvp due to O_CLOEXEC.

So, chances are the chroot did have /dev/null, but the fd simply got closed.

I would argue that /dev has to be at least partially populated for anything
that gets executed in the chroot. At the very least special nodes like null,
zero and {u,}random are needed.

CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be
done in the child, which also removes the need to chroot out in the
parent.

Assuming populated /dev is problematic/not feasible, at the very least
the open(/dev/null) should be performed in the child just prior to
chroot.

Current patch seems to work around shortcomings of the current API.

Side content:
      if (flag_copy_stdin) close (flag_copy_fd);
      waitpid (pid, NULL, 0);
      return -1;

but some lines below there is:
  if (flag_copy_stdin && close (flag_copy_fd) == -1) {
    perror ("close");
    return -1;
  }

  /* Get the exit status of the command. */
  if (waitpid (pid, &r, 0) != pid) {
    perror ("waitpid");
    return -1;
  }


close() does not return an error unless extraterrestial circumstances occur,
and even then the fd is no longer in use by the process. As such, I would argue
checking for errors here is not necessary. Note that prior sample does not
check.

In case an error was returned, the code fails to waitpid() for the child.

-- 
Mateusz Guzik

On Tuesday 01 December 2015 15:59:56 Mateusz Guzik
wrote:
> On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano wrote:
> > When running commands in the mounted guest (using the
"command" API, and
> > APIs based on it), provide the /dev/null from the appliance as open fd
> > for stdin.  Commands usually assume stdin is open if they didn't
close
> > it explicitly, so this should avoid crashes or misbehavings due to
that.
> 
> This does not look right.
> 
> > +  /* Provide /dev/null as stdin for the command, since we want
> > +   * to make sure processes have an open stdin, and it is not
> > +   * possible to rely on the guest to provide it (Linux guests
> > +   * get /dev dynamically populated at runtime by udev).
> > +   */
> > +  fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
> > +  if (fd == -1) {
> > +    reply_with_perror ("/dev/null");
> > +    return NULL;
> > +  }
> > +
> 
> I disagree with this (see below).
> 
> >    if (bind_mount (&bind_state) == -1)
> >      return NULL;
> 
> nit: this leaks the fd on error, but it may not matter much.
> 
> >    if (enable_network) {
> > @@ -266,8 +279,10 @@ do_command (char *const *argv)
> >        return NULL;
> >    }
> >  
> 
> nit: same.
Both these leaks need to be fixed, thanks for reporting them.
> > +  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd;
> > +
> >    CHROOT_IN;
> > -  r = commandv (&out, &err, (const char * const *) argv);
> > +  r = commandvf (&out, &err, flags, (const char * const *)
argv);
> >    CHROOT_OUT;
> >  
> >    free_bind_state (&bind_state);
> 
> According to the analysis in
> https://bugzilla.redhat.com/show_bug.cgi?id=1280029 the problem was that
> the target program was being executed in a chroot which did not have
> /dev/null populated, hence the open in commandrvf failed and the process
> was left without fd 0.
> 
> commandrvf does the following in the child:
>     close (0);
>     if (flag_copy_stdin) {
>       dup2 (flag_copy_fd, STDIN_FILENO);
>     } else {
>       /* Set stdin to /dev/null (ignore failure) */
>       ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC));
>     }
>     [..]
>     execvp (argv[0], (void *) argv);
> 
> First observation is that regardless of whether this
open("/dev/null", ..)
> succeeds, there is no fd 0 in the process after execvp due to O_CLOEXEC.
Correct, so O_CLOEXEC could be dropped in this case.
> So, chances are the chroot did have /dev/null, but the fd simply got
closed.
No, the actual issue was that there was nothing at all in the /dev of
guest, so open failed.
> I would argue that /dev has to be at least partially populated for anything
> that gets executed in the chroot. At the very least special nodes like
null,
> zero and {u,}random are needed.
We do not assume anything about guests, which could be Linux with a
static /dev (rare these days), but also non-Linux guests, including
Windows.
> CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be
> done in the child, which also removes the need to chroot out in the
> parent.
That could be another way to make the command* functions in the daemon
safer, which wouldn't solve the issue of this email thread: even if you
fork and then chroot, the guest has nothing in /dev, so you cannot
open /dev/null at that point. This means you still need to carry on an
open /dev/null from the appliance.
> Assuming populated /dev is problematic/not feasible, at the very least
> the open(/dev/null) should be performed in the child just prior to
> chroot.
See above.
> Current patch seems to work around shortcomings of the current API.
> 
> Side content:
>       if (flag_copy_stdin) close (flag_copy_fd);
>       waitpid (pid, NULL, 0);
>       return -1;
This is done only in the main "while (quit < 2)" loop, and only on
error there, which will return with -1.
> but some lines below there is:
>   if (flag_copy_stdin && close (flag_copy_fd) == -1) {
>     perror ("close");
>     return -1;
>   }
> 
>   /* Get the exit status of the command. */
>   if (waitpid (pid, &r, 0) != pid) {
>     perror ("waitpid");
>     return -1;
>   }
> 
> 
> close() does not return an error unless extraterrestial circumstances
occur,
> and even then the fd is no longer in use by the process. As such, I would
argue
> checking for errors here is not necessary. Note that prior sample does not
check.
The code for flag_copy_stdin & flag_copy_fd is unrelated to the bit
quoted above, see above for the explanation.
> In case an error was returned, the code fails to waitpid() for the child.
This needs to be fixed indeed.

Thanks,
-- 
Pino Toscano

On Tue, Dec 01, 2015 at 03:59:56PM +0100, Mateusz Guzik
wrote:
> CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be
> done in the child, which also removes the need to chroot out in the
> parent.
The CHROOT_IN/OUT business does need to be rewritten.  Every
instance where we currently do something like:

  CHROOT_IN;
  r = stat (fd, &statbuf);
  CHROOT_OUT

[https://github.com/libguestfs/libguestfs/blob/master/daemon/stat.c#L93-L95]

should instead be forking a subprocess, chrooting in the subprocess,
and doing the system call in the subprocess.

The problem which makes it not so easy is that instead of using a nice
local variable, we would have to pass back the result from a
subprocess to the parent process (the pair (r, statbuf) in the above
example).

So that means .. a pipe, and serializing the result down the pipe.

The good news is that since this all runs on the same machine in the
same compiled program, it's quite acceptable to dump a C struct into
the pipe.

But it's still a lot of work .. patches welcome of course.

Rich.

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