Libguestfs - Jun 2010 - [PATCH 0/3] some guestfish sub commands can not handle special files properly (RHBZ#582484)

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
-------------- next part --------------
>From e3befe5a2e85179dcc5a52aa7d74b9cc5f3430ec Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 4 Jun 2010 11:23:01 +0100
Subject: [PATCH 1/3] daemon: Rearrange code in 'file' command.

  path = path to access file (/sysroot/.. or /dev/..)
  display_path = original path, saved so we can display it
  buf = optional buffer which is freed along return codepaths

There should be no change to the semantics of the code.
---
 daemon/file.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/daemon/file.c b/daemon/file.c
index 7600064..2594207 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -512,20 +512,18 @@ do_pwrite (const char *path, const char *content, size_t
size, int64_t offset)
 char *
 do_file (const char *path)
 {
-  char *out, *err;
-  int r, freeit = 0;
-  char *buf;
-  int len;
+  char *buf = NULL;
+  const char *display_path = path;
 
-  if (STREQLEN (path, "/dev/", 5))
-    buf = (char *) path;
-  else {
+  int is_dev = STRPREFIX (path, "/dev/");
+
+  if (!is_dev) {
     buf = sysroot_path (path);
     if (!buf) {
       reply_with_perror ("malloc");
       return NULL;
     }
-    freeit = 1;
+    path = buf;
   }
 
   /* file(1) manpage claims "file returns 0 on success, and non-zero on
@@ -533,26 +531,27 @@ do_file (const char *path)
    * every scenario I can think up.  So check the target is readable
    * first.
    */
-  if (access (buf, R_OK) == -1) {
-    if (freeit) free (buf);
-    reply_with_perror ("access: %s", path);
+  if (access (path, R_OK) == -1) {
+    reply_with_perror ("access: %s", display_path);
+    free (buf);
     return NULL;
   }
 
-  r = command (&out, &err, "file", "-zbsL", buf,
NULL);
-  if (freeit) free (buf);
+  char *out, *err;
+  int r = command (&out, &err, "file", "-zbsL",
path, NULL);
+  free (buf);
 
   if (r == -1) {
     free (out);
-    reply_with_error ("%s: %s", path, err);
+    reply_with_error ("%s: %s", display_path, err);
     free (err);
     return NULL;
   }
   free (err);
 
   /* We need to remove the trailing \n from output of file(1). */
-  len = strlen (out);
-  if (out[len-1] == '\n')
+  size_t len = strlen (out);
+  if (len > 0 && out[len-1] == '\n')
     out[len-1] = '\0';
 
   return out;			/* caller frees */
-- 
1.6.6.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
-------------- next part --------------
>From 74958b0ad44df6ed703cd3009983d04ade3a8e93 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 4 Jun 2010 11:55:54 +0100
Subject: [PATCH 2/3] touch: Restrict touch to regular files only (RHBZ#582484).

---
 daemon/file.c    |   20 ++++++++++++++++++++
 src/generator.ml |    5 ++++-
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/daemon/file.c b/daemon/file.c
index 2594207..9824472 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -34,6 +34,26 @@ do_touch (const char *path)
 {
   int fd;
   int r;
+  struct stat buf;
+
+  /* RHBZ#582484: Restrict touch to regular files.  It's also OK
+   * here if the file does not exist, since we will create it.
+   */
+  CHROOT_IN;
+  r = lstat (path, &buf);
+  CHROOT_OUT;
+
+  if (r == -1) {
+    if (errno != ENOENT) {
+      reply_with_perror ("lstat: %s", path);
+      return -1;
+    }
+  } else {
+    if (! S_ISREG (buf.st_mode)) {
+      reply_with_error ("%s: touch can only be used on a regular
files", path);
+      return -1;
+    }
+  }
 
   CHROOT_IN;
   fd = open (path, O_WRONLY | O_CREAT | O_NOCTTY, 0666);
diff --git a/src/generator.ml b/src/generator.ml
index ec6123a..c7dbdfc 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -992,7 +992,10 @@ closing the handle.");
    "\
 Touch acts like the L<touch(1)> command.  It can be used to
 update the timestamps on a file, or, if the file does not exist,
-to create a new zero-length file.");
+to create a new zero-length file.
+
+This command only works on regular files, and will fail on other
+file types such as directories, symbolic links, block special etc.");
 
   ("cat", (RString "content", [Pathname "path"]),
4, [ProtocolLimitWarning],
    [InitISOFS, Always, TestOutput (
-- 
1.6.6.1

I'm not entirely happy about the semantic change to the 'file'
command
here, but symbolic links probably didn't work correctly before for the
majority of use cases, and this also fixes a potential DoS attack from
untrusted disk images.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
-------------- next part --------------
>From 4df593496e116dfb635731c058b7627e81fc179c Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 4 Jun 2010 11:45:06 +0100
Subject: [PATCH 3/3] file: Restrict to regular files (RHBZ#582484).

The file call can hang if called on char devices (because we are
using the file -s option).

This is hard to solve cleanly without adding another file API.

However this restricts file to regular files, unless called explicitly
with a /dev/ path.  For non-regular files, it will now return a
string like "directory".

There is a small semantic change for symbolic links.  Previously
it would not have worked at all on absolute links (or rather, the
results would have been undefined).  It would have treated relative
symlinks to regular files as the regular file itself.  Now it will
return the string "symbolic link" in both cases.

This commit also makes the API safe when called on untrusted
filesystems.  Previously a filesystem might have been set up so
that (eg) /etc/redhat-release was a char device, which would have
caused virt-inspector and virt-v2v to hang.  Now it will not hang.
---
 daemon/file.c    |   46 +++++++++++++++++++++++++++++++++++-----------
 src/generator.ml |   23 ++++++++++++++++++-----
 2 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/daemon/file.c b/daemon/file.c
index 9824472..a55c606 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -544,21 +544,45 @@ do_file (const char *path)
       return NULL;
     }
     path = buf;
-  }
 
-  /* file(1) manpage claims "file returns 0 on success, and non-zero on
-   * error", but this is evidently not true.  It always returns 0, in
-   * every scenario I can think up.  So check the target is readable
-   * first.
-   */
-  if (access (path, R_OK) == -1) {
-    reply_with_perror ("access: %s", display_path);
-    free (buf);
-    return NULL;
+    /* For non-dev, check this is a regular file, else just return the
+     * file type as a string (RHBZ#582484).
+     */
+    struct stat statbuf;
+    if (lstat (path, &statbuf) == -1) {
+      reply_with_perror ("lstat: %s", display_path);
+      free (buf);
+      return NULL;
+    }
+
+    if (! S_ISREG (statbuf.st_mode)) {
+      char *ret;
+
+      free (buf);
+
+      if (S_ISDIR (statbuf.st_mode))
+        ret = strdup ("directory");
+      else if (S_ISCHR (statbuf.st_mode))
+        ret = strdup ("character device");
+      else if (S_ISBLK (statbuf.st_mode))
+        ret = strdup ("block device");
+      else if (S_ISFIFO (statbuf.st_mode))
+        ret = strdup ("FIFO");
+      else if (S_ISLNK (statbuf.st_mode))
+        ret = strdup ("symbolic link");
+      else if (S_ISSOCK (statbuf.st_mode))
+        ret = strdup ("socket");
+      else
+        ret = strdup ("unknown, not regular file");
+
+      if (ret == NULL)
+        reply_with_perror ("strdup");
+      return ret;
+    }
   }
 
   char *out, *err;
-  int r = command (&out, &err, "file", "-zbsL",
path, NULL);
+  int r = command (&out, &err, "file", "-zbs",
path, NULL);
   free (buf);
 
   if (r == -1) {
diff --git a/src/generator.ml b/src/generator.ml
index c7dbdfc..37d63f2 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -1632,19 +1632,32 @@ and physical volumes.");
     InitISOFS, Always, TestOutput (
       [["file"; "/known-1"]], "ASCII text");
     InitISOFS, Always, TestLastFail (
-      [["file"; "/notexists"]])],
+      [["file"; "/notexists"]]);
+    InitISOFS, Always, TestOutput (
+      [["file"; "/abssymlink"]], "symbolic
link");
+    InitISOFS, Always, TestOutput (
+      [["file"; "/directory"]], "directory")],
    "determine file type",
    "\
 This call uses the standard L<file(1)> command to determine
-the type or contents of the file.  This also works on devices,
-for example to find out whether a partition contains a filesystem.
+the type or contents of the file.
 
 This call will also transparently look inside various types
 of compressed file.
 
-The exact command which runs is C<file -zbsL path>.  Note in
+The exact command which runs is C<file -zbs path>.  Note in
 particular that the filename is not prepended to the output
-(the C<-b> option).");
+(the C<-b> option).
+
+This command can also be used on C</dev/> devices
+(and partitions, LV names).  You can for example use this
+to determine if a device contains a filesystem, although
+it's usually better to use C<guestfs_vfs_type>.
+
+If the C<path> does not begin with C</dev/> then
+this command only works for the content of regular files.
+For other file types (directory, symbolic link etc) it
+will just return the string C<directory> etc.");
 
   ("command", (RString "output", [StringList
"arguments"]), 50, [ProtocolLimitWarning],
    [InitBasicFS, Always, TestOutput (
-- 
1.6.6.1