Libguestfs - Nov 2019 - [PATCH 1/2] options: Fix segfault when multiple --key parameters given.

The first patch fixes a rather serious bug, the second patch allows
multiple --key parameters and default parameters.

There is a third patch to libguestfs which adds a test, coming up.

I did not yet review and fix the documentation.  I think we need to
centralize it in one place because at the moment the same
documentation for --key is copy/pasted all over the tools.

Rich.

Easily reproducible using:

  $ guestfish --key dev1:key:key1 --key dev2:key:key2

causing this stack trace (or others depending on where the memory
corruption was caught):

  Program received signal SIGABRT, Aborted.
  0x00007ffff7905625 in raise () from /lib64/libc.so.6
  (gdb) bt
  #0  0x00007ffff7905625 in raise () from /lib64/libc.so.6
  #1  0x00007ffff78ee8d9 in abort () from /lib64/libc.so.6
  #2  0x00007ffff79494af in __libc_message () from /lib64/libc.so.6
  #3  0x00007ffff7950a6c in malloc_printerr () from /lib64/libc.so.6
  #4  0x00007ffff79528d0 in _int_free () from /lib64/libc.so.6
  #5  0x00005555555bdd6e in free_key_store ()
  #6  0x0000555555589027 in main ()
  (gdb) quit
---
 options/keys.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/options/keys.c b/options/keys.c
index 7f68986..f783066 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -216,7 +216,8 @@ key_store_import_key (struct key_store *ks, const struct
key_store_key *key)
   }
   assert (ks != NULL);
 
-  new_keys = realloc (ks->keys, sizeof (*ks->keys) + 1);
+  new_keys = realloc (ks->keys,
+                      (ks->nr_keys + 1) * sizeof (struct key_store_key));
   if (!new_keys)
     error (EXIT_FAILURE, errno, "realloc");
 
-- 
2.23.0

This allows multiple --key parameters on the command line to match a
single device.  This could either be specified as:

  tool --key /dev/sda1:key:trykey1 --key /dev/sda1:key:trykey2

which would try "trykey1" and "trykey2" against /dev/sda1.

And/or you can specify default keys which are tried against each
device (after more specific keys fail), eg:

  tool --key :key:defaultkey1 --key :key:defaultkey2

which would try "defaultkey1" and "defaultkey2" against all
devices.
---
 options/decrypt.c | 41 ++++++++++++++++++-----
 options/keys.c    | 83 +++++++++++++++++++++++++++++------------------
 options/options.h |  8 +++--
 3 files changed, 89 insertions(+), 43 deletions(-)

diff --git a/options/decrypt.c b/options/decrypt.c
index 234163d..131e79c 100644
--- a/options/decrypt.c
+++ b/options/decrypt.c
@@ -26,6 +26,8 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <libintl.h>
+#include <error.h>
 
 #include "c-ctype.h"
 
@@ -74,21 +76,42 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks)
   if (partitions == NULL)
     exit (EXIT_FAILURE);
 
-  int need_rescan = 0;
-  size_t i;
+  int need_rescan = 0, r;
+  size_t i, j;
+
   for (i = 0; partitions[i] != NULL; ++i) {
     CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]);
     if (type && STREQ (type, "crypto_LUKS")) {
       char mapname[32];
       make_mapname (partitions[i], mapname, sizeof mapname);
 
-      CLEANUP_FREE char *key = get_key (ks, partitions[i]);
-      /* XXX Should we call guestfs_luks_open_ro if readonly flag
-       * is set?  This might break 'mount_ro'.
-       */
-      if (guestfs_luks_open (g, partitions[i], key, mapname) == -1)
-        exit (EXIT_FAILURE);
-
+      CLEANUP_FREE_STRING_LIST char **keys = get_keys (ks, partitions[i]);
+
+      if (guestfs_int_count_strings (keys) == 0)
+        error (EXIT_FAILURE, 0,
+               _("no key was provided to open LUKS encrypted %s, "
+                 "try using --key on the command line"),
+               partitions[i]);
+
+      /* Try each key in turn. */
+      for (j = 0; keys[j] != NULL; ++j) {
+        /* XXX Should we call guestfs_luks_open_ro if readonly flag
+         * is set?  This might break 'mount_ro'.
+         */
+        guestfs_push_error_handler (g, NULL, NULL);
+        r = guestfs_luks_open (g, partitions[i], keys[j], mapname);
+        guestfs_pop_error_handler (g);
+        if (r == 0)
+          goto opened;
+      }
+      error (EXIT_FAILURE, 0,
+             _("could not find key to open LUKS encrypted %s.\n\n"
+               "Try using --key on the command line.\n\n"
+               "Original error: %s (%d)"),
+             partitions[i], guestfs_last_error (g),
+             guestfs_last_errno (g));
+
+    opened:
       need_rescan = 1;
     }
   }
