gpg: Avoid importing secret keys if the keyblock is not valid.

* g10/keydb.h (struct kbnode_struct): Replace unused field RECNO by
new field TAG.
* g10/kbnode.c (alloc_node): Change accordingly.
* g10/import.c (import_one): Add arg r_valid.
(sec_to_pub_keyblock): Set tags.
(resync_sec_with_pub_keyblock): New.
(import_secret_one): Change return code to gpg_error_t.   Return an
error code if sec_to_pub_keyblock failed.  Resync secret keyblock.
--

When importing an invalid secret key ring for example without key
binding signatures or no UIDs, gpg used to let gpg-agent store the
secret keys anyway.  This is clearly a bug because the diagnostics
before claimed that for example the subkeys have been skipped.
Importing the secret key parameters then anyway is surprising in
particular because a gpg -k does not show the key.  After importing
the public key the secret keys suddenly showed up.

This changes the behaviour of
GnuPG-bug-id: 4392
to me more consistent but is not a solution to the actual bug.

Caution: The ecc.scm test now fails because two of the sample keys
         don't have binding signatures.

Signed-off-by: Werner Koch <wk@gnupg.org>

g10/import.c

@@ -109,8 +109,8 @@ static gpg_error_t import_one (ctrl_t ctrl,
                        unsigned char **fpr, size_t *fpr_len,
                        unsigned int options, int from_sk, int silent,
                        import_screener_t screener, void *screener_arg,
-                       int origin, const char *url);
+                       int origin, const char *url, int *r_valid);
-static int import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
+static gpg_error_t import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
                               struct import_stats_s *stats, int batch,
                               unsigned int options, int for_migration,
                               import_screener_t screener, void *screener_arg);
