gpg: Fix writing ECDH keys to OpenPGP smartcards.

* agent/command.c (cmd_keytocard): Add new arg for ECDH params.
* scd/app-openpgp.c (ecc_writekey): Use provided ECDH params to
compute the fingerprint.
* g10/call-agent.c (agent_keytocard): Add arg ecdh_param_str.
* g10/keyid.c (ecdh_param_str_from_pk): New.
* g10/card-util.c (card_store_subkey): Pass ECDH params to writekey.
* g10/keygen.c (card_store_key_with_backup): Ditto.

* scd/app-openpgp.c (store_fpr): Add arg update.
(rsa_read_pubkey, ecc_read_pubkey): Add arg meta_update and avoid
writing the fingerprint back to the card if not set.
(read_public_key): Also add arg meta_update.
(get_public_key): Do not pass it as true here...
(do_genkey): ... but here.
(rsa_write_key, ecc_writekey): Force string the fingerprint.
--

The problem showed up because in 2.4 we changed the standard ECDH
parameter some years ago.  Now when trying to write an ECDH key
created by 2.2 with 2.4 to an openpgp card, scdaemon computes a wrong
fingerprint and thus gpg was not able to find the key again by
fingerprint.

The patch also avoids updating the stored fingerprint in certain
situations.

This fix is somewhat related to
GnuPG-bug-id: 6378

8 files changed, 181 insertions, 37 deletions

diff --git a/agent/command.c b/agent/command.c
index 9481f47c3..dd7cb5e57 100644
--- a/agent/command.c
+++ b/agent/command.c
@@ -3175,9 +3175,10 @@ cmd_delete_key (assuan_context_t ctx, char *line)
 #endif
 
 static const char hlp_keytocard[] =
-  "KEYTOCARD [--force] <hexgrip> <serialno> <keyref> [<timestamp>]\n"
+  "KEYTOCARD [--force] <hexgrip> <serialno> <keyref> [<timestamp> [<ecdh>]]\n"
   "\n"
-  "TIMESTAMP is required for OpenPGP and defaults to the Epoch.  The\n"
+  "TIMESTAMP is required for OpenPGP and defaults to the Epoch.\n"
+  "ECDH are the hexified ECDH parameters for OpenPGP.\n"
   "SERIALNO is used for checking; use \"-\" to disable the check.";
 static gpg_error_t
 cmd_keytocard (assuan_context_t ctx, char *line)
@@ -3194,6 +3195,9 @@ cmd_keytocard (assuan_context_t ctx, char *line)
   size_t keydatalen;
   unsigned char *shadow_info = NULL;
   time_t timestamp;
+  char *ecdh_params = NULL;
+  unsigned int ecdh_params_len;
+  unsigned int extralen1, extralen2;
 
   if (ctrl->restricted)
     return leave_cmd (ctx, gpg_error (GPG_ERR_FORBIDDEN));
@@ -3240,10 +3244,38 @@ cmd_keytocard (assuan_context_t ctx, char *line)
 
   /* Default to the creation time as stored in the private key.  The
    * parameter is here so that gpg can make sure that the timestamp as
-   * used for key creation (and thus the openPGP fingerprint) is
-   * used.  */
+   * used.  It is also important for OpenPGP cards to allow computing
+   * of the fingerprint.  Same goes for the ECDH params.  */
   if (argc > 3)
-    timestamp = isotime2epoch (argv[3]);
+    {
+      timestamp = isotime2epoch (argv[3]);
+      if (argc > 4)
+        {
+          size_t n;
+
+          err = parse_hexstring (ctx, argv[4], &n);
+          if (err)
+            goto leave;  /* Badly formatted ecdh params. */
+          n /= 2;
+          if (n < 4)
+            {
+              err = set_error (GPG_ERR_ASS_PARAMETER, "ecdh param too short");
+              goto leave;
+            }
+          ecdh_params_len = n;
+          ecdh_params = xtrymalloc (ecdh_params_len);
+          if (!ecdh_params)
+            {
+              err = gpg_error_from_syserror ();
+              goto leave;
+            }
+          if (hex2bin (argv[4], ecdh_params, ecdh_params_len) < 0)
+            {
+              err = set_error (GPG_ERR_BUG, "hex2bin");
+              goto leave;
+            }
+        }
+    }
   else if (timestamp == (time_t)(-1))
     timestamp = isotime2epoch ("19700101T000000");
 
