From 0772309ccec28f8bba7b870cf78cd8ac2cf34287 Mon Sep 17 00:00:00 2001 From: Daniel Adam Date: Tue, 18 Apr 2023 10:48:41 +0200 Subject: [PATCH] security: fix parsing of UUID from Common Name field Fix segfault when the Common Name field wasn't in the expected format and add tests. --- security/oc_certs.c | 73 +++++++++++++++++++++++---------- security/oc_certs_internal.h | 4 ++ security/unittest/certstest.cpp | 70 ++++++++++++++++++++++++++++++- 3 files changed, 124 insertions(+), 23 deletions(-) diff --git a/security/oc_certs.c b/security/oc_certs.c index 934f768699..0a620a210f 100644 --- a/security/oc_certs.c +++ b/security/oc_certs.c @@ -25,10 +25,12 @@ #include "oc_helpers.h" #include "oc_uuid.h" #include "port/oc_assert.h" +#include "port/oc_log.h" #include "security/oc_certs_internal.h" #include "security/oc_certs_validate_internal.h" #include "security/oc_entropy_internal.h" #include "security/oc_tls_internal.h" +#include "util/oc_macros.h" #include #include @@ -39,9 +41,9 @@ #include #define UUID_PREFIX "uuid:" -#define UUID_PREFIX_LEN (sizeof(UUID_PREFIX) - 1) +#define UUID_PREFIX_LEN (OC_CHAR_ARRAY_LEN(UUID_PREFIX)) #define CN_UUID_PREFIX "CN=uuid:" -#define CN_UUID_PREFIX_LEN (sizeof(CN_UUID_PREFIX) - 1) +#define CN_UUID_PREFIX_LEN (OC_CHAR_ARRAY_LEN(CN_UUID_PREFIX)) // message digest used for signature of generated certificates or certificate // signing requests (CSRs) @@ -395,15 +397,54 @@ oc_certs_encode_CN_with_UUID(const oc_uuid_t *uuid, char *buf, size_t buf_len) return true; } +static const char * +certs_find_uuid_prefix(const char *haystack, size_t haystack_len) +{ + for (size_t i = 0; i < haystack_len - UUID_PREFIX_LEN + 1; ++i) { + const char *start = haystack + i; + if (memcmp(start, UUID_PREFIX, UUID_PREFIX_LEN) == 0) { + return start; + } + } + return NULL; +} + bool -oc_certs_extract_CN_for_UUID(const mbedtls_x509_crt *cert, char *buffer, - size_t buffer_size) +oc_certs_parse_CN_buffer_for_UUID(mbedtls_asn1_buf val, char *buffer, + size_t buffer_size) { if (buffer_size < OC_UUID_LEN) { OC_ERR("buffer too small"); return false; } + const char *uuid_CN = (const char *)val.p; + const char *uuid_prefix = NULL; + if (val.len >= UUID_PREFIX_LEN + OC_UUID_LEN - + 1) { // -1 because val is not nul-terminated + uuid_prefix = certs_find_uuid_prefix(uuid_CN, val.len); + } + if (uuid_prefix == NULL) { +#ifdef OC_DEBUG + oc_string_t cn; + oc_new_string(&cn, uuid_CN, val.len); + OC_ERR("invalid Common Name field (tag:%d val:%s)", val.tag, oc_string(cn)); + oc_free_string(&cn); +#endif /* OC_DEBUG */ + return false; + } + + size_t uuid_prefix_len = (uuid_prefix - uuid_CN) + UUID_PREFIX_LEN; + memcpy(buffer, val.p + uuid_prefix_len, OC_UUID_LEN - 1); + buffer[OC_UUID_LEN - 1] = '\0'; + return true; +} + +bool +oc_certs_extract_CN_for_UUID(const mbedtls_x509_crt *cert, char *buffer, + size_t buffer_size) +{ + const mbedtls_asn1_named_data *subject = (mbedtls_asn1_named_data *)&(cert->subject); while (subject != NULL) { @@ -417,18 +458,7 @@ oc_certs_extract_CN_for_UUID(const mbedtls_x509_crt *cert, char *buffer, return false; } - if (subject->val.len < UUID_PREFIX_LEN + OC_UUID_LEN - - 1) { // -1 because val is not nul-terminated - OC_ERR("invalid Common Name field"); - return false; - } - const char *uuid_CN = (const char *)subject->val.p; - const char *uuid_prefix = strstr(uuid_CN, UUID_PREFIX); - size_t uuid_prefix_len = (uuid_CN - uuid_prefix) + UUID_PREFIX_LEN; - - memcpy(buffer, subject->val.p + uuid_prefix_len, OC_UUID_LEN - 1); - buffer[OC_UUID_LEN - 1] = '\0'; - return true; + return oc_certs_parse_CN_buffer_for_UUID(subject->val, buffer, buffer_size); } bool @@ -523,7 +553,8 @@ oc_certs_extract_first_role(const mbedtls_x509_crt *cert, oc_string_t *role, if (oc_certs_DN_is_CN(directoryName)) { dnRole = directoryName; } - /* Look for an Organizational Unit (OU) component in the directoryName */ + /* Look for an Organizational Unit (OU) component in the directoryName + */ else if (oc_certs_DN_is_OU(directoryName)) { dnAuthority = directoryName; } @@ -538,10 +569,10 @@ oc_certs_extract_first_role(const mbedtls_x509_crt *cert, oc_string_t *role, } if (dnAuthority == NULL) { - /* If the OU component was absent in the directoryName, it is assumed that - * the issuer of this role certificate is the "authority". Accordingly, - * extract the issuer's name from the issuer's Common Name (CN) component - * and store it. + /* If the OU component was absent in the directoryName, it is assumed + * that the issuer of this role certificate is the "authority". + * Accordingly, extract the issuer's name from the issuer's Common Name + * (CN) component and store it. */ dnAuthority = oc_certs_CN_extract_issuer(cert); } diff --git a/security/oc_certs_internal.h b/security/oc_certs_internal.h index 48d68c8080..34e990ab9a 100644 --- a/security/oc_certs_internal.h +++ b/security/oc_certs_internal.h @@ -123,6 +123,10 @@ int oc_certs_parse_public_key_to_oc_string(const unsigned char *cert, bool oc_certs_encode_CN_with_UUID(const oc_uuid_t *uuid, char *buf, size_t buf_len); +/** @brief Helper function to extract encoded UUID in a mbedTLS buffer */ +bool oc_certs_parse_CN_buffer_for_UUID(mbedtls_asn1_buf val, char *buffer, + size_t buffer_size); + /** * @brief Extract uuid stored in the subject's Common name field in a x509 * certificate. diff --git a/security/unittest/certstest.cpp b/security/unittest/certstest.cpp index dad1c46454..5015554f04 100644 --- a/security/unittest/certstest.cpp +++ b/security/unittest/certstest.cpp @@ -19,16 +19,22 @@ #if defined(OC_SECURITY) && defined(OC_PKI) #include "oc_certs.h" +#include "port/oc_random.h" #include "security/oc_certs_internal.h" #include #include #include +#include #include #include class TestCerts : public testing::Test { public: + static void SetUpTestCase() { oc_random_init(); } + + static void TearDownTestCase() { oc_random_destroy(); } + void TearDown() override { // restore defaults @@ -38,8 +44,8 @@ class TestCerts : public testing::Test { template static std::vector toArray(const std::string &str) { - std::vector res; - std::copy(str.begin(), str.end(), std::back_inserter(res)); + std::vector res{}; + std::copy(std::begin(str), std::end(str), std::back_inserter(res)); return res; } }; @@ -204,4 +210,64 @@ TEST_F(TestCerts, AllowedEllipticCurves) } } +static mbedtls_asn1_buf +getMbedTLSAsn1Buffer(std::vector &bytes) +{ + mbedtls_asn1_buf buf{}; + buf.p = bytes.data(); + buf.len = bytes.size(); + return buf; +} + +static std::string +getUUID(const std::string &prefix = "") +{ + oc_uuid_t uuid{}; + oc_gen_uuid(&uuid); + std::array uuid_str{}; + oc_uuid_to_str(&uuid, uuid_str.data(), uuid_str.size()); + return prefix + uuid_str.data(); +} + +TEST_F(TestCerts, ExtractUUIDFromCommonNameFail) +{ + // invalid CN: empty str + auto empty = toArray(""); + std::array CN_uuid{}; + EXPECT_FALSE(oc_certs_parse_CN_buffer_for_UUID( + getMbedTLSAsn1Buffer(empty), CN_uuid.data(), CN_uuid.size())); + + // invalid CN: invalid format (missing prefix "uuid:") + auto invalid_uuid = toArray(getUUID()); + EXPECT_FALSE(oc_certs_parse_CN_buffer_for_UUID( + getMbedTLSAsn1Buffer(invalid_uuid), CN_uuid.data(), CN_uuid.size())); + + // invalid CN: invalid format (invalid prefix "leet:") + invalid_uuid = toArray(getUUID("leet:")); + EXPECT_FALSE(oc_certs_parse_CN_buffer_for_UUID( + getMbedTLSAsn1Buffer(invalid_uuid), CN_uuid.data(), CN_uuid.size())); + + // correct format (uuid:), but buffer is too small + auto valid_uuid = toArray(getUUID("uuid:")); + std::array too_small{}; + EXPECT_FALSE(oc_certs_parse_CN_buffer_for_UUID( + getMbedTLSAsn1Buffer(valid_uuid), too_small.data(), too_small.size())); +} + +TEST_F(TestCerts, ExtractUUIDFromCommonName) +{ + std::string uuid = getUUID(); + auto uuid_encoded = toArray("uuid:" + uuid); + std::array CN_uuid{}; + EXPECT_TRUE(oc_certs_parse_CN_buffer_for_UUID( + getMbedTLSAsn1Buffer(uuid_encoded), CN_uuid.data(), CN_uuid.size())); + EXPECT_STREQ(uuid.c_str(), CN_uuid.data()); + + uuid_encoded = + toArray("prefix data in the CN field, uuid:" + uuid); + EXPECT_TRUE(oc_certs_parse_CN_buffer_for_UUID( + getMbedTLSAsn1Buffer(uuid_encoded), CN_uuid.data(), CN_uuid.size())); + EXPECT_STREQ(uuid.c_str(), CN_uuid.data()); +} + #endif /* OC_SECURITY && OC_PKI */