Libguestfs - Jun 2022 - [guestfs-tools PATCH 0/4] LUKS decryption with Clevis+Tang

Hi,

* in response to this cover letter, I'm going to post four series (one
for each of libguestfs-common, libguestfs, guestfs-tools, virt-v2v).
These four series implement LUKS decryption with Clevis+Tang:

https://bugzilla.redhat.com/show_bug.cgi?id=1809453

* The first patch in the libguestfs-common series fixes a bug that I'd
found while working on the feature, and ended up receiving a CVE number
(CVE-2022-2211):

https://bugzilla.redhat.com/show_bug.cgi?id=2100862

This patch is an integral part of the larger Clevis+Tang feature.
However, it can be backported easily to stable branches that only want
the bugfix.

* Correspondingly, the first patch in the libguestfs series documents
the new CVE (and updates the common submodule just enough to get the CVE
fix). This patch should also be easy to backport to stable branches.

A later patch in the libguestfs series updates the "common" submodule
checkout to the end of the libguestfs-common series.

* In each of the guestfs-tools and virt-v2v series, the full "common"
submodule series is consumed right in the first patch, covering both the
CVE fix and the new stuff needed for the Clevis feature.

Thanks,
Laszlo

[Adding packagers to CC for visibility.]

On Tue, Jun 28, 2022 at 11:00:43AM +0200, Laszlo Ersek
wrote:
> Hi,
> 
> * in response to this cover letter, I'm going to post four series (one
> for each of libguestfs-common, libguestfs, guestfs-tools, virt-v2v).
> These four series implement LUKS decryption with Clevis+Tang:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1809453
> 
> * The first patch in the libguestfs-common series fixes a bug that I'd
> found while working on the feature, and ended up receiving a CVE number
> (CVE-2022-2211):
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=2100862
> 
> This patch is an integral part of the larger Clevis+Tang feature.
> However, it can be backported easily to stable branches that only want
> the bugfix.
> 
> * Correspondingly, the first patch in the libguestfs series documents
> the new CVE (and updates the common submodule just enough to get the CVE
> fix). This patch should also be easy to backport to stable branches.
> 
> A later patch in the libguestfs series updates the "common"
submodule
> checkout to the end of the libguestfs-common series.
> 
> * In each of the guestfs-tools and virt-v2v series, the full
"common"
> submodule series is consumed right in the first patch, covering both the
> CVE fix and the new stuff needed for the Clevis feature.
> 
> Thanks,
> Laszlo
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2100862

Please refer to the parent cover letter
<https://listman.redhat.com/archives/libguestfs/2022-June/029274.html>
regarding the relationship between the CVE fix and the larger series.

The first four patches are bugfixes (of varying importance).

The rest are refactorings and feature-let additions, intermixed as
needed.

Thanks,
Laszlo

Laszlo Ersek (12):
  options: fix buffer overflow in get_keys() [CVE-2022-2211]
  options: fix UUID comparison logic bug in get_keys()
  mltools/tools_utils: remove unused function "key_store_to_cli"
  mltools/tools_utils: allow multiple "--key" options for OCaml tools
    too
  options: replace NULL-termination with number-of-elements in
    get_keys()
  options: wrap each passphrase from get_keys() into a struct
  options: add back-end for LUKS decryption with Clevis+Tang
  options: introduce selector tpe "key_clevis"
  options: generalize "--key" selector parsing for C-language
utilities
  mltools/tools_utils: generalize "--key" selector parsing for OCaml
    utils
  options, mltools/tools_utils: parse "--key ID:clevis" options
  options, mltools/tools_utils: add helper for network dependency

 mltools/tools_utils-c.c |  47 ++++---
 mltools/tools_utils.ml  |  51 ++++----
 mltools/tools_utils.mli |  12 +-
 options/decrypt.c       |  24 ++--
 options/key-option.pod  |   9 ++
 options/keys.c          | 130 ++++++++++++++------
 options/options.h       |  19 ++-
 7 files changed, 195 insertions(+), 97 deletions(-)

-- 
2.19.1.3.g30247aa5d201

When calculating the greatest possible number of matching keys in
get_keys(), the current expression

  MIN (1, ks->nr_keys)

is wrong -- it will return at most 1.

If all "nr_keys" keys match however, then we require
"nr_keys" non-NULL
entries in the result array; in other words, we need

  MAX (1, ks->nr_keys)

(The comment just above the expression is correct; the code is wrong.)

This buffer overflow is easiest to trigger in those guestfs tools that
parse the "--key" option in C; that is, with "OPTION_key".
For example,
the command

$ virt-cat $(seq -f '--key /dev/sda2:key:%g' 200) -d DOMAIN
/no-such-file

which passes 200 (different) passphrases for the LUKS-encrypted block
device "/dev/sda2", crashes with a SIGSEGV.

