From c7f700c795b7b17b9694fef5b53dcc58f1bc46f7 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 14 Feb 2023 14:34:15 +0000 Subject: [PATCH 1/9] Fix incorrect printing of OIDs The first 2 components of an OID are combined together into the same subidentifier via the formula: subidentifier = (component1 * 40) + component2 The current code extracts component1 and component2 using division and modulo as one would expect. However, there is a subtlety in the specification[1]: >This packing of the first two object identifier components recognizes >that only three values are allocated from the root node, and at most >39 subsequent values from nodes reached by X = 0 and X = 1. If the root node (component1) is 2, the subsequent node (component2) may be greater than 38. For example, the following are real OIDs: * 2.40.0.25, UPU standard S25 * 2.49.0.0.826.0, Met Office * 2.999, Allocated example OID This has 2 implications that the current parsing code does not take account of: 1. The second component may be > 39, so (subidentifier % 40) is not correct in all circumstances. 2. The first subidentifier (containing the first 2 components) may be more than one byte long. Currently we assume it is just 1 byte. Improve parsing code to deal with these cases correctly. [1] Rec. ITU-T X.690 (02/2021), 8.19.4 Signed-off-by: David Horstmann --- library/oid.c | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/library/oid.c b/library/oid.c index e7c12248a..5668e2aa5 100644 --- a/library/oid.c +++ b/library/oid.c @@ -834,14 +834,39 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, p = buf; n = size; - /* First byte contains first two dots */ - if (oid->len > 0) { - ret = mbedtls_snprintf(p, n, "%d.%d", oid->p[0] / 40, oid->p[0] % 40); - OID_SAFE_SNPRINTF; + /* First subidentifier contains first two OID components */ + i = 0; + value = 0; + while (i < oid->len && ((oid->p[i] & 0x80) != 0)) { + /* Prevent overflow in value. */ + if (((value << 7) >> 7) != value) { + return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + } + + value += oid->p[i] & 0x7F; + value <<= 7; + i++; } + if (i >= oid->len) { + return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + } + /* Last byte of first subidentifier */ + value += oid->p[i] & 0x7F; + i++; + + unsigned int component1 = value / 40; + if (component1 > 2) { + /* The first component can only be 0, 1 or 2. + * If oid->p[0] / 40 is greater than 2, the leftover belongs to + * the second component. */ + component1 = 2; + } + unsigned int component2 = value - (40 * component1); + ret = mbedtls_snprintf(p, n, "%u.%u", component1, component2); + OID_SAFE_SNPRINTF; value = 0; - for (i = 1; i < oid->len; i++) { + for (; i < oid->len; i++) { /* Prevent overflow in value. */ if (((value << 7) >> 7) != value) { return MBEDTLS_ERR_OID_BUF_TOO_SMALL; From f01de145bd5fdb2e843bab427f45a566d358b225 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 14 Feb 2023 17:29:16 +0000 Subject: [PATCH 2/9] Add tests for mbedtls_oid_get_numeric_string() Signed-off-by: David Horstmann --- tests/suites/test_suite_oid.data | 24 ++++++++++++++++++++++++ tests/suites/test_suite_oid.function | 21 +++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data index 1738841d5..18b019a05 100644 --- a/tests/suites/test_suite_oid.data +++ b/tests/suites/test_suite_oid.data @@ -89,3 +89,27 @@ oid_get_md_alg_id:"2b24030201":MBEDTLS_MD_RIPEMD160 OID hash id - invalid oid oid_get_md_alg_id:"2B864886f70d0204":-1 +OID get numeric string - hardware module name +oid_get_numeric_string:"2B06010505070804":0:"1.3.6.1.5.5.7.8.4" + +OID get numeric string - multi-byte subidentifier +oid_get_numeric_string:"29903C":0:"1.1.2108" + +OID get numeric string - second component greater than 39 +oid_get_numeric_string:"81010000863A00":0:"2.49.0.0.826.0" + +OID get numeric string - multi-byte first subidentifier +oid_get_numeric_string:"8837":0:"2.999" + +OID get numeric string - empty oid buffer +oid_get_numeric_string:"":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" + +OID get numeric string - no final / all bytes have top bit set +oid_get_numeric_string:"818181":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" + +# Encodes the number 0x0400000000 as a subidentifier which overflows 32-bits +OID get numeric string - 32-bit overflow +oid_get_numeric_string:"C080808000":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" + +OID get numeric string - 32-bit overflow, second subidentifier +oid_get_numeric_string:"2BC080808000":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" diff --git a/tests/suites/test_suite_oid.function b/tests/suites/test_suite_oid.function index 687b2168a..3004b65fe 100644 --- a/tests/suites/test_suite_oid.function +++ b/tests/suites/test_suite_oid.function @@ -96,3 +96,24 @@ void oid_get_md_alg_id(data_t *oid, int exp_md_id) } } /* END_CASE */ + +/* BEGIN_CASE */ +void oid_get_numeric_string(data_t *oid, int error_ret, char *result_str) +{ + char buf[256]; + mbedtls_asn1_buf input_oid = { 0, 0, NULL }; + int ret; + + input_oid.tag = MBEDTLS_ASN1_OID; + input_oid.p = oid->x; + input_oid.len = oid->len; + + ret = mbedtls_oid_get_numeric_string(buf, sizeof(buf), &input_oid); + + if (error_ret == 0) { + TEST_ASSERT(strcmp(buf, result_str) == 0); + } else { + TEST_EQUAL(ret, error_ret); + } +} +/* END_CASE */ From 9c1887c4c7baaa388cf561c11ed32ed07935aa77 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 15 Feb 2023 11:48:13 +0000 Subject: [PATCH 3/9] Disallow overlong encoding when parsing OIDs OID subidentifiers are encoded as follow. For every byte: * The top bit is 1 if there is another byte to come, 0 if this is the last byte. * The other 7 bits form 7 bits of the number. These groups of 7 are concatenated together in big-endian order. Overlong encodings are explicitly disallowed by the BER/DER/X690 specification. For example, the number 1 cannot be encoded as: 0x80 0x80 0x01 It must be encoded as: 0x01 Enforce this in Mbed TLS' OID DER-to-string parser. Signed-off-by: David Horstmann --- library/oid.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/library/oid.c b/library/oid.c index 5668e2aa5..17d3e093a 100644 --- a/library/oid.c +++ b/library/oid.c @@ -837,6 +837,11 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, /* First subidentifier contains first two OID components */ i = 0; value = 0; + if ((oid->p[0]) == 0x80) { + /* Overlong encoding is not allowed */ + return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + } + while (i < oid->len && ((oid->p[i] & 0x80) != 0)) { /* Prevent overflow in value. */ if (((value << 7) >> 7) != value) { @@ -871,6 +876,10 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, if (((value << 7) >> 7) != value) { return MBEDTLS_ERR_OID_BUF_TOO_SMALL; } + if ((value == 0) && ((oid->p[i]) == 0x80)) { + /* Overlong encoding is not allowed */ + return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + } value <<= 7; value += oid->p[i] & 0x7F; From 895eb7c9b5eb58a2c86650ed6834f9146b214695 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 15 Feb 2023 11:58:40 +0000 Subject: [PATCH 4/9] Add testcases for overlong encoding of OIDs Signed-off-by: David Horstmann --- tests/suites/test_suite_oid.data | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data index 18b019a05..f4801c426 100644 --- a/tests/suites/test_suite_oid.data +++ b/tests/suites/test_suite_oid.data @@ -113,3 +113,9 @@ oid_get_numeric_string:"C080808000":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" OID get numeric string - 32-bit overflow, second subidentifier oid_get_numeric_string:"2BC080808000":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" + +OID get numeric string - overlong encoding +oid_get_numeric_string:"8001":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" + +OID get numeric string - overlong encoding, second subidentifier +oid_get_numeric_string:"2B8001":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" From 21b83879299d27c1d3f5b1399354d68e30d89b9e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 15 Feb 2023 13:07:49 +0000 Subject: [PATCH 5/9] Add ChangeLog for OID-to-string fixes Signed-off-by: David Horstmann --- ChangeLog.d/fix-oid-to-string-bugs.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/fix-oid-to-string-bugs.txt diff --git a/ChangeLog.d/fix-oid-to-string-bugs.txt b/ChangeLog.d/fix-oid-to-string-bugs.txt new file mode 100644 index 000000000..799f44474 --- /dev/null +++ b/ChangeLog.d/fix-oid-to-string-bugs.txt @@ -0,0 +1,6 @@ +Bugfix + * Fix bug in conversion from OID to string in + mbedtls_oid_get_numeric_string(). OIDs such as 2.40.0.25 are now printed + correctly. + * Reject OIDs with overlong-encoded subidentifiers when converting + OID-to-string. From 34b3f1b7576565975715f4732c506e4dd49cccdd Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 15 Feb 2023 13:46:53 +0000 Subject: [PATCH 6/9] Make overflow checks more readable 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 17d3e093a..22f1f1c23 100644 --- a/library/oid.c +++ b/library/oid.c @@ -844,7 +844,7 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, while (i < oid->len && ((oid->p[i] & 0x80) != 0)) { /* Prevent overflow in value. */ - if (((value << 7) >> 7) != value) { + if (value > (UINT_MAX >> 7)) { return MBEDTLS_ERR_OID_BUF_TOO_SMALL; } @@ -873,7 +873,7 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, value = 0; for (; i < oid->len; i++) { /* Prevent overflow in value. */ - if (((value << 7) >> 7) != value) { + if (value > (UINT_MAX >> 7)) { return MBEDTLS_ERR_OID_BUF_TOO_SMALL; } if ((value == 0) && ((oid->p[i]) == 0x80)) { From f51851dc702c475eb6729fa04c1d3f1bec11e1c2 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 15 Feb 2023 15:44:24 +0000 Subject: [PATCH 7/9] Change += to |= for clearer semantics 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 22f1f1c23..acea12585 100644 --- a/library/oid.c +++ b/library/oid.c @@ -848,7 +848,7 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, return MBEDTLS_ERR_OID_BUF_TOO_SMALL; } - value += oid->p[i] & 0x7F; + value |= oid->p[i] & 0x7F; value <<= 7; i++; } @@ -856,7 +856,7 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, return MBEDTLS_ERR_OID_BUF_TOO_SMALL; } /* Last byte of first subidentifier */ - value += oid->p[i] & 0x7F; + value |= oid->p[i] & 0x7F; i++; unsigned int component1 = value / 40; @@ -882,7 +882,7 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, } value <<= 7; - value += oid->p[i] & 0x7F; + value |= oid->p[i] & 0x7F; if (!(oid->p[i] & 0x80)) { /* Last byte */ From 5b5a0b618c21b372f3db800991181a6f41e12e1f Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 20 Feb 2023 14:21:23 +0000 Subject: [PATCH 8/9] Change error codes to more appropriate codes The more precise error codes are borrowed from the ASN1 module. Signed-off-by: David Horstmann --- library/oid.c | 10 +++++----- tests/suites/test_suite_oid.data | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/library/oid.c b/library/oid.c index acea12585..86214b23a 100644 --- a/library/oid.c +++ b/library/oid.c @@ -839,13 +839,13 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, value = 0; if ((oid->p[0]) == 0x80) { /* Overlong encoding is not allowed */ - return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + return MBEDTLS_ERR_ASN1_INVALID_DATA; } while (i < oid->len && ((oid->p[i] & 0x80) != 0)) { /* Prevent overflow in value. */ if (value > (UINT_MAX >> 7)) { - return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + return MBEDTLS_ERR_ASN1_INVALID_DATA; } value |= oid->p[i] & 0x7F; @@ -853,7 +853,7 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, i++; } if (i >= oid->len) { - return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + return MBEDTLS_ERR_ASN1_OUT_OF_DATA; } /* Last byte of first subidentifier */ value |= oid->p[i] & 0x7F; @@ -874,11 +874,11 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size, for (; i < oid->len; i++) { /* Prevent overflow in value. */ if (value > (UINT_MAX >> 7)) { - return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + return MBEDTLS_ERR_ASN1_INVALID_DATA; } if ((value == 0) && ((oid->p[i]) == 0x80)) { /* Overlong encoding is not allowed */ - return MBEDTLS_ERR_OID_BUF_TOO_SMALL; + return MBEDTLS_ERR_ASN1_INVALID_DATA; } value <<= 7; diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data index f4801c426..b9fa6543d 100644 --- a/tests/suites/test_suite_oid.data +++ b/tests/suites/test_suite_oid.data @@ -102,20 +102,20 @@ OID get numeric string - multi-byte first subidentifier oid_get_numeric_string:"8837":0:"2.999" OID get numeric string - empty oid buffer -oid_get_numeric_string:"":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" +oid_get_numeric_string:"":MBEDTLS_ERR_ASN1_OUT_OF_DATA:"" OID get numeric string - no final / all bytes have top bit set -oid_get_numeric_string:"818181":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" +oid_get_numeric_string:"818181":MBEDTLS_ERR_ASN1_OUT_OF_DATA:"" # Encodes the number 0x0400000000 as a subidentifier which overflows 32-bits OID get numeric string - 32-bit overflow -oid_get_numeric_string:"C080808000":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" +oid_get_numeric_string:"C080808000":MBEDTLS_ERR_ASN1_INVALID_DATA:"" OID get numeric string - 32-bit overflow, second subidentifier -oid_get_numeric_string:"2BC080808000":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" +oid_get_numeric_string:"2BC080808000":MBEDTLS_ERR_ASN1_INVALID_DATA:"" OID get numeric string - overlong encoding -oid_get_numeric_string:"8001":MBEDTLS_ERR_OID_BUF_TOO_SMALL:"" +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_OID_BUF_TOO_SMALL:"" +oid_get_numeric_string:"2B8001":MBEDTLS_ERR_ASN1_INVALID_DATA:"" From a4fad2ba6742699ba6daa9a82f9c461b3cda7f66 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 20 Feb 2023 14:57:47 +0000 Subject: [PATCH 9/9] Correct error code in test_suite_x509parse.data Signed-off-by: David Horstmann --- tests/suites/test_suite_x509parse.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 01da08b54..048e4f74c 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -2550,7 +2550,7 @@ X509 OID numstring #4 (larger number) x509_oid_numstr:"2a864886f70d":"1.2.840.113549":15:14 X509 OID numstring #5 (arithmetic overflow) -x509_oid_numstr:"2a8648f9f8f7f6f5f4f3f2f1f001":"":100:MBEDTLS_ERR_OID_BUF_TOO_SMALL +x509_oid_numstr:"2a8648f9f8f7f6f5f4f3f2f1f001":"":100:MBEDTLS_ERR_ASN1_INVALID_DATA X509 CRT keyUsage #1 (no extension, expected KU) depends_on:MBEDTLS_RSA_C:MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA_BASED_ON_USE_PSA