Fix mbedtls_mpi_random when N has leading zeros

mbedtls_mpi_random() uses mbedtls_mpi_cmp_mpi_ct(), which requires its
two arguments to have the same storage size. This was not the case
when the upper bound passed to mbedtls_mpi_random() had leading zero
limbs.

Fix this by forcing the result MPI to the desired size. Since this is
not what mbedtls_mpi_fill_random() does, don't call it from
mbedtls_mpi_random(), but instead call a new auxiliary function.

Add tests to cover this and other conditions with varying sizes for
the two arguments.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

library/bignum.c

@@ -2395,6 +2395,30 @@ cleanup:
     return( ret );
 }
 
+/* Fill X with n_bytes random bytes.
+ * X must already have room for those bytes.
+ * n_bytes must not be 0.
+ */
+static int mpi_fill_random_internal(
+    mbedtls_mpi *X, size_t n_bytes,
+    int (*f_rng)(void *, unsigned char *, size_t), void *p_rng )
+{
+    int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
+    const size_t limbs = CHARS_TO_LIMBS( n_bytes );
+    const size_t overhead = ( limbs * ciL ) - n_bytes;
+
+    if( X->n < limbs )
+        return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA );
+    MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) );
+    MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) );
+
+    MBEDTLS_MPI_CHK( f_rng( p_rng, (unsigned char *) X->p + overhead, n_bytes ) );
+    mpi_bigendian_to_host( X->p, limbs );
+
+cleanup:
+    return( ret );
+}
+
 /*
  * Fill X with size bytes of random.
  *
@@ -2408,8 +2432,6 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size,
 {
     int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
     size_t const limbs = CHARS_TO_LIMBS( size );
-    size_t const overhead = ( limbs * ciL ) - size;
-    unsigned char *Xp;
 
     MPI_VALIDATE_RET( X     != NULL );
     MPI_VALIDATE_RET( f_rng != NULL );
@@ -2421,12 +2443,10 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size,
         mbedtls_mpi_init( X );
         MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) );
     }
-    MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) );
+    if( size == 0 )
+        return( 0 );
 
-    Xp = (unsigned char*) X->p;
-    MBEDTLS_MPI_CHK( f_rng( p_rng, Xp + overhead, size ) );
-
-    mpi_bigendian_to_host( X->p, limbs );
+    ret = mpi_fill_random_internal( X, size, f_rng, p_rng );
 
 cleanup:
     return( ret );
@@ -2450,6 +2470,16 @@ int mbedtls_mpi_random( mbedtls_mpi *X,
     if( mbedtls_mpi_cmp_int( N, min ) <= 0 )
         return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA );
 
+    /* Ensure that target MPI has exactly the same number of limbs
+     * as the upper bound, even if the upper bound has leading zeros.
+     * This is necessary for the mbedtls_mpi_lt_mpi_ct() check. */
+    if( X->n != N->n )
+    {
+        mbedtls_mpi_free( X );
+        mbedtls_mpi_init( X );
+        MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, N->n ) );
+    }
+
     /*
      * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA)
      * when f_rng is a suitably parametrized instance of HMAC_DRBG:
@@ -2460,7 +2490,7 @@ int mbedtls_mpi_random( mbedtls_mpi *X,
      */
     do
     {
-        MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( X, n_bytes, f_rng, p_rng ) );
+        MBEDTLS_MPI_CHK( mpi_fill_random_internal( X, n_bytes, f_rng, p_rng ) );
         MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( X, 8 * n_bytes - n_bits ) );
 
         /*

tests/suites/test_suite_mpi.data

@@ -1132,6 +1132,33 @@ mpi_random_many:1:"04":1000
 MPI random in range: 3..4
 mpi_random_many:1:"04":1000
 
+MPI random in range: smaller result
+mpi_random_grown:1:"aaaaaaaaaaaaaaaabbbbbbbbbbbbbbbb":1
+
+MPI random in range: same size result (32-bit limbs)
+mpi_random_grown:1:"aaaaaaaaaaaaaaaa":2
+
+MPI random in range: same size result (64-bit limbs)
+mpi_random_grown:1:"aaaaaaaaaaaaaaaa":1
+
+MPI random in range: larger result
+mpi_random_grown:1:"aaaaaaaaaaaaaaaa":3
+
+MPI random in range: leading 0 limb in upper bound #0
+mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":0
+
+MPI random in range: leading 0 limb in upper bound #1
+mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":1
+
+MPI random in range: leading 0 limb in upper bound #2
+mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":2
+
+MPI random in range: leading 0 limb in upper bound #3
+mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":3
+
+MPI random in range: leading 0 limb in upper bound #4
+mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":4
+
 MPI random bad arguments: min < 0
 mpi_random_fail:-1:"04":MBEDTLS_ERR_MPI_BAD_INPUT_DATA
 

tests/suites/test_suite_mpi.function

@@ -1537,6 +1537,29 @@ exit:
 }
 /* END_CASE */
 
+/* BEGIN_CASE */
+void mpi_random_grown( int min, data_t *bound_bytes, int nlimbs )
+{
+    mbedtls_mpi upper_bound;
+    mbedtls_mpi result;
+
+    mbedtls_mpi_init( &upper_bound );
+    mbedtls_mpi_init( &result );
+
+    TEST_EQUAL( 0, mbedtls_mpi_grow( &result, nlimbs ) );
+    TEST_EQUAL( 0, mbedtls_mpi_read_binary( &upper_bound,
+                                            bound_bytes->x, bound_bytes->len ) );
+    TEST_EQUAL( 0, mbedtls_mpi_random( &result, min, &upper_bound,
+                                       mbedtls_test_rnd_std_rand, NULL ) );
+    TEST_ASSERT( mbedtls_mpi_cmp_mpi( &result, &upper_bound ) < 0 );
+    TEST_ASSERT( mbedtls_mpi_cmp_int( &result, min ) >= 0 );
+
+exit:
+    mbedtls_mpi_free( &upper_bound );
+    mbedtls_mpi_free( &result );
+}
+/* END_CASE */
+
 /* BEGIN_CASE */
 void mpi_random_fail( int min, data_t *bound_bytes, int expected_ret )
 {