From 970dcbf453489a10fa544ef3fe1aeb56e254eaef Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 10 Oct 2019 19:21:12 +0200 Subject: [PATCH] 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 e9172413d..10333d3ed 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 f794db7fc..defbd01bb 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*/ ; }