Bug in get_best_pubkey_byname
Open, HighPublic

Description

When there are multiple active keys, get_best_pubkey_byname may not work correctly as expected.
I have two active keys on tokens, and I recently begin to use new machine.
I import my new key at first, and then, import old key. I configure trust settings.

In this situation, old key is selected, but apparently get_best_pubkey_byname should return new key.

gniibe created this task.Sep 29 2019, 10:44 AM
gniibe added a comment.Oct 4 2019, 8:56 AM
This comment was removed by gniibe.
gniibe added a comment.Oct 4 2019, 8:58 AM
diff --git a/g10/getkey.c b/g10/getkey.c
index de5024198..051b21203 100644
--- a/g10/getkey.c
+++ b/g10/getkey.c
@@ -1272,6 +1272,48 @@ only_expired_enc_subkeys (kbnode_t keyblock)
   return any? 1 : 0;
 }
 
+static int
+fill_pubkey_cmp_cookie (ctrl_t ctrl, struct pubkey_cmp_cookie *new,
+                        const char *name, KBNODE keyblock)
+{
+  kbnode_t n;
+
+  new->creation_time = 0;
+  for (n = find_next_kbnode (keyblock, PKT_PUBLIC_SUBKEY);
+       n; n = find_next_kbnode (n, PKT_PUBLIC_SUBKEY))
+    {
+      PKT_public_key *sub = n->pkt->pkt.public_key;
+
+      if ((sub->pubkey_usage & PUBKEY_USAGE_ENC) == 0)
+        continue;
+
+      if (! subkey_is_ok (sub))
+        continue;
+
+      if (sub->timestamp > new->creation_time)
+        new->creation_time = sub->timestamp;
+    }
+
+  for (n = find_next_kbnode (keyblock, PKT_USER_ID);
+       n; n = find_next_kbnode (n, PKT_USER_ID))
+    {
+      PKT_user_id *uid = n->pkt->pkt.user_id;
+      char *mbox = mailbox_from_userid (uid->name, 0);
+      int match = mbox ? strcasecmp (name, mbox) == 0 : 0;
+
+      xfree (mbox);
+      if (! match)
+        continue;
+
+      new->uid = scopy_user_id (uid);
+      new->validity =
+        get_validity (ctrl, keyblock, &new->key, uid, NULL, 0) & TRUST_MASK;
+      new->valid = 1;
+    }
+
+  return new->valid;
+}
+
 /* Finally this function compares a NEW key to the former candidate
  * OLD.  Returns < 0 if the old key is worse, > 0 if the old key is
  * better, == 0 if it is a tie.  */
@@ -1346,6 +1388,7 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
   struct getkey_ctx_s *ctx = NULL;
   int is_mbox = is_valid_mailbox (name);
   int wkd_tried = 0;
+  kbnode_t keyblock = NULL;
 
   if (retctx)
     *retctx = NULL;
