Richard W.M. Jones
2022-Jun-28 14:17 UTC
[Libguestfs] [libguestfs-common PATCH 01/12] options: fix buffer overflow in get_keys() [CVE-2022-2211]
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
Laszlo Ersek
2022-Jun-29 11:50 UTC
[Libguestfs] [libguestfs-common PATCH 01/12] options: fix buffer overflow in get_keys() [CVE-2022-2211]
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.imgI'll include this in the commit message.> >> ( >> >> 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