[PATCH 0/4] T1756 gpg-agent doesn't accept ssh certificates

[GnuPG - Dev mailing list]

This set of patches updates support for certificates and
addresses (at least part of) https://dev.gnupg.org/T1756.

With thes patches user shall be able to add RSA key and
certificate to the gpg-agent and get a passwordless sign
through signed certificates.

Looking forward to feedback and comments.


Signed-off-by: Igor Okulist <[hidden email]>


Igor Okulist (4):
  ssh: update certificate support
  ssh: update certificate support
  ssh: update certificate support
  ssh: update certificate support

 agent/agent.h       |   3 +-
 agent/command-ssh.c | 117 +++++++++++++++++++++++++++++++++++++++++---
 agent/cvt-openpgp.c |  12 ++++-
 agent/findkey.c     |  46 ++++++++++++++---
 4 files changed, 159 insertions(+), 19 deletions(-)

--
2.25.1


_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[GnuPG - Dev mailing list]

---
 agent/agent.h       |   3 +-
 agent/command-ssh.c | 156 ++++++++++++++++++++++++++++++++++++++++----
 agent/cvt-openpgp.c |  12 +++-
 agent/findkey.c     |  58 +++++++++++++---
 4 files changed, 204 insertions(+), 25 deletions(-)

diff --git a/agent/agent.h b/agent/agent.h
index fb4641259..207ed45a9 100644
--- a/agent/agent.h
+++ b/agent/agent.h
@@ -619,6 +619,7 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
                      const char **r_algoname, int *r_npkey, int *r_nskey,
                      const char **r_format,
                      gcry_mpi_t *mpi_array, int arraysize,
-                     gcry_sexp_t *r_curve, gcry_sexp_t *r_flags);
+                     gcry_sexp_t *r_curve, gcry_sexp_t *r_flags,
+                     gcry_sexp_t *key_type);
 
 #endif /*AGENT_H*/
diff --git a/agent/command-ssh.c b/agent/command-ssh.c
index bcc78bd15..d8ccbafb0 100644
--- a/agent/command-ssh.c
+++ b/agent/command-ssh.c
@@ -1897,6 +1897,10 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
       goto out;
     }
 
+  gcry_sexp_t list = gcry_sexp_find_token (sexp, "key-type", 0);
+  size_t len = 0;
+  const char *key_type = gcry_sexp_nth_data (list, 1, &len);
+
   /* Get key value list.  */
   value_list = gcry_sexp_cadr (sexp);
   if (!value_list)
@@ -1933,10 +1937,42 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
     }
   else
     {
-      /* Note: This is also used for EdDSA.  */
-      err = stream_write_cstring (stream, key_spec.ssh_identifier);
-      if (err)
-        goto out;
+       if (key_type)
+        {
+          // err = stream_write_string (stream, key_type, len);
+          // if (err)
+          //   goto out;
+          // err = stream_write_string (stream, "<nonce>", strlen ("<nonce>"));
+          // if (err)
+          //   goto out;
+
+          gcry_sexp_t certificate_sexp = gcry_sexp_find_token (sexp, "certificate", 0);
+          size_t certificate_sexp_b64_len = 0;
+          const char *certificate_sexp_b64 = gcry_sexp_nth_data(certificate_sexp, 1, &certificate_sexp_b64_len);
+
+          char *certificate = xtrymalloc (certificate_sexp_b64_len + 1);
+          strncpy(certificate, certificate_sexp_b64, certificate_sexp_b64_len);
+          certificate[certificate_sexp_b64_len] = '\0';
+
+          struct b64state b64s = {};
+          long int len = 0;
+
+          err = b64dec_start (&b64s, NULL);
+          err = b64dec_proc (&b64s, certificate, certificate_sexp_b64_len, &len);
+          err = b64dec_finish (&b64s);
+          err = stream_write_data (stream, certificate, len);
+
+          xfree (certificate);
+
+          goto done;
+        }
+      else
+        {
+          /* Note: This is also used for EdDSA.  */
+          err = stream_write_cstring (stream, key_spec.ssh_identifier);
+          if (err)
+            goto out;
+        }
     }
 
   /* Write the parameters.  */
@@ -1986,6 +2022,7 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
         }
     }
 
+done:
   if (es_fclose_snatch (stream, &blob, &blob_size))
     {
       err = gpg_error_from_syserror ();
@@ -2005,7 +2042,6 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
 
   return err;
 }
-
 
 /*
 
@@ -2065,23 +2101,31 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
   if (err)
     goto out;
 
+  log_info("key type: %s", key_type);
+
   err = ssh_key_type_lookup (key_type, 0, &spec);
   if (err)
     goto out;
 
+  log_info("key spec flags: 0x%x", spec.flags);
+
+  unsigned char *cert_buffer = NULL;
+  u32 cert_buffer_len = 0;
+
   if ((spec.flags & SPEC_FLAG_WITH_CERT))
     {
+      /* in case of certs it reads part of cert to get public mpints
+      and parts of the payload for private mpints -- so -cert.pub files
+      have private mpints? or is it just reading random data to fill out the
+      key struct ? */
       /* This is an OpenSSH certificate+private key.  The certificate
          is an SSH string and which we store in an estream object. */
-      unsigned char *buffer;
-      u32 buflen;
       char *cert_key_type;
 
-      err = stream_read_string (stream, 0, &buffer, &buflen);
+      err = stream_read_string (stream, 0, &cert_buffer, &cert_buffer_len);
       if (err)
         goto out;
-      cert = es_fopenmem_init (0, "rb", buffer, buflen);
-      xfree (buffer);
+      cert = es_fopenmem_init (0, "rb", cert_buffer, cert_buffer_len);
       if (!cert)
         {
           err = gpg_error_from_syserror ();
@@ -2092,6 +2136,9 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
       err = stream_read_cstring (cert, &cert_key_type);
       if (err)
         goto out;
+
+      log_info ("certificate type: %s", cert_key_type);
+
       if (strcmp (cert_key_type, key_type) )
         {
           xfree (cert_key_type);
@@ -2211,6 +2258,7 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
       err = stream_read_cstring (stream, &comment);
       if (err)
  goto out;
+      log_info("key comment: %s", comment);
     }
 
   if (secret)
@@ -2246,12 +2294,55 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
                                  comment? comment:"");
         }
     }
-  else
+  else if (0==strcmp(spec.ssh_identifier, "[hidden email]"))
     {
+          struct b64state b64s;
+          estream_t stream;
+          long int len;
+
+          stream = es_fopenmem(0, "wt");
+          err = b64enc_start_es (&b64s, stream, "");
+          err = b64enc_write (&b64s, cert_buffer, cert_buffer_len);
+          err = b64enc_finish (&b64s);
+          len = es_ftell (stream);
+
+          char *result = xtrymalloc (len + 1);
+          size_t nread;
+
+          es_fseek(stream, 0, SEEK_SET);
+          es_read (stream, result, len, &nread);
+          result[len] = 0;
+
+          es_fclose (stream);
+
+          err = gcry_sexp_build (&key, NULL,
+            "(private-key "
+            " (rsa "
+            "  (n %m)"
+            "  (e %m)"
+            "  (d %m)"
+            "  (p %m)"
+            "  (q %m)"
+            "  (u %m)"
+            "  )"
+            " (comment %s)"
+            " (key-type %s)"
+            " (certificate %s)"
+            " )",
+            mpi_list[1], // !swapped 1 and 0
+            mpi_list[0],
+            mpi_list[2],
+            mpi_list[3],
+            mpi_list[4],
+            mpi_list[5],
+            comment!=NULL?comment:"",
+            spec.ssh_identifier,
+            result);
+    }
+  else
+    {  
       err = sexp_key_construct (&key, spec, secret, curve_name, mpi_list,
-                                comment? comment:"");
-      if (err)
-        goto out;
+                            comment? comment:"");
     }
 
   if (key_spec)
@@ -2264,6 +2355,8 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
   xfree (key_type);
   xfree (comment);
 
+  xfree (cert_buffer);
+
   return err;
 }
 
@@ -2366,6 +2459,28 @@ ssh_key_grip (gcry_sexp_t key, unsigned char *buffer)
       return err? err : gpg_error (GPG_ERR_INTERNAL);
     }
 
+  gcry_sexp_t list = NULL;
+  size_t len = 0;
+  const char *data = NULL;
+
+  list = gcry_sexp_find_token (key, "key-type", 0);
+  len = 0;
+  data = gcry_sexp_nth_data(list, 1, &len);
+
+  if (data)
+    {
+      gcry_md_hd_t md = NULL;
+      gcry_md_open (&md, GCRY_MD_SHA1, 0);
+
+      gcry_md_write (md, buffer, 20);
+      gcry_md_write (md, data, len);
+
+      memcpy (buffer, gcry_md_read (md, GCRY_MD_SHA1), 20);
+      gcry_md_close (md);
+    }
+
+  gcry_sexp_release(list);
+
   return 0;
 }
 
@@ -2652,6 +2767,13 @@ ssh_handler_request_identities (ctrl_t ctrl,
                      gpg_strerror (err));
           continue;
         }
+      if (opt.verbose > 1) {
+          log_info ("fname: %s", cf->fname);
+          log_info ("hexgrip: %s", cf->item.hexgrip);
+          char debug_buffer[8192] = "\0";
+          err = gcry_sexp_sprint (key_public, GCRYSEXP_FMT_ADVANCED, debug_buffer, sizeof(debug_buffer));
+          log_info ("key sExpression: %s", debug_buffer);
+      }
 
       err = ssh_send_key_public (key_blobs, key_public, NULL);
       if (err)
@@ -3132,6 +3254,12 @@ ssh_identity_register (ctrl_t ctrl, ssh_key_type_spec_t *spec,
  }
     }
 
+  if (opt.verbose > 1) {
+      char debug_buffer[8192] = "\0";
+      err = gcry_sexp_sprint (key, GCRYSEXP_FMT_ADVANCED, debug_buffer, sizeof(debug_buffer));
+      log_info ("key Sexpression: %s", debug_buffer);
+  }
+  
   err = ssh_key_to_protected_buffer (key, pi->pin, &buffer, &buffer_n);
   if (err)
     goto out;
diff --git a/agent/cvt-openpgp.c b/agent/cvt-openpgp.c
index ff153c358..54f38997c 100644
--- a/agent/cvt-openpgp.c
+++ b/agent/cvt-openpgp.c
@@ -1194,7 +1194,8 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
                      const char **r_algoname, int *r_npkey, int *r_nskey,
                      const char **r_elems,
                      gcry_mpi_t *array, int arraysize,
-                     gcry_sexp_t *r_curve, gcry_sexp_t *r_flags)
+                     gcry_sexp_t *r_curve, gcry_sexp_t *r_flags,
+                     gcry_sexp_t *r_key_type)
 {
   gpg_error_t err;
   gcry_sexp_t list, l2;
@@ -1203,6 +1204,7 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
   int npkey, nskey;
   gcry_sexp_t curve = NULL;
   gcry_sexp_t flags = NULL;
+  gcry_sexp_t key_type = NULL;
 
   *r_curve = NULL;
   *r_flags = NULL;
@@ -1224,6 +1226,8 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
       return gpg_error (GPG_ERR_BAD_SECKEY);
     }
 
+  key_type = gcry_sexp_find_token (list, "key_type", 0);
+
   l2 = gcry_sexp_cadr (list);
   gcry_sexp_release (list);
   list = l2;
@@ -1305,6 +1309,9 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
       *r_curve = curve;
       *r_flags = flags;
 
+      if (r_key_type)
+        *r_key_type = key_type;
+
       return 0;
     }
 }
@@ -1323,6 +1330,7 @@ convert_to_openpgp (ctrl_t ctrl, gcry_sexp_t s_key, const char *passphrase,
   gcry_mpi_t array[10];
   gcry_sexp_t curve = NULL;
   gcry_sexp_t flags = NULL;
+  gcry_sexp_t key_name = NULL;
   char protect_iv[16];
   char salt[8];
   unsigned long s2k_count;
@@ -1336,7 +1344,7 @@ convert_to_openpgp (ctrl_t ctrl, gcry_sexp_t s_key, const char *passphrase,
     array[i] = NULL;
 
   err = extract_private_key (s_key, 1, &algoname, &npkey, &nskey, NULL,
-                             array, DIM (array), &curve, &flags);
+                             array, DIM (array), &curve, &flags, &key_name);
   if (err)
     return err;
 
diff --git a/agent/findkey.c b/agent/findkey.c
index cea21959f..5b54de7cb 100644
--- a/agent/findkey.c
+++ b/agent/findkey.c
@@ -1245,13 +1245,13 @@ agent_public_key_from_file (ctrl_t ctrl,
   gcry_mpi_t array[10];
   gcry_sexp_t curve = NULL;
   gcry_sexp_t flags = NULL;
-  gcry_sexp_t uri_sexp, comment_sexp;
-  const char *uri, *comment;
-  size_t uri_length, comment_length;
-  int uri_intlen, comment_intlen;
+  gcry_sexp_t uri_sexp, comment_sexp, key_type_sexp, certificate_sexp;
+  const char *uri, *comment, *key_type, *certificate;
+  size_t uri_length, comment_length, key_type_length, certificate_length;
+  int uri_intlen, comment_intlen, key_type_intlen, certificate_intlen;
   char *format, *p;
-  void *args[2+7+2+2+1]; /* Size is 2 + max. # of elements + 2 for uri + 2
-                            for comment + end-of-list.  */
+  void *args[2+7+2+2+2+1]; /* Size is 2 + max. # of elements + 2 for uri + 2
+                            for comment + key_type + certificate +  end-of-list.  */
   int argidx;
   gcry_sexp_t list = NULL;
   const char *s;
@@ -1264,11 +1264,21 @@ agent_public_key_from_file (ctrl_t ctrl,
   if (err)
     return err;
 
+  if (opt.verbose > 1) {
+      char hexgrip[40+4+1];
+      bin2hex (grip, 20, hexgrip);
+
+      log_info ("hexgrip: %s", hexgrip);
+      char debug_buffer[8192] = "\0";
+      err = gcry_sexp_sprint (s_skey, GCRYSEXP_FMT_ADVANCED, debug_buffer, sizeof(debug_buffer));
+      log_info ("loaded key sExpression: %s", debug_buffer);
+  }
+  
   for (i=0; i < DIM (array); i++)
     array[i] = NULL;
 
   err = extract_private_key (s_skey, 0, &algoname, &npkey, NULL, &elems,
-                             array, DIM (array), &curve, &flags);
+                             array, DIM (array), &curve, &flags, &key_type_sexp);
   if (err)
     {
       gcry_sexp_release (s_skey);
@@ -1287,10 +1297,25 @@ agent_public_key_from_file (ctrl_t ctrl,
   if (comment_sexp)
     comment = gcry_sexp_nth_data (comment_sexp, 1, &comment_length);
 
+  key_type = NULL;
+  key_type_length = 0;
+  key_type_sexp = gcry_sexp_find_token (s_skey, "key-type", 0);
+  if (key_type_sexp)
+    key_type = gcry_sexp_nth_data (key_type_sexp, 1, &key_type_length);
+
+  certificate = NULL;
+  certificate_length = 0;
+  certificate_sexp = gcry_sexp_find_token (s_skey, "certificate", 0);
+  if (certificate_sexp)
+    certificate = gcry_sexp_nth_data (certificate_sexp, 1, &certificate_length);
+
+
   gcry_sexp_release (s_skey);
   s_skey = NULL;
 
 
+  // TODO: the following FIXME is so true -- following code is
+  // prone to buffer overrun
   /* FIXME: The following thing is pretty ugly code; we should
      investigate how to make it cleaner.  Probably code to handle
      canonical S-expressions in a memory buffer is better suited for
@@ -1299,7 +1324,7 @@ agent_public_key_from_file (ctrl_t ctrl,
      them.  */
   assert (sizeof (size_t) <= sizeof (void*));
 
-  format = xtrymalloc (15+4+7*npkey+10+15+1+1);
+  format = xtrymalloc (15+4+7*npkey+10+15+1+1+5+4096);
   if (!format)
     {
       err = gpg_error_from_syserror ();
@@ -1342,6 +1367,23 @@ agent_public_key_from_file (ctrl_t ctrl,
       args[argidx++] = (void *)&comment_intlen;
       args[argidx++] = (void*)&comment;
     }
+  if (key_type)
+    {
+      p = stpcpy (p, "(key-type %b)");
+      log_assert (argidx+1 < DIM (args));
+      key_type_intlen = (int)key_type_length;
+      args[argidx++] = (void *)&key_type_intlen;
+      args[argidx++] = (void*)&key_type;
+    }
+  if (certificate)
+    {
+      p = stpcpy (p, "(certificate %b)");
+      log_assert (argidx+1 < DIM (args));
+      certificate_intlen = (int)certificate_length;
+      args[argidx++] = (void *)&certificate_intlen;
+      args[argidx++] = (void*)&certificate;
+    }
+  
   *p++ = ')';
   *p = 0;
   assert (argidx < DIM (args));
--
2.25.1


_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[GnuPG - Dev mailing list]

code cleanup
---
 agent/command-ssh.c | 147 ++++++++++++++++++++------------------------
 agent/findkey.c     |  10 ---
 2 files changed, 66 insertions(+), 91 deletions(-)

diff --git a/agent/command-ssh.c b/agent/command-ssh.c
index d8ccbafb0..dfdc36f97 100644
--- a/agent/command-ssh.c
+++ b/agent/command-ssh.c
@@ -1897,10 +1897,6 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
       goto out;
     }
 
-  gcry_sexp_t list = gcry_sexp_find_token (sexp, "key-type", 0);
-  size_t len = 0;
-  const char *key_type = gcry_sexp_nth_data (list, 1, &len);
-
   /* Get key value list.  */
   value_list = gcry_sexp_cadr (sexp);
   if (!value_list)
@@ -1937,15 +1933,12 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
     }
   else
     {
-       if (key_type)
-        {
-          // err = stream_write_string (stream, key_type, len);
-          // if (err)
-          //   goto out;
-          // err = stream_write_string (stream, "<nonce>", strlen ("<nonce>"));
-          // if (err)
-          //   goto out;
+      gcry_sexp_t list = gcry_sexp_find_token (sexp, "key-type", 0);
+      size_t len = 0;
+      const char *key_type = gcry_sexp_nth_data (list, 1, &len);
 
+      if (key_type)
+        {
           gcry_sexp_t certificate_sexp = gcry_sexp_find_token (sexp, "certificate", 0);
           size_t certificate_sexp_b64_len = 0;
           const char *certificate_sexp_b64 = gcry_sexp_nth_data(certificate_sexp, 1, &certificate_sexp_b64_len);
@@ -1955,15 +1948,17 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
           certificate[certificate_sexp_b64_len] = '\0';
 
           struct b64state b64s = {};
-          long int len = 0;
+          long int certificate_len = 0;
 
           err = b64dec_start (&b64s, NULL);
-          err = b64dec_proc (&b64s, certificate, certificate_sexp_b64_len, &len);
-          err = b64dec_finish (&b64s);
-          err = stream_write_data (stream, certificate, len);
+          err = err || b64dec_proc (&b64s, certificate, certificate_sexp_b64_len, &certificate_len);
+          err = err || b64dec_finish (&b64s);
+          err = err | stream_write_data (stream, certificate, certificate_len);
 
           xfree (certificate);
 
+          if (err)
+            goto out;
           goto done;
         }
       else
@@ -2021,7 +2016,7 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
             goto out;
         }
     }
-
+    
 done:
   if (es_fclose_snatch (stream, &blob, &blob_size))
     {
@@ -2101,23 +2096,21 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
   if (err)
     goto out;
 
-  log_info("key type: %s", key_type);
+  if (opt.verbose)
+    log_info("key type: %s", key_type);
 
   err = ssh_key_type_lookup (key_type, 0, &spec);
   if (err)
     goto out;
 
-  log_info("key spec flags: 0x%x", spec.flags);
+  if (opt.verbose)
+    log_info("key spec flags: 0x%x", spec.flags);
 
   unsigned char *cert_buffer = NULL;
   u32 cert_buffer_len = 0;
 
   if ((spec.flags & SPEC_FLAG_WITH_CERT))
     {
-      /* in case of certs it reads part of cert to get public mpints
-      and parts of the payload for private mpints -- so -cert.pub files
-      have private mpints? or is it just reading random data to fill out the
-      key struct ? */
       /* This is an OpenSSH certificate+private key.  The certificate
          is an SSH string and which we store in an estream object. */
       char *cert_key_type;
@@ -2137,7 +2130,8 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
       if (err)
         goto out;
 
-      log_info ("certificate type: %s", cert_key_type);
+      if (opt.verbose)
+        log_info ("certificate type: %s", cert_key_type);
 
       if (strcmp (cert_key_type, key_type) )
         {
@@ -2258,7 +2252,8 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
       err = stream_read_cstring (stream, &comment);
       if (err)
  goto out;
-      log_info("key comment: %s", comment);
+      if (opt.verbose)
+        log_info("key comment: %s", comment);
     }
 
   if (secret)
@@ -2294,55 +2289,57 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
                                  comment? comment:"");
         }
     }
-  else if (0==strcmp(spec.ssh_identifier, "[hidden email]"))
+  else if (0 == strcmp(spec.ssh_identifier, "[hidden email]"))
     {
-          struct b64state b64s;
-          estream_t stream;
-          long int len;
+      struct b64state b64s = {};
+      estream_t stream = NULL;
+      long int b64_cert_buffer_len = 0;
 
-          stream = es_fopenmem(0, "wt");
-          err = b64enc_start_es (&b64s, stream, "");
-          err = b64enc_write (&b64s, cert_buffer, cert_buffer_len);
-          err = b64enc_finish (&b64s);
-          len = es_ftell (stream);
+      stream = es_fopenmem(0, "wt");
+      err = b64enc_start_es (&b64s, stream, "");
+      err = err || b64enc_write (&b64s, cert_buffer, cert_buffer_len);
+      err = err || b64enc_finish (&b64s);
+      if (err)
+        goto out;
 
-          char *result = xtrymalloc (len + 1);
-          size_t nread;
+      b64_cert_buffer_len = es_ftell (stream);
 
-          es_fseek(stream, 0, SEEK_SET);
-          es_read (stream, result, len, &nread);
-          result[len] = 0;
+      char *b64_cert_buffer = xtrymalloc (b64_cert_buffer_len + 1);
+      size_t nread = 0;
 
-          es_fclose (stream);
+      err = es_fseek(stream, 0, SEEK_SET);
+      err = err || es_read (stream, b64_cert_buffer, b64_cert_buffer_len, &nread);
+      if (err)
+        goto out;
+      b64_cert_buffer[b64_cert_buffer_len] = "\0";
 
-          err = gcry_sexp_build (&key, NULL,
-            "(private-key "
-            " (rsa "
-            "  (n %m)"
-            "  (e %m)"
-            "  (d %m)"
-            "  (p %m)"
-            "  (q %m)"
-            "  (u %m)"
-            "  )"
-            " (comment %s)"
-            " (key-type %s)"
-            " (certificate %s)"
-            " )",
-            mpi_list[1], // !swapped 1 and 0
-            mpi_list[0],
-            mpi_list[2],
-            mpi_list[3],
-            mpi_list[4],
-            mpi_list[5],
-            comment!=NULL?comment:"",
-            spec.ssh_identifier,
-            result);
+      es_fclose (stream);
+
+      err = gcry_sexp_build (&key, NULL,
+        "(private-key "
+        " (rsa (n %m) (e %m) (d %m) (p %m) (q %m) (u %m) )"
+        " (comment %s)"
+        " (key-type %s)"
+        " (certificate %s)"
+        " )",
+        // swapped! 1 and 0 required
+        mpi_list[1],  mpi_list[0],  mpi_list[2], mpi_list[3], mpi_list[4], mpi_list[5],
+        comment!=NULL?comment:"",
+        spec.ssh_identifier,
+        b64_cert_buffer);
+
+      xfree(b64_cert_buffer);
+      b64_cert_buffer = NULL;
+
+      if (err)
+        goto out;
     }
   else
     {  
       err = sexp_key_construct (&key, spec, secret, curve_name, mpi_list,
                             comment? comment:"");
+      if (err)
+        goto out;
     }
 
   if (key_spec)
@@ -2449,7 +2446,7 @@ ssh_read_key_public_from_blob (unsigned char *blob, size_t blob_size,
 
 /* This function calculates the key grip for the key contained in the
    S-Expression KEY and writes it to BUFFER, which must be large
-   enough to hold it.  Returns usual error code.  */
+   enough to hold 20 characters.  Returns usual error code.  */
 static gpg_error_t
 ssh_key_grip (gcry_sexp_t key, unsigned char *buffer)
 {
@@ -2459,13 +2456,14 @@ ssh_key_grip (gcry_sexp_t key, unsigned char *buffer)
       return err? err : gpg_error (GPG_ERR_INTERNAL);
     }
 
+  // if the key contains "key-type" update the gcry_pk_get_keygrip computed
+  // keygrip by the hashing it with key-type value
   gcry_sexp_t list = NULL;
-  size_t len = 0;
   const char *data = NULL;
+  size_t data_len = 0;
 
   list = gcry_sexp_find_token (key, "key-type", 0);
-  len = 0;
-  data = gcry_sexp_nth_data(list, 1, &len);
+  data = gcry_sexp_nth_data(list, 1, &data_len);
 
   if (data)
     {
@@ -2473,7 +2471,7 @@ ssh_key_grip (gcry_sexp_t key, unsigned char *buffer)
       gcry_md_open (&md, GCRY_MD_SHA1, 0);
 
       gcry_md_write (md, buffer, 20);
-      gcry_md_write (md, data, len);
+      gcry_md_write (md, data, data_len);
 
       memcpy (buffer, gcry_md_read (md, GCRY_MD_SHA1), 20);
       gcry_md_close (md);
@@ -2767,13 +2765,6 @@ ssh_handler_request_identities (ctrl_t ctrl,
                      gpg_strerror (err));
           continue;
         }
-      if (opt.verbose > 1) {
-          log_info ("fname: %s", cf->fname);
-          log_info ("hexgrip: %s", cf->item.hexgrip);
-          char debug_buffer[8192] = "\0";
-          err = gcry_sexp_sprint (key_public, GCRYSEXP_FMT_ADVANCED, debug_buffer, sizeof(debug_buffer));
-          log_info ("key sExpression: %s", debug_buffer);
-      }
 
       err = ssh_send_key_public (key_blobs, key_public, NULL);
       if (err)
@@ -3253,12 +3244,6 @@ ssh_identity_register (ctrl_t ctrl, ssh_key_type_spec_t *spec,
   goto next_try;
  }
     }
-
-  if (opt.verbose > 1) {
-      char debug_buffer[8192] = "\0";
-      err = gcry_sexp_sprint (key, GCRYSEXP_FMT_ADVANCED, debug_buffer, sizeof(debug_buffer));
-      log_info ("key Sexpression: %s", debug_buffer);
-  }
   
   err = ssh_key_to_protected_buffer (key, pi->pin, &buffer, &buffer_n);
   if (err)
diff --git a/agent/findkey.c b/agent/findkey.c
index 5b54de7cb..b558ab893 100644
--- a/agent/findkey.c
+++ b/agent/findkey.c
@@ -1263,16 +1263,6 @@ agent_public_key_from_file (ctrl_t ctrl,
   err = read_key_file (grip, &s_skey);
   if (err)
     return err;
-
-  if (opt.verbose > 1) {
-      char hexgrip[40+4+1];
-      bin2hex (grip, 20, hexgrip);
-
-      log_info ("hexgrip: %s", hexgrip);
-      char debug_buffer[8192] = "\0";
-      err = gcry_sexp_sprint (s_skey, GCRYSEXP_FMT_ADVANCED, debug_buffer, sizeof(debug_buffer));
-      log_info ("loaded key sExpression: %s", debug_buffer);
-  }
   
   for (i=0; i < DIM (array); i++)
     array[i] = NULL;
--
2.25.1


_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[GnuPG - Dev mailing list]

remove useful but not feature related log messages
---
 agent/command-ssh.c | 30 +++++++++---------------------
 agent/findkey.c     |  6 ++----
 2 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/agent/command-ssh.c b/agent/command-ssh.c
index dfdc36f97..3983bbeb4 100644
--- a/agent/command-ssh.c
+++ b/agent/command-ssh.c
@@ -1963,11 +1963,11 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
         }
       else
         {
-          /* Note: This is also used for EdDSA.  */
-          err = stream_write_cstring (stream, key_spec.ssh_identifier);
-          if (err)
-            goto out;
-        }
+      /* Note: This is also used for EdDSA.  */
+      err = stream_write_cstring (stream, key_spec.ssh_identifier);
+      if (err)
+        goto out;
+    }
     }
 
   /* Write the parameters.  */
@@ -2016,7 +2016,7 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
             goto out;
         }
     }
-    
+
 done:
   if (es_fclose_snatch (stream, &blob, &blob_size))
     {
@@ -2096,16 +2096,10 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
   if (err)
     goto out;
 
-  if (opt.verbose)
-    log_info("key type: %s", key_type);
-
   err = ssh_key_type_lookup (key_type, 0, &spec);
   if (err)
     goto out;
 
-  if (opt.verbose)
-    log_info("key spec flags: 0x%x", spec.flags);
-
   unsigned char *cert_buffer = NULL;
   u32 cert_buffer_len = 0;
 
@@ -2129,10 +2123,6 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
       err = stream_read_cstring (cert, &cert_key_type);
       if (err)
         goto out;
-
-      if (opt.verbose)
-        log_info ("certificate type: %s", cert_key_type);
-
       if (strcmp (cert_key_type, key_type) )
         {
           xfree (cert_key_type);
@@ -2252,8 +2242,6 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
       err = stream_read_cstring (stream, &comment);
       if (err)
  goto out;
-      if (opt.verbose)
-        log_info("key comment: %s", comment);
     }
 
   if (secret)
@@ -2335,9 +2323,9 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
         goto out;
     }
   else
-    {  
+    {
       err = sexp_key_construct (&key, spec, secret, curve_name, mpi_list,
-                            comment? comment:"");
+                                comment? comment:"");
       if (err)
         goto out;
     }
@@ -3244,7 +3232,7 @@ ssh_identity_register (ctrl_t ctrl, ssh_key_type_spec_t *spec,
   goto next_try;
  }
     }
-  
+
   err = ssh_key_to_protected_buffer (key, pi->pin, &buffer, &buffer_n);
   if (err)
     goto out;
diff --git a/agent/findkey.c b/agent/findkey.c
index b558ab893..63964ce69 100644
--- a/agent/findkey.c
+++ b/agent/findkey.c
@@ -1263,7 +1263,7 @@ agent_public_key_from_file (ctrl_t ctrl,
   err = read_key_file (grip, &s_skey);
   if (err)
     return err;
-  
+
   for (i=0; i < DIM (array); i++)
     array[i] = NULL;
 
@@ -1304,8 +1304,6 @@ agent_public_key_from_file (ctrl_t ctrl,
   s_skey = NULL;
 
 
-  // TODO: the following FIXME is so true -- following code is
-  // prone to buffer overrun
   /* FIXME: The following thing is pretty ugly code; we should
      investigate how to make it cleaner.  Probably code to handle
      canonical S-expressions in a memory buffer is better suited for
@@ -1314,7 +1312,7 @@ agent_public_key_from_file (ctrl_t ctrl,
      them.  */
   assert (sizeof (size_t) <= sizeof (void*));
 
-  format = xtrymalloc (15+4+7*npkey+10+15+1+1+5+4096);
+  format = xtrymalloc (15+4+7*npkey+10+15+1+1+5+10);
   if (!format)
     {
       err = gpg_error_from_syserror ();
--
2.25.1


_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[GnuPG - Dev mailing list]

GnuPG-bug-id: https://dev.gnupg.org/T1756
Signed-off-by: Igor Okulist <[hidden email]>
---
 agent/command-ssh.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/agent/command-ssh.c b/agent/command-ssh.c
index 3983bbeb4..101bbc691 100644
--- a/agent/command-ssh.c
+++ b/agent/command-ssh.c
@@ -60,7 +60,7 @@
 #include "../common/ssh-utils.h"
 
 
-
+
 
 /* Request types. */
 #define SSH_REQUEST_REQUEST_IDENTITIES    11
@@ -1953,7 +1953,7 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
           err = b64dec_start (&b64s, NULL);
           err = err || b64dec_proc (&b64s, certificate, certificate_sexp_b64_len, &certificate_len);
           err = err || b64dec_finish (&b64s);
-          err = err | stream_write_data (stream, certificate, certificate_len);
+          err = err || stream_write_data (stream, certificate, certificate_len);
 
           xfree (certificate);
 
@@ -2299,7 +2299,7 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
       err = err || es_read (stream, b64_cert_buffer, b64_cert_buffer_len, &nread);
       if (err)
         goto out;
-      b64_cert_buffer[b64_cert_buffer_len] = "\0";
+      b64_cert_buffer[b64_cert_buffer_len] = '\0';
 
       es_fclose (stream);
 
--
2.25.1


_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[GnuPG - Dev mailing list]

Igor Okulist via Gnupg-devel wrote:

> [...]
> @@ -1304,8 +1304,6 @@ agent_public_key_from_file (ctrl_t ctrl,
>    s_skey = NULL;
>  
>  
> -  // TODO: the following FIXME is so true -- following code is
> -  // prone to buffer overrun
>    /* FIXME: The following thing is pretty ugly code; we should
>       investigate how to make it cleaner.  Probably code to handle
>       canonical S-expressions in a memory buffer is better suited for
> @@ -1314,7 +1312,7 @@ agent_public_key_from_file (ctrl_t ctrl,
>       them.  */
>    assert (sizeof (size_t) <= sizeof (void*));
>  
> -  format = xtrymalloc (15+4+7*npkey+10+15+1+1+5+4096);
> +  format = xtrymalloc (15+4+7*npkey+10+15+1+1+5+10);
>    if (!format)
>      {
>        err = gpg_error_from_syserror ();
>  

Are you sure about this?  Removing a comment that warns of possible
buffer overruns that need to be addressed without (as far as I can tell)
actually addressing the possible issue while also *reducing* the size of
an allocated buffer strikes me as odd.


-- Jacob


_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[GnuPG - Dev mailing list]

On Wed, 17 Mar 2021 17:39, Jacob Bachmeyer said:

> Are you sure about this?  Removing a comment that warns of possible
> buffer overruns that need to be addressed without (as far as I can

Anyway, I took the opportunity to rewrite that part to use a membuf.
Not a generic solution to create such s-expression but better than
fragile in-advance byte counting.


Shalom-Salam,

   Werner

--
Die Gedanken sind frei.  Ausnahmen regelt ein Bundesgesetz.

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[NIIBE Yutaka]

Igor Okulist wrote:
> This set of patches updates support for certificates and
> addresses (at least part of) https://dev.gnupg.org/T1756.
>
> With thes patches user shall be able to add RSA key and
> certificate to the gpg-agent and get a passwordless sign
> through signed certificates.

AFAIU, ssh-agent (or gpg-agent's ssh-agent emulation) has no way to
_use_ certificates, when transferred from ssh-add.

Please use -k option for ssh-add.  Then, no changes are required to
current implementation of gpg-agent.

Please let us know your use case(s), if it's real.
--

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[GnuPG - Dev mailing list]

On Thu, Mar 18, 2021 at 11:25 PM NIIBE Yutaka <[hidden email]> wrote:

>
> Igor Okulist wrote:
> > This set of patches updates support for certificates and
> > addresses (at least part of) https://dev.gnupg.org/T1756.
> >
> > With thes patches user shall be able to add RSA key and
> > certificate to the gpg-agent and get a passwordless sign
> > through signed certificates.
>
> AFAIU, ssh-agent (or gpg-agent's ssh-agent emulation) has no way to
> _use_ certificates, when transferred from ssh-add.
>
> Please use -k option for ssh-add.  Then, no changes are required to
> current implementation of gpg-agent.
>
> Please let us know your use case(s), if it's real.
> --

Thanks for review NIIBE,

You are absolutely right, but current functionality of gpg-agent does not allow
certificate based login. Here is a workflow (and test script) showing usage of
ssh-agent and gpg-agent and unfortunately it would not work with
gpg-agent as is.

So looking for a way to use gpg-agent with ssh and actually other tools as well,
the attached patch allowed it to work, but I would be curious if there
is another way to do that.

Regards,
Igor

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

[GnuPG - Dev mailing list]

The link to the workflow (and test script):
https://github.com/okigan/gnupg-workspace/blob/feature/tp-5487-on-2.2.24/issues/tp-5487/repro.sh#L76

On Fri, Mar 26, 2021 at 5:32 PM Igor Okulist <[hidden email]> wrote:

>
> On Thu, Mar 18, 2021 at 11:25 PM NIIBE Yutaka <[hidden email]> wrote:
> >
> > Igor Okulist wrote:
> > > This set of patches updates support for certificates and
> > > addresses (at least part of) https://dev.gnupg.org/T1756.
> > >
> > > With thes patches user shall be able to add RSA key and
> > > certificate to the gpg-agent and get a passwordless sign
> > > through signed certificates.
> >
> > AFAIU, ssh-agent (or gpg-agent's ssh-agent emulation) has no way to
> > _use_ certificates, when transferred from ssh-add.
> >
> > Please use -k option for ssh-add.  Then, no changes are required to
> > current implementation of gpg-agent.
> >
> > Please let us know your use case(s), if it's real.
> > --
>
>
> Thanks for review NIIBE,
>
> You are absolutely right, but current functionality of gpg-agent does not allow
> certificate based login. Here is a workflow (and test script) showing usage of
> ssh-agent and gpg-agent and unfortunately it would not work with
> gpg-agent as is.
>
> So looking for a way to use gpg-agent with ssh and actually other tools as well,
> the attached patch allowed it to work, but I would be curious if there
> is another way to do that.
>
> Regards,
> Igor

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel