From 9a3cf3174ddf479ad8c534d04297d153bee753fc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 27 Jan 2021 22:24:30 +0100 Subject: [PATCH 1/2] Add mpi_sub_abs negative tests with a larger-in-size second operand Add test cases for mbedtls_mpi_sub_abs() where the second operand has more limbs than the first operand (which, if the extra limbs are not all zero, implies that the function returns MBEDTLS_ERR_MPI_NEGATIVE_VALUE). This exposes a buffer overflow (reported in #4042). Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 6c520eb44..5229253f6 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -472,18 +472,30 @@ mbedtls_mpi_add_int:10:"20395687835640197740576586692903457728019399331434826309 Test mbedtls_mpi_add_int #2 mbedtls_mpi_add_int:10:"2039568783564019774057658669290345772801939933143482630947726464532830627227012776329":-9871232:10:"2039568783564019774057658669290345772801939933143482630947726464532830627227002905097" -Base test mbedtls_mpi_sub_abs #1 (Test with larger second input) +Base test mbedtls_mpi_sub_abs #1 (|B| > |A|) mbedtls_mpi_sub_abs:10:"5":10:"7":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE -Base test mbedtls_mpi_sub_abs #2 (Test with larger second input) +Base test mbedtls_mpi_sub_abs #2 (|B| > |A|) mbedtls_mpi_sub_abs:10:"-5":10:"-7":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE -Base test mbedtls_mpi_sub_abs #3 (Test with larger second input) +Base test mbedtls_mpi_sub_abs #3 (|B| > |A|) mbedtls_mpi_sub_abs:10:"-5":10:"7":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE -Base test mbedtls_mpi_sub_abs #4 (Test with larger second input) +Base test mbedtls_mpi_sub_abs #4 (|B| > |A|) mbedtls_mpi_sub_abs:10:"5":10:"-7":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE +Base test mbedtls_mpi_sub_abs #1 (|B| >> |A| with more limbs) +mbedtls_mpi_sub_abs:10:"5":16:"123456789abcdef01":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE + +Base test mbedtls_mpi_sub_abs #2 (|B| >> |A| with more limbs) +mbedtls_mpi_sub_abs:10:"-5":16:"-123456789abcdef01":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE + +Base test mbedtls_mpi_sub_abs #3 (|B| >> |A| with more limbs) +mbedtls_mpi_sub_abs:10:"-5":16:"123456789abcdef01":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE + +Base test mbedtls_mpi_sub_abs #4 (|B| >> |A| with more limbs) +mbedtls_mpi_sub_abs:10:"5":16:"-123456789abcdef01":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE + Base test mbedtls_mpi_sub_abs #1 mbedtls_mpi_sub_abs:10:"7":10:"5":10:"2":0 From 6260b707170ec21c88b627ad8ef31072b92f79e2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 27 Jan 2021 22:30:43 +0100 Subject: [PATCH 2/2] mbedtls_mpi_sub_abs: fix buffer overflow in error case Fix a buffer overflow in mbedtls_mpi_sub_abs() when calculating |A| - |B| where |B| is larger than |A| and has more limbs (so the function should return MBEDTLS_ERR_MPI_NEGATIVE_VALUE). Fix #4042 Signed-off-by: Gilles Peskine --- ChangeLog.d/mpi_sub_abs.txt | 7 +++++++ library/bignum.c | 6 ++++++ 2 files changed, 13 insertions(+) create mode 100644 ChangeLog.d/mpi_sub_abs.txt diff --git a/ChangeLog.d/mpi_sub_abs.txt b/ChangeLog.d/mpi_sub_abs.txt new file mode 100644 index 000000000..9f34ee74b --- /dev/null +++ b/ChangeLog.d/mpi_sub_abs.txt @@ -0,0 +1,7 @@ +Security + * Fix a buffer overflow in mbedtls_mpi_sub_abs() when calculating + |A| - |B| where |B| is larger than |A| and has more limbs (so the + function should return MBEDTLS_ERR_MPI_NEGATIVE_VALUE). Only + applications calling mbedtls_mpi_sub_abs() directly are affected: + all calls inside the library were safe since this function is + only called with |A| >= |B|. Reported by Guido Vranken in #4042. diff --git a/library/bignum.c b/library/bignum.c index 2feb727d8..f133f6c13 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1354,6 +1354,12 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi for( n = B->n; n > 0; n-- ) if( B->p[n - 1] != 0 ) break; + if( n > A->n ) + { + /* B >= (2^ciL)^n > A */ + ret = MBEDTLS_ERR_MPI_NEGATIVE_VALUE; + goto cleanup; + } carry = mpi_sub_hlp( n, X->p, B->p ); if( carry != 0 )