Merge pull request #7276 from yanrayw/2.28-6173-split-TLS-connection-func-into-ssl_helpers
Backport 2.28: Move TLS connection helper code from test_suite_ssl.function to ssl_helpers.c
diff --git a/ChangeLog.d/fix-oid-to-string-bugs.txt b/ChangeLog.d/fix-oid-to-string-bugs.txt
index 799f444..3cf02c3 100644
--- a/ChangeLog.d/fix-oid-to-string-bugs.txt
+++ b/ChangeLog.d/fix-oid-to-string-bugs.txt
@@ -3,4 +3,8 @@
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.
+ them to a string.
+ * Reject OIDs with subidentifier values exceeding UINT_MAX. Such
+ subidentifiers can be valid, but Mbed TLS cannot currently handle them.
+ * Reject OIDs that have unterminated subidentifiers, or (equivalently)
+ have the most-significant bit set in their last byte.
diff --git a/library/common.h b/library/common.h
index 2786c97..e162aa3 100644
--- a/library/common.h
+++ b/library/common.h
@@ -29,6 +29,7 @@
#include "mbedtls/config.h"
#endif
+#include <assert.h>
#include <stddef.h>
#include <stdint.h>
@@ -347,4 +348,18 @@
}
#endif
+/* Always provide a static assert macro, so it can be used unconditionally.
+ * It will expand to nothing on some systems.
+ * Can be used outside functions (but don't add a trailing ';' in that case:
+ * the semicolon is included here to avoid triggering -Wextra-semi when
+ * MBEDTLS_STATIC_ASSERT() expands to nothing).
+ * Can't use the C11-style `defined(static_assert)` on FreeBSD, since it
+ * defines static_assert even with -std=c99, but then complains about it.
+ */
+#if defined(static_assert) && !defined(__FreeBSD__)
+#define MBEDTLS_STATIC_ASSERT(expr, msg) static_assert(expr, msg);
+#else
+#define MBEDTLS_STATIC_ASSERT(expr, msg)
+#endif
+
#endif /* MBEDTLS_LIBRARY_COMMON_H */
diff --git a/library/oid.c b/library/oid.c
index 4ec752f..12a9650 100644
--- a/library/oid.c
+++ b/library/oid.c
@@ -775,65 +775,26 @@
cipher_alg)
#endif /* MBEDTLS_PKCS12_C */
-#define OID_SAFE_SNPRINTF \
- do { \
- if (ret < 0 || (size_t) ret >= n) \
- return MBEDTLS_ERR_OID_BUF_TOO_SMALL; \
- \
- n -= (size_t) ret; \
- p += (size_t) ret; \
- } while (0)
-
/* Return the x.y.z.... style numeric string for the given OID */
int mbedtls_oid_get_numeric_string(char *buf, size_t size,
const mbedtls_asn1_buf *oid)
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
- size_t i, n;
- unsigned int value;
- char *p;
+ char *p = buf;
+ size_t n = size;
+ unsigned int value = 0;
- p = buf;
- n = 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_ASN1_INVALID_DATA;
+ if (size > INT_MAX) {
+ /* Avoid overflow computing return value */
+ return MBEDTLS_ERR_ASN1_INVALID_LENGTH;
}
- while (i < oid->len && ((oid->p[i] & 0x80) != 0)) {
- /* Prevent overflow in value. */
- if (value > (UINT_MAX >> 7)) {
- return MBEDTLS_ERR_ASN1_INVALID_DATA;
- }
-
- value |= oid->p[i] & 0x7F;
- value <<= 7;
- i++;
- }
- if (i >= oid->len) {
+ if (oid->len <= 0) {
+ /* OID must not be empty */
return MBEDTLS_ERR_ASN1_OUT_OF_DATA;
}
- /* 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 < oid->len; i++) {
+ for (size_t i = 0; i < oid->len; i++) {
/* Prevent overflow in value. */
if (value > (UINT_MAX >> 7)) {
return MBEDTLS_ERR_ASN1_INVALID_DATA;
@@ -848,12 +809,38 @@
if (!(oid->p[i] & 0x80)) {
/* Last byte */
- ret = mbedtls_snprintf(p, n, ".%u", value);
- OID_SAFE_SNPRINTF;
+ if (n == size) {
+ int component1;
+ unsigned int component2;
+ /* First subidentifier contains first two OID components */
+ if (value >= 80) {
+ component1 = '2';
+ component2 = value - 80;
+ } else if (value >= 40) {
+ component1 = '1';
+ component2 = value - 40;
+ } else {
+ component1 = '0';
+ component2 = value;
+ }
+ ret = mbedtls_snprintf(p, n, "%c.%u", component1, component2);
+ } else {
+ ret = mbedtls_snprintf(p, n, ".%u", value);
+ }
+ if (ret < 2 || (size_t) ret >= n) {
+ return MBEDTLS_ERR_OID_BUF_TOO_SMALL;
+ }
+ n -= (size_t) ret;
+ p += ret;
value = 0;
}
}
+ if (value != 0) {
+ /* Unterminated subidentifier */
+ return MBEDTLS_ERR_ASN1_OUT_OF_DATA;
+ }
+
return (int) (size - n);
}
diff --git a/library/psa_crypto.c b/library/psa_crypto.c
index d8a9940..bad7f46 100644
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@@ -47,7 +47,6 @@
#include "psa_crypto_random_impl.h"
-#include <assert.h>
#include <stdlib.h>
#include <string.h>
#include "mbedtls/platform.h"
@@ -1512,14 +1511,12 @@
return (status == PSA_SUCCESS) ? unlock_status : status;
}
-#if defined(static_assert)
-static_assert((MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
- "One or more key attribute flag is listed as both external-only and dual-use");
-static_assert((PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
- "One or more key attribute flag is listed as both internal-only and dual-use");
-static_assert((PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY) == 0,
- "One or more key attribute flag is listed as both internal-only and external-only");
-#endif
+MBEDTLS_STATIC_ASSERT((MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
+ "One or more key attribute flag is listed as both external-only and dual-use")
+MBEDTLS_STATIC_ASSERT((PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
+ "One or more key attribute flag is listed as both internal-only and dual-use")
+MBEDTLS_STATIC_ASSERT((PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY) == 0,
+ "One or more key attribute flag is listed as both internal-only and external-only")
/** Validate that a key policy is internally well-formed.
*
@@ -1782,11 +1779,10 @@
psa_key_slot_number_t slot_number =
psa_key_slot_get_slot_number(slot);
-#if defined(static_assert)
- static_assert(sizeof(slot_number) ==
- sizeof(data.slot_number),
- "Slot number size does not match psa_se_key_data_storage_t");
-#endif
+ MBEDTLS_STATIC_ASSERT(sizeof(slot_number) ==
+ sizeof(data.slot_number),
+ "Slot number size does not match psa_se_key_data_storage_t");
+
memcpy(&data.slot_number, &slot_number, sizeof(slot_number));
status = psa_save_persistent_key(&slot->attr,
(uint8_t *) &data,
diff --git a/library/psa_crypto_se.c b/library/psa_crypto_se.c
index b660393..7bea10a 100644
--- a/library/psa_crypto_se.c
+++ b/library/psa_crypto_se.c
@@ -22,7 +22,6 @@
#if defined(MBEDTLS_PSA_CRYPTO_SE_C)
-#include <assert.h>
#include <stdint.h>
#include <string.h>
@@ -315,10 +314,8 @@
}
/* Driver table entries are 0-initialized. 0 is not a valid driver
* location because it means a transparent key. */
-#if defined(static_assert)
- static_assert(PSA_KEY_LOCATION_LOCAL_STORAGE == 0,
- "Secure element support requires 0 to mean a local key");
-#endif
+ MBEDTLS_STATIC_ASSERT(PSA_KEY_LOCATION_LOCAL_STORAGE == 0,
+ "Secure element support requires 0 to mean a local key");
if (location == PSA_KEY_LOCATION_LOCAL_STORAGE) {
return PSA_ERROR_INVALID_ARGUMENT;
}
diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data
index 38d8b7e..2d33141 100644
--- a/tests/suites/test_suite_oid.data
+++ b/tests/suites/test_suite_oid.data
@@ -101,12 +101,30 @@
OID get numeric string - multi-byte first subidentifier
oid_get_numeric_string:"8837":0:"2.999"
+OID get numeric string - second subidentifier not terminated
+oid_get_numeric_string:"0081":MBEDTLS_ERR_ASN1_OUT_OF_DATA:""
+
OID get numeric string - empty oid buffer
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_ASN1_OUT_OF_DATA:""
+OID get numeric string - 0.39
+oid_get_numeric_string:"27":0:"0.39"
+
+OID get numeric string - 1.0
+oid_get_numeric_string:"28":0:"1.0"
+
+OID get numeric string - 1.39
+oid_get_numeric_string:"4f":0:"1.39"
+
+OID get numeric string - 2.0
+oid_get_numeric_string:"50":0:"2.0"
+
+OID get numeric string - 1 byte first subidentifier beyond 2.39
+oid_get_numeric_string:"7f":0:"2.47"
+
# 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_ASN1_INVALID_DATA:""
diff --git a/tests/suites/test_suite_oid.function b/tests/suites/test_suite_oid.function
index 7759baf..c06e337 100644
--- a/tests/suites/test_suite_oid.function
+++ b/tests/suites/test_suite_oid.function
@@ -104,13 +104,16 @@
int ret;
input_oid.tag = MBEDTLS_ASN1_OID;
- input_oid.p = oid->x;
+ /* Test that an empty OID is not dereferenced */
+ input_oid.p = oid->len ? oid->x : (void *) 1;
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);
+ TEST_EQUAL(ret, strlen(result_str));
+ TEST_ASSERT(ret >= 3);
+ TEST_EQUAL(strcmp(buf, result_str), 0);
} else {
TEST_EQUAL(ret, error_ret);
}