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 <[email protected]>
(cherry picked from commit f799e9728bcadb3d4148a47848c78c5647860ea4)

5 files changed, 111 insertions, 30 deletions

diff --git a/g10/import.c b/g10/import.c
index 88aa86b70..4e23e6d28 100644
--- a/g10/import.c
+++ b/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);
-static int import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
+                       int origin, const char *url, int *r_valid);
+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);
@@ -584,7 +584,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,
@@ -1645,7 +1645,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,
@@ -1653,7 +1655,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;
@@ -1672,6 +1674,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;
@@ -1692,7 +1697,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) );
@@ -1818,6 +1823,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. */
@@ -2431,14 +2440,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
@@ -2468,6 +2484,7 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock)
 	{
 	  pubnode = clone_kbnode (secnode);
 	}
+      pubnode->tag = secnode->tag;
 
       if (!pub_keyblock)
 	pub_keyblock = pubnode;
@@ -2478,23 +2495,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,
-                   int for_migration,
+                   struct import_stats_s *stats, int batch,
+                   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];
@@ -2518,7 +2579,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)
@@ -2578,20 +2639,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
-	 cancel the secret key import in this case.  */
-      release_kbnode (pub_keyblock);
+      /* The secret keyblock may not have nodes which are deleted in
+       * the public keyblock.  Otherwise we would import just the
+       * 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
@@ -2599,7 +2675,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.  */
@@ -2609,8 +2686,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};
 
@@ -2648,6 +2723,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);
@@ -2655,7 +2731,9 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
         }
     }
 
-  return rc;
+ leave:
+  release_kbnode (pub_keyblock);
+  return err;
 }
 
 
diff --git a/g10/kbnode.c b/g10/kbnode.c
index c2aaacd76..9ed6cafbf 100644
--- a/g10/kbnode.c
+++ b/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;
 }
 
diff --git a/g10/keydb.h b/g10/keydb.h
index 6fb4e5e60..7aa204853 100644
--- a/g10/keydb.h
+++ b/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 {
-    KBNODE next;
-    PACKET *pkt;
-    int flag;
-    int private_flag;
-    ulong recno;  /* used while updating the trustdb */
+struct kbnode_struct
+{
+  kbnode_t next;
+  PACKET *pkt;
+  int flag;          /* Local use during keyblock processing (not cloned).*/
+  unsigned int tag;  /* Ditto. */
+  int private_flag;
 };
 
 #define is_deleted_kbnode(a)  ((a)->private_flag & 1)
diff --git a/tests/openpgp/ecc.scm b/tests/openpgp/ecc.scm
index d7c02a5e2..a63ec45bd 100755
--- a/tests/openpgp/ecc.scm
+++ b/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
diff --git a/tests/openpgp/samplekeys/README b/tests/openpgp/samplekeys/README
index 9f1648bdf..f8a7e9ed7 100644
--- a/tests/openpgp/samplekeys/README
+++ b/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.