@@ -588,7 +588,7 @@ import (ctrl_t ctrl, IOBUF inp, const char* fname,struct import_stats_s *stats,
       if (keyblock->pkt->pkttype == PKT_PUBLIC_KEY)
         rc = import_one (ctrl, keyblock,
                          stats, fpr, fpr_len, options, 0, 0,
-                         screener, screener_arg, origin, url);
+                         screener, screener_arg, origin, url, NULL);
       else if (keyblock->pkt->pkttype == PKT_SECRET_KEY)
         rc = import_secret_one (ctrl, keyblock, stats,
                                 opt.batch, options, 0,
@@ -1667,7 +1667,9 @@ update_key_origin (kbnode_t keyblock, u32 curtime, int origin, const char *url)
  * programs which called gpg.  If SILENT is no messages are printed -
  * even most error messages are suppressed.  ORIGIN is the origin of
  * the key (0 for unknown) and URL the corresponding URL.  FROM_SK
- * indicates that the key has been made from a secret key.
+ * indicates that the key has been made from a secret key.  If R_SAVED
+ * is not NULL a boolean will be stored indicating whether the keyblock
+ * has valid parts.
  */
 static gpg_error_t
 import_one (ctrl_t ctrl,
@@ -1675,7 +1677,7 @@ import_one (ctrl_t ctrl,
 	    unsigned char **fpr, size_t *fpr_len, unsigned int options,
 	    int from_sk, int silent,
             import_screener_t screener, void *screener_arg,
-            int origin, const char *url)
+            int origin, const char *url, int *r_valid)
 {
   gpg_error_t err = 0;
   PKT_public_key *pk;
@@ -1694,6 +1696,9 @@ import_one (ctrl_t ctrl,
   int any_filter = 0;
   KEYDB_HANDLE hd = NULL;
 
+  if (r_valid)
+    *r_valid = 0;
+
   /* If show-only is active we don't won't any extra output.  */
   if ((options & (IMPORT_SHOW | IMPORT_DRY_RUN)))
     silent = 1;
@@ -1714,7 +1719,7 @@ import_one (ctrl_t ctrl,
   if (opt.verbose && !opt.interactive && !silent && !from_sk)
     {
       /* Note that we do not print this info in FROM_SK mode
-       * because import_one already printed that.  */
+       * because import_secret_one already printed that.  */
       log_info ("pub  %s/%s %s  ",
                 pubkey_string (pk, pkstrbuf, sizeof pkstrbuf),
                 keystr_from_pk(pk), datestr_from_pk(pk) );
@@ -1849,6 +1854,10 @@ import_one (ctrl_t ctrl,
       return 0;
     }
 
+  /* The keyblock is valid and ready for real import.  */
+  if (r_valid)
+    *r_valid = 1;
+
   /* Show the key in the form it is merged or inserted.  We skip this
    * if "import-export" is also active without --armor or the output
    * file has explicily been given. */
@@ -2463,14 +2472,21 @@ transfer_secret_keys (ctrl_t ctrl, struct import_stats_s *stats,
 
 
 /* Walk a secret keyblock and produce a public keyblock out of it.
-   Returns a new node or NULL on error. */
+ * Returns a new node or NULL on error.  Modifies the tag field of the
+ * nodes.  */
 static kbnode_t
 sec_to_pub_keyblock (kbnode_t sec_keyblock)
 {
   kbnode_t pub_keyblock = NULL;
   kbnode_t ctx = NULL;
   kbnode_t secnode, pubnode;
+  unsigned int tag = 0;
 
+  /* Set a tag to all nodes.  */
+  for (secnode = sec_keyblock; secnode; secnode = secnode->next)
+    secnode->tag = ++tag;
+
+  /* Copy.  */
   while ((secnode = walk_kbnode (sec_keyblock, &ctx, 0)))
     {
       if (secnode->pkt->pkttype == PKT_SECRET_KEY
@@ -2500,6 +2516,7 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock)
 	{
 	  pubnode = clone_kbnode (secnode);
 	}
+      pubnode->tag = secnode->tag;
 
       if (!pub_keyblock)
 	pub_keyblock = pubnode;
@@ -2510,23 +2527,67 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock)
   return pub_keyblock;
 }
 
+
+/* Delete all notes in the keyblock at R_KEYBLOCK which are not in
+ * PUB_KEYBLOCK.  Modifies the tags of both keyblock's nodes.  */
+static gpg_error_t
+resync_sec_with_pub_keyblock (kbnode_t *r_keyblock, kbnode_t pub_keyblock)
+{
+  kbnode_t sec_keyblock = *r_keyblock;
+  kbnode_t node;
+  unsigned int *taglist;
+  unsigned int ntaglist, n;
+
+  /* Collect all tags in an array for faster searching.  */
+  for (ntaglist = 0, node = pub_keyblock; node; node = node->next)
+    ntaglist++;
+  taglist = xtrycalloc (ntaglist, sizeof *taglist);
+  if (!taglist)
+    return gpg_error_from_syserror ();
+  for (ntaglist = 0, node = pub_keyblock; node; node = node->next)
+    taglist[ntaglist++] = node->tag;
+
+  /* Walks over the secret keyblock and delete all nodes which are not
+   * in the tag list.  Those nodes have been delete in the
+   * pub_keyblock.  Sequential search is a bit lazt and could be
+   * optimized by sorting and bsearch; however secret key rings are
+   * short and there are easier weaus to DoS gpg.  */
+  for (node = sec_keyblock; node; node = node->next)
+    {
+      for (n=0; n < ntaglist; n++)
+        if (taglist[n] == node->tag)
+          break;
+      if (n == ntaglist)
+        delete_kbnode (node);
+    }
+
+  xfree (taglist);
+
+  /* Commit the as deleted marked nodes and return the possibly
+   * modified keyblock.  */
+  commit_kbnode (&sec_keyblock);
+  *r_keyblock = sec_keyblock;
+  return 0;
+}
+
+
 /****************
  * Ditto for secret keys.  Handling is simpler than for public keys.
  * We allow secret key importing only when allow is true, this is so
  * that a secret key can not be imported accidentally and thereby tampering
  * with the trust calculation.
  */
-static int
+static gpg_error_t
 import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
-                   struct import_stats_s *stats, int batch, unsigned int options,
+                   struct import_stats_s *stats, int batch,
-                   int for_migration,
+                   unsigned int options, int for_migration,
                    import_screener_t screener, void *screener_arg)
 {
   PKT_public_key *pk;
   struct seckey_info *ski;
   kbnode_t node, uidnode;
   u32 keyid[2];
-  int rc = 0;
+  gpg_error_t err = 0;
   int nr_prev;
   kbnode_t pub_keyblock;
   char pkstrbuf[PUBKEY_STRING_SIZE];
@@ -2550,7 +2611,7 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
 
   if (opt.verbose && !for_migration)
     {
-      log_info ("sec  %s/%s %s   ",
+      log_info ("sec  %s/%s %s  ",
                 pubkey_string (pk, pkstrbuf, sizeof pkstrbuf),
                 keystr_from_pk (pk), datestr_from_pk (pk));
       if (uidnode)
@@ -2610,20 +2671,35 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
   /* Make a public key out of the key. */
   pub_keyblock = sec_to_pub_keyblock (keyblock);
   if (!pub_keyblock)
-    log_error ("key %s: failed to create public key from secret key\n",
+    {
-                   keystr_from_pk (pk));
+      err = gpg_error_from_syserror ();
+      log_error ("key %s: failed to create public key from secret key\n",
+                 keystr_from_pk (pk));
+    }
   else
     {
+      int valid;
+
       /* Note that this outputs an IMPORT_OK status message for the
 	 public key block, and below we will output another one for
 	 the secret keys.  FIXME?  */
       import_one (ctrl, pub_keyblock, stats,
 		  NULL, NULL, options, 1, for_migration,
-                  screener, screener_arg, 0, NULL);
+                  screener, screener_arg, 0, NULL, &valid);
 
-      /* Fixme: We should check for an invalid keyblock and
+      /* The secret keyblock may not have nodes which are deleted in
-	 cancel the secret key import in this case.  */
+       * the public keyblock.  Otherwise we would import just the
-      release_kbnode (pub_keyblock);
+       * secret key without having the public key.  That would be
+       * surprising and clutters out private-keys-v1.d.  */
+      err = resync_sec_with_pub_keyblock (&keyblock, pub_keyblock);
+      if (err)
+        goto leave;
+
+      if (!valid)
+        {
+          err = gpg_error (GPG_ERR_NO_SECKEY);
+          goto leave;
+        }
 
       /* At least we cancel the secret key import when the public key
 	 import was skipped due to MERGE_ONLY option and a new
@@ -2631,7 +2707,8 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
       if (!(opt.dry_run || (options & IMPORT_DRY_RUN))
           && stats->skipped_new_keys <= nr_prev)
 	{
-          /* Read the keyblock again to get the effects of a merge.  */
+          /* Read the keyblock again to get the effects of a merge for
+           * the public key.  */
           /* Fixme: we should do this based on the fingerprint or
              even better let import_one return the merged
              keyblock.  */
@@ -2641,8 +2718,6 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
                        keystr_from_pk (pk));
           else
             {
-              gpg_error_t err;
-
               /* transfer_secret_keys collects subkey stats.  */
               struct import_stats_s subkey_stats = {0};
 
@@ -2680,6 +2755,7 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
 
                   if (is_status_enabled ())
                     print_import_ok (pk, status);
+
                   check_prefs (ctrl, node);
                 }
               release_kbnode (node);
@@ -2687,7 +2763,9 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
         }
     }
 
-  return rc;
+ leave:
+  release_kbnode (pub_keyblock);
+  return err;
 }
 
 

g10/kbnode.c

@@ -68,8 +68,8 @@ alloc_node (void)
   n->next = NULL;
   n->pkt = NULL;
   n->flag = 0;
+  n->tag = 0;
   n->private_flag=0;
-  n->recno = 0;
   return n;
 }
 

g10/keydb.h

@@ -52,12 +52,13 @@ typedef struct getkey_ctx_s *getkey_ctx_t;
  * This structure is also used to bind arbitrary packets together.
  */
 
-struct kbnode_struct {
+struct kbnode_struct
-    KBNODE next;
+{
-    PACKET *pkt;
+  kbnode_t next;
-    int flag;
+  PACKET *pkt;
-    int private_flag;
+  int flag;          /* Local use during keyblock processing (not cloned).*/
-    ulong recno;  /* used while updating the trustdb */
+  unsigned int tag;  /* Ditto. */
+  int private_flag;
 };
 
 #define is_deleted_kbnode(a)  ((a)->private_flag & 1)

g10/pubkey-enc.c

@@ -117,7 +117,7 @@ get_session_key (ctrl_t ctrl, struct pubkey_enc_list *list, DEK *dek)
        * - On-card keys of an active card
        * - On-disk keys with protection
        * - On-card keys from cards which are not plugged it.  Here a
-       *   cancel-all button should stop aksing for other cards.
+       *   cancel-all button should stop asking for other cards.
        * Without any anonymous keys the sorting can be skipped.
        */
       for (k = list; k; k = k->next)

tests/openpgp/ecc.scm

@@ -175,7 +175,7 @@ Rg==
 	 (display "This is one line\n" (fdopen fd "wb")))
 
   (for-each-p
-   "Checking ECDSA decryption"
+   "Checking ECDH decryption"
    (lambda (test)
      (lettmp (x y)
        (call-with-output-file

tests/openpgp/samplekeys/README

@@ -29,3 +29,5 @@ Notes:
   such a file is created which is then directly followed by a separate
   armored public key block.  To create such a sample concatenate
   pgp-desktop-skr.asc and E657FB607BB4F21C90BB6651BC067AF28BC90111.asc
+- ecc-sample-2-sec.asc and ecc-sample-3-sec.asc do not have and
+  binding signatures either.  ecc-sample-1-sec.asc has them, though.