From d6ee62d23880e688206638fbb251a5544c4a8087 Mon Sep 17 00:00:00 2001 From: eric Date: Wed, 18 Sep 2019 11:26:30 +0000 Subject: [PATCH] Implement server certificate validation in smtp(1). Check certificate against MX name in smtpd(8) mta. ok gilles@ --- usr.sbin/smtpd/dns.c | 7 +- usr.sbin/smtpd/mta.c | 15 +- usr.sbin/smtpd/mta_session.c | 23 ++- usr.sbin/smtpd/smtp/Makefile | 4 +- usr.sbin/smtpd/smtpc.c | 66 ++++++-- usr.sbin/smtpd/smtpd.h | 5 +- usr.sbin/smtpd/smtpd/Makefile | 3 +- usr.sbin/smtpd/ssl.h | 5 +- usr.sbin/smtpd/ssl_verify.c | 296 ++++++++++++++++++++++++++++++++++ 9 files changed, 396 insertions(+), 28 deletions(-) create mode 100644 usr.sbin/smtpd/ssl_verify.c diff --git a/usr.sbin/smtpd/dns.c b/usr.sbin/smtpd/dns.c index 4d369e2479f..d5b66af98ad 100644 --- a/usr.sbin/smtpd/dns.c +++ b/usr.sbin/smtpd/dns.c @@ -1,4 +1,4 @@ -/* $OpenBSD: dns.c,v 1.88 2018/09/26 16:28:34 eric Exp $ */ +/* $OpenBSD: dns.c,v 1.89 2019/09/18 11:26:30 eric Exp $ */ /* * Copyright (c) 2008 Gilles Chehade @@ -43,6 +43,7 @@ struct dns_lookup { struct dns_session *session; + char *host; int preference; }; @@ -144,6 +145,7 @@ dns_imsg(struct mproc *p, struct imsg *imsg) if (domainname_is_addr(domain, sa, &sl)) { m_create(s->p, IMSG_MTA_DNS_HOST, 0, 0, -1); m_add_id(s->p, s->reqid); + m_add_string(s->p, sockaddr_to_text(sa)); m_add_sockaddr(s->p, sa); m_add_int(s->p, -1); m_close(s->p); @@ -208,10 +210,12 @@ dns_dispatch_host(struct asr_result *ar, void *arg) s->mxfound++; m_create(s->p, IMSG_MTA_DNS_HOST, 0, 0, -1); m_add_id(s->p, s->reqid); + m_add_string(s->p, lookup->host); m_add_sockaddr(s->p, ai->ai_addr); m_add_int(s->p, lookup->preference); m_close(s->p); } + free(lookup->host); free(lookup); if (ar->ar_addrinfo) freeaddrinfo(ar->ar_addrinfo); @@ -338,6 +342,7 @@ dns_lookup_host(struct dns_session *s, const char *host, int preference) lookup = xcalloc(1, sizeof *lookup); lookup->preference = preference; + lookup->host = xstrdup(host); lookup->session = s; s->refcount++; diff --git a/usr.sbin/smtpd/mta.c b/usr.sbin/smtpd/mta.c index 1c60abd97d6..926b089a5f5 100644 --- a/usr.sbin/smtpd/mta.c +++ b/usr.sbin/smtpd/mta.c @@ -1,4 +1,4 @@ -/* $OpenBSD: mta.c,v 1.230 2019/09/14 06:20:27 gilles Exp $ */ +/* $OpenBSD: mta.c,v 1.231 2019/09/18 11:26:30 eric Exp $ */ /* * Copyright (c) 2008 Pierre-Yves Ritschard @@ -76,7 +76,7 @@ static void mta_drain(struct mta_relay *); static void mta_delivery_flush_event(int, short, void *); static void mta_flush(struct mta_relay *, int, const char *); static struct mta_route *mta_find_route(struct mta_connector *, time_t, int*, - time_t*); + time_t*, struct mta_mx **); static void mta_log(const struct mta_envelope *, const char *, const char *, const char *, const char *); @@ -255,11 +255,13 @@ mta_imsg(struct mproc *p, struct imsg *imsg) case IMSG_MTA_DNS_HOST: m_msg(&m, imsg); m_get_id(&m, &reqid); + m_get_string(&m, &hostname); m_get_sockaddr(&m, (struct sockaddr*)&ss); m_get_int(&m, &preference); m_end(&m); domain = tree_xget(&wait_mx, reqid); mx = xcalloc(1, sizeof *mx); + mx->mxname = xstrdup(hostname); mx->host = mta_host((struct sockaddr*)&ss); mx->preference = preference; TAILQ_FOREACH(imx, &domain->mxs, entry) { @@ -1146,6 +1148,7 @@ static void mta_connect(struct mta_connector *c) { struct mta_route *route; + struct mta_mx *mx; struct mta_limits *l = c->relay->limits; int limits; time_t nextconn, now; @@ -1234,7 +1237,7 @@ mta_connect(struct mta_connector *c) /* We can connect now, find a route */ if (!limits && nextconn <= now) - route = mta_find_route(c, now, &limits, &nextconn); + route = mta_find_route(c, now, &limits, &nextconn, &mx); else route = NULL; @@ -1281,7 +1284,7 @@ mta_connect(struct mta_connector *c) route->dst->nconn += 1; route->dst->lastconn = c->lastconn; - mta_session(c->relay, route); /* this never fails synchronously */ + mta_session(c->relay, route, mx->mxname); /* this never fails synchronously */ mta_relay_ref(c->relay); goto again; @@ -1510,7 +1513,7 @@ mta_flush(struct mta_relay *relay, int fail, const char *error) */ static struct mta_route * mta_find_route(struct mta_connector *c, time_t now, int *limits, - time_t *nextconn) + time_t *nextconn, struct mta_mx **pmx) { struct mta_route *route, *best; struct mta_limits *l = c->relay->limits; @@ -1654,6 +1657,7 @@ mta_find_route(struct mta_connector *c, time_t now, int *limits, if (best) mta_route_unref(best); /* from here */ best = route; + *pmx = mx; log_debug("debug: mta-routing: selecting candidate route %s", mta_route_to_text(route)); } @@ -2197,6 +2201,7 @@ mta_domain_unref(struct mta_domain *d) while ((mx = TAILQ_FIRST(&d->mxs))) { TAILQ_REMOVE(&d->mxs, mx, entry); mta_host_unref(mx->host); /* from IMSG_DNS_HOST */ + free(mx->mxname); free(mx); } diff --git a/usr.sbin/smtpd/mta_session.c b/usr.sbin/smtpd/mta_session.c index 1a668f126e6..f5366623d3f 100644 --- a/usr.sbin/smtpd/mta_session.c +++ b/usr.sbin/smtpd/mta_session.c @@ -1,4 +1,4 @@ -/* $OpenBSD: mta_session.c,v 1.120 2019/08/11 11:11:02 gilles Exp $ */ +/* $OpenBSD: mta_session.c,v 1.121 2019/09/18 11:26:30 eric Exp $ */ /* * Copyright (c) 2008 Pierre-Yves Ritschard @@ -103,6 +103,7 @@ struct mta_session { struct mta_relay *relay; struct mta_route *route; char *helo; + char *mxname; int flags; @@ -187,7 +188,7 @@ mta_session_init(void) } void -mta_session(struct mta_relay *relay, struct mta_route *route) +mta_session(struct mta_relay *relay, struct mta_route *route, const char *mxname) { struct mta_session *s; struct timeval tv; @@ -198,6 +199,7 @@ mta_session(struct mta_relay *relay, struct mta_route *route) s->id = generate_uid(); s->relay = relay; s->route = route; + s->mxname = xstrdup(mxname); if (relay->flags & RELAY_LMTP) s->flags |= MTA_LMTP; @@ -366,6 +368,7 @@ mta_free(struct mta_session *s) relay = s->relay; route = s->route; + free(s->mxname); free(s); stat_decrement("mta.session", 1); mta_route_collect(relay, route); @@ -1521,13 +1524,27 @@ static void mta_cert_verify_cb(void *arg, int status) { struct mta_session *s = arg; - int resume = 0; + int match, resume = 0; + X509 *cert; if (s->flags & MTA_WAIT) { mta_tree_pop(&wait_tls_verify, s->id); resume = 1; } + if (status == CERT_OK) { + cert = SSL_get_peer_certificate(io_tls(s->io)); + if (!cert) + status = CERT_NOCERT; + else { + match = 0; + (void)ssl_check_name(cert, s->mxname, &match); + X509_free(cert); + if (!match) + status = CERT_INVALID; + } + } + if (status == CERT_OK) s->flags |= MTA_TLS_VERIFIED; else if (s->relay->flags & RELAY_TLS_VERIFY) { diff --git a/usr.sbin/smtpd/smtp/Makefile b/usr.sbin/smtpd/smtp/Makefile index 6708ab6e20d..fc47fa03935 100644 --- a/usr.sbin/smtpd/smtp/Makefile +++ b/usr.sbin/smtpd/smtp/Makefile @@ -1,4 +1,4 @@ -# $OpenBSD: Makefile,v 1.2 2019/06/12 17:42:53 eric Exp $ +# $OpenBSD: Makefile,v 1.3 2019/09/18 11:26:30 eric Exp $ .PATH: ${.CURDIR}/.. @@ -13,7 +13,7 @@ SRCS+= log.c SRCS+= smtp_client.c SRCS+= smtpc.c SRCS+= ssl.c -SRCS+= ssl_smtpd.c +SRCS+= ssl_verify.c CPPFLAGS+= -DIO_TLS diff --git a/usr.sbin/smtpd/smtpc.c b/usr.sbin/smtpd/smtpc.c index 66be5fa389b..fb6d711d95f 100644 --- a/usr.sbin/smtpd/smtpc.c +++ b/usr.sbin/smtpd/smtpc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: smtpc.c,v 1.8 2019/09/02 20:05:21 eric Exp $ */ +/* $OpenBSD: smtpc.c,v 1.9 2019/09/18 11:26:30 eric Exp $ */ /* * Copyright (c) 2018 Eric Faurot @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -30,12 +31,12 @@ #include #include +#include + #include "smtp.h" +#include "ssl.h" #include "log.h" -void ssl_init(void); -void *ssl_mta_init(void *, char *, off_t, const char *); - static void parse_server(char *); static void parse_message(FILE *); static void resume(void); @@ -46,6 +47,9 @@ static int noaction = 0; static struct addrinfo *res0, *ai; static struct smtp_params params; static struct smtp_mail mail; +static const char *servname = NULL; + +static SSL_CTX *ssl_ctx; static void usage(void) @@ -53,7 +57,7 @@ usage(void) extern char *__progname; fprintf(stderr, - "usage: %s [-Chnv] [-F from] [-H helo] [-s server] rcpt ...\n", + "usage: %s [-Chnv] [-F from] [-H helo] [-s server] [-S name] rcpt ...\n", __progname); exit(1); } @@ -87,7 +91,7 @@ main(int argc, char **argv) memset(&mail, 0, sizeof(mail)); mail.from = pw->pw_name; - while ((ch = getopt(argc, argv, "CF:H:hns:v")) != -1) { + while ((ch = getopt(argc, argv, "CF:H:S:hns:v")) != -1) { switch (ch) { case 'C': params.tls_verify = 0; @@ -98,6 +102,9 @@ main(int argc, char **argv) case 'H': params.helo = optarg; break; + case 'S': + servname = optarg; + break; case 'h': usage(); break; @@ -132,6 +139,14 @@ main(int argc, char **argv) ssl_init(); event_init(); + ssl_ctx = ssl_ctx_create(NULL, NULL, 0, NULL); + if (!SSL_CTX_load_verify_locations(ssl_ctx, + X509_get_default_cert_file(), NULL)) + fatal("SSL_CTX_load_verify_locations"); + if (!SSL_CTX_set_ssl_version(ssl_ctx, SSLv23_client_method())) + fatal("SSL_CTX_set_ssl_version"); + SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_NONE , NULL); + if (pledge("stdio inet dns tmppath", NULL) == -1) fatal("pledge"); @@ -245,6 +260,9 @@ parse_server(char *server) if (port == NULL) port = "smtp"; + if (servname == NULL) + servname = host; + memset(&hints, 0, sizeof(hints)); hints.ai_family = AF_UNSPEC; hints.ai_socktype = SOCK_STREAM; @@ -330,20 +348,42 @@ log_trace(int lvl, const char *emsg, ...) void smtp_verify_server_cert(void *tag, struct smtp_client *proto, void *ctx) { - log_debug("validating server certificate..."); + SSL *ssl = ctx; + X509 *cert; + long res; + int r, match; + + if ((cert = SSL_get_peer_certificate(ssl))) { + r = ssl_check_name(cert, servname, &match); + X509_free(cert); + res = SSL_get_verify_result(ssl); + if (res == X509_V_OK) { + if (match) { + log_debug("valid certificate"); + smtp_cert_verified(proto, CERT_OK); + } + else { + log_debug("certificate does not match hostname"); + smtp_cert_verified(proto, CERT_INVALID); + } + return; + } + log_debug("certificate validation error %ld", res); + } + else + log_debug("no certificate provided"); - /* Not implemented for now. */ - smtp_cert_verified(proto, CERT_UNKNOWN); + smtp_cert_verified(proto, CERT_INVALID); } void smtp_require_tls(void *tag, struct smtp_client *proto) { - void *ctx; - - ctx = ssl_mta_init(NULL, NULL, 0, NULL); + SSL *ssl = NULL; - smtp_set_tls(proto, ctx); + if ((ssl = SSL_new(ssl_ctx)) == NULL) + fatal("SSL_new"); + smtp_set_tls(proto, ssl); } void diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h index 804b940d6e0..261c03008a1 100644 --- a/usr.sbin/smtpd/smtpd.h +++ b/usr.sbin/smtpd/smtpd.h @@ -1,4 +1,4 @@ -/* $OpenBSD: smtpd.h,v 1.636 2019/09/11 04:19:19 martijn Exp $ */ +/* $OpenBSD: smtpd.h,v 1.637 2019/09/18 11:26:30 eric Exp $ */ /* * Copyright (c) 2008 Gilles Chehade @@ -679,6 +679,7 @@ struct mta_host { struct mta_mx { TAILQ_ENTRY(mta_mx) entry; struct mta_host *host; + char *mxname; int preference; }; @@ -1475,7 +1476,7 @@ const char *mta_relay_to_text(struct mta_relay *); /* mta_session.c */ -void mta_session(struct mta_relay *, struct mta_route *); +void mta_session(struct mta_relay *, struct mta_route *, const char *); void mta_session_imsg(struct mproc *, struct imsg *); diff --git a/usr.sbin/smtpd/smtpd/Makefile b/usr.sbin/smtpd/smtpd/Makefile index d37ff4e3388..46e80c6a5ea 100644 --- a/usr.sbin/smtpd/smtpd/Makefile +++ b/usr.sbin/smtpd/smtpd/Makefile @@ -1,4 +1,4 @@ -# $OpenBSD: Makefile,v 1.102 2019/08/10 16:07:02 gilles Exp $ +# $OpenBSD: Makefile,v 1.103 2019/09/18 11:26:30 eric Exp $ .PATH: ${.CURDIR}/.. @@ -52,6 +52,7 @@ SRCS+= smtp_session.c SRCS+= smtpd.c SRCS+= ssl.c SRCS+= ssl_smtpd.c +SRCS+= ssl_verify.c SRCS+= stat_backend.c SRCS+= table.c SRCS+= to.c diff --git a/usr.sbin/smtpd/ssl.h b/usr.sbin/smtpd/ssl.h index dfa6994cdb8..1c0dc072d11 100644 --- a/usr.sbin/smtpd/ssl.h +++ b/usr.sbin/smtpd/ssl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl.h,v 1.20 2016/04/21 14:27:41 jsing Exp $ */ +/* $OpenBSD: ssl.h,v 1.21 2019/09/18 11:26:30 eric Exp $ */ /* * Copyright (c) 2013 Gilles Chehade * @@ -65,3 +65,6 @@ int ssl_ctx_fake_private_key(SSL_CTX *, const void *, size_t, /* ssl_privsep.c */ int ssl_by_mem_ctrl(X509_LOOKUP *, int, const char *, long, char **); + +/* ssl_verify.c */ +int ssl_check_name(X509 *, const char *, int *); diff --git a/usr.sbin/smtpd/ssl_verify.c b/usr.sbin/smtpd/ssl_verify.c new file mode 100644 index 00000000000..ebc79870290 --- /dev/null +++ b/usr.sbin/smtpd/ssl_verify.c @@ -0,0 +1,296 @@ +/* $OpenBSD: ssl_verify.c,v 1.1 2019/09/18 11:26:30 eric Exp $ */ +/* + * Copyright (c) 2014 Jeremie Courreges-Anglas + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +/* Adapted from lib/libtls/tls_verify.c */ + + +#include + +#include +#include + +#include +#include + +#include + +#if 0 +#include +#include "tls_internal.h" +#endif + +#include "ssl.h" +#include "log.h" + +struct tls; +#define tls_set_errorx(ctx, ...) log_warnx(__VA_ARGS__) +union tls_addr { + struct in_addr in; + struct in6_addr in6; +}; + +static int +tls_match_name(const char *cert_name, const char *name) +{ + const char *cert_domain, *domain, *next_dot; + + if (strcasecmp(cert_name, name) == 0) + return 0; + + /* Wildcard match? */ + if (cert_name[0] == '*') { + /* + * Valid wildcards: + * - "*.domain.tld" + * - "*.sub.domain.tld" + * - etc. + * Reject "*.tld". + * No attempt to prevent the use of eg. "*.co.uk". + */ + cert_domain = &cert_name[1]; + /* Disallow "*" */ + if (cert_domain[0] == '\0') + return -1; + /* Disallow "*foo" */ + if (cert_domain[0] != '.') + return -1; + /* Disallow "*.." */ + if (cert_domain[1] == '.') + return -1; + next_dot = strchr(&cert_domain[1], '.'); + /* Disallow "*.bar" */ + if (next_dot == NULL) + return -1; + /* Disallow "*.bar.." */ + if (next_dot[1] == '.') + return -1; + + domain = strchr(name, '.'); + + /* No wildcard match against a name with no host part. */ + if (name[0] == '.') + return -1; + /* No wildcard match against a name with no domain part. */ + if (domain == NULL || strlen(domain) == 1) + return -1; + + if (strcasecmp(cert_domain, domain) == 0) + return 0; + } + + return -1; +} + +/* + * 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, + 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 = 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 0; + + if (inet_pton(AF_INET, name, &addrbuf) == 1) { + type = GEN_IPADD; + addrlen = 4; + } else if (inet_pton(AF_INET6, name, &addrbuf) == 1) { + type = GEN_IPADD; + addrlen = 16; + } else { + type = GEN_DNS; + addrlen = 0; + } + + count = sk_GENERAL_NAME_num(altname_stack); + for (i = 0; i < count; i++) { + 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; + + if (type == GEN_DNS) { + unsigned char *data; + int format, len; + + format = ASN1_STRING_type(altname->d.dNSName); + if (format == V_ASN1_IA5STRING) { + data = ASN1_STRING_data(altname->d.dNSName); + len = ASN1_STRING_length(altname->d.dNSName); + + if (len < 0 || (size_t)len != strlen(data)) { + tls_set_errorx(ctx, + "error verifying name '%s': " + "NUL byte in subjectAltName, " + "probably a malicious certificate", + name); + rv = -1; + break; + } + + /* + * Per RFC 5280 section 4.2.1.6: + * " " is a legal domain name, but that + * dNSName must be rejected. + */ + if (strcmp(data, " ") == 0) { + tls_set_errorx(ctx, + "error verifying name '%s': " + "a dNSName of \" \" must not be " + "used", name); + rv = -1; + break; + } + + if (tls_match_name(data, name) == 0) { + *alt_match = 1; + break; + } + } else { +#ifdef DEBUG + fprintf(stdout, "%s: unhandled subjectAltName " + "dNSName encoding (%d)\n", getprogname(), + format); +#endif + } + + } else if (type == GEN_IPADD) { + unsigned char *data; + int datalen; + + datalen = ASN1_STRING_length(altname->d.iPAddress); + data = ASN1_STRING_data(altname->d.iPAddress); + + if (datalen < 0) { + tls_set_errorx(ctx, + "Unexpected negative length for an " + "IP address: %d", datalen); + rv = -1; + break; + } + + /* + * Per RFC 5280 section 4.2.1.6: + * IPv4 must use 4 octets and IPv6 must use 16 octets. + */ + if (datalen == addrlen && + memcmp(data, &addrbuf, addrlen) == 0) { + *alt_match = 1; + break; + } + } + } + + sk_GENERAL_NAME_pop_free(altname_stack, GENERAL_NAME_free); + return rv; +} + +static int +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 = 0; + + *cn_match = 0; + + subject_name = X509_get_subject_name(cert); + 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) + goto done; + + common_name = calloc(common_name_len + 1, 1); + if (common_name == NULL) + goto done; + + X509_NAME_get_text_by_NID(subject_name, NID_commonName, common_name, + common_name_len + 1); + + /* NUL bytes in CN? */ + if (common_name_len < 0 || + (size_t)common_name_len != strlen(common_name)) { + tls_set_errorx(ctx, "error verifying name '%s': " + "NUL byte in Common Name field, " + "probably a malicious certificate", name); + rv = -1; + goto done; + } + + /* + * 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) { + if (strcmp(common_name, name) == 0) + *cn_match = 1; + goto done; + } + + if (tls_match_name(common_name, name) == 0) + *cn_match = 1; + + done: + free(common_name); + return rv; +} + +int +ssl_check_name(X509 *cert, const char *name, int *match) +{ + int alt_exists; + + *match = 0; + + if (tls_check_subject_altname(NULL, 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(NULL, cert, name, match); +} -- 2.20.1