Tighten ecp_mul() validity checks

include/polarssl/ecp.h

@@ -459,22 +459,19 @@ int ecp_sub( const ecp_group *grp, ecp_point *R,
  * \param p_rng     RNG parameter
  *
  * \return          0 if successful,
+ *                  POLARSSL_ERR_ECP_INVALID_KEY if m is not a valid privkey
+ *                  or P is not a valid pubkey,
  *                  POLARSSL_ERR_MPI_MALLOC_FAILED if memory allocation failed
- *                  POLARSSL_ERR_ECP_BAD_INPUT_DATA if m < 0 of m has greater
- *                  bit length than N, the number of points in the group.
  *
  * \note            In order to prevent simple timing attacks, this function
  *                  executes a constant number of operations (that is, point
  *                  doubling and addition of distinct points) for random m in
  *                  the allowed range.
  *
- * \note            If f_rng is not NULL, it is used to randomize projective
+ * \note            If f_rng is not NULL, it is used to randomize intermediate
- *                  coordinates of indermediate results, in order to prevent
+ *                  results in order to prevent potential attacks targetting
- *                  more elaborate timing attacks relying on intermediate
+ *                  these results. It is recommended to always provide a
- *                  operations. (This is a prophylactic measure since no such
+ *                  non-NULL f_rng (the overhead is negligible).
- *                  attack has been published yet.) Since this contermeasure
- *                  has very low overhead, it is recommended to always provide
- *                  a non-NULL f_rng parameter when using secret inputs.
  */
 int ecp_mul( ecp_group *grp, ecp_point *R,
              const mpi *m, const ecp_point *P,

library/ecp.c

@@ -1440,21 +1440,24 @@ int ecp_mul( ecp_group *grp, ecp_point *R,
     ecp_point Q, *T = NULL, S[2];
     mpi M;
 
-    if( mpi_cmp_int( m, 0 ) < 0 || mpi_msb( m ) > grp->nbits )
+    /*
-        return( POLARSSL_ERR_ECP_BAD_INPUT_DATA );
+     * Sanity checks (before we even initialize anything)
+     */
+    if( ( ret = ecp_check_privkey( grp, m ) ) != 0 )
+        return( ret );
+
+    /* We'll need this later, but do it now to possibly avoid cheking P */
+    p_eq_g = ( mpi_cmp_int( &P->Z, 1 ) == 0 &&
+               mpi_cmp_mpi( &P->Y, &grp->G.Y ) == 0 &&
+               mpi_cmp_mpi( &P->X, &grp->G.X ) == 0 );
+    if( ! p_eq_g && ( ret = ecp_check_pubkey( grp, P ) ) != 0 )
+        return( ret );
 
     mpi_init( &M );
     ecp_point_init( &Q );
     ecp_point_init( &S[0] );
     ecp_point_init( &S[1] );
 
-    /*
-     * Check if P == G
-     */
-    p_eq_g = ( mpi_cmp_int( &P->Z, 1 ) == 0 &&
-               mpi_cmp_mpi( &P->Y, &grp->G.Y ) == 0 &&
-               mpi_cmp_mpi( &P->X, &grp->G.X ) == 0 );
-
     /*
      * Minimize the number of multiplications, that is minimize
      * 10 * d * w + 18 * 2^(w-1) + 11 * d + 7 * w
@@ -2061,13 +2064,12 @@ int ecp_self_test( int verbose )
     /* exponents especially adapted for secp192r1 */
     const char *exponents[] =
     {
-        "000000000000000000000000000000000000000000000000", /* zero */
         "000000000000000000000000000000000000000000000001", /* one */
-        "FFFFFFFFFFFFFFFFFFFFFFFF99DEF836146BC9B1B4D22831", /* N */
+        "FFFFFFFFFFFFFFFFFFFFFFFF99DEF836146BC9B1B4D22830", /* N - 1 */
         "5EA6F389A38B8BC81E767753B15AA5569E1782E30ABE7D25", /* random */
-        "400000000000000000000000000000000000000000000000",
+        "400000000000000000000000000000000000000000000000", /* one and zeros */
-        "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF",
+        "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF", /* all ones */
-        "555555555555555555555555555555555555555555555555",
+        "555555555555555555555555555555555555555555555555", /* 101010... */
     };
 
     ecp_group_init( &grp );

tests/suites/test_suite_ecp.data

@@ -50,10 +50,10 @@ ECP small subtraction #9
 ecp_small_sub:0:"14":"11":0:"14":"36":0:27:30
 
 ECP small multiplication negative
-ecp_small_mul:-1:0:0:0:POLARSSL_ERR_ECP_BAD_INPUT_DATA
+ecp_small_mul:-1:0:0:0:POLARSSL_ERR_ECP_INVALID_KEY
 
 ECP small multiplication #0
-ecp_small_mul:0:1:0:0:0
+ecp_small_mul:0:1:0:0:POLARSSL_ERR_ECP_INVALID_KEY
 
 ECP small multiplication #1
 ecp_small_mul:1:0:17:42:0
@@ -92,16 +92,10 @@ ECP small multiplication #12
 ecp_small_mul:12:0:17:05:0
 
 ECP small multiplication #13
-ecp_small_mul:13:1:0:0:0
+ecp_small_mul:13:1:0:0:POLARSSL_ERR_ECP_INVALID_KEY
 
 ECP small multiplication #14
-ecp_small_mul:1:0:17:42:0
+ecp_small_mul:14:0:17:42:POLARSSL_ERR_ECP_INVALID_KEY
-
-ECP small multiplication #15
-ecp_small_mul:2:0:20:01:0
-
-ECP small multiplication too big
-ecp_small_mul:-1:0:0:0:POLARSSL_ERR_ECP_BAD_INPUT_DATA
 
 ECP small check pubkey #1
 ecp_small_check_pub:1:1:0:POLARSSL_ERR_ECP_INVALID_KEY

tests/suites/test_suite_ecp.function

@@ -115,12 +115,15 @@ void ecp_small_mul( int m_str, int r_zero, int x_r, int y_r, int ret )
 
     TEST_ASSERT( ecp_mul( &grp, &R, &m, &grp.G, NULL, NULL ) == ret );
 
-    if( r_zero )
+    if( ret == 0 )
-        TEST_ASSERT( mpi_cmp_int( &R.Z, 0 ) == 0 );
-    else
     {
-        TEST_ASSERT( mpi_cmp_int( &R.X, x_r ) == 0 );
+        if( r_zero )
-        TEST_ASSERT( mpi_cmp_int( &R.Y, y_r ) == 0 );
+            TEST_ASSERT( mpi_cmp_int( &R.Z, 0 ) == 0 );
+        else
+        {
+            TEST_ASSERT( mpi_cmp_int( &R.X, x_r ) == 0 );
+            TEST_ASSERT( mpi_cmp_int( &R.Y, y_r ) == 0 );
+        }
     }
 
     /* try again with randomization */
@@ -129,12 +132,15 @@ void ecp_small_mul( int m_str, int r_zero, int x_r, int y_r, int ret )
     TEST_ASSERT( ecp_mul( &grp, &R, &m, &grp.G,
                           &rnd_pseudo_rand, &rnd_info ) == ret );
 
-    if( r_zero )
+    if( ret == 0 )
-        TEST_ASSERT( mpi_cmp_int( &R.Z, 0 ) == 0 );
-    else
     {
-        TEST_ASSERT( mpi_cmp_int( &R.X, x_r ) == 0 );
+        if( r_zero )
-        TEST_ASSERT( mpi_cmp_int( &R.Y, y_r ) == 0 );
+            TEST_ASSERT( mpi_cmp_int( &R.Z, 0 ) == 0 );
+        else
+        {
+            TEST_ASSERT( mpi_cmp_int( &R.X, x_r ) == 0 );
+            TEST_ASSERT( mpi_cmp_int( &R.Y, y_r ) == 0 );
+        }
     }
 
     ecp_group_free( &grp );