Libguestfs - Mar 2015 - [PATCH] inspection: add support for systemd .mount files

Adds support for systemd .mount files, uses Augeas to extract mount points.
Fixes RHBZ#1113153.

Maros Zatko (1):
  inspection: add support for systemd .mount files

 src/inspect-fs-unix.c | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 240 insertions(+)

-- 
1.9.3

Fixes RHBZ#1113153.
---
 src/inspect-fs-unix.c | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 240 insertions(+)

diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
index 2abbf24..6dfc299 100644
--- a/src/inspect-fs-unix.c
+++ b/src/inspect-fs-unix.c
@@ -96,6 +96,9 @@ static char *resolve_fstab_device (guestfs_h *g, const char
*spec,
                                    Hash_table *md_map,
                                    enum inspect_os_type os_type);
 static int inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs, const char
**configfiles, int (*f) (guestfs_h *, struct inspect_fs *));
+static int inspect_with_augeas2 (guestfs_h *g, struct inspect_fs *fs, const
char **configfiles, int (*f) (guestfs_h *, struct inspect_fs *, const char *));
+static int check_systemd_mounts (guestfs_h *g, struct inspect_fs *fs);
+static int check_systemd_mnt (guestfs_h *g, struct inspect_fs *fs, const char
*path);
 
 /* Hash structure for uuid->path lookups */
 typedef struct md_uuid {
@@ -582,6 +585,9 @@ guestfs_int_check_linux_root (guestfs_h *g, struct
inspect_fs *fs)
   if (check_hostname_unix (g, fs) == -1)
     return -1;
 
+  if (check_systemd_mounts (g, fs) == -1)
+    return -1;
+
   return 0;
 }
 
@@ -981,6 +987,180 @@ check_hostname_freebsd (guestfs_h *g, struct inspect_fs
*fs)
 }
 
 static int