@@ -3254,9 +3286,12 @@ cmd_keytocard (assuan_context_t ctx, char *line)
     }
 
   /* Note: We can't use make_canon_sexp because we need to allocate a
-   * few extra bytes for our hack below.  */
+   * few extra bytes for our hack below.  The 20 for extralen2
+   * accounts for the sexp length of ecdh_params.  */
   keydatalen = gcry_sexp_sprint (s_skey, GCRYSEXP_FMT_CANON, NULL, 0);
-  keydata = xtrymalloc_secure (keydatalen + 30);
+  extralen1 = 30;
+  extralen2 = ecdh_params? (20+20+ecdh_params_len) : 0;
+  keydata = xtrymalloc_secure (keydatalen + extralen1 + extralen2);
   if (keydata == NULL)
     {
       err = gpg_error_from_syserror ();
@@ -3265,15 +3300,31 @@ cmd_keytocard (assuan_context_t ctx, char *line)
   gcry_sexp_sprint (s_skey, GCRYSEXP_FMT_CANON, keydata, keydatalen);
   gcry_sexp_release (s_skey);
   s_skey = NULL;
+
   keydatalen--;			/* Decrement for last '\0'.  */
+
   /* Hack to insert the timestamp "created-at" into the private key.  */
-  snprintf (keydata+keydatalen-1, 30, KEYTOCARD_TIMESTAMP_FORMAT, timestamp);
+  snprintf (keydata+keydatalen-1, extralen1, KEYTOCARD_TIMESTAMP_FORMAT,
+            timestamp);
   keydatalen += 10 + 19 - 1;
 
+  /* Hack to insert the timestamp "ecdh-params" into the private key.  */
+  if (ecdh_params)
+    {
+      snprintf (keydata+keydatalen-1, extralen2, "(11:ecdh-params%u:",
+                ecdh_params_len);
+      keydatalen += strlen (keydata+keydatalen-1) -1;
+      memcpy (keydata+keydatalen, ecdh_params, ecdh_params_len);
+      keydatalen += ecdh_params_len;
+      memcpy (keydata+keydatalen, "))", 3);
+      keydatalen += 2;
+    }
+
   err = divert_writekey (ctrl, force, serialno, keyref, keydata, keydatalen);
   xfree (keydata);
 
  leave:
+  xfree (ecdh_params);
   gcry_sexp_release (s_skey);
   xfree (shadow_info);
   return leave_cmd (ctx, err);
diff --git a/g10/call-agent.c b/g10/call-agent.c
index 131f56ae7..b0bccc0a5 100644
--- a/g10/call-agent.c
+++ b/g10/call-agent.c
@@ -1096,7 +1096,8 @@ agent_keytotpm (ctrl_t ctrl, const char *hexgrip)
  */
 int
 agent_keytocard (const char *hexgrip, int keyno, int force,
-                 const char *serialno, const char *timestamp)
+                 const char *serialno, const char *timestamp,
+                 const char *ecdh_param_str)
 {
   int rc;
   char line[ASSUAN_LINELENGTH];
@@ -1104,8 +1105,9 @@ agent_keytocard (const char *hexgrip, int keyno, int force,
 
   memset (&parm, 0, sizeof parm);
 
-  snprintf (line, DIM(line), "KEYTOCARD %s%s %s OPENPGP.%d %s",
-            force?"--force ": "", hexgrip, serialno, keyno, timestamp);
+  snprintf (line, DIM(line), "KEYTOCARD %s%s %s OPENPGP.%d %s%s%s",
+            force?"--force ": "", hexgrip, serialno, keyno, timestamp,
+            ecdh_param_str? " ":"", ecdh_param_str? ecdh_param_str:"");
 
   rc = start_agent (NULL, 1);
   if (rc)
diff --git a/g10/call-agent.h b/g10/call-agent.h
index 80595cacc..45af95422 100644
--- a/g10/call-agent.h
+++ b/g10/call-agent.h
@@ -135,7 +135,8 @@ int agent_keytotpm (ctrl_t ctrl, const char *hexgrip);
 
 /* Send the KEYTOCARD command. */
 int agent_keytocard (const char *hexgrip, int keyno, int force,
-                     const char *serialno, const char *timestamp);
+                     const char *serialno, const char *timestamp,
+                     const char *ecdh_param_str);
 
 /* Send a SETATTR command to the SCdaemon. */
 gpg_error_t agent_scd_setattr (const char *name,
diff --git a/g10/card-util.c b/g10/card-util.c
index a3297fb71..d680c4d0a 100644
--- a/g10/card-util.c
+++ b/g10/card-util.c
@@ -1805,8 +1805,9 @@ card_store_subkey (KBNODE node, int use, strlist_t *processed_keys)
   int  keyno;
   PKT_public_key *pk;
   gpg_error_t err;
-  char *hexgrip;
+  char *hexgrip = NULL;
   int rc;
+  char *ecdh_param_str = NULL;
   gnupg_isotime_t timebuf;
 
   log_assert (node->pkt->pkttype == PKT_PUBLIC_KEY
@@ -1880,8 +1881,19 @@ card_store_subkey (KBNODE node, int use, strlist_t *processed_keys)
     goto leave;
 
   epoch2isotime (timebuf, (time_t)pk->timestamp);
-  rc = agent_keytocard (hexgrip, keyno, rc, info.serialno, timebuf);
 
+  if (pk->pubkey_algo == PUBKEY_ALGO_ECDH)
+    {
+      ecdh_param_str = ecdh_param_str_from_pk (pk);
+      if (!ecdh_param_str)
+        {
+          err = gpg_error_from_syserror ();
+          goto leave;
+        }
+    }
+
+  rc = agent_keytocard (hexgrip, keyno, rc, info.serialno,
+                        timebuf, ecdh_param_str);
   if (rc)
     log_error (_("KEYTOCARD failed: %s\n"), gpg_strerror (rc));
   else
@@ -1890,9 +1902,10 @@ card_store_subkey (KBNODE node, int use, strlist_t *processed_keys)
       if (processed_keys)
         add_to_strlist (processed_keys, hexgrip);
     }
-  xfree (hexgrip);
 
  leave:
+  xfree (hexgrip);
+  xfree (ecdh_param_str);
   agent_release_card_info (&info);
   return okay;
 }
diff --git a/g10/keydb.h b/g10/keydb.h
index 9323e3137..1a66d664e 100644
--- a/g10/keydb.h
+++ b/g10/keydb.h
@@ -576,6 +576,7 @@ char *format_hexfingerprint (const char *fingerprint,
                              char *buffer, size_t buflen);
 gpg_error_t keygrip_from_pk (PKT_public_key *pk, unsigned char *array);
 gpg_error_t hexkeygrip_from_pk (PKT_public_key *pk, char **r_grip);
+char *ecdh_param_str_from_pk (PKT_public_key *pk);
 
 
 /*-- kbnode.c --*/
diff --git a/g10/keygen.c b/g10/keygen.c
index c97783124..7f54f7da0 100644
--- a/g10/keygen.c
+++ b/g10/keygen.c
@@ -5327,12 +5327,20 @@ card_store_key_with_backup (ctrl_t ctrl, PKT_public_key *sub_psk,
   char *cache_nonce = NULL;
   void *kek = NULL;
   size_t keklen;
+  char *ecdh_param_str = NULL;
 
   sk = copy_public_key (NULL, sub_psk);
   if (!sk)
     return gpg_error_from_syserror ();
 
   epoch2isotime (timestamp, (time_t)sk->timestamp);
+  if (sk->pubkey_algo == PUBKEY_ALGO_ECDH)
+    {
+      ecdh_param_str = ecdh_param_str_from_pk (sk);
+      if (!ecdh_param_str)
+        return gpg_error_from_syserror ();
+    }
+
   err = hexkeygrip_from_pk (sk, &hexgrip);
   if (err)
     goto leave;
@@ -5345,7 +5353,8 @@ card_store_key_with_backup (ctrl_t ctrl, PKT_public_key *sub_psk,
       goto leave;
     }
 
-  rc = agent_keytocard (hexgrip, 2, 1, info.serialno, timestamp);
+  rc = agent_keytocard (hexgrip, 2, 1, info.serialno,
+                        timestamp, ecdh_param_str);
   xfree (info.serialno);
   if (rc)
     {
@@ -5388,6 +5397,7 @@ card_store_key_with_backup (ctrl_t ctrl, PKT_public_key *sub_psk,
     agent_scd_learn (NULL, 1);
 
  leave:
+  xfree (ecdh_param_str);
   xfree (cache_nonce);
   gcry_cipher_close (cipherhd);
   xfree (kek);
diff --git a/g10/keyid.c b/g10/keyid.c
index ca6564c5c..9191fec92 100644
--- a/g10/keyid.c
+++ b/g10/keyid.c
@@ -1141,3 +1141,25 @@ hexkeygrip_from_pk (PKT_public_key *pk, char **r_grip)
     }
   return err;
 }
+
+
+/* Return a hexfied malloced string of the ECDH parameters for an ECDH
+ * key from the public key PK.  Returns NULL on error.  */
+char *
+ecdh_param_str_from_pk (PKT_public_key *pk)
+{
+  const unsigned char *s;
+  unsigned int n;
+
+  if (!pk
+      || pk->pubkey_algo != PUBKEY_ALGO_ECDH
+      || !gcry_mpi_get_flag (pk->pkey[2], GCRYMPI_FLAG_OPAQUE)
+      || !(s = gcry_mpi_get_opaque (pk->pkey[2], &n)) || !n)
+    {
+      gpg_err_set_errno (EINVAL);
+      return NULL;  /* Invalid parameter */
+    }
+
+  n = (n+7)/8;
+  return bin2hex (s, n, NULL);
+}
diff --git a/scd/app-openpgp.c b/scd/app-openpgp.c
index d3f460106..66ec9f4a9 100644
--- a/scd/app-openpgp.c
+++ b/scd/app-openpgp.c
@@ -869,10 +869,12 @@ parse_login_data (app_t app)
 
 #define MAX_ARGS_STORE_FPR 3
 
-/* Note, that FPR must be at least 20 bytes. */
+/* Note, that FPR must be at least 20 bytes.  If UPDATE is not set,
+ * the fingerprint and the creation date is not actually stored but
+ * the fingerprint is only returned in FPR.  */
 static gpg_error_t
-store_fpr (app_t app, int keynumber, u32 timestamp, unsigned char *fpr,
-           int algo, ...)
+store_fpr (app_t app, int update, int keynumber, u32 timestamp,
+           unsigned char *fpr, int algo, ...)
 {
   unsigned int n, nbits;
   unsigned char *buffer, *p;
@@ -937,6 +939,9 @@ store_fpr (app_t app, int keynumber, u32 timestamp, unsigned char *fpr,
 
   xfree (buffer);
 
+  if (!update)
+    return 0;
+
   tag = (app->appversion > 0x0007? 0xC7 : 0xC6) + keynumber;
   flush_cache_item (app, 0xC5);
   tag2 = 0xCE + keynumber;
@@ -1605,7 +1610,8 @@ retrieve_key_material (FILE *fp, const char *hexkeyid,
 
 
 static gpg_error_t
-rsa_read_pubkey (app_t app, ctrl_t ctrl, u32 created_at,  int keyno,
+rsa_read_pubkey (app_t app, ctrl_t ctrl, int meta_update,
+                 u32 created_at,  int keyno,
                  const unsigned char *data, size_t datalen, gcry_sexp_t *r_sexp)
 {
   gpg_error_t err;
@@ -1642,7 +1648,11 @@ rsa_read_pubkey (app_t app, ctrl_t ctrl, u32 created_at,  int keyno,
     {
       unsigned char fprbuf[20];
 
-      err = store_fpr (app, keyno, created_at, fprbuf, PUBKEY_ALGO_RSA,
+      /* If META_UPDATE is not set we only compute but not store the
+       * fingerprint.  This might return a wrong fingerprint if
+       * CREATED_AT is not set.  */
+      err = store_fpr (app, meta_update, keyno,
+                       created_at, fprbuf, PUBKEY_ALGO_RSA,
                        m, mlen, e, elen);
       if (err)
         return err;
@@ -1714,7 +1724,8 @@ ecdh_params (const char *curve)
 }
 
 static gpg_error_t
-ecc_read_pubkey (app_t app, ctrl_t ctrl, u32 created_at, int keyno,
+ecc_read_pubkey (app_t app, ctrl_t ctrl, int meta_update,
+                 u32 created_at, int keyno,
                  const unsigned char *data, size_t datalen, gcry_sexp_t *r_sexp)
 {
   gpg_error_t err;
@@ -1783,7 +1794,12 @@ ecc_read_pubkey (app_t app, ctrl_t ctrl, u32 created_at, int keyno,
     {
       unsigned char fprbuf[20];
 
-      err = store_fpr (app, keyno, created_at, fprbuf, algo, oidbuf, oid_len,
+      /* If META_UPDATE is not set we only compute but not store the
+       * fingerprint.  This might return a wrong fingerprint if
+       * CREATED_AT is not set or the ECDH params do not match the
+       * current defaults. */
+      err = store_fpr (app, meta_update, keyno,
+                       created_at, fprbuf, algo, oidbuf, oid_len,
                        qbuf, ecc_q_len, ecdh_params (curve), (size_t)4);
       if (err)
         goto leave;
@@ -1826,13 +1842,15 @@ store_keygrip (app_t app, int keyno)
 
 
 /* Parse tag-length-value data for public key in BUFFER of BUFLEN
-   length.  Key of KEYNO in APP is updated with an S-expression of
-   public key.  When CTRL is not NULL, fingerprint is computed with
-   CREATED_AT, and fingerprint is written to the card, and key data
-   and fingerprint are send back to the client side.
+ * length.  Key of KEYNO in APP is updated with an S-expression of
+ * public key.  If CTRL is not NULL, the fingerprint is computed with
+ * CREATED_AT and key data and fingerprint are send back to the client
+ * side.  If also META_UPDATE is true the fingerprint and the creation
+ * date are also written to the card.
  */
 static gpg_error_t
-read_public_key (app_t app, ctrl_t ctrl, u32 created_at, int keyno,
+read_public_key (app_t app, ctrl_t ctrl, int meta_update,
+                 u32 created_at, int keyno,
                  const unsigned char *buffer, size_t buflen)
 {
   gpg_error_t err;
@@ -1848,10 +1866,10 @@ read_public_key (app_t app, ctrl_t ctrl, u32 created_at, int keyno,
     }
 
   if (app->app_local->keyattr[keyno].key_type == KEY_TYPE_RSA)
-    err = rsa_read_pubkey (app, ctrl, created_at, keyno,
+    err = rsa_read_pubkey (app, ctrl, meta_update, created_at, keyno,
                            data, datalen, &s_pkey);
   else if (app->app_local->keyattr[keyno].key_type == KEY_TYPE_ECC)
-    err = ecc_read_pubkey (app, ctrl, created_at, keyno,
+    err = ecc_read_pubkey (app, ctrl, meta_update, created_at, keyno,
                            data, datalen, &s_pkey);
   else
     err = gpg_error (GPG_ERR_NOT_IMPLEMENTED);
@@ -1947,14 +1965,19 @@ get_public_key (app_t app, int keyno)
           /* Yubikey returns wrong code.  Fix it up.  */
           if (APP_CARD(app)->cardtype == CARDTYPE_YUBIKEY)
             err = gpg_error (GPG_ERR_NO_OBJ);
-	  /* Yubikey NEO (!CARDTYPE_YUBIKEY) also returns wrong code.  Fix it up.  */
+	  /* Yubikey NEO (!CARDTYPE_YUBIKEY) also returns wrong code.
+           * Fix it up.  */
 	  else if (gpg_err_code (err) == GPG_ERR_CARD)
             err = gpg_error (GPG_ERR_NO_OBJ);
           log_error (_("reading public key failed: %s\n"), gpg_strerror (err));
           goto leave;
         }
 
-      err = read_public_key (app, NULL, 0U, keyno, buffer, buflen);
+      /* Note that we use 0 for the creation date and thus the - via
+       * status lines - returned fingerprint will only be valid if the
+       * key has also been created with that date.  A similar problem
+       * occurs with the ECDH params which are fixed in the code.  */
+      err = read_public_key (app, NULL, 0, 0U, keyno, buffer, buflen);
     }
   else
     {
@@ -4520,7 +4543,7 @@ rsa_writekey (app_t app, ctrl_t ctrl,
       goto leave;
     }
 
-  err = store_fpr (app, keyno, created_at, fprbuf, PUBKEY_ALGO_RSA,
+  err = store_fpr (app, 1, keyno, created_at, fprbuf, PUBKEY_ALGO_RSA,
                    rsa_n, rsa_n_len, rsa_e, rsa_e_len);
   if (err)
     goto leave;
@@ -4545,6 +4568,8 @@ ecc_writekey (app_t app, ctrl_t ctrl,
   const unsigned char *ecc_q = NULL;
   const unsigned char *ecc_d = NULL;
   size_t ecc_q_len, ecc_d_len;
+  const unsigned char *ecdh_param = NULL;
+  size_t ecdh_param_len = 0;
   const char *curve = NULL;
   u32 created_at = 0;
   const char *oidstr;
@@ -4557,7 +4582,7 @@ ecc_writekey (app_t app, ctrl_t ctrl,
   unsigned char fprbuf[20];
   size_t ecc_d_fixed_len;
 
-  /* (private-key(ecc(curve%s)(q%m)(d%m))(created-at%d)):
+  /* (private-key(ecc(curve%s)(q%m)(d%m))(created-at%d)(ecdh-params%s)):
      curve = "NIST P-256" */
   /* (private-key(ecc(curve%s)(q%m)(d%m))(created-at%d)):
      curve = "secp256k1" */
@@ -4652,6 +4677,7 @@ ecc_writekey (app_t app, ctrl_t ctrl,
         }
       if ((err = parse_sexp (&buf, &buflen, &depth, &tok, &toklen)))
         goto leave;
+
       if (tok && toklen == 10 && !memcmp ("created-at", tok, toklen))
         {
           if ((err = parse_sexp (&buf,&buflen,&depth,&tok,&toklen)))
@@ -4663,6 +4689,17 @@ ecc_writekey (app_t app, ctrl_t ctrl,
                 created_at = created_at*10 + (*tok - '0');
             }
         }
+      else if (tok && toklen == 11 && !memcmp ("ecdh-params", tok, toklen))
+        {
+          if ((err = parse_sexp (&buf,&buflen,&depth,&tok,&toklen)))
+            goto leave;
+          if (tok)
+            {
+              ecdh_param = tok;
+              ecdh_param_len = toklen;
+            }
+        }
+
       /* Skip until end of list. */
       last_depth2 = depth;
       while (!(err = parse_sexp (&buf, &buflen, &depth, &tok, &toklen))
@@ -4694,6 +4731,13 @@ ecc_writekey (app_t app, ctrl_t ctrl,
   else
     algo = PUBKEY_ALGO_ECDSA;
 
+  if (algo == PUBKEY_ALGO_ECDH && !ecdh_param)
+    {
+      log_error ("opgp: ecdh parameters missing\n");
+      err = gpg_error (GPG_ERR_INV_VALUE);
+      goto leave;
+    }
+
   oidstr = openpgp_curve_to_oid (curve, &n, NULL);
   ecc_d_fixed_len = (n+7)/8;
   err = openpgp_oid_from_str (oidstr, &oid);
@@ -4795,8 +4839,8 @@ ecc_writekey (app_t app, ctrl_t ctrl,
       goto leave;
     }
 
-  err = store_fpr (app, keyno, created_at, fprbuf, algo, oidbuf, oid_len,
-                   ecc_q, ecc_q_len, ecdh_params (curve), (size_t)4);
+  err = store_fpr (app, 1, keyno, created_at, fprbuf, algo, oidbuf, oid_len,
+                   ecc_q, ecc_q_len, ecdh_param, ecdh_param_len);
 
  leave:
   gcry_mpi_release (oid);
@@ -5024,7 +5068,7 @@ do_genkey (app_t app, ctrl_t ctrl,  const char *keyref, const char *keyalgo,
   send_status_info (ctrl, "KEY-CREATED-AT",
                     numbuf, (size_t)strlen(numbuf), NULL, 0);
 
-  err = read_public_key (app, ctrl, created_at, keyno, buffer, buflen);
+  err = read_public_key (app, ctrl, 1, created_at, keyno, buffer, buflen);
  leave:
   xfree (buffer);
   return err;