(

  The buffer overflow is possible to trigger in OCaml-language tools as
  well; that is, those that call "create_standard_options" with
  ~key_opts:true.

  Triggering the problem that way is less trivial. The reason is that when
  the OCaml tools parse the "--key" options, they de-duplicate the
options
  first, based on the device identifier.

  Thus, in theory, this de-duplication masks the issue, as (one would
  think) only one "--key" option could belong to a single device, and
  therefore the buffer overflow would not be triggered in practice.

  This is not the case however: the de-duplication does not collapse keys
  that are provided for the same device, but use different identifier
  types (such as pathname of device node versus LUKS UUID) -- in that
  situation, two entries in the keystore will match the device, and the
  terminating NULL entry will not be present once get_keys() returns. In
  this scenario, we don't have an out-of-bounds write, but an
  out-of-bounds read, in decrypt_mountables() [options/decrypt.c].

  There is *yet another* bug in get_keys() though that undoes the above
  "masking". The "uuid" parameter of get_keys() may be NULL
(for example
  when the device to decrypt uses BitLocker and not LUKS). When this
  happens, get_keys() adds all keys in the keystore to the result array.
  Therefore, the out-of-bounds write is easy to trigger with
  OCaml-language tools as well, as long as we attempt to decrypt a
  BitLocker (not LUKS) device, and we pass the "--key" options with
  different device identifiers.

  Subsequent patches in this series fix all of the above; this patch fixes
  the security bug.

)

Rather than replacing MIN with MAX, open-code the comparison, as we first
set "len" to 1 anyway.

While at it, rework the NULL-termination such that the (len+1) addition
not go unchecked.

Fixes: c10c8baedb88e7c2988a01b70fc5f81fa8e4885c
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2100862
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 options/keys.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/options/keys.c b/options/keys.c
index 798315c2e95a..d27a7123e67e 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -126,21 +126,27 @@ read_first_line_from_file (const char *filename)
  * keystore, ask the user.
  */
 char **
 get_keys (struct key_store *ks, const char *device, const char *uuid)
 {
-  size_t i, j, len;
+  size_t i, j, nmemb;
   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 *));
+  nmemb = 1;
+  if (ks && ks->nr_keys > nmemb)
+    nmemb = ks->nr_keys;
+
+  /* make room for the terminating NULL */
+  if (nmemb == (size_t)-1)
+    error (EXIT_FAILURE, 0, _("size_t overflow"));
+  nmemb++;
+
+  r = calloc (nmemb, sizeof (char *));
   if (r == NULL)
     error (EXIT_FAILURE, errno, "calloc");
 
   j = 0;
 
-- 
2.19.1.3.g30247aa5d201

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2100862

Please refer to the parent cover letter
<https://listman.redhat.com/archives/libguestfs/2022-June/029274.html>
regarding the relationship between the CVE fix and the larger series.

Thanks,
Laszlo

Laszlo Ersek (3):
  docs/guestfs-security: document CVE-2022-2211
  introduce the "clevis_luks_unlock" API
  guestfish, guestmount: enable networking for "--key ID:clevis"

 appliance/packagelist.in  |  4 ++
 common                    |  2 +-
 daemon/Makefile.am        |  1 +
 daemon/clevis-luks.c      | 58 ++++++++++++++++++++
 docs/guestfs-security.pod | 28 ++++++++++
 fish/fish.c               |  3 +
 fuse/guestmount.c         |  4 ++
 generator/actions_core.ml | 38 +++++++++++++
 generator/proc_nr.ml      |  1 +
 lib/MAX_PROC_NR           |  2 +-
 lib/guestfs.pod           | 19 +++++--
 11 files changed, 154 insertions(+), 6 deletions(-)
 create mode 100644 daemon/clevis-luks.c

-- 
2.19.1.3.g30247aa5d201

Short log for the common submodule, commit range
f8de5508fe75..35b49ce142fb:

Laszlo Ersek (2):
      mlcustomize: factor out pkg install/update/uninstall from guestfs-tools
      options: fix buffer overflow in get_keys() [CVE-2022-2211]

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2100862
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    "Commit 35b49ce142fb" needs to updated here, in both the submodule
    checkout and the commit message, once the libguestfs-common series is
    merged.

 common                    |  2 +-
 docs/guestfs-security.pod | 28 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/common b/common
index f8de5508fe75..35b49ce142fb 160000
--- a/common
+++ b/common
@@ -1 +1 @@
-Subproject commit f8de5508fe755acca99c9ab40c501d6c3e2bcb1e
+Subproject commit 35b49ce142fb906bcf91edc6a6718d28c8fca9e3
diff --git a/docs/guestfs-security.pod b/docs/guestfs-security.pod
index 9ceef5623370..efa35b29d301 100644
--- a/docs/guestfs-security.pod
+++ b/docs/guestfs-security.pod
@@ -404,10 +404,38 @@ S<C<guestfs_set_network (g, 1)>> (which is not
the default).
 The libvirt backend is not affected.
 
 The solution is to update qemu to a version containing the fix (see
 L<https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01012.html>).
 
