From ef4183858ac8b86a46c12248aa7e705ecddd215b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 18:56:27 +0100 Subject: [PATCH 1/2] Document how tested prefix lengths are chosen --- tests/suites/test_suite_asn1parse.function | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index d747cc254..63e3a31c7 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -185,6 +185,10 @@ void parse_prefixes( const data_t *input, size_t buffer_size; int ret; + /* Test every prefix of the input, except the empty string. + * The first byte of the string is the tag. Without a tag byte, + * we wouldn't know what to parse the input as. + */ for( buffer_size = 1; buffer_size <= input->len; buffer_size++ ) { test_set_step( buffer_size ); @@ -221,6 +225,12 @@ void get_len( const data_t *input, int actual_length_arg ) size_t actual_length = actual_length_arg; size_t buffer_size; + /* Test prefixes of a buffer containing the given length string + * followed by `actual_length` bytes of payload. To save a bit of + * time, we skip some "boring" prefixes: we don't test prefixes where + * the payload is truncated more than one byte away from either end, + * and we only test the empty string on a 1-byte input. + */ for( buffer_size = 1; buffer_size <= input->len + 1; buffer_size++ ) { if( ! get_len_step( input, buffer_size, actual_length ) ) From 95c893d17f64f6549b2acfdead419d1711189282 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 21:26:36 +0100 Subject: [PATCH 2/2] More systematic handling of trailing garbage in parse_prefixes Before, the string to parse may contain trailing garbage (there was never more than one byte), and there was a separate argument indicating the length of the content. Now, the string to parse is the exact content, and the test code runs an extra test step with a trailing byte added. --- tests/suites/test_suite_asn1parse.data | 39 +++++++++++---------- tests/suites/test_suite_asn1parse.function | 40 ++++++++++++++++------ 2 files changed, 50 insertions(+), 29 deletions(-) diff --git a/tests/suites/test_suite_asn1parse.data b/tests/suites/test_suite_asn1parse.data index e26f93af7..f129e6507 100644 --- a/tests/suites/test_suite_asn1parse.data +++ b/tests/suites/test_suite_asn1parse.data @@ -1,56 +1,59 @@ Empty length -parse_prefixes:"04":0:MBEDTLS_ERR_ASN1_INVALID_LENGTH +parse_prefixes:"04":MBEDTLS_ERR_ASN1_OUT_OF_DATA:UNPREDICTABLE_RESULT + +Incomplete length +parse_prefixes:"0481":MBEDTLS_ERR_ASN1_OUT_OF_DATA:UNPREDICTABLE_RESULT Prefixes of OCTET STRING, length=0 -parse_prefixes:"04007e":2:0 +parse_prefixes:"0400":0:0 Prefixes of OCTET STRING, length=0 (0 length bytes) -parse_prefixes:"04807e":2:MBEDTLS_ERR_ASN1_INVALID_LENGTH +parse_prefixes:"0480":MBEDTLS_ERR_ASN1_INVALID_LENGTH:MBEDTLS_ERR_ASN1_INVALID_LENGTH Prefixes of OCTET STRING, length=1 -parse_prefixes:"0401417e":3:0 +parse_prefixes:"040141":0:0 Prefixes of OCTET STRING, length=2 -parse_prefixes:"040241427e":4:0 +parse_prefixes:"04024142":0:0 Prefixes of BOOLEAN, length=0 -parse_prefixes:"01007e":2:MBEDTLS_ERR_ASN1_INVALID_LENGTH +parse_prefixes:"0100":MBEDTLS_ERR_ASN1_INVALID_LENGTH:MBEDTLS_ERR_ASN1_INVALID_LENGTH Prefixes of BOOLEAN, length=1 -parse_prefixes:"0101007e":3:0 +parse_prefixes:"010100":0:0 Prefixes of BOOLEAN, length=2 -parse_prefixes:"010200007e":4:MBEDTLS_ERR_ASN1_INVALID_LENGTH +parse_prefixes:"01020000":MBEDTLS_ERR_ASN1_INVALID_LENGTH:MBEDTLS_ERR_ASN1_INVALID_LENGTH Prefixes of INTEGER, length=1 -parse_prefixes:"0201417e":3:0 +parse_prefixes:"020141":0:0 Prefixes of INTEGER, length=2 -parse_prefixes:"020241427e":4:0 +parse_prefixes:"02024142":0:0 Prefixes of INTEGER, length=5 -parse_prefixes:"020541424344457e":7:0 +parse_prefixes:"02054142434445":0:0 Prefixes of empty BIT STRING -parse_prefixes:"03007e":2:MBEDTLS_ERR_ASN1_OUT_OF_DATA +parse_prefixes:"0300":MBEDTLS_ERR_ASN1_OUT_OF_DATA:UNPREDICTABLE_RESULT Prefixes of BIT STRING, unused_bits=0, payload_length=0 -parse_prefixes:"030100":3:0 +parse_prefixes:"030100":0:MBEDTLS_ERR_ASN1_LENGTH_MISMATCH Prefixes of BIT STRING, unused_bits=0, payload_length=1 -parse_prefixes:"0302002a":4:0 +parse_prefixes:"0302002a":0:MBEDTLS_ERR_ASN1_LENGTH_MISMATCH Prefixes of BIT STRING, unused_bits=1, payload_length=1 -parse_prefixes:"0302012a":4:0 +parse_prefixes:"0302012a":0:MBEDTLS_ERR_ASN1_LENGTH_MISMATCH Prefixes of empty SEQUENCE -parse_prefixes:"30007e":2:0 +parse_prefixes:"3000":0:0 Prefixes of SEQUENCE of BOOLEAN, INTEGER, INTEGER -parse_prefixes:"300b01010102012a02031234567e":13:0 +parse_prefixes:"300b01010102012a0203123456":0:0 Prefixes of SEQUENCE of (SEQUENCE of INTEGER, INTEGER), INTEGER -parse_prefixes:"300b30060201410201420201617e":13:0 +parse_prefixes:"300b3006020141020142020161":0:0 length=0 (short form) get_len:"00":0 diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index 63e3a31c7..94e34fb47 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -9,8 +9,13 @@ #include "mbedtls/asn1write.h" #endif +/* Used internally to report an error that indicates a bug in a parsing function. */ #define ERR_PARSE_INCONSISTENCY INT_MAX +/* Use this magic value in some tests to indicate that the expected result + * should not be checked. */ +#define UNPREDICTABLE_RESULT 0x5552 + static int nested_parse( unsigned char **const p, const unsigned char *const end ) { @@ -176,10 +181,15 @@ exit: /* BEGIN_CASE */ void parse_prefixes( const data_t *input, - int actual_length_arg, - int last_result ) + int full_result, + int overfull_result ) { - size_t actual_length = actual_length_arg; + /* full_result: expected result from parsing the given string. */ + /* overfull_result: expected_result from parsing the given string plus + * some trailing garbage. This may be UNPREDICTABLE_RESULT to accept + * any result: use this for invalid inputs that may or may not become + * valid depending on what the trailing garbage is. */ + unsigned char *buf = NULL; unsigned char *p = NULL; size_t buffer_size; @@ -188,8 +198,9 @@ void parse_prefixes( const data_t *input, /* Test every prefix of the input, except the empty string. * The first byte of the string is the tag. Without a tag byte, * we wouldn't know what to parse the input as. + * Also test the input followed by an extra byte. */ - for( buffer_size = 1; buffer_size <= input->len; buffer_size++ ) + for( buffer_size = 1; buffer_size <= input->len + 1; buffer_size++ ) { test_set_step( buffer_size ); /* Allocate a new buffer of exactly the length to parse each time. @@ -198,18 +209,25 @@ void parse_prefixes( const data_t *input, memcpy( buf, input->x, buffer_size ); p = buf; ret = nested_parse( &p, buf + buffer_size ); + if( ret == ERR_PARSE_INCONSISTENCY ) goto exit; - if( actual_length > 0 && buffer_size >= actual_length ) - { - TEST_EQUAL( ret, last_result ); - if( ret == 0 ) - TEST_ASSERT( p == buf + actual_length ); - } - else + if( buffer_size < input->len ) { TEST_EQUAL( ret, MBEDTLS_ERR_ASN1_OUT_OF_DATA ); } + else if( buffer_size == input->len ) + { + TEST_EQUAL( ret, full_result ); + } + else /* ( buffer_size > input->len ) */ + { + if( overfull_result != UNPREDICTABLE_RESULT ) + TEST_EQUAL( ret, overfull_result ); + } + if( ret == 0 ) + TEST_ASSERT( p == buf + input->len ); + mbedtls_free( buf ); buf = NULL; }