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

@@ -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 <mbedtls/bignum.h>
 #include <mbedtls/ctr_drbg.h>
@@ -39,9 +41,9 @@
 #include <string.h>
 
 #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);
     }

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.

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 <algorithm>
 #include <array>
 #include <gtest/gtest.h>
+#include <mbedtls/asn1.h>
 #include <string>
 #include <vector>
 
 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<class T>
   static std::vector<T> toArray(const std::string &str)
   {
-    std::vector<T> res;
-    std::copy(str.begin(), str.end(), std::back_inserter(res));
+    std::vector<T> 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<unsigned char> &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<char, OC_UUID_LEN> 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<unsigned char>("");
+  std::array<char, OC_UUID_LEN> 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<unsigned char>(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<unsigned char>(getUUID("leet:"));
+  EXPECT_FALSE(oc_certs_parse_CN_buffer_for_UUID(
+    getMbedTLSAsn1Buffer(invalid_uuid), CN_uuid.data(), CN_uuid.size()));
+
+  // correct format (uuid:<UUID string>), but buffer is too small
+  auto valid_uuid = toArray<unsigned char>(getUUID("uuid:"));
+  std::array<char, OC_UUID_LEN - 1> 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<unsigned char>("uuid:" + uuid);
+  std::array<char, OC_UUID_LEN> 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<unsigned char>("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 */