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 <rsalz@openssl.org>

diff --git a/doc/ssl/SSL_CTX_dane_enable.pod b/doc/ssl/SSL_CTX_dane_enable.pod
index 8463a3d093e74880734a0479fa6e059afefd2499..d6d447d9a11dfc81cccbb6a3bbcd9b2a82fc85ba 100644 (file)
--- 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<data> field is provided in binary (wire RDATA) form, not the hexadecimal
 ASCII presentation form, with an explicit length passed via B<dlen>.
 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<mode> argument of L<SSL_set_verify(3)> 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 1672849c8328cdfc3b7597478ecfbaf48f2845cf..557534adece4e960f610cf202bbf9a6b09929204 100644 (file)
--- 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 06d972349a9dab25ef1c551b233f35dcb5d38f27..994d09346681ce77456a983b3f6f43ca34ed555d 100644 (file)
--- 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;