[Libguestfs] [libguestfs-common PATCH 01/12] options: fix buffer overflow in get_keys() [CVE-2022-2211]
Laszlo Ersek
lersek at redhat.com
Wed Jun 29 13:22:07 UTC 2022
On 06/29/22 13:50, Laszlo Ersek wrote:
> On 06/28/22 16:17, Richard W.M. Jones wrote:
>> 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
>
> I'll include this in the commit message.
In order to get the CVE fix upstream reasonably quickly, I've pushed this one (--> commit 35467027f657), with the following commit message updates:
1: 35b49ce142fb ! 1: 35467027f657 options: fix buffer overflow in get_keys() [CVE-2022-2211]
@@ -25,6 +25,14 @@
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
@@ -71,6 +79,8 @@
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>
+ Message-Id: <20220628114915.5030-2-lersek at redhat.com>
+ Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
diff --git a/options/keys.c b/options/keys.c
--- a/options/keys.c
Thanks,
Laszlo
>
>>
>>> (
>>>
>>> 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>
>
> Thanks!
> Laszlo
>
More information about the Libguestfs
mailing list