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

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(a)redhat.com&gt; --- 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