@@ -1353,16 +1396,13 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
  start_over:
   if (ctx)  /* Clear  in case of a start over.  */
     {
-      if (ret_keyblock)
-        {
-          release_kbnode (*ret_keyblock);
-          *ret_keyblock = NULL;
-        }
+      release_kbnode (keyblock);
       getkey_end (ctrl, ctx);
       ctx = NULL;
+      keyblock = NULL;
     }
   err = get_pubkey_byname (ctrl, mode,
-                           &ctx, pk, name, ret_keyblock,
+                           &ctx, pk, name, &keyblock,
                            NULL, include_unusable);
   if (err)
     {
@@ -1373,10 +1413,10 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
   /* If the keyblock was retrieved from the local database and the key
    * has expired, do further checks.  However, we can do this only if
    * the caller requested a keyblock.  */
-  if (is_mbox && ctx && ctx->found_via_akl == AKL_LOCAL && ret_keyblock)
+  if (is_mbox && ctx && ctx->found_via_akl == AKL_LOCAL)
     {
       u32 now = make_timestamp ();
-      PKT_public_key *pk2 = (*ret_keyblock)->pkt->pkt.public_key;
+      PKT_public_key *pk2 = keyblock->pkt->pkt.public_key;
       int found;
 
       /* If the key has expired and its origin was the WKD then try to
@@ -1388,7 +1428,7 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
        * (see import.c:import_one).  */
       if (!wkd_tried && pk2->keyorg == KEYORG_WKD
           && (pk2->keyupdate + 3*3600) < now
-          && (pk2->has_expired || only_expired_enc_subkeys (*ret_keyblock)))
+          && (pk2->has_expired || only_expired_enc_subkeys (keyblock)))
         {
           if (opt.verbose)
             log_info (_("checking for a fresh copy of an expired key via %s\n"),
@@ -1409,6 +1449,12 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
       struct pubkey_cmp_cookie new = { 0 };
       kbnode_t new_keyblock;
 
+      if (pk)
+        copy_public_key (&best.key, pk);
+      else
+        copy_public_key (&best.key, keyblock->pkt->pkt.public_key);
+      fill_pubkey_cmp_cookie (ctrl, &best, name, keyblock);
+
       while (getkey_next (ctrl, ctx, &new.key, &new_keyblock) == 0)
         {
           int diff = pubkey_cmp (ctrl, name, &best, &new, new_keyblock);
@@ -1439,50 +1485,48 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
       free_user_id (best.uid);
       best.uid = NULL;
 
-      if (best.valid)
+      if (retctx)
         {
-          if (retctx || ret_keyblock)
+          ctx = xtrycalloc (1, sizeof **retctx);
+          if (! ctx)
+            err = gpg_error_from_syserror ();
+          else
             {
-              ctx = xtrycalloc (1, sizeof **retctx);
-              if (! ctx)
-                err = gpg_error_from_syserror ();
+              ctx->kr_handle = keydb_new (ctrl);
+              if (! ctx->kr_handle)
+                {
+                  err = gpg_error_from_syserror ();
+                  xfree (ctx);
+                  ctx = NULL;
+                  if (retctx)
+                    *retctx = NULL;
+                }
               else
                 {
-                  ctx->kr_handle = keydb_new (ctrl);
-                  if (! ctx->kr_handle)
+                  u32 *keyid = pk_keyid (&best.key);
+                  ctx->exact = 1;
+                  ctx->nitems = 1;
+                  ctx->items[0].mode = KEYDB_SEARCH_MODE_LONG_KID;
+                  ctx->items[0].u.kid[0] = keyid[0];
+                  ctx->items[0].u.kid[1] = keyid[1];
+
+                  if (ret_keyblock)
                     {
-                      err = gpg_error_from_syserror ();
-                      xfree (ctx);
-                      ctx = NULL;
-                      if (retctx)
-                        *retctx = NULL;
-                    }
-                  else
-                    {
-                      u32 *keyid = pk_keyid (&best.key);
-                      ctx->exact = 1;
-                      ctx->nitems = 1;
-                      ctx->items[0].mode = KEYDB_SEARCH_MODE_LONG_KID;
-                      ctx->items[0].u.kid[0] = keyid[0];
-                      ctx->items[0].u.kid[1] = keyid[1];
-
-                      if (ret_keyblock)
-                        {
-                          release_kbnode (*ret_keyblock);
-                          *ret_keyblock = NULL;
-                          err = getkey_next (ctrl, ctx, NULL, ret_keyblock);
-                        }
+                      *ret_keyblock = NULL;
+                      err = getkey_next (ctrl, ctx, NULL, ret_keyblock);
                     }
                 }
             }
-
-          if (pk)
-            *pk = best.key;
-          else
-            release_public_key_parts (&best.key);
         }
+
+      if (pk)
+        *pk = best.key;
+      else
+        release_public_key_parts (&best.key);
     }
 
+  release_kbnode (keyblock);
+
   if (err && ctx)
     {
       getkey_end (ctrl, ctx);
gniibe added a comment.Oct 7 2019, 3:58 AM

If we can assume ret_keyblock != NULL (it is, in current implementation), it can be as simple as:

diff --git a/g10/getkey.c b/g10/getkey.c
index 6802026f6..27bbd354c 100644
--- a/g10/getkey.c
+++ b/g10/getkey.c
@@ -1354,6 +1354,8 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
   int is_mbox = is_valid_mailbox (name);
   int wkd_tried = 0;
 
+  log_assert (ret_keyblock != NULL);
+
   if (retctx)
     *retctx = NULL;
 
@@ -1416,7 +1418,10 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
       struct pubkey_cmp_cookie new = { 0 };
       kbnode_t new_keyblock;
 
-      while (getkey_next (ctrl, ctx, &new.key, &new_keyblock) == 0)
+      copy_public_key (&new.key, (*ret_keyblock)->pkt->pkt.public_key);
+      new_keyblock = clone_kbnode (*ret_keyblock);
+
+      do
         {
           int diff = pubkey_cmp (ctrl, name, &best, &new, new_keyblock);
           release_kbnode (new_keyblock);
@@ -1441,6 +1446,8 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
             }
           new.uid = NULL;
         }
+      while (getkey_next (ctrl, ctx, &new.key, &new_keyblock) == 0);
+
       getkey_end (ctrl, ctx);
       ctx = NULL;
       free_user_id (best.uid);
gniibe changed the task status from Open to Testing.Oct 9 2019, 2:44 AM

I believe that constraint of ret_keyblock != NULL is OK.
Pushing the fix.
Perhaps, backport to 2.2 should be done, too.

werner changed the task status from Testing to Open.Oct 15 2019, 2:47 PM
werner added a subscriber: werner.

There are some problems with the definition of --locate-key. Further discussion required.

In my opinion, --locate-key should locate encryption key.

I also think this makes the most sense.

I found more wrong cases of get_best_pubkey_byname.
For ranking results,
(1) It may return non-encryption primary key as the most relevant key, when its validity is higher.
(2) It may not select encryption primary key even if its creation time is newer.

Those two cases are fixed in rG286d4c607574: gpg: Fix two other cases in get_best_pubkey_byname..

gniibe added a comment.EditedOct 17 2019, 3:38 AM

I think that we should apply further change:

diff --git a/g10/getkey.c b/g10/getkey.c
index 077209415..1c337149c 100644
--- a/g10/getkey.c
+++ b/g10/getkey.c
@@ -1369,7 +1369,7 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode,
     *retctx = NULL;
 
   memset (&pk0, 0, sizeof pk0);
-  pk0.req_usage = pk? pk->req_usage : 0;
+  pk0.req_usage = pk? pk->req_usage : PUBKEY_USAGE_ENC;
 
  start_over:
   if (ctx)  /* Clear  in case of a start over.  */

But, tests/openpgp/key-selection.scm assumes that --locate-key may return non-encryption key and expired key.

Let me clarify the point.

PROBLEM:
Key which is shown by gpg --locate-key someone should be used when `gpg -r someone -e'. Currently, the key by --locate-key may not be used (because no encryption capability/subkey is available) and fails, which is confusing.

Or... it could be a feature, not bug, so that failure of -e -r someone can be examined by --locate-keys someone.

In 2.2.18, this fix is not included. (partial fix was reverted)