+check_systemd_mnt (guestfs_h *g, struct inspect_fs *fs, const char *fname)
+{
+  CLEANUP_FREE_STRING_LIST char **entries = NULL;
+  char **entry;
+  char augpath[256];
+  CLEANUP_HASH_FREE Hash_table *md_map = NULL;
+  bool is_bsd = (fs->type == OS_TYPE_FREEBSD ||
+                 fs->type == OS_TYPE_NETBSD ||
+                 fs->type == OS_TYPE_OPENBSD);
+
+  /* Generate a map of MD device paths listed in /etc/mdadm.conf to MD device
+   * paths in the guestfs appliance */
+  if (map_md_devices (g, &md_map) == -1) {
+    printf ("map_md_dev fail\n");
+    return -1;
+  }
+
+  char expr[512];
+  snprintf (expr, sizeof expr, "/files%s", fname);
+  entries = guestfs_aug_match (g, expr);
+  if (entries == NULL) {
+    return 0;
+  }
+
+  for (entry = entries; *entry != NULL; entry++) {
+    CLEANUP_FREE char *spec = NULL;
+    CLEANUP_FREE char *mp = NULL;
+    CLEANUP_FREE char *mountable = NULL;
+    CLEANUP_FREE char *vfstype = NULL;
+
+    snprintf (augpath, sizeof augpath, "%s/Mount/What/value",
*entry);
+    spec = guestfs_aug_get (g, augpath);
+    if (spec == NULL)
+      return -1;
+
+    /* Ignore /dev/fd (floppy disks) (RHBZ#642929) and CD-ROM drives.
+     *
+     * /dev/iso9660/FREEBSD_INSTALL can be found in FreeBSDs installation
+     * discs.
+     */
+    if ((STRPREFIX (spec, "/dev/fd") && c_isdigit (spec[7]))
||
+        STREQ (spec, "/dev/floppy") ||
+        STREQ (spec, "/dev/cdrom") ||
+        STRPREFIX (spec, "/dev/iso9660/"))
+      continue;
+
+    snprintf (augpath, sizeof augpath, "%s/Mount/Where/value",
*entry);
+    mp = guestfs_aug_get (g, augpath);
+    if (mp == NULL)
+      return -1;
+
+    /* Ignore certain mountpoints. */
+    if (STRPREFIX (mp, "/dev/") ||
+        STREQ (mp, "/dev") ||
+        STRPREFIX (mp, "/media/") ||
+        STRPREFIX (mp, "/proc/") ||
+        STREQ (mp, "/proc") ||
+        STRPREFIX (mp, "/selinux/") ||
+        STREQ (mp, "/selinux") ||
+        STRPREFIX (mp, "/sys/") ||
+        STREQ (mp, "/sys"))
+      continue;
+
+    /* Resolve UUID= and LABEL= to the actual device. */
+    if (STRPREFIX (spec, "UUID="))
+      mountable = guestfs_findfs_uuid (g, &spec[5]);
+    else if (STRPREFIX (spec, "LABEL="))
+      mountable = guestfs_findfs_label (g, &spec[6]);
+    /* Ignore "/.swap" (Pardus) and pseudo-devices like
"tmpfs". */
+    else if (STREQ (spec, "/dev/root") || (is_bsd && STREQ
(mp, "/")))
+      /* Resolve /dev/root to the current device.
+       * Do the same for the / partition of the *BSD systems, since the
+       * BSD -> Linux device translation is not straight forward.
+       */
+      mountable = safe_strdup (g, fs->mountable);
+    else if (STRPREFIX (spec, "/dev/"))
+      /* Resolve guest block device names. */
+      mountable = resolve_fstab_device (g, spec, md_map, fs->type);
+    else if (match (g, spec, re_openbsd_duid)) {
+      /* In OpenBSD's fstab you can specify partitions on a disk by
appending a
+       * period and a partition letter to a Disklable Unique Identifier. The
+       * DUID is a 16 hex digit field found in the OpenBSD's altered BSD
+       * disklabel. For more info see here:
+       * http://www.openbsd.org/faq/faq14.html#intro
+       */
+       char device[10]; /* /dev/sd[0-9][a-z] */
+       char part = spec[17];
+
+       /* We cannot peep into disklables, we can only assume that this is the
+        * first disk.
+        */
+       snprintf(device, 10, "%s%c", "/dev/sd0", part);
+       mountable = resolve_fstab_device (g, device, md_map, fs->type);
+    }
+
+    /* If we haven't resolved the device successfully by this point,
+     * we don't care, just ignore it.
+     */
+    if (mountable == NULL)
+      continue;
+
+    snprintf (augpath, sizeof augpath, "%s/Mount/Type/value",
*entry);
+    vfstype = guestfs_aug_get (g, augpath);
+    if (vfstype == NULL) return -1;
+
+    if (STREQ (vfstype, "btrfs")) {
+      char **opt;
+
+      snprintf (augpath, sizeof augpath, "%s/Mount/Options/value",
*entry);
+      CLEANUP_FREE_STRING_LIST char **opts = guestfs_aug_match (g, augpath);
+      if (opts == NULL) return -1;
+
+      for (opt = opts; *opt; opt++) {
+        CLEANUP_FREE char *optname = guestfs_aug_get (g, augpath);
+        if (optname == NULL) return -1;
+
+        if (STREQ (optname, "subvol")) {
+          CLEANUP_FREE char *subvol = NULL;
+          char *new;
+
+          snprintf (augpath, sizeof augpath, "%s/value", *opt);
+          subvol = guestfs_aug_get (g, augpath);
+          if (subvol == NULL) return -1;
+
+          new = safe_asprintf (g, "btrfsvol:%s/%s", mountable,
subvol);
+          free (mountable);
+          mountable = new;
+        }
+      }
+    }
+
+    if  (add_fstab_entry (g, fs, mountable, mp) == -1) return -1;
+  }
+
+  return 0;
+}
+
+static int
+check_systemd_mounts (guestfs_h *g, struct inspect_fs *fs)
+{
+  const char *dirs[] = {"/etc/systemd/", "/lib/systemd/",
+    "/usr/lib/systemd/", NULL};
+  const char **dir;
+  for (dir = dirs; *dir != NULL; dir++) {
+    CLEANUP_FREE_STRING_LIST char **entries = NULL;
+    char **entry;
+
+    entries = guestfs_find (g, *dir);
+    size_t cnt = guestfs_int_count_strings (entries);
+
+    CLEANUP_FREE_STRING_LIST char **filenames +      safe_malloc (g, (cnt + 1)
* sizeof (char *));
+
+    size_t idx = 0;
+    for (entry = entries; *entry != NULL; entry++) {
+      if (STRSUFFIX (*entry, ".mount")) {
+        size_t entry_len = strlen(*dir) + strlen(*entry) + 1;
+        filenames[idx] = safe_malloc (g, entry_len);
+        snprintf (filenames[idx], entry_len, "%s%s", *dir, *entry);
+        ++idx;
+      }
+    }
+    filenames[idx] = NULL;
+
+    if (inspect_with_augeas2 (g, fs,
+          (const char**) filenames, check_systemd_mnt) == -1) {
+      return -1;
+    }
+  }
+
+  return 0;
+}
+
+static int
 check_fstab (guestfs_h *g, struct inspect_fs *fs)
 {
   CLEANUP_FREE_STRING_LIST char **entries = NULL;
@@ -1705,6 +1885,66 @@ static char *make_augeas_path_expression (guestfs_h *g,
const char **configfiles
  * to 'f' the Augeas handle is closed.
  */
 static int
+inspect_with_augeas2 (guestfs_h *g, struct inspect_fs *fs,
+                     const char **configfiles,
+                     int (*f) (guestfs_h *, struct inspect_fs *, const char *))
+{
+  size_t i;
+  int64_t size;
+  int r;
+  CLEANUP_FREE char *pathexpr = NULL;
+
+  /* Security: Refuse to do this if a config file is too large. */
+  for (i = 0; configfiles[i] != NULL; ++i) {
+    if (guestfs_is_file_opts (g, configfiles[i],
+                              GUESTFS_IS_FILE_OPTS_FOLLOWSYMLINKS, 1, -1) == 0)
+      continue;
+
+    size = guestfs_filesize (g, configfiles[i]);
+    if (size == -1)
+      /* guestfs_filesize failed and has already set error in handle */
+      return -1;
+    if (size > MAX_AUGEAS_FILE_SIZE) {
+      error (g, _("size of %s is unreasonably large (%" PRIi64 "
bytes)"),
+             configfiles[i], size);
+      return -1;
+    }
+  }
+
+  if (guestfs_aug_init (g, "/", 16|32 /* AUG_SAVE_NOOP|AUG_NO_LOAD
*/) == -1)
+    return -1;
+
+  r = -1;
+
+  /* Tell Augeas to only load configfiles and no other files.  This
+   * prevents a rogue guest from performing a denial of service attack
+   * by having large, over-complicated configuration files which are
+   * unrelated to the task at hand.  (Thanks Dominic Cleal).
+   * Note this requires Augeas >= 1.0.0 because of RHBZ#975412.
+   */
+  pathexpr = make_augeas_path_expression (g, configfiles);
+  if (guestfs_aug_rm (g, pathexpr) == -1)
+    goto out;
+
+  if (guestfs_aug_load (g) == -1)
+    goto out;
+
+  for (i = 0; configfiles[i] != NULL; ++i) {
+    r = f (g, fs, configfiles[i]);
+  }
+
+ out:
+  guestfs_aug_close (g);
+
+  return r;
+}
+
+/* Call 'f' with Augeas opened and having parsed 'configfiles'
(these
+ * files must exist).  As a security measure, this bails if any file
+ * is too large for a reasonable configuration file.  After the call
+ * to 'f' the Augeas handle is closed.
+ */
+static int
 inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs,
                      const char **configfiles,
                      int (*f) (guestfs_h *, struct inspect_fs *))
-- 
1.9.3

On Mon, Mar 16, 2015 at 07:09:51PM +0100, Maros Zatko
wrote:
> Fixes RHBZ#1113153.
> ---
>  src/inspect-fs-unix.c | 240
++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 240 insertions(+)
> 
> diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
> index 2abbf24..6dfc299 100644
> --- a/src/inspect-fs-unix.c
> +++ b/src/inspect-fs-unix.c
> @@ -96,6 +96,9 @@ static char *resolve_fstab_device (guestfs_h *g, const
char *spec,
>                                     Hash_table *md_map,
>                                     enum inspect_os_type os_type);
>  static int inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs, const
char **configfiles, int (*f) (guestfs_h *, struct inspect_fs *));
> +static int inspect_with_augeas2 (guestfs_h *g, struct inspect_fs *fs,
const char **configfiles, int (*f) (guestfs_h *, struct inspect_fs *, const char
*));
I think there is a case for splitting out the commit which adds
this new function.

