From ee95f6c4c9726eed26d6cfbf19ce5f3125e9f991 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Sun, 9 Jan 2022 05:46:18 +0000 Subject: [PATCH] Don't allow Z coordinate being unset in ecp_add_mixed() Previously, ecp_add_mixed(), commputing say P+Q, would allow for the Q parameter to have an unset Z coordinate as a shortcut for Z == 1. This was leveraged during computation and usage of the T-table (storing low multiples of the to-be-multiplied point on the curve). It is a potentially error-prone corner case, though, since an MPIs with unset data pointer coordinate and limb size 0 is also a valid representation of the number 0. As a first step towards removing ECP points with unset Z coordinate, the constant time T-array getter ecp_select_comb() has previously been modified to return 'full' mbedtls_ecp_point structures, including a 1-initialized Z-coordinate. Similarly, this commit ... - Modifies ecp_normalize_jac_many() to set the Z coordinates of the points it operates on to 1 instead of freeing them. - Frees the Z-coordinates of the T[]-array explicitly once the computation and normalization of the T-table has finished. As a minimal functional difference between old and new code, the new code also frees the Z-coordinate of T[0]=P, which the old code did not. - Modifies ecp_add_mixed() to no longer allow unset Z coordinates. Except for the post-precomputation storage form of the T[] array, the code does therefore no longer use EC points with unset Z coordinate. Signed-off-by: Hanno Becker --- library/ecp.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 47ac5c0903..f5ae8ee471 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1386,7 +1386,8 @@ static int ecp_normalize_jac_many( const mbedtls_ecp_group *grp, */ MBEDTLS_MPI_CHK( mbedtls_mpi_shrink( &T[i]->X, grp->P.n ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_shrink( &T[i]->Y, grp->P.n ) ); - mbedtls_mpi_free( &T[i]->Z ); + + MPI_ECP_LSET( &T[i]->Z, 1 ); if( i == 0 ) break; @@ -1529,8 +1530,6 @@ cleanup: * due to the choice of precomputed points in the modified comb method. * So branches for these cases do not leak secret information. * - * We accept Q->Z being unset (saving memory in tables) as meaning 1. - * * Cost: 1A := 8M + 3S */ static int ecp_add_mixed( const mbedtls_ecp_group *grp, mbedtls_ecp_point *R, @@ -1558,19 +1557,22 @@ static int ecp_add_mixed( const mbedtls_ecp_group *grp, mbedtls_ecp_point *R, mbedtls_mpi * const Y = &R->Y; mbedtls_mpi * const Z = &R->Z; + if( !MPI_ECP_VALID( &Q->Z ) ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + /* * Trivial cases: P == 0 or Q == 0 (case 1) */ if( MPI_ECP_CMP_INT( &P->Z, 0 ) == 0 ) return( mbedtls_ecp_copy( R, Q ) ); - if( MPI_ECP_VALID( &Q->Z ) && MPI_ECP_CMP_INT( &Q->Z, 0 ) == 0 ) + if( MPI_ECP_CMP_INT( &Q->Z, 0 ) == 0 ) return( mbedtls_ecp_copy( R, P ) ); /* * Make sure Q coordinates are normalized */ - if( MPI_ECP_VALID( &Q->Z ) && MPI_ECP_CMP_INT( &Q->Z, 1 ) != 0 ) + if( MPI_ECP_CMP_INT( &Q->Z, 1 ) != 0 ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); MPI_ECP_SQR( &tmp[0], &P->Z ); @@ -1918,6 +1920,14 @@ norm_add: MBEDTLS_MPI_CHK( ecp_normalize_jac_many( grp, TT, j ) ); + /* Free Z coordinate (=1 after normalization) to save RAM. + * This makes T[i] invalid as mbedtls_ecp_points, but this is OK + * since from this point onwards, they are only accessed indirectly + * via the getter function ecp_select_comb() which does set the + * target's Z coordinate to 1. */ + for( i = 0; i < T_size; i++ ) + mbedtls_mpi_free( &T[i].Z ); + cleanup: mbedtls_mpi_free( &tmp[0] );