gpg: Improve import's repair-key duplicate signature detection.
authorWerner Koch <wk@gnupg.org>
Thu, 7 Jun 2018 16:41:17 +0000 (18:41 +0200)
committerWerner Koch <wk@gnupg.org>
Thu, 7 Jun 2018 16:41:17 +0000 (18:41 +0200)
* g10/key-check.c (key_check_all_keysigs): Factor some code out to ...
(remove_duplicate_sigs): new.
(key_check_all_keysigs): Call remove_duplicate_sigs again after
reordering.
--

This is a follupup for commit 26bce2f01d2029ea2b8a8dbbe36118e3c83c5cba
to cleanup the code and to add a second de-duplicate step when needed.

GnuPG-bug-id: 3994
Signed-off-by: Werner Koch <wk@gnupg.org>
g10/key-check.c

index 17f2dae..c17b12c 100644 (file)
@@ -1,7 +1,7 @@
 /* key-check.c - Detect and fix various problems with keys
  * Copyright (C) 1998-2010 Free Software Foundation, Inc.
  * Copyright (C) 1998-2017 Werner Koch
- * Copyright (C) 2015-2017 g10 Code GmbH
+ * Copyright (C) 2015-2018 g10 Code GmbH
  *
  * This file is part of GnuPG.
  *
@@ -101,6 +101,125 @@ sig_comparison (const void *av, const void *bv)
 }
 
 
+static gpg_error_t
+remove_duplicate_sigs (kbnode_t kb, int *dups, int *modified)
+{
+  gpg_error_t err;
+  kbnode_t n;
+  int nsigs;
+  kbnode_t *sigs;  /* Allocated array with the signature packet.  */
+  int i;
+  int last_i;
+  int block;
+  PKT_signature *sig;
+
+  /* Count the sigs.  */
+  for (nsigs = 0, n = kb; n; n = n->next)
+    {
+      if (is_deleted_kbnode (n))
+        continue;
+      else if (n->pkt->pkttype == PKT_SIGNATURE)
+        nsigs ++;
+    }
+
+  if (!nsigs)
+    return 0; /* No signatures at all.  */
+
+  /* Add them all to the SIGS array.  */
+  sigs = xtrycalloc (nsigs, sizeof *sigs);
+  if (!sigs)
+    {
+      err = gpg_error_from_syserror ();
+      log_error (_("error allocating memory: %s\n"), gpg_strerror (err));
+      return err;
+    }
+
+  block = 0;
+  i = 0;
+  for (n = kb; n; n = n->next)
+    {
+      if (is_deleted_kbnode (n))
+        continue;
+
+      if (n->pkt->pkttype != PKT_SIGNATURE)
+        {
+          switch (n->pkt->pkttype)
+            {
+            case PKT_PUBLIC_SUBKEY:
+            case PKT_SECRET_SUBKEY:
+            case PKT_USER_ID:
+            case PKT_ATTRIBUTE:
+              /* Bump the block number so that we only consider
+               * signatures below the same object as duplicates.  */
+              block++;
+              break;
+            default:
+              break;
+            }
+          continue;
+        }
+      sig = n->pkt->pkt.signature;
+      sig->help_counter = block;
+      sigs[i++] = n;
+    }
+  log_assert (i == nsigs);
+
+  qsort (sigs, nsigs, sizeof (sigs[0]), sig_comparison);
+
+  last_i = 0;
+  for (i = 1; i < nsigs; i ++)
+    {
+      log_assert (sigs[last_i]);
+      log_assert (sigs[last_i]->pkt->pkttype == PKT_SIGNATURE);
+      log_assert (sigs[i]);
+      log_assert (sigs[i]->pkt->pkttype == PKT_SIGNATURE);
+
+      if (sig_comparison (&sigs[last_i], &sigs[i]) == 0)
+        {
+          /* They are the same.  Kill the latter.  */
+          if (DBG_PACKET)
+            {
+              sig = sigs[i]->pkt->pkt.signature;
+
+              log_debug ("Signature appears multiple times, "
+                         "deleting duplicate:\n");
+              log_debug ("  sig: class 0x%x, issuer: %s,"
+                         " timestamp: %s (%lld), digest: %02x %02x\n",
+                         sig->sig_class, keystr (sig->keyid),
+                         isotimestamp (sig->timestamp),
+                         (long long) sig->timestamp,
+                         sig->digest_start[0], sig->digest_start[1]);
+            }
+
+          /* Remove sigs[i] from the keyblock.  */
+          {
+            kbnode_t z, *prevp;
+            int to_kill = last_i;
+            last_i = i;
+
+            for (prevp = &kb, z = kb; z; prevp = &z->next, z = z->next)
+              if (z == sigs[to_kill])
+                break;
+
+            *prevp = sigs[to_kill]->next;
+
+            sigs[to_kill]->next = NULL;
+            release_kbnode (sigs[to_kill]);
+            sigs[to_kill] = NULL;
+
+            ++*dups;
+            *modified = 1;
+          }
+        }
+      else
+        last_i = i;
+    }
+
+  xfree (sigs);
+  return 0;
+}
+
+
 /* Perform a few sanity checks on a keyblock is okay and possibly
  * repair some damage.  Concretely:
  *
@@ -146,121 +265,11 @@ key_check_all_keysigs (ctrl_t ctrl, int mode, kbnode_t kb,
   pk = kb->pkt->pkt.public_key;
 
   /* First we look for duplicates.  */
