ecc: Fix corner cases for X25519.
authorNIIBE Yutaka <gniibe@fsij.org>
Wed, 13 Apr 2016 01:10:53 +0000 (10:10 +0900)
committerNIIBE Yutaka <gniibe@fsij.org>
Wed, 13 Apr 2016 01:26:01 +0000 (10:26 +0900)
* cipher/ecc.c (ecc_encrypt_raw): For invalid input, returns
GPG_ERR_INV_DATA instead of aborting with log_fatal.  For X25519,
it's not an error, thus, let it return 0.
(ecc_decrypt_raw): Use the flag PUBKEY_FLAG_DJB_TWEAK to distinguish
X25519, not by the name of the curve.
(ecc_decrypt_raw): For invalid input, returns GPG_ERR_INV_DATA instead
of aborting with log_fatal.  For X25519, it's not an error by its
definition, but we deliberately let it return the error to detect
looks-like-encrypted-message.
* tests/t-cv25519.c: Add points to record the issue.

--

For X25519 ECDH, this change introduces incompatibility to
crypto_scalarmult with the input which makes shared secret to be 0.
For crypto_scalarmult, the result is 0.  In libgcrypt, it's an error
of GPG_ERR_INV_DATA (we consider the input is invalid).

Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>
cipher/ecc.c
tests/t-cv25519.c

index f6b2b69..a437a1f 100644 (file)
@@ -1395,7 +1395,23 @@ ecc_encrypt_raw (gcry_sexp_t *r_ciph, gcry_sexp_t s_data, gcry_sexp_t keyparms)
     _gcry_mpi_ec_mul_point (&R, data, &pk.Q, ec);
 
     if (_gcry_mpi_ec_get_affine (x, y, &R, ec))
-      log_fatal ("ecdh: Failed to get affine coordinates for kdG\n");
+      {
+        /*
+         * Here, X is 0.  In the X25519 computation on Curve25519, X0
+         * function maps infinity to zero.  So, when PUBKEY_FLAG_DJB_TWEAK
+         * is enabled, return the result of 0 not raising an error.
+         *
+         * This is a corner case.  It never occurs with properly
+         * generated public keys, but it might happen with blindly
+         * imported public key which might not follow the key
+         * generation procedure.
+         */
+        if (!(flags & PUBKEY_FLAG_DJB_TWEAK))
+          { /* It's not for X25519, then, the input data was simply wrong.  */
+            rc = GPG_ERR_INV_DATA;
+            goto leave;
+          }
+      }
     if (y)
       mpi_s = _gcry_ecc_ec2os (x, y, pk.E.p);
     else
@@ -1416,7 +1432,10 @@ ecc_encrypt_raw (gcry_sexp_t *r_ciph, gcry_sexp_t s_data, gcry_sexp_t keyparms)
     _gcry_mpi_ec_mul_point (&R, data, &pk.E.G, ec);
 
     if (_gcry_mpi_ec_get_affine (x, y, &R, ec))
-      log_fatal ("ecdh: Failed to get affine coordinates for kG\n");
+      {
+        rc = GPG_ERR_INV_DATA;
+        goto leave;
+      }
     if (y)
       mpi_e = _gcry_ecc_ec2os (x, y, pk.E.p);
     else
@@ -1598,8 +1617,8 @@ ecc_decrypt_raw (gcry_sexp_t *r_plain, gcry_sexp_t s_data, gcry_sexp_t keyparms)
   if (DBG_CIPHER)
     log_printpnt ("ecc_decrypt    kG", &kG, NULL);
 