+=head2 CVE-2022-2211
+
+L<https://bugzilla.redhat.com/CVE-2022-2211>
+
+The C<get_keys> function in F<libguestfs-common/options/keys.c>
collects
+those I<--key> options from the command line into a new array that match
+a particular block device that's being decrypted for inspection. The
+function intends to size the result array such that potentially all
+I<--key> options, plus a terminating C<NULL> element, fit into it.
The
+code mistakenly uses the C<MIN> macro instead of C<MAX>, and
therefore
+only one element is allocated before the C<NULL> terminator.
+
+Passing precisely two I<--key ID:...> options on the command line for
+the encrypted block device C<ID> causes C<get_keys> to overwrite
the
+terminating C<NULL>, leading to an out-of-bounds read in
+C<decrypt_mountables>, file F<libguestfs-common/options/decrypt.c>.
+
+Passing more than two I<--key ID:...> options on the command line for
+the encrypted block device C<ID> causes C<get_keys> itself to
perform
+out-of-bounds writes. The most common symptom is a crash with C<SIGSEGV>
+later on.
+
+This issue affects -- broadly speaking -- all libguestfs-based utilities
+that accept I<--key>, namely: C<guestfish>, C<guestmount>,
C<virt-cat>,
+C<virt-customize>, C<virt-diff>, C<virt-edit>,
C<virt-get-kernel>,
+C<virt-inspector>, C<virt-log>, C<virt-ls>,
C<virt-sparsify>,
+C<virt-sysprep>, C<virt-tail>, C<virt-v2v>.
+
 =head1 SEE ALSO
 
 L<guestfs(3)>,
 L<guestfs-internals(1)>,
 L<guestfs-release-notes(1)>,
-- 
2.19.1.3.g30247aa5d201

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453

Laszlo Ersek (4):
  cat, log, ls, tail, diff, edit, insp.: set networking for "--key
    ID:clevis"
  get-kernel, sparsify: set networking for "--key ID:clevis"
  customize: add reminder about "--key ID:clevis"
  sysprep: set networking for "--key ID:clevis"

 cat/cat.c                   | 3 +++
 cat/log.c                   | 3 +++
 cat/ls.c                    | 3 +++
 cat/tail.c                  | 3 +++
 common                      | 2 +-
 customize/customize_main.ml | 7 +++++++
 diff/diff.c                 | 8 ++++++++
 edit/edit.c                 | 3 +++
 get-kernel/get_kernel.ml    | 1 +
 inspector/inspector.c       | 3 +++
 sparsify/copying.ml         | 1 +
 sparsify/in_place.ml        | 1 +
 sysprep/main.ml             | 7 +++++--
 13 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.19.1.3.g30247aa5d201

Virt-v2v enables appliance networking already, for the sake of
"unconfigure_vmware". We now have a second use case for networking:
"--key
ID:clevis". Update the comment in the code.

(Short log for libguestfs-common commit range 9e990f3e4530..0399dea30e63:

Laszlo Ersek (12):
      options: fix buffer overflow in get_keys() [CVE-2022-2211]
      options: fix UUID comparison logic bug in get_keys()
      mltools/tools_utils: remove unused function "key_store_to_cli"
      mltools/tools_utils: allow multiple "--key" options for OCaml
tools too
      options: replace NULL-termination with number-of-elements in get_keys()
      options: wrap each passphrase from get_keys() into a struct
      options: add back-end for LUKS decryption with Clevis+Tang
      options: introduce selector tpe "key_clevis"
      options: generalize "--key" selector parsing for C-language
utilities
      mltools/tools_utils: generalize "--key" selector parsing for
OCaml utils
      options, mltools/tools_utils: parse "--key ID:clevis" options
      options, mltools/tools_utils: add helper for network dependency
).

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    The submodule commit range 9e990f3e4530..0399dea30e63 needs to be
    refreshed in both the commit message and the "common" hunk, once
the
    libguestfs-common series is upstream.

 convert/convert.ml | 3 ++-
 common             | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/convert/convert.ml b/convert/convert.ml
index 87fca7252ba3..17a75e3fad32 100644
--- a/convert/convert.ml
+++ b/convert/convert.ml
@@ -54,11 +54,12 @@ let rec convert dir options source    g#set_memsize
(g#get_memsize () * 2);
   (* Setting the number of vCPUs allows parallel mkinitrd, but make
    * sure this is not too large because each vCPU consumes guest RAM.
    *)
   g#set_smp (min 8 (Sysconf.nr_processors_online ()));
-  (* The network is only used by the unconfigure_vmware () function. *)
+  (* The network is used by the unconfigure_vmware () function, and the
"--key
+   * ID:clevis" command line options (if any). *)
   g#set_network true;
   List.iter (
     fun { s_disk_id = i } ->
       (* NB: Old virt-v2v used copyonread here, when it was using a
        * qcow2 file as overlay.  We MUST NOT use copyonread!  It
diff --git a/common b/common
index 9e990f3e4530..0399dea30e63 160000
--- a/common
+++ b/common
@@ -1 +1 @@
-Subproject commit 9e990f3e4530df3708d176bc50e0bc68cf07d3ff
+Subproject commit 0399dea30e6353870183a07a82d05b4ec8f20ca0
-- 
2.19.1.3.g30247aa5d201