From 92337c0e620d2162e6fb7f0dbaec286670beefc2 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 18 Jan 2023 18:40:49 +0000 Subject: [PATCH 01/27] Add function to parse an OID from a string Signed-off-by: David Horstmann --- include/mbedtls/oid.h | 14 ++++ library/oid.c | 164 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 178 insertions(+) diff --git a/include/mbedtls/oid.h b/include/mbedtls/oid.h index a592e63c4..1284aa929 100644 --- a/include/mbedtls/oid.h +++ b/include/mbedtls/oid.h @@ -466,6 +466,20 @@ typedef struct mbedtls_oid_descriptor_t { */ int mbedtls_oid_get_numeric_string(char *buf, size_t size, const mbedtls_asn1_buf *oid); +/** + * \brief Translate a string containing a numeric representation + * of an ASN.1 OID into its encoded form + * (e.g. "1.2.840.113549" into "\x2A\x86\x48\x86\xF7\x0D") + * + * \param buf buffer to put representation in + * \param size size of the buffer + * \param oid OID to translate + * + * \return Length of the string written (excluding final NULL) or + * MBEDTLS_ERR_OID_BUF_TOO_SMALL in case of error + */ +int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, const char *buf, size_t size); + /** * \brief Translate an X.509 extension OID into local values * diff --git a/library/oid.c b/library/oid.c index 86214b23a..9b8bef6de 100644 --- a/library/oid.c +++ b/library/oid.c @@ -895,4 +895,168 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, return (int) (size - n); } +static int oid_parse_number(const char **p, const char *bound) +{ + int ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + int num = 0; + while (*p < bound && **p >= '0' && **p <= '9') { + ret = 0; + if (num > (INT_MAX / 10)) { + return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + } + num *= 10; + num += **p - '0'; + (*p)++; + } + if (ret != 0) { + return ret; + } else { + return num; + } +} + +static size_t oid_subidentifier_num_bytes(unsigned int value) +{ + size_t num_bytes = 1; + value >>= 7; + while (value != 0) { + num_bytes++; + value >>= 7; + } + return num_bytes; +} + +static int oid_subidentifier_encode_into(unsigned char **p, + unsigned char *bound, + unsigned int value) +{ + size_t num_bytes = oid_subidentifier_num_bytes(value); + if ((size_t) (bound - *p) < num_bytes) { + return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + } + (*p)[num_bytes - 1] = (unsigned char) (value & 0x7f); + value >>= 7; + + for (size_t i = 2; i <= num_bytes; i++) { + (*p)[num_bytes - i] = 0x80 | (unsigned char) (value & 0x7f); + value >>= 7; + } + *p += num_bytes; + + return 0; +} + +/* Return the OID for the given x.y.z.... style numeric string */ +int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, + const char *buf, size_t size) +{ + int ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + + const char *str_ptr = buf; + const char *str_bound = buf + size; + int val = 0; + size_t encoded_len; + + int component1 = oid_parse_number(&str_ptr, str_bound); + if (component1 < 0) { + return component1; + } + if (component1 > 2) { + /* First component can't be > 2 */ + return MBEDTLS_ERR_ASN1_INVALID_DATA; + } + if (str_ptr >= str_bound || *str_ptr != '.') { + return MBEDTLS_ERR_ASN1_INVALID_DATA; + } + str_ptr++; + + int component2 = oid_parse_number(&str_ptr, str_bound); + if (component2 < 0) { + return component2; + } + if ((component1 < 2) && (component2 > 38)) { + /* Root nodes 0 and 1 may have up to 39 children */ + return MBEDTLS_ERR_ASN1_INVALID_DATA; + } + if (str_ptr < str_bound && *str_ptr != '\0') { + if (*str_ptr == '.') { + str_ptr++; + } else { + return MBEDTLS_ERR_ASN1_INVALID_DATA; + } + } + + if ((UINT_MAX - (unsigned int) component2) <= + ((unsigned int) component1 * 40)) { + return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + } + encoded_len = oid_subidentifier_num_bytes(((unsigned int) component1 * 40) + + (unsigned int) component2); + + while (str_ptr < str_bound && *str_ptr != '\0') { + val = oid_parse_number(&str_ptr, str_bound); + if (val < 0) { + return val; + } + if (str_ptr < str_bound && *str_ptr != '\0') { + if (*str_ptr == '.') { + str_ptr++; + } else { + return MBEDTLS_ERR_ASN1_INVALID_DATA; + } + } + + size_t num_bytes = oid_subidentifier_num_bytes(val); + if ((SIZE_MAX - encoded_len) <= num_bytes) { + return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + } + encoded_len += num_bytes; + } + + oid->p = mbedtls_calloc(encoded_len, sizeof(unsigned char)); + if (oid->p == NULL) { + return MBEDTLS_ERR_ASN1_ALLOC_FAILED; + } + oid->len = encoded_len; + + /* Now that we've allocated the buffer, go back to the start and encode */ + str_ptr = buf; + unsigned char *out_ptr = oid->p; + unsigned char *out_bound = oid->p + oid->len; + + /* No need to do validation this time, as we did it on the first pass */ + component1 = oid_parse_number(&str_ptr, str_bound); + /* Skip past the '.' */ + str_ptr++; + component2 = oid_parse_number(&str_ptr, str_bound); + /* Skip past the '.' */ + str_ptr++; + ret = oid_subidentifier_encode_into(&out_ptr, out_bound, + (component1 * 40) + component2); + if (ret != 0) { + mbedtls_free(oid->p); + oid->p = NULL; + oid->len = 0; + return ret; + } + while (str_ptr < str_bound && *str_ptr != '\0') { + val = oid_parse_number(&str_ptr, str_bound); + if (str_ptr < str_bound && *str_ptr == '.') { + /* Skip past the '.' */ + str_ptr++; + } + + ret = oid_subidentifier_encode_into(&out_ptr, out_bound, val); + if (ret != 0) { + mbedtls_free(oid->p); + oid->p = NULL; + oid->len = 0; + return ret; + } + } + oid->tag = MBEDTLS_ASN1_OID; + + return 0; +} + #endif /* MBEDTLS_OID_C */ From 0f852c92772e28fb04fc184c62015c223e530e4e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 15 Feb 2023 11:09:10 +0000 Subject: [PATCH 02/27] Add tests for OID parsing from string Signed-off-by: David Horstmann --- tests/suites/test_suite_oid.data | 12 ++++++++++++ tests/suites/test_suite_oid.function | 26 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data index b9fa6543d..9e47ef42a 100644 --- a/tests/suites/test_suite_oid.data +++ b/tests/suites/test_suite_oid.data @@ -119,3 +119,15 @@ oid_get_numeric_string:"8001":MBEDTLS_ERR_ASN1_INVALID_DATA:"" OID get numeric string - overlong encoding, second subidentifier oid_get_numeric_string:"2B8001":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - hardware module name +oid_from_numeric_string:"1.3.6.1.5.5.7.8.4":0:"2B06010505070804" + +OID from numeric string - multi-byte subidentifier +oid_from_numeric_string:"1.1.2108":0:"29903C" + +OID from numeric string - second component greater than 39 +oid_from_numeric_string:"2.49.0.0.826.0":0:"81010000863A00" + +OID from numeric string - multi-byte first subidentifier +oid_from_numeric_string:"2.999":0:"8837" diff --git a/tests/suites/test_suite_oid.function b/tests/suites/test_suite_oid.function index 3004b65fe..329bd8b48 100644 --- a/tests/suites/test_suite_oid.function +++ b/tests/suites/test_suite_oid.function @@ -117,3 +117,29 @@ void oid_get_numeric_string(data_t *oid, int error_ret, char *result_str) } } /* END_CASE */ + +/* BEGIN_CASE */ +void oid_from_numeric_string(char *oid_str, int error_ret, + data_t *exp_oid_buf) +{ + mbedtls_asn1_buf oid = { 0, 0, NULL }; + mbedtls_asn1_buf exp_oid = { 0, 0, NULL }; + int ret; + + exp_oid.tag = MBEDTLS_ASN1_OID; + exp_oid.p = exp_oid_buf->x; + exp_oid.len = exp_oid_buf->len; + + ret = mbedtls_oid_from_numeric_string(&oid, oid_str, strlen(oid_str)); + + if (error_ret == 0) { + TEST_EQUAL(oid.len, exp_oid.len); + TEST_ASSERT(memcmp(oid.p, exp_oid.p, oid.len) == 0); + mbedtls_free(oid.p); + oid.p = NULL; + oid.len = 0; + } else { + TEST_EQUAL(ret, error_ret); + } +} +/* END_CASE */ From 18ec9d7da120cbd7d916483de69961b6dca41c48 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 20 Feb 2023 17:18:45 +0000 Subject: [PATCH 03/27] Change some error codes to be more accurate Signed-off-by: David Horstmann --- library/oid.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/oid.c b/library/oid.c index 9b8bef6de..996b5419e 100644 --- a/library/oid.c +++ b/library/oid.c @@ -902,7 +902,7 @@ static int oid_parse_number(const char **p, const char *bound) while (*p < bound && **p >= '0' && **p <= '9') { ret = 0; if (num > (INT_MAX / 10)) { - return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + return MBEDTLS_ERR_ASN1_INVALID_DATA; } num *= 10; num += **p - '0'; @@ -988,7 +988,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, if ((UINT_MAX - (unsigned int) component2) <= ((unsigned int) component1 * 40)) { - return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + return MBEDTLS_ERR_ASN1_INVALID_DATA; } encoded_len = oid_subidentifier_num_bytes(((unsigned int) component1 * 40) + (unsigned int) component2); @@ -1008,7 +1008,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, size_t num_bytes = oid_subidentifier_num_bytes(val); if ((SIZE_MAX - encoded_len) <= num_bytes) { - return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + return MBEDTLS_ERR_ASN1_INVALID_DATA; } encoded_len += num_bytes; } From 03329970dec32c99d40e3129bd998022e49493c7 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 20 Feb 2023 17:28:36 +0000 Subject: [PATCH 04/27] Correct error in processing of second component Root nodes 0 and 1 may have up to 40 children (0 - 39), not 39 children (0 - 38) as previously thought. Signed-off-by: David Horstmann --- library/oid.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/oid.c b/library/oid.c index 996b5419e..06127c26e 100644 --- a/library/oid.c +++ b/library/oid.c @@ -974,8 +974,8 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, if (component2 < 0) { return component2; } - if ((component1 < 2) && (component2 > 38)) { - /* Root nodes 0 and 1 may have up to 39 children */ + if ((component1 < 2) && (component2 > 39)) { + /* Root nodes 0 and 1 may have up to 40 children, numbered 0-39 */ return MBEDTLS_ERR_ASN1_INVALID_DATA; } if (str_ptr < str_bound && *str_ptr != '\0') { From 59400ffed5562cc64b6ea1fb373834e3a1e817e4 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 21 Feb 2023 15:27:16 +0000 Subject: [PATCH 05/27] Improve header docs and rename parameter Signed-off-by: David Horstmann --- include/mbedtls/oid.h | 17 +++++++++++------ library/oid.c | 8 ++++---- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/include/mbedtls/oid.h b/include/mbedtls/oid.h index 1284aa929..49f4af520 100644 --- a/include/mbedtls/oid.h +++ b/include/mbedtls/oid.h @@ -470,15 +470,20 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, const mbedtls_asn1_bu * \brief Translate a string containing a numeric representation * of an ASN.1 OID into its encoded form * (e.g. "1.2.840.113549" into "\x2A\x86\x48\x86\xF7\x0D") + * On success, this function allocates oid->buf from the + * heap. It must be free'd by the caller. * - * \param buf buffer to put representation in - * \param size size of the buffer - * \param oid OID to translate + * \param oid mbedtls_asn1_buf to populate with the DER-encoded OID + * \param oid_str string representation of the OID to parse + * \param size length of the OID string * - * \return Length of the string written (excluding final NULL) or - * MBEDTLS_ERR_OID_BUF_TOO_SMALL in case of error + * \return 0 if successful + * \return #MBEDTLS_ERR_ASN1_INVALID_DATA if oid_str does not + * represent a valid OID + * \return #MBEDTLS_ERR_ASN1_ALLOC_FAILED if the function fails to + * allocate oid->buf */ -int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, const char *buf, size_t size); +int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, const char *oid_str, size_t size); /** * \brief Translate an X.509 extension OID into local values diff --git a/library/oid.c b/library/oid.c index 06127c26e..6c62b949b 100644 --- a/library/oid.c +++ b/library/oid.c @@ -948,12 +948,12 @@ static int oid_subidentifier_encode_into(unsigned char **p, /* Return the OID for the given x.y.z.... style numeric string */ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, - const char *buf, size_t size) + const char *oid_str, size_t size) { int ret = MBEDTLS_ERR_ASN1_INVALID_DATA; - const char *str_ptr = buf; - const char *str_bound = buf + size; + const char *str_ptr = oid_str; + const char *str_bound = oid_str + size; int val = 0; size_t encoded_len; @@ -1020,7 +1020,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, oid->len = encoded_len; /* Now that we've allocated the buffer, go back to the start and encode */ - str_ptr = buf; + str_ptr = oid_str; unsigned char *out_ptr = oid->p; unsigned char *out_bound = oid->p + oid->len; From 0f4ee418d8f4a7b9e46eb953f29a8e07d2f854e0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 21 Feb 2023 16:17:41 +0000 Subject: [PATCH 06/27] Use return for errors only in oid_parse_number() Signed-off-by: David Horstmann --- library/oid.c | 47 +++++++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/library/oid.c b/library/oid.c index 6c62b949b..91f6da6e5 100644 --- a/library/oid.c +++ b/library/oid.c @@ -895,24 +895,20 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, return (int) (size - n); } -static int oid_parse_number(const char **p, const char *bound) +static int oid_parse_number(unsigned int *num, const char **p, const char *bound) { int ret = MBEDTLS_ERR_ASN1_INVALID_DATA; - int num = 0; + *num = 0; while (*p < bound && **p >= '0' && **p <= '9') { ret = 0; - if (num > (INT_MAX / 10)) { + if (*num > (INT_MAX / 10)) { return MBEDTLS_ERR_ASN1_INVALID_DATA; } - num *= 10; - num += **p - '0'; + *num *= 10; + *num += **p - '0'; (*p)++; } - if (ret != 0) { - return ret; - } else { - return num; - } + return ret; } static size_t oid_subidentifier_num_bytes(unsigned int value) @@ -956,10 +952,11 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, const char *str_bound = oid_str + size; int val = 0; size_t encoded_len; + unsigned int component1, component2; - int component1 = oid_parse_number(&str_ptr, str_bound); - if (component1 < 0) { - return component1; + ret = oid_parse_number(&component1, &str_ptr, str_bound); + if (ret != 0) { + return ret; } if (component1 > 2) { /* First component can't be > 2 */ @@ -970,9 +967,9 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, } str_ptr++; - int component2 = oid_parse_number(&str_ptr, str_bound); - if (component2 < 0) { - return component2; + ret = oid_parse_number(&component2, &str_ptr, str_bound); + if (ret != 0) { + return ret; } if ((component1 < 2) && (component2 > 39)) { /* Root nodes 0 and 1 may have up to 40 children, numbered 0-39 */ @@ -986,17 +983,15 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, } } - if ((UINT_MAX - (unsigned int) component2) <= - ((unsigned int) component1 * 40)) { + if ((UINT_MAX - component2) <= (component1 * 40)) { return MBEDTLS_ERR_ASN1_INVALID_DATA; } - encoded_len = oid_subidentifier_num_bytes(((unsigned int) component1 * 40) - + (unsigned int) component2); + encoded_len = oid_subidentifier_num_bytes((component1 * 40) + component2); while (str_ptr < str_bound && *str_ptr != '\0') { - val = oid_parse_number(&str_ptr, str_bound); - if (val < 0) { - return val; + oid_parse_number(&val, &str_ptr, str_bound); + if (ret != 0) { + return ret; } if (str_ptr < str_bound && *str_ptr != '\0') { if (*str_ptr == '.') { @@ -1025,10 +1020,10 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, unsigned char *out_bound = oid->p + oid->len; /* No need to do validation this time, as we did it on the first pass */ - component1 = oid_parse_number(&str_ptr, str_bound); + oid_parse_number(&component1, &str_ptr, str_bound); /* Skip past the '.' */ str_ptr++; - component2 = oid_parse_number(&str_ptr, str_bound); + oid_parse_number(&component2, &str_ptr, str_bound); /* Skip past the '.' */ str_ptr++; ret = oid_subidentifier_encode_into(&out_ptr, out_bound, @@ -1040,7 +1035,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, return ret; } while (str_ptr < str_bound && *str_ptr != '\0') { - val = oid_parse_number(&str_ptr, str_bound); + oid_parse_number(&val, &str_ptr, str_bound); if (str_ptr < str_bound && *str_ptr == '.') { /* Skip past the '.' */ str_ptr++; From 7cdfda12da9619e68e15c8516654c8018f4463f3 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 21 Feb 2023 16:20:52 +0000 Subject: [PATCH 07/27] Fixup: Correct signedness of val local variable Signed-off-by: David Horstmann --- library/oid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/oid.c b/library/oid.c index 91f6da6e5..3b5b31a5e 100644 --- a/library/oid.c +++ b/library/oid.c @@ -950,7 +950,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, const char *str_ptr = oid_str; const char *str_bound = oid_str + size; - int val = 0; + unsigned int val = 0; size_t encoded_len; unsigned int component1, component2; From 89d67bd472dec4aff1b770004da3a488ac749051 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 21 Feb 2023 16:24:38 +0000 Subject: [PATCH 08/27] Remove superfluous sizeof(unsigned char) Signed-off-by: David Horstmann --- library/oid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/oid.c b/library/oid.c index 3b5b31a5e..103199012 100644 --- a/library/oid.c +++ b/library/oid.c @@ -1008,7 +1008,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, encoded_len += num_bytes; } - oid->p = mbedtls_calloc(encoded_len, sizeof(unsigned char)); + oid->p = mbedtls_calloc(encoded_len, 1); if (oid->p == NULL) { return MBEDTLS_ERR_ASN1_ALLOC_FAILED; } From 376e8df9d6098539c7ece0bbbce99214f1d5b412 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 21 Feb 2023 16:33:40 +0000 Subject: [PATCH 09/27] Clarify structure of parsing with comments: 1. Parse through to get the required buffer length. 2. Having allocated a buffer, parse into the buffer. Signed-off-by: David Horstmann --- library/oid.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/library/oid.c b/library/oid.c index 103199012..811d34324 100644 --- a/library/oid.c +++ b/library/oid.c @@ -954,6 +954,8 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, size_t encoded_len; unsigned int component1, component2; + /* First pass - parse the string to get the length of buffer required */ + ret = oid_parse_number(&component1, &str_ptr, str_bound); if (ret != 0) { return ret; @@ -1014,7 +1016,9 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, } oid->len = encoded_len; - /* Now that we've allocated the buffer, go back to the start and encode */ + /* Second pass - now that we've allocated the buffer, go back to the + * start and encode */ + str_ptr = oid_str; unsigned char *out_ptr = oid->p; unsigned char *out_bound = oid->p + oid->len; From e91cbcfb2cbc66705e0b7bc9058ca332b89d8d28 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 21 Feb 2023 17:19:45 +0000 Subject: [PATCH 10/27] Add negative test cases for OID parsing Signed-off-by: David Horstmann --- tests/suites/test_suite_oid.data | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data index 9e47ef42a..d4a7dea21 100644 --- a/tests/suites/test_suite_oid.data +++ b/tests/suites/test_suite_oid.data @@ -131,3 +131,30 @@ oid_from_numeric_string:"2.49.0.0.826.0":0:"81010000863A00" OID from numeric string - multi-byte first subidentifier oid_from_numeric_string:"2.999":0:"8837" + +OID from numeric string - empty string input +oid_from_numeric_string:"":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - first component not a number +oid_from_numeric_string:"abc.1.2":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - second component not a number +oid_from_numeric_string:"1.abc.2":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - first component too large +oid_from_numeric_string:"3.1":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - first component < 2, second > 39 +oid_from_numeric_string:"1.40":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - third component not a number +oid_from_numeric_string:"1.2.abc":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - non-'.' separator between first and second +oid_from_numeric_string:"1/2.3.4":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - non-'.' separator between second and third +oid_from_numeric_string:"1.2/3.4":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - non-'.' separator between third and fourth +oid_from_numeric_string:"1.2.3/4":MBEDTLS_ERR_ASN1_INVALID_DATA:"" From ce16474d9119cdef01aa6bda53d0210d0821b853 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 23 Feb 2023 13:50:48 +0000 Subject: [PATCH 11/27] Correct INT_MAX overflow check to UINT_MAX Signed-off-by: David Horstmann --- library/oid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/oid.c b/library/oid.c index 811d34324..3c27bfc73 100644 --- a/library/oid.c +++ b/library/oid.c @@ -901,7 +901,7 @@ static int oid_parse_number(unsigned int *num, const char **p, const char *bound *num = 0; while (*p < bound && **p >= '0' && **p <= '9') { ret = 0; - if (*num > (INT_MAX / 10)) { + if (*num > (UINT_MAX / 10)) { return MBEDTLS_ERR_ASN1_INVALID_DATA; } *num *= 10; From 099be74d28edd04dff9753751c2f0f6a5cdeb9fe Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 23 Feb 2023 13:51:43 +0000 Subject: [PATCH 12/27] Change free'd to freed for consistency Also clarify that the user must use mbedtls_free(). Signed-off-by: David Horstmann --- include/mbedtls/oid.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/oid.h b/include/mbedtls/oid.h index 49f4af520..b2f5dd196 100644 --- a/include/mbedtls/oid.h +++ b/include/mbedtls/oid.h @@ -471,7 +471,7 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, const mbedtls_asn1_bu * of an ASN.1 OID into its encoded form * (e.g. "1.2.840.113549" into "\x2A\x86\x48\x86\xF7\x0D") * On success, this function allocates oid->buf from the - * heap. It must be free'd by the caller. + * heap. It must be freed by the caller using mbedtls_free(). * * \param oid mbedtls_asn1_buf to populate with the DER-encoded OID * \param oid_str string representation of the OID to parse From 861e5d2742ddcdd68ab89e8177aa85381eb12ff0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 19 Apr 2023 18:15:51 +0100 Subject: [PATCH 13/27] Change to using an alloc-realloc strategy Allocate enough memory to guarantee we can store the OID, encode into the buffer, then realloc and copy into a buffer of exactly the right size. Signed-off-by: David Horstmann --- library/oid.c | 119 +++++++++++++++++++++++++------------------------- 1 file changed, 60 insertions(+), 59 deletions(-) diff --git a/library/oid.c b/library/oid.c index 3c27bfc73..139a707c8 100644 --- a/library/oid.c +++ b/library/oid.c @@ -951,111 +951,112 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, const char *str_ptr = oid_str; const char *str_bound = oid_str + size; unsigned int val = 0; - size_t encoded_len; unsigned int component1, component2; - /* First pass - parse the string to get the length of buffer required */ + /* Count the number of dots to get a worst-case allocation size. */ + size_t num_dots = 0; + for (size_t i = 0; (i < size) && (oid_str[i] != '\0'); i++) { + if (oid_str[i] == '.') { + num_dots++; + } + } + /* Allocate maximum possible required memory: + * There are (num_dots + 1) integer components, but the first 2 share the + * same subidentifier, so we only need num_dots subidentifiers maximum. */ + if (num_dots == 0 || (num_dots > SIZE_MAX / sizeof(unsigned int))) { + return MBEDTLS_ERR_ASN1_INVALID_DATA; + } + size_t max_possible_bytes = num_dots * sizeof(unsigned int); + oid->p = mbedtls_calloc(max_possible_bytes, 1); + if (oid->p == NULL) { + return MBEDTLS_ERR_ASN1_ALLOC_FAILED; + } + unsigned char *out_ptr = oid->p; + unsigned char *out_bound = oid->p + max_possible_bytes; ret = oid_parse_number(&component1, &str_ptr, str_bound); if (ret != 0) { - return ret; + goto error; } if (component1 > 2) { /* First component can't be > 2 */ - return MBEDTLS_ERR_ASN1_INVALID_DATA; + ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + goto error; } if (str_ptr >= str_bound || *str_ptr != '.') { - return MBEDTLS_ERR_ASN1_INVALID_DATA; + ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + goto error; } str_ptr++; ret = oid_parse_number(&component2, &str_ptr, str_bound); if (ret != 0) { - return ret; + goto error; } if ((component1 < 2) && (component2 > 39)) { /* Root nodes 0 and 1 may have up to 40 children, numbered 0-39 */ - return MBEDTLS_ERR_ASN1_INVALID_DATA; + ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + goto error; } if (str_ptr < str_bound && *str_ptr != '\0') { if (*str_ptr == '.') { str_ptr++; } else { - return MBEDTLS_ERR_ASN1_INVALID_DATA; + ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + goto error; } } if ((UINT_MAX - component2) <= (component1 * 40)) { - return MBEDTLS_ERR_ASN1_INVALID_DATA; + ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + goto error; + } + ret = oid_subidentifier_encode_into(&out_ptr, out_bound, + (component1 * 40) + component2); + if (ret != 0) { + goto error; } - encoded_len = oid_subidentifier_num_bytes((component1 * 40) + component2); while (str_ptr < str_bound && *str_ptr != '\0') { - oid_parse_number(&val, &str_ptr, str_bound); + ret = oid_parse_number(&val, &str_ptr, str_bound); if (ret != 0) { - return ret; + goto error; } if (str_ptr < str_bound && *str_ptr != '\0') { if (*str_ptr == '.') { str_ptr++; } else { - return MBEDTLS_ERR_ASN1_INVALID_DATA; + ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + goto error; } } - size_t num_bytes = oid_subidentifier_num_bytes(val); - if ((SIZE_MAX - encoded_len) <= num_bytes) { - return MBEDTLS_ERR_ASN1_INVALID_DATA; - } - encoded_len += num_bytes; - } - - oid->p = mbedtls_calloc(encoded_len, 1); - if (oid->p == NULL) { - return MBEDTLS_ERR_ASN1_ALLOC_FAILED; - } - oid->len = encoded_len; - - /* Second pass - now that we've allocated the buffer, go back to the - * start and encode */ - - str_ptr = oid_str; - unsigned char *out_ptr = oid->p; - unsigned char *out_bound = oid->p + oid->len; - - /* No need to do validation this time, as we did it on the first pass */ - oid_parse_number(&component1, &str_ptr, str_bound); - /* Skip past the '.' */ - str_ptr++; - oid_parse_number(&component2, &str_ptr, str_bound); - /* Skip past the '.' */ - str_ptr++; - ret = oid_subidentifier_encode_into(&out_ptr, out_bound, - (component1 * 40) + component2); - if (ret != 0) { - mbedtls_free(oid->p); - oid->p = NULL; - oid->len = 0; - return ret; - } - while (str_ptr < str_bound && *str_ptr != '\0') { - oid_parse_number(&val, &str_ptr, str_bound); - if (str_ptr < str_bound && *str_ptr == '.') { - /* Skip past the '.' */ - str_ptr++; - } - ret = oid_subidentifier_encode_into(&out_ptr, out_bound, val); if (ret != 0) { - mbedtls_free(oid->p); - oid->p = NULL; - oid->len = 0; - return ret; + goto error; } } + + size_t encoded_len = out_ptr - oid->p; + unsigned char *minimum_mem = mbedtls_calloc(encoded_len, 1); + if (minimum_mem == NULL) { + ret = MBEDTLS_ERR_ASN1_ALLOC_FAILED; + goto error; + } + memcpy(minimum_mem, oid->p, encoded_len); + mbedtls_free(oid->p); + oid->p = minimum_mem; + oid->len = encoded_len; + oid->tag = MBEDTLS_ASN1_OID; return 0; + +error: + mbedtls_free(oid->p); + oid->p = NULL; + oid->len = 0; + return ret; } #endif /* MBEDTLS_OID_C */ From 9643575d92440b8efc91cb60b7f0270e23f5fae8 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 26 Apr 2023 11:50:14 +0100 Subject: [PATCH 14/27] Limit OIDs to 128 components The longest OID known by oid-info.com is 34 components[1], so 128 should be plenty and will limit the potential for attacks. [1] http://oid-info.com/get/1.3.6.1.4.1.1248.1.1.2.1.3.21.69.112.115.111.110.32.83.116.121.108.117.115.32.80.114.111.32.52.57.48.48 Signed-off-by: David Horstmann --- include/mbedtls/oid.h | 5 +++++ library/oid.c | 2 +- tests/suites/test_suite_oid.data | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/oid.h b/include/mbedtls/oid.h index b2f5dd196..1d73506dc 100644 --- a/include/mbedtls/oid.h +++ b/include/mbedtls/oid.h @@ -63,6 +63,11 @@ #define MBEDTLS_OID_X509_EXT_FRESHEST_CRL (1 << 14) #define MBEDTLS_OID_X509_EXT_NS_CERT_TYPE (1 << 16) +/* + * Maximum number of OID components allowed + */ +#define MBEDTLS_OID_MAX_COMPONENTS 128 + /* * Top level OID tuples */ diff --git a/library/oid.c b/library/oid.c index 139a707c8..8da410380 100644 --- a/library/oid.c +++ b/library/oid.c @@ -963,7 +963,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, /* Allocate maximum possible required memory: * There are (num_dots + 1) integer components, but the first 2 share the * same subidentifier, so we only need num_dots subidentifiers maximum. */ - if (num_dots == 0 || (num_dots > SIZE_MAX / sizeof(unsigned int))) { + if (num_dots == 0 || (num_dots > MBEDTLS_OID_MAX_COMPONENTS - 1)) { return MBEDTLS_ERR_ASN1_INVALID_DATA; } size_t max_possible_bytes = num_dots * sizeof(unsigned int); diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data index d4a7dea21..c5f13175b 100644 --- a/tests/suites/test_suite_oid.data +++ b/tests/suites/test_suite_oid.data @@ -158,3 +158,6 @@ oid_from_numeric_string:"1.2/3.4":MBEDTLS_ERR_ASN1_INVALID_DATA:"" OID from numeric string - non-'.' separator between third and fourth oid_from_numeric_string:"1.2.3/4":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - OID greater than max length (129 components) +oid_from_numeric_string:"1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1":MBEDTLS_ERR_ASN1_INVALID_DATA:"" From b6ff8a2c4bf117d67500f04f346d77604214055e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 26 Apr 2023 12:10:36 +0100 Subject: [PATCH 15/27] Add ChangeLog entry for string-to-OID parsing Signed-off-by: David Horstmann --- ChangeLog.d/oid-parse-from-numeric-string.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 ChangeLog.d/oid-parse-from-numeric-string.txt diff --git a/ChangeLog.d/oid-parse-from-numeric-string.txt b/ChangeLog.d/oid-parse-from-numeric-string.txt new file mode 100644 index 000000000..459bedc83 --- /dev/null +++ b/ChangeLog.d/oid-parse-from-numeric-string.txt @@ -0,0 +1,3 @@ +Features + * Add a function mbedtls_oid_from_numeric_string to parse an OID from a + string to a DER-encoded mbedtls_asn1_buf. From 57b5d22a9e6b9bae09fde67b794f34ab36e44199 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 31 May 2023 14:36:41 +0100 Subject: [PATCH 16/27] Reword ChangeLog entry for consistency Signed-off-by: David Horstmann --- ChangeLog.d/oid-parse-from-numeric-string.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/oid-parse-from-numeric-string.txt b/ChangeLog.d/oid-parse-from-numeric-string.txt index 459bedc83..82ed2fd71 100644 --- a/ChangeLog.d/oid-parse-from-numeric-string.txt +++ b/ChangeLog.d/oid-parse-from-numeric-string.txt @@ -1,3 +1,3 @@ Features - * Add a function mbedtls_oid_from_numeric_string to parse an OID from a + * Add function mbedtls_oid_from_numeric_string() to parse an OID from a string to a DER-encoded mbedtls_asn1_buf. From b97b689832d77464b63651fdabbec9d7bdd7dca0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 31 May 2023 14:41:11 +0100 Subject: [PATCH 17/27] Reword function description slightly Use of the term "dotted-decimal" improves clarity. Put a full-stop where one should have been. Signed-off-by: David Horstmann --- include/mbedtls/oid.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/oid.h b/include/mbedtls/oid.h index 1d73506dc..5b75077ea 100644 --- a/include/mbedtls/oid.h +++ b/include/mbedtls/oid.h @@ -472,9 +472,9 @@ typedef struct mbedtls_oid_descriptor_t { int mbedtls_oid_get_numeric_string(char *buf, size_t size, const mbedtls_asn1_buf *oid); /** - * \brief Translate a string containing a numeric representation - * of an ASN.1 OID into its encoded form - * (e.g. "1.2.840.113549" into "\x2A\x86\x48\x86\xF7\x0D") + * \brief Translate a string containing a dotted-decimal + * representation of an ASN.1 OID into its encoded form + * (e.g. "1.2.840.113549" into "\x2A\x86\x48\x86\xF7\x0D"). * On success, this function allocates oid->buf from the * heap. It must be freed by the caller using mbedtls_free(). * From ada7d72447e996b31448af1d8a22fc7bbb8d2261 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 31 May 2023 14:49:56 +0100 Subject: [PATCH 18/27] Improve line spacing after variable declarations Signed-off-by: David Horstmann --- library/oid.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/library/oid.c b/library/oid.c index 8da410380..d2efbed1f 100644 --- a/library/oid.c +++ b/library/oid.c @@ -898,7 +898,9 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, static int oid_parse_number(unsigned int *num, const char **p, const char *bound) { int ret = MBEDTLS_ERR_ASN1_INVALID_DATA; + *num = 0; + while (*p < bound && **p >= '0' && **p <= '9') { ret = 0; if (*num > (UINT_MAX / 10)) { @@ -914,7 +916,9 @@ static int oid_parse_number(unsigned int *num, const char **p, const char *bound static size_t oid_subidentifier_num_bytes(unsigned int value) { size_t num_bytes = 1; + value >>= 7; + while (value != 0) { num_bytes++; value >>= 7; @@ -927,6 +931,7 @@ static int oid_subidentifier_encode_into(unsigned char **p, unsigned int value) { size_t num_bytes = oid_subidentifier_num_bytes(value); + if ((size_t) (bound - *p) < num_bytes) { return MBEDTLS_ERR_OID_BUF_TOO_SMALL; } @@ -947,14 +952,13 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, const char *oid_str, size_t size) { int ret = MBEDTLS_ERR_ASN1_INVALID_DATA; - const char *str_ptr = oid_str; const char *str_bound = oid_str + size; unsigned int val = 0; unsigned int component1, component2; - /* Count the number of dots to get a worst-case allocation size. */ size_t num_dots = 0; + for (size_t i = 0; (i < size) && (oid_str[i] != '\0'); i++) { if (oid_str[i] == '.') { num_dots++; From 25d65e85274b4eb788be1dda8781625ec35dc4a9 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 31 May 2023 14:53:07 +0100 Subject: [PATCH 19/27] Refactor while loop for simplicity Signed-off-by: David Horstmann --- library/oid.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/library/oid.c b/library/oid.c index d2efbed1f..ea1e70bf0 100644 --- a/library/oid.c +++ b/library/oid.c @@ -915,14 +915,13 @@ static int oid_parse_number(unsigned int *num, const char **p, const char *bound static size_t oid_subidentifier_num_bytes(unsigned int value) { - size_t num_bytes = 1; + size_t num_bytes = 0; - value >>= 7; - - while (value != 0) { - num_bytes++; + do { value >>= 7; - } + num_bytes++; + } while (value != 0); + return num_bytes; } From 6883358c16f2e369129753d3999bb345e57c3f29 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 31 May 2023 17:27:28 +0100 Subject: [PATCH 20/27] Hoist variable declarations to before goto This should appease IAR, which does not like declarations in the middle of goto sequences. Signed-off-by: David Horstmann --- library/oid.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/oid.c b/library/oid.c index ea1e70bf0..87e5d892f 100644 --- a/library/oid.c +++ b/library/oid.c @@ -957,6 +957,8 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, unsigned int component1, component2; /* Count the number of dots to get a worst-case allocation size. */ size_t num_dots = 0; + size_t encoded_len; + unsigned char *minimum_mem; for (size_t i = 0; (i < size) && (oid_str[i] != '\0'); i++) { if (oid_str[i] == '.') { @@ -1040,8 +1042,8 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, } } - size_t encoded_len = out_ptr - oid->p; - unsigned char *minimum_mem = mbedtls_calloc(encoded_len, 1); + encoded_len = out_ptr - oid->p; + minimum_mem = mbedtls_calloc(encoded_len, 1); if (minimum_mem == NULL) { ret = MBEDTLS_ERR_ASN1_ALLOC_FAILED; goto error; From d1a203a382a65a458a5736525237525dc3ca9697 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 1 Jun 2023 15:02:15 +0100 Subject: [PATCH 21/27] Cosmetic fixes to doxygen comment Signed-off-by: David Horstmann --- include/mbedtls/oid.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/oid.h b/include/mbedtls/oid.h index 5b75077ea..da3d70ed5 100644 --- a/include/mbedtls/oid.h +++ b/include/mbedtls/oid.h @@ -478,12 +478,12 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, const mbedtls_asn1_bu * On success, this function allocates oid->buf from the * heap. It must be freed by the caller using mbedtls_free(). * - * \param oid mbedtls_asn1_buf to populate with the DER-encoded OID + * \param oid #mbedtls_asn1_buf to populate with the DER-encoded OID * \param oid_str string representation of the OID to parse * \param size length of the OID string * * \return 0 if successful - * \return #MBEDTLS_ERR_ASN1_INVALID_DATA if oid_str does not + * \return #MBEDTLS_ERR_ASN1_INVALID_DATA if \p oid_str does not * represent a valid OID * \return #MBEDTLS_ERR_ASN1_ALLOC_FAILED if the function fails to * allocate oid->buf From 017139751a81895faa33ce59f1d1ff5ced5b741b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 1 Jun 2023 15:04:20 +0100 Subject: [PATCH 22/27] Change behaviour away from NUL-terminated strings Instead, require the length of the string to be passed. This is more useful for our use-case, as it is likely we will parse OIDs from the middle of strings. Signed-off-by: David Horstmann --- include/mbedtls/oid.h | 5 +++-- library/oid.c | 8 ++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/oid.h b/include/mbedtls/oid.h index da3d70ed5..68ae72317 100644 --- a/include/mbedtls/oid.h +++ b/include/mbedtls/oid.h @@ -479,8 +479,9 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, const mbedtls_asn1_bu * heap. It must be freed by the caller using mbedtls_free(). * * \param oid #mbedtls_asn1_buf to populate with the DER-encoded OID - * \param oid_str string representation of the OID to parse - * \param size length of the OID string + * \param oid_str string representation of the OID to parse, not + * NUL-terminated + * \param size length of the OID string, not including any NUL terminator * * \return 0 if successful * \return #MBEDTLS_ERR_ASN1_INVALID_DATA if \p oid_str does not diff --git a/library/oid.c b/library/oid.c index 87e5d892f..312a6375b 100644 --- a/library/oid.c +++ b/library/oid.c @@ -960,7 +960,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, size_t encoded_len; unsigned char *minimum_mem; - for (size_t i = 0; (i < size) && (oid_str[i] != '\0'); i++) { + for (size_t i = 0; i < size; i++) { if (oid_str[i] == '.') { num_dots++; } @@ -1003,7 +1003,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, ret = MBEDTLS_ERR_ASN1_INVALID_DATA; goto error; } - if (str_ptr < str_bound && *str_ptr != '\0') { + if (str_ptr < str_bound) { if (*str_ptr == '.') { str_ptr++; } else { @@ -1022,12 +1022,12 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, goto error; } - while (str_ptr < str_bound && *str_ptr != '\0') { + while (str_ptr < str_bound) { ret = oid_parse_number(&val, &str_ptr, str_bound); if (ret != 0) { goto error; } - if (str_ptr < str_bound && *str_ptr != '\0') { + if (str_ptr < str_bound) { if (*str_ptr == '.') { str_ptr++; } else { From 5d074168f38e6ee87c34087343f7a7abc9ac5be4 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 1 Jun 2023 15:09:27 +0100 Subject: [PATCH 23/27] Rearrange declarations for readability Signed-off-by: David Horstmann --- library/oid.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/oid.c b/library/oid.c index 312a6375b..cda87a9dc 100644 --- a/library/oid.c +++ b/library/oid.c @@ -955,11 +955,11 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, const char *str_bound = oid_str + size; unsigned int val = 0; unsigned int component1, component2; - /* Count the number of dots to get a worst-case allocation size. */ - size_t num_dots = 0; size_t encoded_len; unsigned char *minimum_mem; + /* Count the number of dots to get a worst-case allocation size. */ + size_t num_dots = 0; for (size_t i = 0; i < size; i++) { if (oid_str[i] == '.') { num_dots++; From 45d5e2dc1a5c12e732f8b6c2ceadcc1b99130c13 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 1 Jun 2023 15:10:33 +0100 Subject: [PATCH 24/27] Rename minimum_mem to resized_mem This new name is clearer about its purpose. Signed-off-by: David Horstmann --- library/oid.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/library/oid.c b/library/oid.c index cda87a9dc..02e41363e 100644 --- a/library/oid.c +++ b/library/oid.c @@ -956,7 +956,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, unsigned int val = 0; unsigned int component1, component2; size_t encoded_len; - unsigned char *minimum_mem; + unsigned char *resized_mem; /* Count the number of dots to get a worst-case allocation size. */ size_t num_dots = 0; @@ -1043,14 +1043,14 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, } encoded_len = out_ptr - oid->p; - minimum_mem = mbedtls_calloc(encoded_len, 1); - if (minimum_mem == NULL) { + resized_mem = mbedtls_calloc(encoded_len, 1); + if (resized_mem == NULL) { ret = MBEDTLS_ERR_ASN1_ALLOC_FAILED; goto error; } - memcpy(minimum_mem, oid->p, encoded_len); + memcpy(resized_mem, oid->p, encoded_len); mbedtls_free(oid->p); - oid->p = minimum_mem; + oid->p = resized_mem; oid->len = encoded_len; oid->tag = MBEDTLS_ASN1_OID; From bf95e9a0584c54bf7535c76608de31b0e52d29b2 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 1 Jun 2023 15:33:15 +0100 Subject: [PATCH 25/27] Reword description and change NUL to null Signed-off-by: David Horstmann --- include/mbedtls/oid.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/oid.h b/include/mbedtls/oid.h index 68ae72317..7fa014139 100644 --- a/include/mbedtls/oid.h +++ b/include/mbedtls/oid.h @@ -479,9 +479,8 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, const mbedtls_asn1_bu * heap. It must be freed by the caller using mbedtls_free(). * * \param oid #mbedtls_asn1_buf to populate with the DER-encoded OID - * \param oid_str string representation of the OID to parse, not - * NUL-terminated - * \param size length of the OID string, not including any NUL terminator + * \param oid_str string representation of the OID to parse + * \param size length of the OID string, not including any null terminator * * \return 0 if successful * \return #MBEDTLS_ERR_ASN1_INVALID_DATA if \p oid_str does not From 02127ab02245936cb4869d8e2ed3b3fc378ef32b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 2 Jun 2023 14:50:35 +0100 Subject: [PATCH 26/27] Allow subidentifiers of size UINT_MAX Make overflow check more accurate and add testcases Signed-off-by: David Horstmann --- library/oid.c | 2 +- tests/suites/test_suite_oid.data | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/library/oid.c b/library/oid.c index 02e41363e..b13c76b1e 100644 --- a/library/oid.c +++ b/library/oid.c @@ -1012,7 +1012,7 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, } } - if ((UINT_MAX - component2) <= (component1 * 40)) { + if (component2 > (UINT_MAX - (component1 * 40))) { ret = MBEDTLS_ERR_ASN1_INVALID_DATA; goto error; } diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data index c5f13175b..1435507f6 100644 --- a/tests/suites/test_suite_oid.data +++ b/tests/suites/test_suite_oid.data @@ -161,3 +161,9 @@ oid_from_numeric_string:"1.2.3/4":MBEDTLS_ERR_ASN1_INVALID_DATA:"" OID from numeric string - OID greater than max length (129 components) oid_from_numeric_string:"1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1.2.3.4.5.6.7.8.1":MBEDTLS_ERR_ASN1_INVALID_DATA:"" + +OID from numeric string - OID with maximum subidentifier +oid_from_numeric_string:"2.4294967215":0:"8FFFFFFF7F" + +OID from numeric string - OID with overflowing subidentifier +oid_from_numeric_string:"2.4294967216":MBEDTLS_ERR_ASN1_INVALID_DATA:"" From 62e7fae1090544357bcfaf209b9d87f8f827bfeb Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 2 Jun 2023 15:32:20 +0100 Subject: [PATCH 27/27] Fix bug in calculation of maximum possible bytes Each DER-encoded OID byte can only store 7 bits of actual data, so take account of that. Calculate the number of bytes required as: number_of_bytes = ceil(subidentifier_size * 8 / 7) Signed-off-by: David Horstmann --- library/oid.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/library/oid.c b/library/oid.c index b13c76b1e..88165d312 100644 --- a/library/oid.c +++ b/library/oid.c @@ -971,7 +971,14 @@ int mbedtls_oid_from_numeric_string(mbedtls_asn1_buf *oid, if (num_dots == 0 || (num_dots > MBEDTLS_OID_MAX_COMPONENTS - 1)) { return MBEDTLS_ERR_ASN1_INVALID_DATA; } - size_t max_possible_bytes = num_dots * sizeof(unsigned int); + /* Each byte can store 7 bits, calculate number of bytes for a + * subidentifier: + * + * bytes = ceil(subidentifer_size * 8 / 7) + */ + size_t bytes_per_subidentifier = (((sizeof(unsigned int) * 8) - 1) / 7) + + 1; + size_t max_possible_bytes = num_dots * bytes_per_subidentifier; oid->p = mbedtls_calloc(max_possible_bytes, 1); if (oid->p == NULL) { return MBEDTLS_ERR_ASN1_ALLOC_FAILED;