Refactor tls_check_common_name to use lower level API.

X509_NAME_get_text_by_NID is kind of a bad interface that
we wish to make safer, and does not give us the visibility
we really want here to detect hostile things.

Instead call the lower level functions to do some better
checking that should be done by X509_NAME_get_text_by_NID,
but is not in the OpenSSL version. Specifically we will treat
the input as hostile and fail if:

1) The certificate contains more than one CN in the subject.
2) The CN does not decode as UTF-8
3) The CN is of invalid length (must be between 1 and 64 bytes)
4) The CN contains a 0 byte

4) matches the existing logic, 1 and 2, and 3 are new checks.

ok tb@

2 files changed, 51 insertions, 14 deletions

diff --git a/src/lib/libtls/tls_verify.c b/src/lib/libtls/tls_verify.c
index 0cb86f686d..420e278c99 100644
--- a/src/lib/libtls/tls_verify.c
+++ b/src/lib/libtls/tls_verify.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_verify.c,v 1.23 2023/05/11 07:35:27 tb Exp $ */
+/* $OpenBSD: tls_verify.c,v 1.24 2023/05/28 09:02:01 beck Exp $ */
 /*
  * Copyright (c) 2014 Jeremie Courreges-Anglas <jca@openbsd.org>
  *
@@ -205,10 +205,13 @@ static int
 tls_check_common_name(struct tls *ctx, X509 *cert, const char *name,
     int *cn_match)
 {
+        unsigned char *utf8_bytes = NULL;
         X509_NAME *subject_name;
         char *common_name = NULL;
         union tls_addr addrbuf;
         int common_name_len;
+        ASN1_STRING *data;
+        int lastpos = -1;
         int rv = -1;
 
         *cn_match = 0;
@@ -217,29 +220,60 @@ tls_check_common_name(struct tls *ctx, X509 *cert, const char *name,
         if (subject_name == NULL)
                 goto done;
 
-        common_name_len = X509_NAME_get_text_by_NID(subject_name,
-            NID_commonName, NULL, 0);
-        if (common_name_len < 0)
+        lastpos = X509_NAME_get_index_by_NID(subject_name,
+            NID_commonName, lastpos);
+        if (lastpos == -1)
                 goto done;
+        if (X509_NAME_get_index_by_NID(subject_name, NID_commonName, lastpos)
+            != -1) {
+                /*
+                 * Having multiple CN's is possible, and even happened back in
+                 * the glory days of mullets and Hammer pants. In anything like
+                 * a modern TLS cert, CN is as close to deprecated as it gets,
+                 * and having more than one is bad. We therefore fail if we have
+                 * more than one CN fed to us in the subject, treating the
+                 * certificate as hostile.
+                 */
+                tls_set_errorx(ctx, "error verifying name '%s': "
+                    "Certificate subject contains mutiple Common Name fields, "
+                    "probably a malicious or malformed certificate", name);
+                goto err;
+        }
 
-        common_name = calloc(common_name_len + 1, 1);
-        if (common_name == NULL) {
-                tls_set_error(ctx, "out of memory");
+        data = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject_name,
+            lastpos));
+        /*
+         * Fail if we cannot encode as UTF-8, or if the UTF-8 encoding of the
+         * string contains a 0 byte. We treat any certificate with such data
+         * in the CN as hostile and fail.
+         */
+        if ((common_name_len = ASN1_STRING_to_UTF8(&utf8_bytes, data)) < 0) {
+                tls_set_errorx(ctx, "error verifying name '%s': "
+                    "Common Name field cannot be encoded as a UTF-8 string, "
+                    "probably a malicious certificate", name);
                 goto err;
         }
 
-        X509_NAME_get_text_by_NID(subject_name, NID_commonName, common_name,
-            common_name_len + 1);
+        if (common_name_len < 1 || common_name_len > 64) {
+                tls_set_errorx(ctx, "error verifying name '%s': "
+                    "Common Name field has invalid length, "
+                    "probably a malicious certificate", name);
+                goto err;
+        }
 
-        /* NUL bytes in CN? */
-        if (common_name_len < 0 ||
-            (size_t)common_name_len != strlen(common_name)) {
+        if (memchr(utf8_bytes, 0, common_name_len) != NULL) {
                 tls_set_errorx(ctx, "error verifying name '%s': "
                     "NUL byte in Common Name field, "
                     "probably a malicious certificate", name);
                 goto err;
         }
 
+        common_name = strndup(utf8_bytes, common_name_len);
+        if (common_name == NULL) {
+                tls_set_error(ctx, "out of memory");
+                goto err;
+        }
+
         /*
          * We don't want to attempt wildcard matching against IP addresses,
          * so perform a simple comparison here.
@@ -258,6 +292,7 @@ tls_check_common_name(struct tls *ctx, X509 *cert, const char *name,
         rv = 0;
 
  err:
+        free(utf8_bytes);
         free(common_name);
         return rv;
 }
diff --git a/src/regress/lib/libtls/verify/verifytest.c b/src/regress/lib/libtls/verify/verifytest.c
index b41b62fcfb..57aa992149 100644
--- a/src/regress/lib/libtls/verify/verifytest.c
+++ b/src/regress/lib/libtls/verify/verifytest.c
@@ -1,4 +1,4 @@
-/*      $OpenBSD: verifytest.c,v 1.7 2017/04/30 03:53:31 jsing Exp $    */
+/*      $OpenBSD: verifytest.c,v 1.8 2023/05/28 09:02:01 beck Exp $     */
 /*
  * Copyright (c) 2014 Joel Sing <jsing@openbsd.org>
  *
@@ -40,6 +40,7 @@ struct verify_test {
         const char name[128];
         int want_return;
         int want_match;
+        int name_type;
 };
 
 struct verify_test verify_tests[] = {
@@ -474,7 +475,8 @@ do_verify_test(int test_no, struct verify_test *vt)
                 if ((name = X509_NAME_new()) == NULL)
                         errx(1, "failed to malloc X509_NAME");
                 if (X509_NAME_add_entry_by_NID(name, NID_commonName,
-                    MBSTRING_ASC, (unsigned char *)vt->common_name,
+                    vt->name_type ? vt->name_type : MBSTRING_ASC,
+                    (unsigned char *)vt->common_name,
                     vt->common_name_len, -1, 0) == 0)
                         errx(1, "failed to add name entry");
                 if (X509_set_subject_name(cert, name) == 0)