Curve25519 ECDH encryption issue

[NIIBE Yutaka]

On 04/08/2016 05:31 PM, Werner Koch wrote:
> On Fri,  8 Apr 2016 09:02, gniibe at fsij.org said:
> 
>> I think that expected GnuPG behavior in this case is returns an error,
>> not decrypting the message.  Is it right?
> 
> That is probably the best solution.  The other would be to detect the
> case, decrypt, and print a warning.  But that requires a new diagnostic
> string for a very rare case.  Thus I think it is better to return an
> error like GPG_ERR_BAD_MPI.

OK, we agree that it raises an error.

Then, the question is which should do, GnuPG or libgcrypt?

If it's GnuPG, it will be something like following; It's not yet final
version (as we need to implement recognizing ECC ECDH and handling of
detection of all zero accurately duplicating
_gcry_ecc_mont_decodepoint in libgcrypt).

In the mail of mine to libgcrypt, I said once it should be GnuPG to
validate the input (to be compatible to the implementation which
uses NaCl's API).

However, writing the code below, I'm inclined to have different
opinion now.

I think that it's better for libgcrypt to raise the error.

diff --git a/agent/pkdecrypt.c b/agent/pkdecrypt.c
index 8c09b8c..ad8d8c2 100644
--- a/agent/pkdecrypt.c
+++ b/agent/pkdecrypt.c
@@ -77,6 +77,36 @@ agent_pkdecrypt (ctrl_t ctrl, const char *desc_text,
       goto leave;
     }

+  /*
+   * Input validation for X25519 ECDH.
+   */
+  if (agent_is_cv25519_key (s_skey))
+    {
+      gcry_sexp_t data_e = NULL;
+      int i;
+
+      rc = sexp_extract_param (s_cipher, NULL, "e", &data_e, NULL);
+      if (rc)
+	goto leave;
+      rc = gcry_mpi_aprint (GCRYMPI_FMT_USG, &buf, &len, data_e);
+      if (rc)
+	goto leave;
+
+      /* Check if it's all zero.  */
+      if (buf[0] == 0x40 && len == 33)
+	for (i = 0; i < 32; i++)
+	  if (buf[i+1] != 0)
+	    break;
+
+      xfree (buf);
+      buf = NULL;
+      if (i == 33)
+	{ /* Return error when it's all zero.  */
+	  rc = gpg_error (GPG_ERR_INV_DATA);
+	  goto leave;
+	}
+    }
+
   if (shadow_info)
     { /* divert operation to the smartcard */

--