-  if (!(curvename && !strcmp (curvename, "Curve25519"))
-      /* For Curve25519, by its definition, validation should not be done.  */
+  if (!(flags & PUBKEY_FLAG_DJB_TWEAK)
+      /* For X25519, by its definition, validation should not be done.  */
       && !_gcry_mpi_ec_curve_point (&kG, ec))
     {
       rc = GPG_ERR_INV_DATA;
@@ -1619,14 +1638,32 @@ ecc_decrypt_raw (gcry_sexp_t *r_plain, gcry_sexp_t s_data, gcry_sexp_t keyparms)
     else
       y = mpi_new (0);
 
-    /*
-     * Here, x is 0.  In the X25519 computation on Curve25519, X0
-     * function maps infinity to zero.  So, when PUBKEY_FLAG_DJB_TWEAK
-     * is enabled, we can just skip the check to get the result of 0.
-     */
-    if (_gcry_mpi_ec_get_affine (x, y, &R, ec)
-        && !(flags & PUBKEY_FLAG_DJB_TWEAK))
-      log_fatal ("ecdh: Failed to get affine coordinates\n");
+    if (_gcry_mpi_ec_get_affine (x, y, &R, ec))
+      {
+        rc = GPG_ERR_INV_DATA;
+        goto leave;
+        /*
+         * Note for X25519.
+         *
+         * By the definition of X25519, this is the case where X25519
+         * returns 0, mapping infinity to zero.  However, we
+         * deliberately let it return an error.
+         *
+         * For X25519 ECDH, comming here means that it might be
+         * decrypted by anyone with the shared secret of 0 (the result
+         * of this function could be always 0 by other scalar values,
+         * other than the private key of SK.D).
+         *
+         * So, it looks like an encrypted message but it can be
+         * decrypted by anyone, or at least something wrong
+         * happens.  Recipient should not proceed as if it were
+         * properly encrypted message.
+         *
+         * This handling is needed for our major usage of GnuPG,
+         * where it does the One-Pass Diffie-Hellman method,
+         * C(1, 1, ECC CDH), with an ephemeral key.
+         */
+      }
 
     if (y)
       r = _gcry_ecc_ec2os (x, y, sk.E.p);
index 7e551c2..098c66a 100644 (file)
@@ -32,7 +32,7 @@
 #include "stopwatch.h"
 
 #define PGM "t-cv25519"
-#define N_TESTS 6
+#define N_TESTS 18
 
 #define my_isascii(c) (!((c) & 0x80))
 #define digitp(p)   (*(p) >= '0' && *(p) <= '9')
@@ -499,6 +499,80 @@ check_cv25519 (void)
            "4a5d9d5ba4ce2de1728e3bf480350f25e07e21c947d19e3376f09b3c1e161742");
   ntests++;
 
+  /* Seven tests which results 0. */
+  test_cv (7,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "0000000000000000000000000000000000000000000000000000000000000000",
+           "0000000000000000000000000000000000000000000000000000000000000000");
+  ntests++;
+
+  test_cv (8,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "0100000000000000000000000000000000000000000000000000000000000000",
+           "0000000000000000000000000000000000000000000000000000000000000000");
+  ntests++;
+
+  test_cv (9,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "e0eb7a7c3b41b8ae1656e3faf19fc46ada098deb9c32b1fd866205165f49b800",
+           "0000000000000000000000000000000000000000000000000000000000000000");
+  ntests++;
+
+  test_cv (10,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "5f9c95bca3508c24b1d0b1559c83ef5b04445cc4581c8e86d8224eddd09f1157",
+           "0000000000000000000000000000000000000000000000000000000000000000");
+  ntests++;
+
+  test_cv (11,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "ecffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f",
+           "0000000000000000000000000000000000000000000000000000000000000000");
+  ntests++;
+
+  test_cv (12,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "edffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f",
+           "0000000000000000000000000000000000000000000000000000000000000000");
+  ntests++;
+
+  test_cv (13,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "eeffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f",
+           "0000000000000000000000000000000000000000000000000000000000000000");
+  ntests++;
+
+  /* Five tests which resulted 0 if decodeUCoordinate didn't change MSB. */
+  test_cv (14,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "cdeb7a7c3b41b8ae1656e3faf19fc46ada098deb9c32b1fd866205165f49b880",
+           "7ce548bc4919008436244d2da7a9906528fe3a6d278047654bd32d8acde9707b");
+  ntests++;
+
+  test_cv (15,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "4c9c95bca3508c24b1d0b1559c83ef5b04445cc4581c8e86d8224eddd09f11d7",
+           "e17902e989a034acdf7248260e2c94cdaf2fe1e72aaac7024a128058b6189939");
+  ntests++;
+
+  test_cv (16,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "d9ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
+           "ea6e6ddf0685c31e152d5818441ac9ac8db1a01f3d6cb5041b07443a901e7145");
+  ntests++;
+
+  test_cv (17,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "daffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
+           "845ddce7b3a9b3ee01a2f1fd4282ad293310f7a232cbc5459fb35d94bccc9d05");
+  ntests++;
+
+  test_cv (18,
+           "a546e36bf0527c9d3b16154b82465edd62144c0ac1fc5a18506a2244ba449ac4",
+           "dbffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
+           "6989e2cb1cea159acf121b0af6bf77493189c9bd32c2dac71669b540f9488247");
+  ntests++;
+
   if (ntests != N_TESTS)
     fail ("did %d tests but expected %d", ntests, N_TESTS);
   else if ((ntests % 256))