From 03c165e1e1ce9e25a0883b28dbd05e249ec670e0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 10 Oct 2019 19:15:18 +0200 Subject: [PATCH 1/6] Fix the build and the tests when MBEDTLS_BIGNUM_C is unset When the asn1parse module is enabled but the bignum module is disabled, the asn1parse test suite did not work. Fix this. * Fix a syntax error in get_integer() (label immediately followed by a closing brace). * Fix an unused variable in get_integer(). * Fix `TEST_ASSERT( *p == q );` in nested_parse() failing because `*p` was not set. * Fix nested_parse() not outputting the length of what it parsed. --- tests/suites/test_suite_asn1parse.function | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index 3bfb1c7037..049763142c 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -59,6 +59,10 @@ static int nested_parse( unsigned char **const p, *p = start; ret = mbedtls_asn1_get_mpi( p, end, &mpi ); mbedtls_mpi_free( &mpi ); +#else + *p = start + 1; + ret = mbedtls_asn1_get_len( p, end, &len ); + *p += len; #endif /* If we're sure that the number fits in an int, also * call mbedtls_asn1_get_int(). */ @@ -254,10 +258,10 @@ void get_integer( const data_t *input, #if defined(MBEDTLS_BIGNUM_C) mbedtls_mpi expected_mpi; mbedtls_mpi actual_mpi; + int expected_result_for_mpi = expected_result; #endif long expected_value; int expected_result_for_int = expected_result; - int expected_result_for_mpi = expected_result; int val; int ret; @@ -310,6 +314,7 @@ exit: mbedtls_mpi_free( &expected_mpi ); mbedtls_mpi_free( &actual_mpi ); #endif + /*empty cleanup in some configurations*/ ; } /* END_CASE */ From 321adb297c1c19be2d2120658cd43929e5f3faa4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 10 Oct 2019 19:18:21 +0200 Subject: [PATCH 2/6] ASN1 tests: Match "Empty INTEGER" with the actual library behavior mbedtls_asn1_get_int() and mbedtls_asn1_get_mpi() behave differently on an empty INTEGER (0200). Don't change the library behavior for now because this might break interoperability in some applications. Write a test function that matches the library behavior. --- tests/suites/test_suite_asn1parse.data | 3 +- tests/suites/test_suite_asn1parse.function | 35 ++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_asn1parse.data b/tests/suites/test_suite_asn1parse.data index c5d9136b77..e9172413d4 100644 --- a/tests/suites/test_suite_asn1parse.data +++ b/tests/suites/test_suite_asn1parse.data @@ -164,8 +164,7 @@ Not BOOLEAN get_boolean:"020101":0:MBEDTLS_ERR_ASN1_UNEXPECTED_TAG Empty INTEGER -depends_on:SUPPORT_NEGATIVE_INTEGERS -get_integer:"0200":"":MBEDTLS_ERR_ASN1_INVALID_LENGTH +empty_integer:"0200" INTEGER 0 get_integer:"020100":"0":0 diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index 049763142c..f794db7fc5 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -250,6 +250,41 @@ void get_boolean( const data_t *input, } /* END_CASE */ +/* BEGIN_CASE */ +void empty_integer( const data_t *input ) +{ + unsigned char *p; +#if defined(MBEDTLS_BIGNUM_C) + mbedtls_mpi actual_mpi; +#endif + int val; + +#if defined(MBEDTLS_BIGNUM_C) + mbedtls_mpi_init( & actual_mpi ); +#endif + + /* An INTEGER with no content is not valid. */ + p = input->x; + TEST_EQUAL( mbedtls_asn1_get_int( &p, input->x + input->len, &val ), + MBEDTLS_ERR_ASN1_INVALID_LENGTH ); + +#if defined(MBEDTLS_BIGNUM_C) + /* INTEGERs are sometimes abused as bitstrings, so the library accepts + * an INTEGER with empty content and gives it the value 0. */ + p = input->x; + TEST_EQUAL( mbedtls_asn1_get_mpi( &p, input->x + input->len, &actual_mpi ), + 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &actual_mpi, 0 ), 0 ); +#endif + +exit: +#if defined(MBEDTLS_BIGNUM_C) + mbedtls_mpi_free( &actual_mpi ); +#endif + /*empty cleanup in some configurations*/ ; +} +/* END_CASE */ + /* BEGIN_CASE */ void get_integer( const data_t *input, const char *expected_hex, int expected_result ) From 970dcbf453489a10fa544ef3fe1aeb56e254eaef Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 10 Oct 2019 19:21:12 +0200 Subject: [PATCH 3/6] ASN1 tests: Match negative INTEGERs with the actual library behavior mbedtls_asn1_get_int() and mbedtls_asn1_get_mpi() behave differently on negative INTEGERs (0200). Don't change the library behavior for now because this might break interoperability in some applications. Change the test function to the library behavior. Fix the test data with negative INTEGERs. These test cases were previously not run (they were introduced but deliberately deactivated in 27d806fab41a11441d97017158fcb1356ef7e74f). The test data was actually wrong: ASN.1 uses two's complement, which has no negative 0, and some encodings were wrong. Now the tests have correct data, and the test code rectifies the expected data to match the library behavior. --- tests/suites/test_suite_asn1parse.data | 18 ++++------ tests/suites/test_suite_asn1parse.function | 42 +++++++++++++++++++++- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/tests/suites/test_suite_asn1parse.data b/tests/suites/test_suite_asn1parse.data index e9172413d4..10333d3edd 100644 --- a/tests/suites/test_suite_asn1parse.data +++ b/tests/suites/test_suite_asn1parse.data @@ -172,27 +172,15 @@ get_integer:"020100":"0":0 INTEGER 0, extra leading 0 get_integer:"02020000":"0":0 -INTEGER -0 -depends_on:SUPPORT_NEGATIVE_INTEGERS -get_integer:"020180":"0":0 - INTEGER 1 get_integer:"020101":"1":0: INTEGER 1, extra leading 0 get_integer:"02020001":"1":0: -INTEGER -1 -depends_on:SUPPORT_NEGATIVE_INTEGERS -get_integer:"020181":"-1":0 - INTEGER 0x7f get_integer:"02017f":"7f":0 -INTEGER -0x7f -depends_on:SUPPORT_NEGATIVE_INTEGERS -get_integer:"0201ff":"-7f":0 - INTEGER 0x80 get_integer:"02020080":"80":0 @@ -226,6 +214,12 @@ get_integer:"0281800123456789abcdef0123456789abcdef0123456789abcdef0123456789abc INTEGER with 128 value octets (leading 0 in length) get_integer:"028200800123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef":0 +INTEGER -1 +get_integer:"0201ff":"-1":0 + +INTEGER -0x7f +get_integer:"020181":"-7f":0 + Not INTEGER get_integer:"010101":"":MBEDTLS_ERR_ASN1_UNEXPECTED_TAG diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index f794db7fc5..defbd01bb6 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -293,6 +293,7 @@ void get_integer( const data_t *input, #if defined(MBEDTLS_BIGNUM_C) mbedtls_mpi expected_mpi; mbedtls_mpi actual_mpi; + mbedtls_mpi complement; int expected_result_for_mpi = expected_result; #endif long expected_value; @@ -303,6 +304,7 @@ void get_integer( const data_t *input, #if defined(MBEDTLS_BIGNUM_C) mbedtls_mpi_init( &expected_mpi ); mbedtls_mpi_init( &actual_mpi ); + mbedtls_mpi_init( &complement ); #endif errno = 0; @@ -314,6 +316,16 @@ void get_integer( const data_t *input, #endif ) ) { + /* The library returns the dubious error code INVALID_LENGTH + * for integers that are out of range. */ + expected_result_for_int = MBEDTLS_ERR_ASN1_INVALID_LENGTH; + } + if( expected_result == 0 && expected_value < 0 ) + { + /* The library does not support negative INTEGERs and + * returns the dubious error code INVALID_LENGTH. + * Test that we preserve the historical behavior. If we + * decide to change the behavior, we'll also change this test. */ expected_result_for_int = MBEDTLS_ERR_ASN1_INVALID_LENGTH; } @@ -339,7 +351,34 @@ void get_integer( const data_t *input, TEST_EQUAL( ret, expected_result_for_mpi ); if( ret == 0 ) { - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &actual_mpi , &expected_mpi ) == 0 ); + if( expected_value >= 0 ) + { + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &actual_mpi, + &expected_mpi ) == 0 ); + } + else + { + /* The library ignores the sign bit in ASN.1 INTEGERs + * (which makes sense insofar as INTEGERs are sometimes + * abused as bit strings), so the result of parsing them + * is a positive integer such that expected_mpi + + * actual_mpi = 2^n where n is the length of the content + * of the INTEGER. (Leading ff octets don't matter for the + * expected value, but they matter for the actual value.) + * Test that we don't change from this behavior. If we + * decide to fix the library to change the behavior on + * negative INTEGERs, we'll fix this test code. */ + unsigned char *q = input->x + 1; + size_t len; + TEST_ASSERT( mbedtls_asn1_get_len( &q, input->x + input->len, + &len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_lset( &complement, 1 ) == 0 ); + TEST_ASSERT( mbedtls_mpi_shift_l( &complement, len * 8 ) == 0 ); + TEST_ASSERT( mbedtls_mpi_add_mpi( &complement, &complement, + &expected_mpi ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &complement, + &actual_mpi ) == 0 ); + } TEST_ASSERT( p == input->x + input->len ); } #endif @@ -348,6 +387,7 @@ exit: #if defined(MBEDTLS_BIGNUM_C) mbedtls_mpi_free( &expected_mpi ); mbedtls_mpi_free( &actual_mpi ); + mbedtls_mpi_free( &complement ); #endif /*empty cleanup in some configurations*/ ; } From 0370b1bd7d1bc4714aede69e03eb5db3c77a8424 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 10 Oct 2019 19:25:39 +0200 Subject: [PATCH 4/6] ASN1 tests: more INTEGER test cases Test more INTEGER values, especially near the boundary of int (which is at 2^31-1 on all our officially supported platforms). --- tests/suites/test_suite_asn1parse.data | 60 ++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/suites/test_suite_asn1parse.data b/tests/suites/test_suite_asn1parse.data index 10333d3edd..4abae0bb4c 100644 --- a/tests/suites/test_suite_asn1parse.data +++ b/tests/suites/test_suite_asn1parse.data @@ -199,9 +199,30 @@ get_integer:"020412345678":"12345678":0 INTEGER 0x12345678, extra leading 0 get_integer:"02050012345678":"12345678":0 +INTEGER 0x7fffffff +get_integer:"02047fffffff":"7fffffff":0 + +INTEGER 0x7fffffff, extra leading 0 +get_integer:"0205007fffffff":"7fffffff":0 + +INTEGER 0x80000000 +get_integer:"02050080000000":"80000000":0 + +INTEGER 0xffffffff +get_integer:"020500ffffffff":"ffffffff":0 + +INTEGER 0x100000000 +get_integer:"02050100000000":"0100000000":0 + INTEGER 0x123456789abcdef0 get_integer:"0208123456789abcdef0":"123456789abcdef0":0 +INTEGER 0xfedcab9876543210 +get_integer:"020900fedcab9876543210":"fedcab9876543210":0 + +INTEGER 0x1fedcab9876543210 +get_integer:"020901fedcab9876543210":"1fedcab9876543210":0 + INTEGER with 127 value octets get_integer:"027f0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcd":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcd":0 @@ -217,9 +238,48 @@ get_integer:"028200800123456789abcdef0123456789abcdef0123456789abcdef0123456789a INTEGER -1 get_integer:"0201ff":"-1":0 +INTEGER -1, extra leading ff +get_integer:"0202ffff":"-1":0 + INTEGER -0x7f get_integer:"020181":"-7f":0 +INTEGER -0x80 +get_integer:"020180":"-80":0 + +INTEGER -0x81 +get_integer:"0202ff7f":"-81":0 + +INTEGER -0xff +get_integer:"0202ff01":"-ff":0 + +INTEGER -0x100 +get_integer:"0202ff00":"-100":0 + +INTEGER -0x7fffffff +get_integer:"020480000001":"-7fffffff":0 + +INTEGER -0x80000000 +get_integer:"020480000000":"-80000000":0 + +INTEGER -0x80000001 +get_integer:"0205ff7fffffff":"-80000001":0 + +INTEGER -0xffffffff +get_integer:"0205ff00000001":"-ffffffff":0 + +INTEGER -0x100000000 +get_integer:"0205ff00000000":"-100000000":0 + +INTEGER -0x123456789abcdef0 +get_integer:"0208edcba98765432110":"-123456789abcdef0":0 + +INTEGER -0xfedcba9876543210 +get_integer:"0209ff0123456789abcdf0":"-fedcba9876543210":0 + +INTEGER -0x1fedcab9876543210 +get_integer:"0209fe0123546789abcdf0":"-1fedcab9876543210":0 + Not INTEGER get_integer:"010101":"":MBEDTLS_ERR_ASN1_UNEXPECTED_TAG From 9fd9794d109ffbd7f9ebed92f4a90dc429e1a1df Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 10 Oct 2019 19:27:53 +0200 Subject: [PATCH 5/6] mbedtls_asn1_get_int: explain the logic No behavior change. --- library/asn1parse.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/library/asn1parse.c b/library/asn1parse.c index 4764ca4cbc..4f9d6aef3e 100644 --- a/library/asn1parse.c +++ b/library/asn1parse.c @@ -149,14 +149,22 @@ int mbedtls_asn1_get_int( unsigned char **p, if( ( ret = mbedtls_asn1_get_tag( p, end, &len, MBEDTLS_ASN1_INTEGER ) ) != 0 ) return( ret ); - if( len == 0 || ( **p & 0x80 ) != 0 ) + /* len==0 is malformed (0 must be represented as 020100). */ + if( len == 0 ) + return( MBEDTLS_ERR_ASN1_INVALID_LENGTH ); + /* This is a cryptography library. Reject negative integers. */ + if( ( **p & 0x80 ) != 0 ) return( MBEDTLS_ERR_ASN1_INVALID_LENGTH ); + /* Skip leading zeros. */ while( len > 0 && **p == 0 ) { ++( *p ); --len; } + + /* Reject integers that don't fit in an int. This code assumes that + * the int type has no padding bit. */ if( len > sizeof( int ) ) return( MBEDTLS_ERR_ASN1_INVALID_LENGTH ); From 37570e81528d3a1d7354ece12dfb972e5f576e39 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 10 Oct 2019 19:29:27 +0200 Subject: [PATCH 6/6] mbedtls_asn1_get_int: fix int overflow Fix a signed int overflow in mbedtls_asn1_get_int() for numbers between INT_MAX+1 and UINT_MAX (typically 0x80000000..0xffffffff). This was undefined behavior which in practice would typically have resulted in an incorrect value, but which may plausibly also have caused the postcondition (*p == initial<*p> + len) to be violated. Credit to OSS-Fuzz. --- library/asn1parse.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/asn1parse.c b/library/asn1parse.c index 4f9d6aef3e..412259e358 100644 --- a/library/asn1parse.c +++ b/library/asn1parse.c @@ -167,6 +167,8 @@ int mbedtls_asn1_get_int( unsigned char **p, * the int type has no padding bit. */ if( len > sizeof( int ) ) return( MBEDTLS_ERR_ASN1_INVALID_LENGTH ); + if( len == sizeof( int ) && ( **p & 0x80 ) != 0 ) + return( MBEDTLS_ERR_ASN1_INVALID_LENGTH ); *val = 0; while( len-- > 0 )