-  {
-    int nsigs;
-    kbnode_t *sigs;
-    int i;
-    int last_i;
-    int block;
-
-    /* Count the sigs.  */
-    for (nsigs = 0, n = kb; n; n = n->next)
-      {
-        if (is_deleted_kbnode (n))
-          continue;
-        else if (n->pkt->pkttype == PKT_SIGNATURE)
-          nsigs ++;
-      }
-
-    if (!nsigs)
-      return 0; /* No signatures at all.  */
-
-    /* Add them all to the SIGS array.  */
-    sigs = xtrycalloc (nsigs, sizeof *sigs);
-    if (!sigs)
-      {
-        log_error (_("error allocating memory: %s\n"),
-                   gpg_strerror (gpg_error_from_syserror ()));
-        return 0;
-      }
-
-    i = 0;
-    block = 0;
-    for (n = kb; n; n = n->next)
-      {
-        if (is_deleted_kbnode (n))
-          continue;
-
-        if (n->pkt->pkttype != PKT_SIGNATURE)
-          {
-            switch (n->pkt->pkttype)
-              {
-              case PKT_PUBLIC_SUBKEY:
-              case PKT_SECRET_SUBKEY:
-              case PKT_USER_ID:
-              case PKT_ATTRIBUTE:
-                /* Bump the block number so that we only consider
-                 * signatures below the same object as duplicates.  */
-                block++;
-                break;
-              default:
-                break;
-              }
-            continue;
-          }
-        sig = n->pkt->pkt.signature;
-        sig->help_counter = block;
-        sigs[i] = n;
-        i ++;
-      }
-    log_assert (i == nsigs);
-
-    qsort (sigs, nsigs, sizeof (sigs[0]), sig_comparison);
+  if (remove_duplicate_sigs (kb, &dups, &modified))
+    goto leave;  /* Error */
 
-    last_i = 0;
-    for (i = 1; i < nsigs; i ++)
-      {
-        log_assert (sigs[last_i]);
-        log_assert (sigs[last_i]->pkt->pkttype == PKT_SIGNATURE);
-        log_assert (sigs[i]);
-        log_assert (sigs[i]->pkt->pkttype == PKT_SIGNATURE);
-
-        if (sig_comparison (&sigs[last_i], &sigs[i]) == 0)
-          /* They are the same.  Kill the latter.  */
-          {
-            if (DBG_PACKET)
-              {
-                sig = sigs[i]->pkt->pkt.signature;
-
-                log_debug ("Signature appears multiple times, "
-                           "deleting duplicate:\n");
-                log_debug ("  sig: class 0x%x, issuer: %s,"
-                           " timestamp: %s (%lld), digest: %02x %02x\n",
-                           sig->sig_class, keystr (sig->keyid),
-                           isotimestamp (sig->timestamp),
-                           (long long) sig->timestamp,
-                           sig->digest_start[0], sig->digest_start[1]);
-              }
-
-            /* Remove sigs[i] from the keyblock.  */
-            {
-              KBNODE z, *prevp;
-              int to_kill = last_i;
-              last_i = i;
-
-              for (prevp = &kb, z = kb; z; prevp = &z->next, z = z->next)
-                if (z == sigs[to_kill])
-                  break;
-
-              *prevp = sigs[to_kill]->next;
-
-              sigs[to_kill]->next = NULL;
-              release_kbnode (sigs[to_kill]);
-              sigs[to_kill] = NULL;
-
-              dups ++;
-              modified = 1;
-            }
-          }
-        else
-          last_i = i;
-      }
-
-    xfree (sigs);
-  }
-
-  /* Make sure the sigs occur after the component (public key, subkey,
-     user id) that they sign.  */
+  /* Now make sure the sigs occur after the component (aka block)
+   * (public key, subkey, user id) that they sign.  */
   issuer = NULL;
   last_printed_component = NULL;
   for (n_prevp = &kb, n = kb;
@@ -598,6 +607,14 @@ key_check_all_keysigs (ctrl_t ctrl, int mode, kbnode_t kb,
     free_public_key (issuer);
   issuer = NULL;
 
+  /* If we reordered signatures we need to de-duplicate again because
+   * a signature can now be a duplicate in another block.  */
+  if (reordered)
+    {
+      if (remove_duplicate_sigs (kb, &dups, &modified))
+        goto leave;
+    }
+
   /* Identify keys / uids that don't have a self-sig.  */
   {
     int has_selfsig = 0;
@@ -667,6 +684,8 @@ key_check_all_keysigs (ctrl_t ctrl, int mode, kbnode_t kb,
       }
   }
 
+
+ leave:
   if (!opt.quiet)
     {
       char prefix[100];