[Libguestfs] [libguestfs-common PATCH 02/12] options: fix UUID comparison logic bug in get_keys()

[Richard W.M. Jones]

On Tue, Jun 28, 2022 at 01:49:05PM +0200, Laszlo Ersek wrote:
> When we iterate over the keys in the keystore, we intend to skip a key
> (i.e., we do not append it to the result array) if:
> 
>   (key_id differs from device name) && (key_id differs from device UUID)
> 
> The problem is how we currently express the second condition:
> 
>   uuid && STRNEQ (key->id, uuid)
> 
> This is wrong: when the LUKS UUID is missing (e.g. because
> guestfs_luks_uuid() failed or we're looking up the keys for a BitLocker
> device), this expression evaluates to 0. Which states that the key_id does
> *not* differ from the device UUID -- in other words, that we have a match.
> 
> Invert the result for when "uuid" is NULL:
> 
>   !uuid || STRNEQ (key->id, uuid)
> 
> (
> 
> - Equivalently: what we currently have for "differs" is:
> 
>     uuid ? STRNEQ (key->id, uuid) : false
> 
>   but we need:
> 
>     uuid ? STRNEQ (key->id, uuid) : true
> 
> - Yet another way to express
> 
>     key_id differs from device UUID
> 
>   correctly is with the "implication operator":
> 
>     uuid → STRNEQ (key->id, uuid)
> 
>   The crucial feature is that, from a false premise, everything follows;
>   therefore, if the UUID is missing, we get "true" (which we want for
>   "differs"). In C, we don't have an "implication operator", but
> 
>     A → B
> 
>   is equivalent to
> 
>     ¬A ∨ B
> 
>   (compare the truth tables!), which we *can* express in C, as
> 
>     !A || B
> 
> )
> 
> In practice the bug has been masked for the following reasons:
> 
> - It's pretty rare that "uuid" is NULL.
> 
> - Even when "uuid" is NULL, and therefore we incorrectly include a key in
>   the result list, the consequence is only bad performance. Namely, we
>   attempt unlocking a LUKS or BitLocker volume with a key that the user
>   never intended for that -- so the attempt will fail, and
>   decrypt_mountables() [options/decrypt.c] will just advance to the next
>   key in the result list. Put differently, we cannot "mistakenly" unlock
>   an encrypted device.
> 
> However, because unlocking disk encryption requires plenty of computation
> by design (in order to resist brute forcing), this waste may not be
> trivial.
> 
> Fixes: bb4a2dc17a78b53437896d4215ae82df8e11b788
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  options/keys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/options/keys.c b/options/keys.c
> index d27a7123e67e..8713372a305e 100644
> --- a/options/keys.c
> +++ b/options/keys.c
> @@ -152,11 +152,11 @@ get_keys (struct key_store *ks, const char *device, const char *uuid)
>  
>    if (ks) {
>      for (i = 0; i < ks->nr_keys; ++i) {
>        struct key_store_key *key = &ks->keys[i];
>  
> -      if (STRNEQ (key->id, device) && (uuid && STRNEQ (key->id, uuid)))
> +      if (STRNEQ (key->id, device) && (!uuid || STRNEQ (key->id, uuid)))
>          continue;
>  
>        switch (key->type) {
>        case key_string:
>          s = strdup (key->string.s);
> -- 
> 2.19.1.3.g30247aa5d201

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
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html