diff --git a/options/keys.c b/options/keys.c
index f783066..817508b 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -121,17 +121,35 @@ read_first_line_from_file (const char *filename)
   return ret;
 }
 
-char *
-get_key (struct key_store *ks, const char *device)
+/* Return the key(s) matching this particular device from the
+ * keystore.  There may be multiple.  If none are read from the
+ * keystore, ask the user.
+ */
+char **
+get_keys (struct key_store *ks, const char *device)
 {
-  size_t i;
+  size_t i, j, len;
+  char **r;
+  char *s;
+
+  /* We know the returned list must have at least one element and not
+   * more than ks->nr_keys.
+   */
+  len = 1;
+  if (ks)
+    len = MIN (1, ks->nr_keys);
+  r = calloc (len+1, sizeof (char *));
+  if (r == NULL)
+    error (EXIT_FAILURE, errno, "calloc");
+
+  j = 0;
 
   if (ks) {
     for (i = 0; i < ks->nr_keys; ++i) {
       struct key_store_key *key = &ks->keys[i];
-      char *s;
 
-      if (STRNEQ (key->device, device))
+      if (STRNEQ (key->device, "") &&
+          STRNEQ (key->device, device))
         continue;
 
       switch (key->type) {
@@ -139,63 +157,64 @@ get_key (struct key_store *ks, const char *device)
         s = strdup (key->string.s);
         if (!s)
           error (EXIT_FAILURE, errno, "strdup");
-        return s;
+        r[j++] = s;
+        break;
       case key_file:
-        return read_first_line_from_file (key->file.name);
+        s = read_first_line_from_file (key->file.name);
+        r[j++] = s;
+        break;
       }
-
-      /* Key not found in the key store, ask the user for it. */
-      break;
     }
   }
 
-  return read_key (device);
+  if (j == 0) {
+    /* Key not found in the key store, ask the user for it. */
+    s = read_key (device);
+    if (!s)
+      error (EXIT_FAILURE, 0, _("could not read key from user"));
+    r[0] = s;
+  }
+
+  return r;
 }
 
 struct key_store *
