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

[Laszlo Ersek]

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