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@

5 files changed, 76 insertions, 47 deletions

diff --git a/src/lib/libtls/tls_client.c b/src/lib/libtls/tls_client.c
index a1e2caa717..0e519684ef 100644
--- a/src/lib/libtls/tls_client.c
+++ b/src/lib/libtls/tls_client.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_client.c,v 1.40 2017/01/26 12:56:37 jsing Exp $ */
+/* $OpenBSD: tls_client.c,v 1.41 2017/04/10 17:11:13 jsing Exp $ */
 /*
  * Copyright (c) 2014 Joel Sing <jsing@openbsd.org>
  *
@@ -289,7 +289,7 @@ int
 tls_handshake_client(struct tls *ctx)
 {
         X509 *cert = NULL;
-        int ssl_ret;
+        int match, ssl_ret;
         int rv = -1;
 
         if ((ctx->flags & TLS_CLIENT) == 0) {
@@ -311,11 +311,11 @@ tls_handshake_client(struct tls *ctx)
                         tls_set_errorx(ctx, "no server certificate");
                         goto err;
                 }
-                if ((rv = tls_check_name(ctx, cert,
-                    ctx->servername)) != 0) {
-                        if (rv != -2)
-                                tls_set_errorx(ctx, "name `%s' not present in"
-                                    " server certificate", ctx->servername);
+                if (tls_check_name(ctx, cert, ctx->servername, &match) == -1)
+                        goto err;
+                if (!match) {
+                        tls_set_errorx(ctx, "name `%s' not present in"
+                            " server certificate", ctx->servername);
                         goto err;
                 }
         }
diff --git a/src/lib/libtls/tls_internal.h b/src/lib/libtls/tls_internal.h
index 7bbc14ca86..bd23249e57 100644
--- a/src/lib/libtls/tls_internal.h
+++ b/src/lib/libtls/tls_internal.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_internal.h,v 1.56 2017/04/07 08:48:30 jsing Exp $ */
+/* $OpenBSD: tls_internal.h,v 1.57 2017/04/10 17:11:13 jsing Exp $ */
 /*
  * Copyright (c) 2014 Jeremie Courreges-Anglas <jca@openbsd.org>
  * Copyright (c) 2014 Joel Sing <jsing@openbsd.org>
@@ -186,7 +186,8 @@ void tls_sni_ctx_free(struct tls_sni_ctx *sni_ctx);
 struct tls *tls_new(void);
 struct tls *tls_server_conn(struct tls *ctx);
 
-int tls_check_name(struct tls *ctx, X509 *cert, const char *servername);
+int tls_check_name(struct tls *ctx, X509 *cert, const char *servername,
+    int *match);
 int tls_configure_server(struct tls *ctx);
 
 int tls_configure_ssl(struct tls *ctx, SSL_CTX *ssl_ctx);
diff --git a/src/lib/libtls/tls_peer.c b/src/lib/libtls/tls_peer.c
index 1a9065dfb1..ec97a30838 100644
--- a/src/lib/libtls/tls_peer.c
+++ b/src/lib/libtls/tls_peer.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_peer.c,v 1.7 2017/04/05 03:19:22 beck Exp $ */
+/* $OpenBSD: tls_peer.c,v 1.8 2017/04/10 17:11:13 jsing Exp $ */
 /*
  * Copyright (c) 2015 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2015 Bob Beck <beck@openbsd.org>
@@ -55,10 +55,15 @@ tls_peer_cert_provided(struct tls *ctx)
 int
 tls_peer_cert_contains_name(struct tls *ctx, const char *name)
 {
+        int match;
+
         if (ctx->ssl_peer_cert == NULL)
                 return (0);
 
-        return (tls_check_name(ctx, ctx->ssl_peer_cert, name) == 0);
+        if (tls_check_name(ctx, ctx->ssl_peer_cert, name, &match) == -1)
+                return (0);
+
+        return (match);
 }
 
 time_t
diff --git a/src/lib/libtls/tls_server.c b/src/lib/libtls/tls_server.c
index 51deff2510..39c6ca79e9 100644
--- a/src/lib/libtls/tls_server.c
+++ b/src/lib/libtls/tls_server.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_server.c,v 1.35 2017/01/31 15:57:43 jsing Exp $ */
+/* $OpenBSD: tls_server.c,v 1.36 2017/04/10 17:11:13 jsing Exp $ */
 /*
  * Copyright (c) 2014 Joel Sing <jsing@openbsd.org>
  *
@@ -75,11 +75,13 @@ tls_servername_cb(SSL *ssl, int *al, void *arg)
         union tls_addr addrbuf;
         struct tls *conn_ctx;
         const char *name;
+        int match;
 
         if ((conn_ctx = SSL_get_app_data(ssl)) == NULL)
                 goto err;
 
-        if ((name = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)) == NULL) {
+        if ((name = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)) ==
+            NULL) {
                 /*
                  * The servername callback gets called even when there is no
                  * TLS servername extension provided by the client. Sigh!
@@ -98,7 +100,10 @@ tls_servername_cb(SSL *ssl, int *al, void *arg)
 
         /* Find appropriate SSL context for requested servername. */
         for (sni_ctx = ctx->sni_ctx; sni_ctx != NULL; sni_ctx = sni_ctx->next) {
-                if (tls_check_name(ctx, sni_ctx->ssl_cert, name) == 0) {
+                if (tls_check_name(ctx, sni_ctx->ssl_cert, name,
+                    &match) == -1)
+                        goto err;
+                if (match) {
                         SSL_set_SSL_CTX(conn_ctx->ssl_conn, sni_ctx->ssl_ctx);
                         return (SSL_TLSEXT_ERR_OK);
                 }
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);
 }