Also, do we need a new function?  I don't see why it's needed at all -
just modify the old inspect_with_augeas so it passes the extra
parameter always, and update the call sites [in a separate commit].
> +static int check_systemd_mounts (guestfs_h *g, struct inspect_fs *fs);
> +static int check_systemd_mnt (guestfs_h *g, struct inspect_fs *fs, const
char *path);
You don't need to prototype check_systemd_mnt at all - just remove
that line as it's confusing for people reading the code.
>  /* Hash structure for uuid->path lookups */
>  typedef struct md_uuid {
> @@ -582,6 +585,9 @@ guestfs_int_check_linux_root (guestfs_h *g, struct
inspect_fs *fs)
>    if (check_hostname_unix (g, fs) == -1)
>      return -1;
>  
> +  if (check_systemd_mounts (g, fs) == -1)
> +    return -1;
> +
>    return 0;
>  }
>  
> @@ -981,6 +987,180 @@ check_hostname_freebsd (guestfs_h *g, struct
inspect_fs *fs)
>  }
>  
>  static int
> +check_systemd_mnt (guestfs_h *g, struct inspect_fs *fs, const char *fname)
> +{
> +  CLEANUP_FREE_STRING_LIST char **entries = NULL;
> +  char **entry;
> +  char augpath[256];
> +  CLEANUP_HASH_FREE Hash_table *md_map = NULL;
> +  bool is_bsd = (fs->type == OS_TYPE_FREEBSD ||
> +                 fs->type == OS_TYPE_NETBSD ||
> +                 fs->type == OS_TYPE_OPENBSD);
> +
> +  /* Generate a map of MD device paths listed in /etc/mdadm.conf to MD
device
> +   * paths in the guestfs appliance */
> +  if (map_md_devices (g, &md_map) == -1) {
> +    printf ("map_md_dev fail\n");
This printf shouldn't be here.
> +    return -1;
> +  }
> +
> +  char expr[512];
> +  snprintf (expr, sizeof expr, "/files%s", fname);
Yikes.  Don't use fixed size buffers, especially where (as seems to be
the case here) the path comes from user input.  Use:

  CLEANUP_FREE char *expr = NULL;
  ...
  expr = safe_asprintf (g, "/files%s", fname);
