gpg: Fix false negatives in Ed25519 signature verification.

* g10/pkglue.c (pk_verify): Fix Ed25519 signatrue values.
* tests/openpgp/verify.scm (msg_ed25519_rshort): New
(msg_ed25519_sshort): New.
("Checking that a valid Ed25519 signature is verified as such"): New.
--

About one out of 256 signature won't verify due to stripped zero
bytes.  See the source comment for details.

Reported-by: Andre Heinecke
Signed-off-by: Werner Koch <[email protected]>

2 files changed, 128 insertions, 3 deletions

diff --git a/g10/pkglue.c b/g10/pkglue.c
index 7de2fa7d0..50de347d4 100644
--- a/g10/pkglue.c
+++ b/g10/pkglue.c
@@ -58,6 +58,7 @@ pk_verify (pubkey_algo_t pkalgo, gcry_mpi_t hash,
 {
   gcry_sexp_t s_sig, s_hash, s_pkey;
   int rc;
+  unsigned int neededfixedlen = 0;
 
   /* Make a sexp from pkey.  */
   if (pkalgo == PUBKEY_ALGO_DSA)
@@ -103,6 +104,9 @@ pk_verify (pubkey_algo_t pkalgo, gcry_mpi_t hash,
                                 curve, pkey[1]);
           xfree (curve);
         }
+
+      if (openpgp_oid_is_ed25519 (pkey[0]))
+        neededfixedlen = 256 / 8;
     }
   else
     return GPG_ERR_PUBKEY_ALGO;