-key_store_add_from_selector (struct key_store *ks, const char *selector_orig)
+key_store_add_from_selector (struct key_store *ks, const char *selector)
 {
-  CLEANUP_FREE char *selector = strdup (selector_orig);
-  const char *elem;
-  char *saveptr;
+  CLEANUP_FREE_STRING_LIST char **fields +    guestfs_int_split_string
(':', selector);
   struct key_store_key key;
 
-  if (!selector)
-    error (EXIT_FAILURE, errno, "strdup");
+  if (!fields)
+    error (EXIT_FAILURE, errno, "guestfs_int_split_string");
 
-  /* 1: device */
-  elem = strtok_r (selector, ":", &saveptr);
-  if (!elem) {
+  if (guestfs_int_count_strings (fields) != 3) {
    invalid_selector:
-    error (EXIT_FAILURE, 0, "invalid selector for --key: %s",
selector_orig);
+    error (EXIT_FAILURE, 0, "invalid selector for --key: %s",
selector);
   }
-  key.device = strdup (elem);
+
+  /* 1: device */
+  key.device = strdup (fields[0]);
   if (!key.device)
     error (EXIT_FAILURE, errno, "strdup");
 
   /* 2: key type */
-  elem = strtok_r (NULL, ":", &saveptr);
-  if (!elem)
-    goto invalid_selector;
-  else if (STREQ (elem, "key"))
+  if (STREQ (fields[1], "key"))
     key.type = key_string;
-  else if (STREQ (elem, "file"))
+  else if (STREQ (fields[1], "file"))
     key.type = key_file;
   else
     goto invalid_selector;
 
   /* 3: actual key */
-  elem = strtok_r (NULL, ":", &saveptr);
-  if (!elem)
-    goto invalid_selector;
   switch (key.type) {
   case key_string:
-    key.string.s = strdup (elem);
+    key.string.s = strdup (fields[2]);
     if (!key.string.s)
       error (EXIT_FAILURE, errno, "strdup");
     break;
   case key_file:
-    key.file.name = strdup (elem);
+    key.file.name = strdup (fields[2]);
     if (!key.file.name)
       error (EXIT_FAILURE, errno, "strdup");
     break;
diff --git a/options/options.h b/options/options.h
index 6fadf1e..6e9b1da 100644
--- a/options/options.h
+++ b/options/options.h
@@ -104,7 +104,11 @@ struct mp {
 
 /* A key in the key store. */
 struct key_store_key {
-  /* The device this key refers to. */
+  /* The device this key refers to.  This is never NULL, but may be
""
+   * which is interpreted as a default key which is tried after any
+   * device-specific keys (there may be multiple defaults in the
+   * list).
+   */
   char *device;
 
   enum {
@@ -146,7 +150,7 @@ extern void print_inspect_prompt (void);
 
 /* in key.c */
 extern char *read_key (const char *param);
-extern char *get_key (struct key_store *ks, const char *device);
+extern char **get_keys (struct key_store *ks, const char *device);
 extern struct key_store *key_store_add_from_selector (struct key_store *ks,
const char *selector);
 extern struct key_store *key_store_import_key (struct key_store *ks, const
struct key_store_key *key);
 extern void free_key_store (struct key_store *ks);
-- 
2.23.0

On Tuesday, 12 November 2019 19:35:11 CET Richard W.M. Jones
wrote:
> Easily reproducible using:
> 
>   $ guestfish --key dev1:key:key1 --key dev2:key:key2
> 
> causing this stack trace (or others depending on where the memory
> corruption was caught):
> 
>   Program received signal SIGABRT, Aborted.
>   0x00007ffff7905625 in raise () from /lib64/libc.so.6
>   (gdb) bt
>   #0  0x00007ffff7905625 in raise () from /lib64/libc.so.6
>   #1  0x00007ffff78ee8d9 in abort () from /lib64/libc.so.6
>   #2  0x00007ffff79494af in __libc_message () from /lib64/libc.so.6
>   #3  0x00007ffff7950a6c in malloc_printerr () from /lib64/libc.so.6
>   #4  0x00007ffff79528d0 in _int_free () from /lib64/libc.so.6
>   #5  0x00005555555bdd6e in free_key_store ()
>   #6  0x0000555555589027 in main ()
>   (gdb) quit
> ---
>  options/keys.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/options/keys.c b/options/keys.c
> index 7f68986..f783066 100644
> --- a/options/keys.c
> +++ b/options/keys.c
> @@ -216,7 +216,8 @@ key_store_import_key (struct key_store *ks, const
struct key_store_key *key)
>    }
>    assert (ks != NULL);
>  
> -  new_keys = realloc (ks->keys, sizeof (*ks->keys) + 1);
> +  new_keys = realloc (ks->keys,
> +                      (ks->nr_keys + 1) * sizeof (struct
key_store_key));
Theoretically, sizeof (*ks->keys) should still be fine, instead of
explicitly spelling the struct name.

Apart from that, LGTM.

-- 
Pino Toscano

On Tuesday, 12 November 2019 19:35:12 CET Richard W.M. Jones
wrote:
> This allows multiple --key parameters on the command line to match a
> single device.  This could either be specified as:
> 
>   tool --key /dev/sda1:key:trykey1 --key /dev/sda1:key:trykey2
> 
> which would try "trykey1" and "trykey2" against
/dev/sda1.
This seems OK for me, so you can attempt multiple keys for a device.
> And/or you can specify default keys which are tried against each
> device (after more specific keys fail), eg:
> 
>   tool --key :key:defaultkey1 --key :key:defaultkey2
> 
> which would try "defaultkey1" and "defaultkey2" against
all devices.
However I do not see the point in this: IMHO you better make it explicit
which key is used for a certain device. Also, this makes it possible so
in case of two similar guests like:
- /dev/sda1 with key "key1", and /dev/sda2 with key "key2"
- /dev/sda1 with key "key2", and /dev/sda2 with key "key1"
the above command like will work in the same way, with no indication of
which key was successfully used for which device -- so you can silently
swap the first guest for the second with no changes to the v2v command
line...
What's the use case for this?

Please split this patch in two:
- multiple keys for the same device
- keys for all devices

Thanks,
-- 
Pino Toscano