> +  entries = guestfs_aug_match (g, expr);
> +  if (entries == NULL) {
> +    return 0;
> +  }
> +
> +  for (entry = entries; *entry != NULL; entry++) {
> +    CLEANUP_FREE char *spec = NULL;
> +    CLEANUP_FREE char *mp = NULL;
> +    CLEANUP_FREE char *mountable = NULL;
> +    CLEANUP_FREE char *vfstype = NULL;
> +
> +    snprintf (augpath, sizeof augpath, "%s/Mount/What/value",
*entry);
> +    spec = guestfs_aug_get (g, augpath);
> +    if (spec == NULL)
> +      return -1;
> +
> +    /* Ignore /dev/fd (floppy disks) (RHBZ#642929) and CD-ROM drives.
> +     *
> +     * /dev/iso9660/FREEBSD_INSTALL can be found in FreeBSDs installation
> +     * discs.
> +     */
> +    if ((STRPREFIX (spec, "/dev/fd") && c_isdigit
(spec[7])) ||
> +        STREQ (spec, "/dev/floppy") ||
> +        STREQ (spec, "/dev/cdrom") ||
> +        STRPREFIX (spec, "/dev/iso9660/"))
> +      continue;
[etc]

There's a lot of common code with check_fstab here.  It's not
completely identical because the augeas queries are slightly
different, but you can parameterize those easily enough.

A nice thing would be a separate commit which first moves the common
code out from check_fstab into its own function.
> +static int
> +check_systemd_mounts (guestfs_h *g, struct inspect_fs *fs)
> +{
> +  const char *dirs[] = {"/etc/systemd/",
"/lib/systemd/",
> +    "/usr/lib/systemd/", NULL};
> +  const char **dir;
> +  for (dir = dirs; *dir != NULL; dir++) {
> +    CLEANUP_FREE_STRING_LIST char **entries = NULL;
> +    char **entry;
> +
> +    entries = guestfs_find (g, *dir);
Error check?  If the command fails it will immediately segfault in the
next line, which is something that inspection code cannot be doing --
it's a DoS attack route.
> +    size_t cnt = guestfs_int_count_strings (entries);
> +
> +    CLEANUP_FREE_STRING_LIST char **filenames > +      safe_malloc (g,
(cnt + 1) * sizeof (char *));
> +
> +    size_t idx = 0;
> +    for (entry = entries; *entry != NULL; entry++) {
> +      if (STRSUFFIX (*entry, ".mount")) {
> +        size_t entry_len = strlen(*dir) + strlen(*entry) + 1;
> +        filenames[idx] = safe_malloc (g, entry_len);
> +        snprintf (filenames[idx], entry_len, "%s%s", *dir,
*entry);
> +        ++idx;
> +      }
> +    }
> +    filenames[idx] = NULL;
> +
> +    if (inspect_with_augeas2 (g, fs,
> +          (const char**) filenames, check_systemd_mnt) == -1) {
> +      return -1;
> +    }
> +  }
> +
> +  return 0;
> +}
> +
> +static int
>  check_fstab (guestfs_h *g, struct inspect_fs *fs)
>  {
>    CLEANUP_FREE_STRING_LIST char **entries = NULL;
> @@ -1705,6 +1885,66 @@ static char *make_augeas_path_expression (guestfs_h
*g, const char **configfiles
>   * to 'f' the Augeas handle is closed.
>   */
>  static int
> +inspect_with_augeas2 (guestfs_h *g, struct inspect_fs *fs,
> +                     const char **configfiles,
> +                     int (*f) (guestfs_h *, struct inspect_fs *, const
char *))
> +{
As discussed above, I don't believe this function is necessary.

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