6:49 a.m.
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(a)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