From 5f3c52056aae232e7d9616c0f5fe51020fe35906 Mon Sep 17 00:00:00 2001 From: jsing Date: Mon, 10 Apr 2017 17:11:13 +0000 Subject: [PATCH] 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 . ok beck@ jca@ --- lib/libtls/tls_client.c | 14 +++---- lib/libtls/tls_internal.h | 5 ++- lib/libtls/tls_peer.c | 9 ++++- lib/libtls/tls_server.c | 11 +++-- lib/libtls/tls_verify.c | 84 ++++++++++++++++++++++++--------------- 5 files changed, 76 insertions(+), 47 deletions(-) diff --git a/lib/libtls/tls_client.c b/lib/libtls/tls_client.c index a1e2caa7178..0e519684efc 100644 --- a/lib/libtls/tls_client.c +++ b/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 * @@ -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/lib/libtls/tls_internal.h b/lib/libtls/tls_internal.h index 7bbc14ca860..bd23249e574 100644 --- a/lib/libtls/tls_internal.h +++ b/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 * Copyright (c) 2014 Joel Sing @@ -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/lib/libtls/tls_peer.c b/lib/libtls/tls_peer.c index 1a9065dfb1a..ec97a308386 100644 --- a/lib/libtls/tls_peer.c +++ b/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 * Copyright (c) 2015 Bob Beck @@ -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/lib/libtls/tls_server.c b/lib/libtls/tls_server.c index 51deff25105..39c6ca79e9e 100644 --- a/lib/libtls/tls_server.c +++ b/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 * @@ -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/lib/libtls/tls_verify.c b/lib/libtls/tls_verify.c index 23e58ebef78..3bd1057d0c4 100644 --- a/lib/libtls/tls_verify.c +++ b/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 * @@ -27,11 +27,6 @@ #include #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); } -- 2.20.1