Fix and simplify sign handling in mbedtls_mpi_read_string

Move the handling of the sign out of the base-specific loops. This
both simplifies the code, and corrects an edge case: the code in the
non-hexadecimal case depended on mbedtls_mpi_mul_int() preserving the
sign bit when multiplying a "negative zero" MPI by an integer, which
used to be the case but stopped with PR #2512.

Fix #4295. Thanks to Guido Vranken for analyzing the cause of the bug.
Credit to OSS-Fuzz.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This commit is contained in:
Gilles Peskine 2021-04-03 18:26:13 +02:00
parent 228b98f24f
commit 984fd07c53

View file

@ -500,6 +500,7 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s )
{ {
int ret; int ret;
size_t i, j, slen, n; size_t i, j, slen, n;
int sign = 1;
mbedtls_mpi_uint d; mbedtls_mpi_uint d;
mbedtls_mpi T; mbedtls_mpi T;
MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( X != NULL );
@ -510,6 +511,12 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s )
mbedtls_mpi_init( &T ); mbedtls_mpi_init( &T );
if( s[0] == '-' )
{
++s;
sign = -1;
}
slen = strlen( s ); slen = strlen( s );
if( radix == 16 ) if( radix == 16 )
@ -524,12 +531,6 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s )
for( i = slen, j = 0; i > 0; i--, j++ ) for( i = slen, j = 0; i > 0; i--, j++ )
{ {
if( i == 1 && s[i - 1] == '-' )
{
X->s = -1;
break;
}
MBEDTLS_MPI_CHK( mpi_get_digit( &d, radix, s[i - 1] ) ); MBEDTLS_MPI_CHK( mpi_get_digit( &d, radix, s[i - 1] ) );
X->p[j / ( 2 * ciL )] |= d << ( ( j % ( 2 * ciL ) ) << 2 ); X->p[j / ( 2 * ciL )] |= d << ( ( j % ( 2 * ciL ) ) << 2 );
} }
@ -540,26 +541,15 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s )
for( i = 0; i < slen; i++ ) for( i = 0; i < slen; i++ )
{ {
if( i == 0 && s[i] == '-' )
{
X->s = -1;
continue;
}
MBEDTLS_MPI_CHK( mpi_get_digit( &d, radix, s[i] ) ); MBEDTLS_MPI_CHK( mpi_get_digit( &d, radix, s[i] ) );
MBEDTLS_MPI_CHK( mbedtls_mpi_mul_int( &T, X, radix ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_int( &T, X, radix ) );
if( X->s == 1 )
{
MBEDTLS_MPI_CHK( mbedtls_mpi_add_int( X, &T, d ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_add_int( X, &T, d ) );
} }
else
{
MBEDTLS_MPI_CHK( mbedtls_mpi_sub_int( X, &T, d ) );
}
}
} }
if( sign < 0 && mbedtls_mpi_bitlen( X ) != 0 )
X->s = -1;
cleanup: cleanup:
mbedtls_mpi_free( &T ); mbedtls_mpi_free( &T );