forked from pool/virt-v2v
91c2742b7b
CVE-2022-2211-options-fix-buffer-overflow-in-get_keys.patch OBS-URL: https://build.opensuse.org/package/show/Virtualization/virt-v2v?expand=0&rev=27
117 lines
4.1 KiB
Diff
117 lines
4.1 KiB
Diff
Subject: options: fix buffer overflow in get_keys() [CVE-2022-2211]
|
|
From: Laszlo Ersek lersek@redhat.com Tue Jun 28 13:49:04 2022 +0200
|
|
Date: Wed Jun 29 15:17:17 2022 +0200:
|
|
Git: 35467027f657de76aca34b48a6f23e9608b23a57
|
|
|
|
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 from Rich Jones is the following, since it
|
|
doesn't require an encrypted guest disk image:
|
|
|
|
$ 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@redhat.com>
|
|
Message-Id: <20220628114915.5030-2-lersek@redhat.com>
|
|
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
|
|
|
|
--- a/common/options/keys.c
|
|
+++ b/common/options/keys.c
|
|
@@ -128,17 +128,23 @@ read_first_line_from_file (const char *f
|
|
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");
|
|
|