Rework name verification code so that a match is indicated via an argument,

rather than return codes. More strictly follow RFC 6125, in particular only
check the CN if there are no SAN identifiers present in the certificate
(per section 6.4.4).

Previous behaviour questioned by Daniel Stenberg <daniel at haxx dot se>.

ok beck@ jca@

1 files changed, 51 insertions, 33 deletions

diff --git a/src/lib/libtls/tls_verify.c b/src/lib/libtls/tls_verify.c
index 23e58ebef7..3bd1057d0c 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.18 2016/11/04 15:32:40 jsing Exp $ */
+/* $OpenBSD: tls_verify.c,v 1.19 2017/04/10 17:11:13 jsing Exp $ */
 /*
  * Copyright (c) 2014 Jeremie Courreges-Anglas <jca@openbsd.org>
  *
@@ -27,11 +27,6 @@
 #include <tls.h>
 #include "tls_internal.h"
 
-static int tls_match_name(const char *cert_name, const char *name);
-static int tls_check_subject_altname(struct tls *ctx, X509 *cert,
-    const char *name);
-static int tls_check_common_name(struct tls *ctx, X509 *cert, const char *name);
-
 static int
 tls_match_name(const char *cert_name, const char *name)
 {
@@ -84,20 +79,28 @@ tls_match_name(const char *cert_name, const char *name)
         return -1;
 }
 
-/* See RFC 5280 section 4.2.1.6 for SubjectAltName details. */
+/*
+ * See RFC 5280 section 4.2.1.6 for SubjectAltName details.
+ * alt_match is set to 1 if a matching alternate name is found.
+ * alt_exists is set to 1 if any known alternate name exists in the certificate.
+ */
 static int
-tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
+tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name,
+    int *alt_match, int *alt_exists)
 {
         STACK_OF(GENERAL_NAME) *altname_stack = NULL;
         union tls_addr addrbuf;
         int addrlen, type;
         int count, i;
-        int rv = -1;
+        int rv = 0;
+
+        *alt_match = 0;
+        *alt_exists = 0;
 
         altname_stack = X509_get_ext_d2i(cert, NID_subject_alt_name,
             NULL, NULL);
         if (altname_stack == NULL)
-                return -1;
+                return 0;
 
         if (inet_pton(AF_INET, name, &addrbuf) == 1) {
                 type = GEN_IPADD;
@@ -115,6 +118,10 @@ tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
                 GENERAL_NAME    *altname;
 
                 altname = sk_GENERAL_NAME_value(altname_stack, i);
+
+                if (altname->type == GEN_DNS || altname->type == GEN_IPADD)
+                        *alt_exists = 1;
+
                 if (altname->type != type)
                         continue;
 
@@ -133,7 +140,7 @@ tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
                                             "NUL byte in subjectAltName, "
                                             "probably a malicious certificate",
                                             name);
-                                        rv = -2;
+                                        rv = -1;
                                         break;
                                 }
 
@@ -143,16 +150,16 @@ tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
                                  * dNSName must be rejected.
                                  */
                                 if (strcmp(data, " ") == 0) {
-                                        tls_set_error(ctx,
+                                        tls_set_errorx(ctx,
                                             "error verifying name '%s': "
                                             "a dNSName of \" \" must not be "
                                             "used", name);
-                                        rv = -2;
+                                        rv = -1;
                                         break;
                                 }
 
                                 if (tls_match_name(data, name) == 0) {
-                                        rv = 0;
+                                        *alt_match = 1;
                                         break;
                                 }
                         } else {
@@ -174,7 +181,7 @@ tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
                                 tls_set_errorx(ctx,
                                     "Unexpected negative length for an "
                                     "IP address: %d", datalen);
-                                rv = -2;
+                                rv = -1;
                                 break;
                         }
 
@@ -184,7 +191,7 @@ tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
                          */
                         if (datalen == addrlen &&
                             memcmp(data, &addrbuf, addrlen) == 0) {
-                                rv = 0;
+                                *alt_match = 1;
                                 break;
                         }
                 }
@@ -195,13 +202,16 @@ tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
 }
 
 static int
-tls_check_common_name(struct tls *ctx, X509 *cert, const char *name)
+tls_check_common_name(struct tls *ctx, X509 *cert, const char *name,
+    int *cn_match)
 {
         X509_NAME *subject_name;
         char *common_name = NULL;
         union tls_addr addrbuf;
         int common_name_len;
-        int rv = -1;
+        int rv = 0;
+
+        *cn_match = 0;
 
         subject_name = X509_get_subject_name(cert);
         if (subject_name == NULL)
@@ -225,38 +235,46 @@ tls_check_common_name(struct tls *ctx, X509 *cert, const char *name)
                 tls_set_errorx(ctx, "error verifying name '%s': "
                     "NUL byte in Common Name field, "
                     "probably a malicious certificate", name);
-                rv = -2;
+                rv = -1;
                 goto out;
         }
 
+        /*
+         * We don't want to attempt wildcard matching against IP addresses,
+         * so perform a simple comparison here.
+         */
         if (inet_pton(AF_INET,  name, &addrbuf) == 1 ||
             inet_pton(AF_INET6, name, &addrbuf) == 1) {
-                /*
-                 * We don't want to attempt wildcard matching against IP
-                 * addresses, so perform a simple comparison here.
-                 */
                 if (strcmp(common_name, name) == 0)
-                        rv = 0;
-                else
-                        rv = -1;
+                        *cn_match = 1;
                 goto out;
         }
 
         if (tls_match_name(common_name, name) == 0)
-                rv = 0;
+                *cn_match = 1;
+
  out:
         free(common_name);
         return rv;
 }
 
 int
-tls_check_name(struct tls *ctx, X509 *cert, const char *name)
+tls_check_name(struct tls *ctx, X509 *cert, const char *name, int *match)
 {
-        int     rv;
+        int alt_exists;
 
-        rv = tls_check_subject_altname(ctx, cert, name);
-        if (rv == 0 || rv == -2)
-                return rv;
+        *match = 0;
+
+        if (tls_check_subject_altname(ctx, cert, name, match,
+            &alt_exists) == -1)
+                return -1;
+
+        /*
+         * As per RFC 6125 section 6.4.4, if any known alternate name existed
+         * in the certificate, we do not attempt to match on the CN.
+         */
+        if (*match || alt_exists)
+                return 0;
 
-        return tls_check_common_name(ctx, cert, name);
+        return tls_check_common_name(ctx, cert, name, match);
 }