Libguestfs - Feb 2014 - Re: [PATCH] list-filesystems: Do not segfault if guestfs_btrfs_subvolume_list returns an error (RHBZ#1064008).

If calling guestfs_list_filesystems with a disk image containing a
corrupt btrfs volume, the library would segfault.  There was a missing
check for a NULL return from guestfs_btrfs_subvolume_list.

This adds a check, returning the real error up through the stack and
out of guestfs_list_filesystems.

This is potentially a denial of service if processing disk images from
untrusted sources, but is not exploitable.

Thanks: Jeff Bastian for reporting the bug.
---
 src/listfs.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/src/listfs.c b/src/listfs.c
index 9102c55..bbdb0a2 100644
--- a/src/listfs.c
+++ b/src/listfs.c
@@ -39,7 +39,7 @@
  */
 
 static void remove_from_list (char **list, const char *item);
-static void check_with_vfs_type (guestfs_h *g, const char *dev, struct
stringsbuf *sb);
+static int check_with_vfs_type (guestfs_h *g, const char *dev, struct
stringsbuf *sb);
 static int is_mbr_partition_type_42 (guestfs_h *g, const char *partition);
 
 char **
@@ -78,17 +78,21 @@ guestfs__list_filesystems (guestfs_h *g)
 
   /* Use vfs-type to check for filesystems on devices. */
   for (i = 0; devices[i] != NULL; ++i)
-    check_with_vfs_type (g, devices[i], &ret);
+    if (check_with_vfs_type (g, devices[i], &ret) == -1)
+      goto error;
 
   /* Use vfs-type to check for filesystems on partitions. */
   for (i = 0; partitions[i] != NULL; ++i) {
-    if (! is_mbr_partition_type_42 (g, partitions[i]))
-      check_with_vfs_type (g, partitions[i], &ret);
+    if (! is_mbr_partition_type_42 (g, partitions[i])) {
+      if (check_with_vfs_type (g, partitions[i], &ret) == -1)
+        goto error;
+    }
   }
 
   /* Use vfs-type to check for filesystems on md devices. */
   for (i = 0; mds[i] != NULL; ++i)
-    check_with_vfs_type (g, mds[i], &ret);
+    if (check_with_vfs_type (g, mds[i], &ret) == -1)
+      goto error;
 
   if (guestfs_feature_available (g, (char **) lvm2)) {
     /* Use vfs-type to check for filesystems on LVs. */
@@ -96,7 +100,8 @@ guestfs__list_filesystems (guestfs_h *g)
     if (lvs == NULL) goto error;
 
     for (i = 0; lvs[i] != NULL; ++i)
-      check_with_vfs_type (g, lvs[i], &ret);
+      if (check_with_vfs_type (g, lvs[i], &ret) == -1)
+        goto error;
   }
 
   if (guestfs_feature_available (g, (char **) ldm)) {
@@ -105,13 +110,15 @@ guestfs__list_filesystems (guestfs_h *g)
     if (ldmvols == NULL) goto error;
 
     for (i = 0; ldmvols[i] != NULL; ++i)
-      check_with_vfs_type (g, ldmvols[i], &ret);
+      if (check_with_vfs_type (g, ldmvols[i], &ret) == -1)
+        goto error;
 
     ldmparts = guestfs_list_ldm_partitions (g);
     if (ldmparts == NULL) goto error;
 
     for (i = 0; ldmparts[i] != NULL; ++i)
-      check_with_vfs_type (g, ldmparts[i], &ret);
+      if (check_with_vfs_type (g, ldmparts[i], &ret) == -1)
+        goto error;
   }
 
   /* Finish off the list and return it. */
@@ -143,7 +150,7 @@ remove_from_list (char **list, const char *item)
  * Apart from some types which we ignore, add the result to the
  * 'ret' string list.
  */
-static void
+static int
 check_with_vfs_type (guestfs_h *g, const char *device, struct stringsbuf *sb)
 {
   const char *v;
@@ -161,6 +168,9 @@ check_with_vfs_type (guestfs_h *g, const char *device,
struct stringsbuf *sb)
     CLEANUP_FREE_BTRFSSUBVOLUME_LIST struct guestfs_btrfssubvolume_list *vols  
guestfs_btrfs_subvolume_list (g, device);
 
+    if (vols == NULL)
+      return -1;
+
     for (size_t i = 0; i < vols->len; i++) {
       struct guestfs_btrfssubvolume *this = &vols->val[i];
       guestfs___add_sprintf (g, sb,
@@ -178,17 +188,19 @@ check_with_vfs_type (guestfs_h *g, const char *device,
struct stringsbuf *sb)
      */
     size_t n = strlen (vfs_type);
     if (n >= 7 && STREQ (&vfs_type[n-7], "_member"))
-      return;
+      return 0;
 
     /* Ignore LUKS-encrypted partitions.  These are also containers. */
     if (STREQ (vfs_type, "crypto_LUKS"))
-      return;
+      return 0;
 
     v = vfs_type;
   }
 
   guestfs___add_string (g, sb, device);
   guestfs___add_string (g, sb, v);
+
+  return 0;
 }
 
 /* We should ignore partitions that have MBR type byte 0x42, because
-- 
1.8.4.2

On Tuesday 11 February 2014 20:16:56 Richard W.M. Jones
wrote:
> If calling guestfs_list_filesystems with a disk image containing a
> corrupt btrfs volume, the library would segfault.  There was a missing
> check for a NULL return from guestfs_btrfs_subvolume_list.
> 
> This adds a check, returning the real error up through the stack and
> out of guestfs_list_filesystems.
> 
> This is potentially a denial of service if processing disk images from
> untrusted sources, but is not exploitable.
> 
> Thanks: Jeff Bastian for reporting the bug.
> ---
>  src/listfs.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/src/listfs.c b/src/listfs.c
> index 9102c55..bbdb0a2 100644
> --- a/src/listfs.c
> +++ b/src/listfs.c
> @@ -39,7 +39,7 @@
>   */
> 
>  static void remove_from_list (char **list, const char *item);
> -static void check_with_vfs_type (guestfs_h *g, const char *dev,
> struct stringsbuf *sb); +static int check_with_vfs_type (guestfs_h
> *g, const char *dev, struct stringsbuf *sb); static int
> is_mbr_partition_type_42 (guestfs_h *g, const char *partition);
> 
>  char **
> @@ -78,17 +78,21 @@ guestfs__list_filesystems (guestfs_h *g)
> 
>    /* Use vfs-type to check for filesystems on devices. */
>    for (i = 0; devices[i] != NULL; ++i)
> -    check_with_vfs_type (g, devices[i], &ret);
> +    if (check_with_vfs_type (g, devices[i], &ret) == -1)
> +      goto error;
> 
>    /* Use vfs-type to check for filesystems on partitions. */
>    for (i = 0; partitions[i] != NULL; ++i) {
> -    if (! is_mbr_partition_type_42 (g, partitions[i]))
> -      check_with_vfs_type (g, partitions[i], &ret);
> +    if (! is_mbr_partition_type_42 (g, partitions[i])) {
> +      if (check_with_vfs_type (g, partitions[i], &ret) == -1)
> +        goto error;
> +    }
>    }
> 
>    /* Use vfs-type to check for filesystems on md devices. */
>    for (i = 0; mds[i] != NULL; ++i)
> -    check_with_vfs_type (g, mds[i], &ret);
> +    if (check_with_vfs_type (g, mds[i], &ret) == -1)
> +      goto error;
> 
>    if (guestfs_feature_available (g, (char **) lvm2)) {
>      /* Use vfs-type to check for filesystems on LVs. */
> @@ -96,7 +100,8 @@ guestfs__list_filesystems (guestfs_h *g)
>      if (lvs == NULL) goto error;
> 
>      for (i = 0; lvs[i] != NULL; ++i)
> -      check_with_vfs_type (g, lvs[i], &ret);
> +      if (check_with_vfs_type (g, lvs[i], &ret) == -1)
> +        goto error;
>    }
> 
>    if (guestfs_feature_available (g, (char **) ldm)) {
> @@ -105,13 +110,15 @@ guestfs__list_filesystems (guestfs_h *g)
>      if (ldmvols == NULL) goto error;
> 
>      for (i = 0; ldmvols[i] != NULL; ++i)
> -      check_with_vfs_type (g, ldmvols[i], &ret);
> +      if (check_with_vfs_type (g, ldmvols[i], &ret) == -1)
> +        goto error;
> 
>      ldmparts = guestfs_list_ldm_partitions (g);
>      if (ldmparts == NULL) goto error;
> 
>      for (i = 0; ldmparts[i] != NULL; ++i)
> -      check_with_vfs_type (g, ldmparts[i], &ret);
> +      if (check_with_vfs_type (g, ldmparts[i], &ret) == -1)
> +        goto error;
>    }
> 
>    /* Finish off the list and return it. */
> @@ -143,7 +150,7 @@ remove_from_list (char **list, const char *item)
>   * Apart from some types which we ignore, add the result to the
>   * 'ret' string list.
>   */
> -static void
> +static int
>  check_with_vfs_type (guestfs_h *g, const char *device, struct
> stringsbuf *sb) {
>    const char *v;
> @@ -161,6 +168,9 @@ check_with_vfs_type (guestfs_h *g, const char
> *device, struct stringsbuf *sb) CLEANUP_FREE_BTRFSSUBVOLUME_LIST
> struct guestfs_btrfssubvolume_list *vols > guestfs_btrfs_subvolume_list
(g, device);
> 
> +    if (vols == NULL)
> +      return -1;
> +
>      for (size_t i = 0; i < vols->len; i++) {
>        struct guestfs_btrfssubvolume *this = &vols->val[i];
>        guestfs___add_sprintf (g, sb,
> @@ -178,17 +188,19 @@ check_with_vfs_type (guestfs_h *g, const char
> *device, struct stringsbuf *sb) */
>      size_t n = strlen (vfs_type);
>      if (n >= 7 && STREQ (&vfs_type[n-7],
"_member"))
> -      return;
> +      return 0;
> 
>      /* Ignore LUKS-encrypted partitions.  These are also containers.
> */ if (STREQ (vfs_type, "crypto_LUKS"))
> -      return;
> +      return 0;
> 
>      v = vfs_type;
>    }
> 
>    guestfs___add_string (g, sb, device);
>    guestfs___add_string (g, sb, v);
> +
> +  return 0;
>  }
> 
>  /* We should ignore partitions that have MBR type byte 0x42, because
Looks good to me.

-- 
Pino Toscano

On Wed, Feb 12, 2014 at 11:38:22AM +0100, Pino Toscano
wrote:
> Looks good to me.
Thanks for reviewing this.

I'm going to push this to libguestfs 1.22 & 1.24 and do new releases
of those branches as well .. coming up later today.

Rich.

-- 
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://people.redhat.com/~rjones/virt-top