[Libguestfs] [libguestfs-common PATCH 01/12] options: fix buffer overflow in get_keys() [CVE-2022-2211]

[Richard W.M. Jones]

On Tue, Jun 28, 2022 at 01:49:04PM +0200, Laszlo Ersek wrote:
> 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.

A slightly better reproducer is this, since it doesn't require you to
have an encrypted guest around:

$ echo TEST | guestfish --keys-from-stdin -N part luks-format /dev/sda1 0
$ virt-cat $(seq -f '--key /dev/sda1:key:%g' 200) -a test1.img /no-such-file
Segmentation fault (core dumped)
$ rm test1.img

> (
> 
>   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;

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW