Page MenuHome GnuPG

Incompatible Ed25519 secret key (no-encryption)
Closed, ResolvedPublic

Description

In master, we use SOS encoding for ECC.

Since (traditional) non-SOS handling removes preceding zero-bytes, in the MPI representation, NBITS will be shorter than input, which causes checksum error.

This is an example key:

Event Timeline

Note that there is no problem for encrypted key, because it is handled by opaque MPI.

Applying following SOS-handling, the key can be handled.

diff --git a/g10/parse-packet.c b/g10/parse-packet.c
index 9cb254e24..be7fc6d67 100644
--- a/g10/parse-packet.c
+++ b/g10/parse-packet.c
@@ -188,6 +188,76 @@ mpi_read (iobuf_t inp, unsigned int *ret_nread, int secure)
 }
 
 
+/* Read an external representation of an SOS and return the opaque MPI
+   with GCRYMPI_FLAG_USER2.  The external format is a 16-bit unsigned
+   value stored in network byte order giving information for the
+   following octets.
+
+   The caller must set *RET_NREAD to the maximum number of bytes to
+   read from the pipeline INP.  This function sets *RET_NREAD to be
+   the number of bytes actually read from the pipeline.
+
+   If SECURE is true, the integer is stored in secure memory
+   (allocated using gcry_xmalloc_secure).  */
+static gcry_mpi_t
+sos_read (iobuf_t inp, unsigned int *ret_nread, int secure)
+{
+  int c, c1, c2, i;
+  unsigned int nmax = *ret_nread;
+  unsigned int nbits, nbytes;
+  size_t nread = 0;
+  gcry_mpi_t a = NULL;
+  byte *buf = NULL;
+  byte *p;
+
+  if (!nmax)
+    goto overflow;
+
+  if ((c = c1 = iobuf_get (inp)) == -1)
+    goto leave;
+  if (++nread == nmax)
+    goto overflow;
+  nbits = c << 8;
+  if ((c = c2 = iobuf_get (inp)) == -1)
+    goto leave;
+  ++nread;
+  nbits |= c;
+  if (nbits > MAX_EXTERN_MPI_BITS)
+    {
+      log_error ("mpi too large (%u bits)\n", nbits);
+      goto leave;
+    }
+
+  nbytes = (nbits + 7) / 8;
+  buf = secure ? gcry_xmalloc_secure (nbytes) : gcry_xmalloc (nbytes);
+  p = buf;
+  for (i = 0; i < nbytes; i++)
+    {
+      if (nread == nmax)
+        goto overflow;
+
+      c = iobuf_get (inp);
+      if (c == -1)
+        goto leave;
+
+      p[i] = c;
+      nread ++;
+    }
+
+  a = gcry_mpi_set_opaque (NULL, buf, nbits);
+  gcry_mpi_set_flag (a, GCRYMPI_FLAG_USER2);
+  *ret_nread = nread;
+  return a;
+
+ overflow:
+  log_error ("mpi larger than indicated length (%u bits)\n", 8*nmax);
+ leave:
+  *ret_nread = nread;
+  gcry_free(buf);
+  return a;
+}
+
+
 /* Register STRING as a known critical notation name.  */
 void
 register_known_notation (const char *string)
@@ -2728,7 +2798,12 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
                   goto leave;
                 }
               n = pktlen;
-              pk->pkey[i] = mpi_read (inp, &n, 0);
+              if (algorithm == PUBKEY_ALGO_ECDSA
+                  || algorithm == PUBKEY_ALGO_EDDSA
+                  || algorithm == PUBKEY_ALGO_ECDH)
+                pk->pkey[i] = sos_read (inp, &n, 0);
+              else
+                pk->pkey[i] = mpi_read (inp, &n, 0);
               pktlen -= n;
               if (list_mode)
                 {

Need another patch to export it:

diff --git a/g10/export.c b/g10/export.c
index 8dd0b07d7..339424e19 100644
--- a/g10/export.c
+++ b/g10/export.c
@@ -627,6 +627,57 @@ canon_pk_algo (enum gcry_pk_algos algo)
 }
 
 
