From 9f6b22b814a306677f6d5a829cf7fd62005ecdc2 Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Thu, 21 Apr 2016 20:00:58 -0400 Subject: [PATCH] Enabled DANE only when at least one TLSA RR was added It is up to the caller of SSL_dane_tlsa_add() to take appropriate action when no records are added successfully or adding some records triggers an internal error (negative return value). With this change the caller can continue with PKIX if desired when none of the TLSA records are usable, or take some appropriate action if DANE is required. Also fixed the internal ssl_dane_dup() function to properly initialize the TLSA RR stack in the target SSL handle. Errors in ssl_dane_dup() are no longer ignored. Reviewed-by: Rich Salz --- doc/ssl/SSL_CTX_dane_enable.pod | 56 ++++++++++++++++++++++++--------- include/internal/dane.h | 3 +- ssl/ssl_lib.c | 16 ++++++++-- 3 files changed, 58 insertions(+), 17 deletions(-) diff --git a/doc/ssl/SSL_CTX_dane_enable.pod b/doc/ssl/SSL_CTX_dane_enable.pod index 8463a3d093..d6d447d9a1 100644 --- a/doc/ssl/SSL_CTX_dane_enable.pod +++ b/doc/ssl/SSL_CTX_dane_enable.pod @@ -71,11 +71,17 @@ The arguments specify the fields of the TLSA record. The B field is provided in binary (wire RDATA) form, not the hexadecimal ASCII presentation form, with an explicit length passed via B. A return value of 0 indicates that "unusable" TLSA records (with invalid or -unsupported parameters) were provided, a negative return value indicates an -internal error in processing the records. -If DANE authentication is enabled, but no TLSA records are added successfully, -authentication will fail, and the handshake may not complete, depending on the -B argument of L and any verification callback. +unsupported parameters) were provided. +A negative return value indicates an internal error in processing the record. + +The caller is expected to check the return value of each SSL_dane_tlsa_add() +call and take appropriate action if none are usable or an internal error +is encountered in processing some records. + +If no TLSA records are added successfully, DANE authentication is not enabled, +and authentication will be based on any configured traditional trust-anchors; +authentication success in this case does not mean that the peer was +DANE-authenticated. SSL_get0_dane_authority() can be used to get more detailed information about the matched DANE trust-anchor after successful connection completion. @@ -149,6 +155,7 @@ the lifetime of the SSL connection. SSL_CTX *ctx; SSL *ssl; + int (*verify_cb)(int ok, X509_STORE_CTX *sctx) = NULL; int num_usable = 0; const char *nexthop_domain = "example.com"; const char *dane_tlsa_domain = "smtp.example.com"; @@ -175,11 +182,19 @@ the lifetime of the SSL connection. /* set usage, selector, mtype, data, len */ - /* Opportunistic DANE TLS clients treat usages 0, 1 as unusable. */ + /* + * Opportunistic DANE TLS clients support only DANE-TA(2) or DANE-EE(3). + * They treat all other certificate usages, and in particular PKIX-TA(0) + * and PKIX-EE(1), as unusable. + */ switch (usage) { + default: case 0: /* PKIX-TA(0) */ case 1: /* PKIX-EE(1) */ continue; + case 2: /* DANE-TA(2) */ + case 3: /* DANE-EE(3) */ + break; } ret = SSL_dane_tlsa_add(ssl, usage, selector, mtype, data, len); @@ -194,16 +209,29 @@ the lifetime of the SSL connection. } /* + * At this point, the verification mode is still the default SSL_VERIFY_NONE. * Opportunistic DANE clients use unauthenticated TLS when all TLSA records * are unusable, so continue the handshake even if authentication fails. */ if (num_usable == 0) { - int (*cb)(int ok, X509_STORE_CTX *sctx) = NULL; - /* Log all records unusable? */ - /* Set cb to a non-NULL callback of your choice? */ - SSL_set_verify(ssl, SSL_VERIFY_NONE, cb); + /* Optionally set verify_cb to a suitable non-NULL callback. */ + SSL_set_verify(ssl, SSL_VERIFY_NONE, verify_cb); + } else { + /* At least one usable record. We expect to verify the peer */ + + /* Optionally set verify_cb to a suitable non-NULL callback. */ + + /* + * Below we elect to fail the handshake when peer verification fails. + * Alternatively, use the permissive SSL_VERIFY_NONE verification mode, + * complete the handshake, check the verification status, and if not + * verified disconnect gracefully at the application layer, especially if + * application protocol supports informing the server that authentication + * failed. + */ + SSL_set_verify(ssl, SSL_VERIFY_PEER, verify_cb); } /* @@ -240,14 +268,14 @@ the lifetime of the SSL connection. } if (peername != NULL) { /* Name checks were in scope and matched the peername */ - printf(bio, "Verified peername: %s\n", peername); + printf("Verified peername: %s\n", peername); } } else { /* * Not authenticated, presumably all TLSA rrs unusable, but possibly a - * callback suppressed connection termination despite presence of TLSA - * usable RRs none of which matched. Do whatever is appropriate for - * unauthenticated connections. + * callback suppressed connection termination despite the presence of + * usable TLSA RRs none of which matched. Do whatever is appropriate for + * fresh unauthenticated connections. */ } diff --git a/include/internal/dane.h b/include/internal/dane.h index 1672849c83..557534adec 100644 --- a/include/internal/dane.h +++ b/include/internal/dane.h @@ -121,7 +121,8 @@ struct ssl_dane_st { int pdpth; /* Depth of PKIX trust */ }; -#define DANETLS_ENABLED(dane) ((dane) != NULL && ((dane)->trecs != NULL)) +#define DANETLS_ENABLED(dane) \ + ((dane) != NULL && sk_danetls_record_num((dane)->trecs) > 0) #define DANETLS_USAGE_BIT(u) (((uint32_t)1) << u) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 06d972349a..994d093466 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -284,10 +284,18 @@ static int ssl_dane_dup(SSL *to, SSL *from) return 1; dane_final(&to->dane); + to->dane.dctx = &to->ctx->dane; + to->dane.trecs = sk_danetls_record_new_null(); + + if (to->dane.trecs == NULL) { + SSLerr(SSL_F_SSL_DANE_DUP, ERR_R_MALLOC_FAILURE); + return 0; + } num = sk_danetls_record_num(from->dane.trecs); for (i = 0; i < num; ++i) { danetls_record *t = sk_danetls_record_value(from->dane.trecs, i); + if (SSL_dane_tlsa_add(to, t->usage, t->selector, t->mtype, t->data, t->dlen) <= 0) return 0; @@ -363,6 +371,7 @@ static int dane_tlsa_add( const EVP_MD *md = NULL; int ilen = (int)dlen; int i; + int num; if (dane->trecs == NULL) { SSLerr(SSL_F_DANE_TLSA_ADD, SSL_R_DANE_NOT_ENABLED); @@ -495,8 +504,10 @@ static int dane_tlsa_add( * The choice of order for the selector is not significant, so we * use the same descending order for consistency. */ - for (i = 0; i < sk_danetls_record_num(dane->trecs); ++i) { + num = sk_danetls_record_num(dane->trecs); + for (i = 0; i < num; ++i) { danetls_record *rec = sk_danetls_record_value(dane->trecs, i); + if (rec->usage > usage) continue; if (rec->usage < usage) @@ -3135,7 +3146,8 @@ SSL *SSL_dup(SSL *s) goto err; } - ssl_dane_dup(ret, s); + if (!ssl_dane_dup(ret, s)) + goto err; ret->version = s->version; ret->options = s->options; ret->mode = s->mode; -- 2.34.1