@@ -144,11 +148,59 @@ pk_verify (pubkey_algo_t pkalgo, gcry_mpi_t hash,
     }
   else if (pkalgo == PUBKEY_ALGO_EDDSA)
     {
-      if (!data[0] || !data[1])
+      gcry_mpi_t r = data[0];
+      gcry_mpi_t s = data[1];
+      size_t rlen, slen, n;  /* (bytes) */
+      char buf[64];
+
+      log_assert (neededfixedlen <= sizeof buf);
+
+      if (!r || !s)
+        rc = gpg_error (GPG_ERR_BAD_MPI);
+      else if ((rlen = (gcry_mpi_get_nbits (r)+7)/8) > neededfixedlen || !rlen)
+        rc = gpg_error (GPG_ERR_BAD_MPI);
+      else if ((slen = (gcry_mpi_get_nbits (s)+7)/8) > neededfixedlen || !slen)
         rc = gpg_error (GPG_ERR_BAD_MPI);
       else
-        rc = gcry_sexp_build (&s_sig, NULL,
-                              "(sig-val(eddsa(r%M)(s%M)))", data[0], data[1]);
+        {
+          /* We need to fixup the length in case of leading zeroes.
+           * OpenPGP does not allow leading zeroes and the parser for
+           * the signature packet has no information on the use curve,
+           * thus we need to do it here.  We won't do it for opaque
+           * MPIs under the assumption that they are known to be fine;
+           * we won't see them here anyway but the check is anyway
+           * required.  Fixme: A nifty feature for gcry_sexp_build
+           * would be a format to left pad the value (e.g. "%*M"). */
+          rc = 0;
+
+          if (rlen < neededfixedlen
+              && !gcry_mpi_get_flag (r, GCRYMPI_FLAG_OPAQUE)
+              && !(rc=gcry_mpi_print (GCRYMPI_FMT_USG, buf, sizeof buf, &n, r)))
+            {
+              log_assert (n < neededfixedlen);
+              memmove (buf + (neededfixedlen - n), buf, n);
+              memset (buf, 0, neededfixedlen - n);
+              r = gcry_mpi_set_opaque_copy (NULL, buf, neededfixedlen * 8);
+            }
+          if (slen < neededfixedlen
+              && !gcry_mpi_get_flag (s, GCRYMPI_FLAG_OPAQUE)
+              && !(rc=gcry_mpi_print (GCRYMPI_FMT_USG, buf, sizeof buf, &n, s)))
+            {
+              log_assert (n < neededfixedlen);
+              memmove (buf + (neededfixedlen - n), buf, n);
+              memset (buf, 0, neededfixedlen - n);
+              s = gcry_mpi_set_opaque_copy (NULL, buf, neededfixedlen * 8);
+            }
+
+          if (!rc)
+            rc = gcry_sexp_build (&s_sig, NULL,
+                                  "(sig-val(eddsa(r%M)(s%M)))", r, s);
+
+          if (r != data[0])
+            gcry_mpi_release (r);
+          if (s != data[1])
+            gcry_mpi_release (s);
+        }
     }
   else if (pkalgo == PUBKEY_ALGO_ELGAMAL || pkalgo == PUBKEY_ALGO_ELGAMAL_E)
     {
diff --git a/tests/openpgp/verify.scm b/tests/openpgp/verify.scm
index de03db531..2f03027e7 100755
--- a/tests/openpgp/verify.scm
+++ b/tests/openpgp/verify.scm
@@ -236,6 +236,67 @@ FWIAQUplk7JWbyRKAJ92ZJyJpWfzb0yc1s7MY65r2qEHrg==
 ;; Two clear text signatures in a row
 (define msg_clsclss_asc_multiple (string-append msg_cls_asc msg_clss_asc))
 
+
+;; An Ed25519 cleartext message with an R parameter of only 247 bits
+;; so that the code to re-insert the stripped zero byte kicks in.  The
+;; S parameter has 253 bits but that does not strip a full byte.
+(define msg_ed25519_rshort "
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA256
+
+Dear Emily:
+	I'm still confused as to what groups articles should be posted
+to.  How about an example?
+		-- Still Confused
+
+Dear Still:
+	Ok.  Let's say you want to report that Gretzky has been traded from
+the Oilers to the Kings.  Now right away you might think rec.sport.hockey
+would be enough.  WRONG.  Many more people might be interested.  This is a
+big trade!  Since it's a NEWS article, it belongs in the news.* hierarchy
+as well.  If you are a news admin, or there is one on your machine, try
+news.admin.  If not, use news.misc.
+	The Oilers are probably interested in geology, so try sci.physics.
+He is a big star, so post to sci.astro, and sci.space because they are also
+interested in stars.  Next, his name is Polish sounding.  So post to
+soc.culture.polish.  But that group doesn't exist, so cross-post to
+news.groups suggesting it should be created.  With this many groups of
+interest, your article will be quite bizarre, so post to talk.bizarre as
+well.  (And post to comp.std.mumps, since they hardly get any articles
+there, and a \"comp\" group will propagate your article further.)
+	You may also find it is more fun to post the article once in each
+group.  If you list all the newsgroups in the same article, some newsreaders
+will only show the the article to the reader once!  Don't tolerate this.
+		-- Emily Postnews Answers Your Questions on Netiquette
+-----BEGIN PGP SIGNATURE-----
+
+iJEEARYIADoWIQSyHeq0+HX7PaQvHR0TlWNoKgINCgUCV772DhwccGF0cmljZS5s
+dW11bWJhQGV4YW1wbGUubmV0AAoJEBOVY2gqAg0KMAIA90EtUwAja0iJGpO91wyz
+GLh9pS5v495V0r94yU6uUyUA/RT/StyPWe1wbnEZuacZnLbUV6Yy/aTXCVAlxf0r
+TusO
+=vQ3f
+-----END PGP SIGNATURE-----
+")
+
+;; An Ed25519 cleartext message with an S parameter of only 248 bits
+;; so that the code to re-insert the stripped zero byte kicks in.
+(define msg_ed25519_sshort "
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA256
+
+All articles that coruscate with resplendence are not truly auriferous.
+-----BEGIN PGP SIGNATURE-----
+
+iJEEARYIADoWIQSyHeq0+HX7PaQvHR0TlWNoKgINCgUCV771QhwccGF0cmljZS5s
+dW11bWJhQGV4YW1wbGUubmV0AAoJEBOVY2gqAg0KHVEBAI66OPDYXKWO3r6SaFT+
+uxmh8x4ZerW41vMA9gkJ4AEKAPjoe/Z7fDqo1lCptIFutFAGbfNxcm/53prfx2fT
+GisM
+=L7sk
+-----END PGP SIGNATURE-----
+")
+
+
+
 ;; Fixme:  We need more tests with manipulated cleartext signatures.
 
 ;;
@@ -272,3 +333,15 @@ FWIAQUplk7JWbyRKAJ92ZJyJpWfzb0yc1s7MY65r2qEHrg==
 	   (pipe:spawn `(,@GPG --verify)))
 	  (error "verification succeded but should not")))
  '(bad_ls_asc bad_fols_asc bad_olsf_asc bad_ools_asc))
+
+
+;;; Need to import the ed25519 sample key used for
+;;; the next two tests.
+(call-check `(,@GPG --quiet --yes --import ,(in-srcdir key-file2)))
+(for-each-p
+ "Checking that a valid Ed25519 signature is verified as such"
+ (lambda (armored-file)
+   (pipe:do
+    (pipe:echo (eval armored-file (current-environment)))
+    (pipe:spawn `(,@GPG --verify))))
+ '(msg_ed25519_rshort msg_ed25519_sshort))