+/* Extract SOS representation from SEXP for PARAM, return the result
+   in R_SOS.  */
+static gpg_error_t
+sexp_extract_param_sos (gcry_sexp_t sexp, const char *param, gcry_mpi_t *r_sos)
+{
+  gpg_error_t err;
+  gcry_sexp_t l2 = gcry_sexp_find_token (sexp, param, 0);
+
+  *r_sos = NULL;
+  if (!l2)
+    err = gpg_error (GPG_ERR_NO_OBJ);
+  else
+    {
+      size_t buflen;
+      void *p0 = gcry_sexp_nth_buffer (l2, 1, &buflen);
+
+      if (!p0)
+        err = gpg_error_from_syserror ();
+      else
+        {
+          gcry_mpi_t sos;
+          unsigned int nbits = buflen*8;
+          unsigned char *p = p0;
+
+          if (*p && nbits >= 8 && !(*p & 0x80))
+            if (--nbits >= 7 && !(*p & 0x40))
+              if (--nbits >= 6 && !(*p & 0x20))
+                if (--nbits >= 5 && !(*p & 0x10))
+                  if (--nbits >= 4 && !(*p & 0x08))
+                    if (--nbits >= 3 && !(*p & 0x04))
+                      if (--nbits >= 2 && !(*p & 0x02))
+                        if (--nbits >= 1 && !(*p & 0x01))
+                          --nbits;
+
+          sos = gcry_mpi_set_opaque (NULL, p0, nbits);
+          if (sos)
+            {
+              gcry_mpi_set_flag (sos, GCRYMPI_FLAG_USER2);
+              *r_sos = sos;
+              err = 0;
+            }
+          else
+            err = gpg_error_from_syserror ();
+        }
+      gcry_sexp_release (l2);
+    }
+
+  return err;
+}
+
+
 /* Take a cleartext dump of a secret key in PK and change the
  * parameter array in PK to include the secret parameters.  */
 static gpg_error_t
@@ -764,9 +815,7 @@ cleartext_secret_key_to_openpgp (gcry_sexp_t s_key, PKT_public_key *pk)
         {
           gcry_mpi_release (pk->pkey[sec_start]);
           pk->pkey[sec_start] = NULL;
-          err = gcry_sexp_extract_param (key, NULL, "d",
-                                         &pk->pkey[sec_start],
-                                         NULL);
+          err = sexp_extract_param_sos (key, "d", &pk->pkey[sec_start]);
         }
 
       if (!err)
@@ -987,6 +1036,7 @@ transfer_format_to_openpgp (gcry_sexp_t s_pgp, PKT_public_key *pk)
           skey[skeyidx] = gcry_mpi_set_opaque (NULL, p, valuelen*8);
           if (!skey[skeyidx])
             goto outofmem;
+          gcry_mpi_set_flag (skey[skeyidx], GCRYMPI_FLAG_USER1);
         }
       else
         {
@@ -1144,7 +1194,7 @@ transfer_format_to_openpgp (gcry_sexp_t s_pgp, PKT_public_key *pk)
   /* Check that the first secret key parameter in SKEY is encrypted
      and that there are no more secret key parameters.  The latter is
      guaranteed by the v4 packet format.  */
-  if (!gcry_mpi_get_flag (skey[npkey], GCRYMPI_FLAG_OPAQUE))
+  if (!gcry_mpi_get_flag (skey[npkey], GCRYMPI_FLAG_USER1))
     goto bad_seckey;
   if (npkey+1 < DIM (skey) && skey[npkey+1])
     goto bad_seckey;
werner lowered the priority of this task from High to Normal.Nov 23 2020, 1:54 PM

gniibe: What's the state of this?

I guess this is solved. Feel free to re-open and schedule for 2.2.34

It's not yet solved.

I was waiting the OpenPGP format update.
https://datatracker.ietf.org/doc/html/draft-ietf-openpgp-crypto-refresh-04#section-9.2
It has been improved. But...

As of today, still there may be two possible interpretations:
(1) SOS (MPI with leading zero octets, which is "malformed" MPI) is allowed
(2) SOS is not allowed

My proposal is applying SOS (MPI with leading zero octets) patches, for 2.2, because there may be existing keys with SOS already.

For GnuPG 2.2, it's better to be conservative (least change of behavior, if any).

So, I am applying only a change to accept non-zero removal secret key (in clear text) for Ed25519.

GnuPG 2.2 keeps exporting such a key removing leading zero.

gniibe removed a project: Restricted Project.

The fix was not right, because gpg-agent side are not changed. See T5953.

To be conservative enough, don't introduce sos_read here, but detect secret key material MPI by SOS and update ski->csum when detected.

Thanks for working on this, @gniibe! Maybe it would be useful to add a test to the test suite that tries to import and use a secret key of this particular structure.

it would be useful to add a test

Yes. I will.

gniibe removed a project: Restricted Project.