From 7f420faf030f27707fb651dd872cd65135d0fb6a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 21 Sep 2023 18:13:17 +0200 Subject: [PATCH] parse_attribute_value_hex_der_encoded: clean up length validation Separate the fits-in-buffer check (*data_length <= data_size) from the we-think-it's-a-sensible-size check (*data_length <= MBEDTLS_X509_MAX_DN_NAME_SIZE). This requires using an intermediate buffer for the DER data, since its maximum sensible size has to be larger than the maximum sensible size for the payload, due to the overhead of the ASN.1 tag+length. Remove test cases focusing on the DER length since the implementation no longer has a threshold for it. Signed-off-by: Gilles Peskine --- library/x509_create.c | 83 ++++++++++++++++---------- tests/suites/test_suite_x509write.data | 8 +-- 2 files changed, 53 insertions(+), 38 deletions(-) diff --git a/library/x509_create.c b/library/x509_create.c index 2bea28ee9..296836943 100644 --- a/library/x509_create.c +++ b/library/x509_create.c @@ -189,11 +189,12 @@ static int parse_attribute_value_string(const char *s, * * \param s A string of \p len bytes hexadecimal digits. * \param len Number of bytes to read from \p s. - * \param data Output buffer of size #MBEDTLS_X509_MAX_DN_NAME_SIZE. + * \param data Output buffer of size \p data_size. * On success, it contains the payload that's DER-encoded * in the input (content without the tag and length). * If the DER tag is a string tag, the payload is guaranteed * not to contain null bytes. + * \param data_size Length of the \p data buffer. * \param data_len On success, the length of the parsed string. * It is guaranteed to be less than * #MBEDTLS_X509_MAX_DN_NAME_SIZE. @@ -203,68 +204,88 @@ static int parse_attribute_value_string(const char *s, * \retval #MBEDTLS_ERR_X509_INVALID_NAME if \p s does not contain * a valid hexstring, * or if the decoded hexstring is not valid DER, - * or if the decoded hexstring does not fit in \p data, + * or if the payload does not fit in \p data, + * or if the payload is more than + * #MBEDTLS_X509_MAX_DN_NAME_SIZE bytes, * of if \p *tag is an ASN.1 string tag and the payload * contains a null byte. + * \retval #MBEDTLS_ERR_X509_ALLOC_FAILED on low memory. */ static int parse_attribute_value_hex_der_encoded(const char *s, size_t len, unsigned char *data, + size_t data_size, size_t *data_len, int *tag) { - /* Step 1: Decode the hex string. - * We use data as an intermediate buffer. This limits the ultimate payload - * to slightly less than MBEDTLS_X509_MAX_DN_NAME_SIZE bytes due to the - * overhead of the DER tag+length. */ + /* Step 1: preliminary length checks. */ /* Each byte is encoded by exactly two hexadecimal digits. */ if (len % 2 != 0) { /* Odd number of hex digits */ return MBEDTLS_ERR_X509_INVALID_NAME; } size_t const der_length = len / 2; - /* Here we treat MBEDTLS_X509_MAX_DN_NAME_SIZE as the maximum length of - * the DER encoding. This is convenient, but seems wrong: should it - * be the length of the payload (which would require a few more bytes - * in the intermediate buffer)? In practice the hex-encoded data is - * expected to be much shorter anyway. */ - if (der_length > MBEDTLS_X509_MAX_DN_NAME_SIZE) { - /* Not enough room in data */ + if (der_length > MBEDTLS_X509_MAX_DN_NAME_SIZE + 4) { + /* The payload would be more than MBEDTLS_X509_MAX_DN_NAME_SIZE + * (after subtracting the ASN.1 tag and length). Reject this early + * to avoid allocating a large intermediate buffer. */ return MBEDTLS_ERR_X509_INVALID_NAME; } + if (der_length < 1) { + /* Avoid empty-buffer shenanigans. A valid DER encoding is never + * empty. */ + return MBEDTLS_ERR_X509_INVALID_NAME; + } + + /* Step 2: Decode the hex string into an intermediate buffer. */ + unsigned char *der = mbedtls_calloc(1, der_length); + if (der == NULL) { + return MBEDTLS_ERR_X509_ALLOC_FAILED; + } + /* Beyond this point, der needs to be freed on exit. */ for (size_t i = 0; i < der_length; i++) { int c = hexpair_to_int(s + 2 * i); if (c < 0) { - return MBEDTLS_ERR_X509_INVALID_NAME; + goto error; } - data[i] = c; + der[i] = c; } - /* Step 2: decode the DER. */ - if (der_length < 1) { - return MBEDTLS_ERR_X509_INVALID_NAME; + /* Step 3: decode the DER. */ + /* We've checked that der_length >= 1 above. */ + *tag = der[0]; + unsigned char *p = der + 1; + if (mbedtls_asn1_get_len(&p, der + der_length, data_len) != 0) { + goto error; } - *tag = data[0]; - unsigned char *p = data + 1; - if (mbedtls_asn1_get_len(&p, data + der_length, data_len) != 0) { - return MBEDTLS_ERR_X509_INVALID_NAME; - } - - /* Step 3: extract the payload. */ - /* Now p points to the first byte of the payload inside data. - * Shift the content of data to move the payload to the beginning. */ - memmove(data, p, *data_len); + /* Now p points to the first byte of the payload inside der, + * and *data_len is the length of the payload. */ /* Step 4: payload validation */ + if (*data_len > MBEDTLS_X509_MAX_DN_NAME_SIZE) { + goto error; + } + /* Strings must not contain null bytes. */ if (MBEDTLS_ASN1_IS_STRING_TAG(*tag)) { for (size_t i = 0; i < *data_len; i++) { - if (data[i] == 0) { - return MBEDTLS_ERR_X509_INVALID_NAME; + if (p[i] == 0) { + goto error; } } } + /* Step 5: output the payload. */ + if (*data_len > data_size) { + goto error; + } + memcpy(data, p, *data_len); + mbedtls_free(der); + return 0; + +error: + mbedtls_free(der); + return MBEDTLS_ERR_X509_INVALID_NAME; } int mbedtls_x509_string_to_names(mbedtls_asn1_named_data **head, const char *name) @@ -312,7 +333,7 @@ int mbedtls_x509_string_to_names(mbedtls_asn1_named_data **head, const char *nam * else branch), hence c - s - 1 >= 0. */ parse_ret = parse_attribute_value_hex_der_encoded( s + 1, c - s - 1, - data, &data_len, &tag); + data, sizeof(data), &data_len, &tag); if (parse_ret != 0) { mbedtls_free(oid.p); return MBEDTLS_ERR_X509_INVALID_NAME; diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index 137b38b9b..cff25ff25 100644 --- a/tests/suites/test_suite_x509write.data +++ b/tests/suites/test_suite_x509write.data @@ -212,14 +212,8 @@ mbedtls_x509_string_to_names:"C=NL, 2.5.4.10=#0C084F6666737061726B, OU=PolarSSL" X509 String to Names (hexstring: trailing garbage after DER is ignored) mbedtls_x509_string_to_names:"C=NL, 2.5.4.10=#0C084F6666737061726Baa, OU=PolarSSL":"C=NL, O=Offspark, OU=PolarSSL":0:0 -X509 String to Names: long hexstring (DER=256 bytes) -mbedtls_x509_string_to_names:"C=NL, 2.5.4.10=#0C81fd41414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141, OU=PolarSSL":"C=NL, O=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA, OU=PolarSSL":0:0 - -X509 String to Names: long hexstring (DER=257 bytes) -mbedtls_x509_string_to_names:"C=NL, 2.5.4.10=#0C81fe4141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141, OU=PolarSSL":"C=NL, O=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA, OU=PolarSSL":MBEDTLS_ERR_X509_INVALID_NAME:0 - X509 String to Names: long hexstring (payload=256 bytes) -mbedtls_x509_string_to_names:"C=NL, 2.5.4.10=#0C82010041414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141, OU=PolarSSL":"C=NL, O=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA, OU=PolarSSL":MBEDTLS_ERR_X509_INVALID_NAME:0 +mbedtls_x509_string_to_names:"C=NL, 2.5.4.10=#0C82010041414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141, OU=PolarSSL":"C=NL, O=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA, OU=PolarSSL":0:MAY_FAIL_DN_GETS X509 String to Names: long hexstring (payload=257 bytes) mbedtls_x509_string_to_names:"C=NL, 2.5.4.10=#0C820101aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, OU=PolarSSL":"C=NL, O=Offspark, OU=PolarSSL":MBEDTLS_ERR_X509_INVALID_NAME:0