DANE: do not override a cert verify failure, in callback. Also fix some test mistakes

2 files changed, 68 insertions, 46 deletions

diff --git a/src/src/dane-openssl.c b/src/src/dane-openssl.c
index 50a2e8aa5..e5f9f9784 100644
--- a/src/src/dane-openssl.c
+++ b/src/src/dane-openssl.c
@@ -190,6 +190,8 @@ typedef struct ssl_dane
 # define X509_V_ERR_HOSTNAME_MISMATCH X509_V_ERR_APPLICATION_VERIFICATION
 #endif
 
+
+
 static int
 match(dane_selector_list slist, X509 *cert, int depth)
 {
@@ -840,6 +842,7 @@ if (gens)
 	continue;
       if (!(dane->mhost = OPENSSL_strdup(certid)))
 	matched = -1;
+      DEBUG(D_tls) debug_printf("Dane name_check: matched SAN %s\n", certid);
       break;
       }
     }
@@ -850,14 +853,16 @@ if (gens)
  * XXX: Should the subjectName be skipped when *any* altnames are present,
  * or only when DNS altnames are present?
  */
-if (got_altname == 0)
+if (!got_altname)
   {
   char *certid = parse_subject_name(cert);
-  if (certid != 0 && *certid
-    && (matched = match_name(certid, dane)) != 0)
+  if (certid != 0 && *certid && (matched = match_name(certid, dane)) != 0)
+    {
+    DEBUG(D_tls) debug_printf("Dane name_check: matched SN %s\n", certid);
     dane->mhost = OPENSSL_strdup(certid);
-    if (certid)
-      OPENSSL_free(certid);
+    }
+  if (certid)
+    OPENSSL_free(certid);
   }
 return matched;
 }
@@ -875,7 +880,7 @@ X509 *cert = ctx->cert;             /* XXX: accessor? */
 int matched = 0;
 int chain_length = sk_X509_num(ctx->chain);
 
-DEBUG(D_tls) debug_printf("Dane verify-chain\n");
+DEBUG(D_tls) debug_printf("Dane verify_chain\n");
 
 issuer_rrs = dane->selectors[DANESSL_USAGE_PKIX_TA];
 leaf_rrs = dane->selectors[DANESSL_USAGE_PKIX_EE];
@@ -897,29 +902,29 @@ if (!matched)
   }
 matched = 0;
 
-    /*
-     * Satisfy at least one usage 0 or 1 constraint, unless we've already
-     * matched a usage 2 trust anchor.
-     *
-     * XXX: internal_verify() doesn't callback with top certs that are not
-     * self-issued.  This should be fixed in a future OpenSSL.
-     */
-    if (dane->roots && sk_X509_num(dane->roots))
-      {
-      X509 *top = sk_X509_value(ctx->chain, dane->depth);
+/*
+ * Satisfy at least one usage 0 or 1 constraint, unless we've already
+ * matched a usage 2 trust anchor.
+ *
+ * XXX: internal_verify() doesn't callback with top certs that are not
+ * self-issued.  This should be fixed in a future OpenSSL.
+ */
+if (dane->roots && sk_X509_num(dane->roots))
+  {
+  X509 *top = sk_X509_value(ctx->chain, dane->depth);
 
-      dane->mdpth = dane->depth;
-      dane->match = top;
-      X509_up_ref(top);
+  dane->mdpth = dane->depth;
+  dane->match = top;
+  X509_up_ref(top);
 
 #ifndef NO_CALLBACK_WORKAROUND
-      if (X509_check_issued(top, top) != X509_V_OK)
-	{
-        ctx->error_depth = dane->depth;
-        ctx->current_cert = top;
-        if (!cb(1, ctx))
-          return 0;
-        }
+  if (X509_check_issued(top, top) != X509_V_OK)
+    {
+    ctx->error_depth = dane->depth;
+    ctx->current_cert = top;
+    if (!cb(1, ctx))
+      return 0;
+    }
 #endif
   /* Pop synthetic trust-anchor ancestors off the chain! */
   while (--chain_length > dane->depth)
@@ -936,6 +941,7 @@ else
    */
   if (leaf_rrs)
     matched = match(leaf_rrs, xn, 0);
+  if (matched) DEBUG(D_tls) debug_printf("Dane verify_chain: matched EE\n");
 
   if (!matched && issuer_rrs)
     for (n = chain_length-1; !matched && n >= 0; --n)
@@ -944,6 +950,8 @@ else
       if (n > 0 || X509_check_issued(xn, xn) == X509_V_OK)
 	matched = match(issuer_rrs, xn, n);
       }
+  if (matched) DEBUG(D_tls) debug_printf("Dane verify_chain: matched %s\n",
+    n>0 ? "CA" : "selfisssued EE");
 
   if (!matched)
     {
@@ -1001,7 +1009,7 @@ int (*cb)(int, X509_STORE_CTX *) = ctx->verify_cb;
 int matched;
 X509 *cert = ctx->cert;             /* XXX: accessor? */
 
-DEBUG(D_tls) debug_printf("Dane verify-cert\n");
+DEBUG(D_tls) debug_printf("Dane verify_cert\n");
 
 if (ssl_idx < 0)
   ssl_idx = SSL_get_ex_data_X509_STORE_CTX_idx();
@@ -1015,7 +1023,7 @@ ssl = X509_STORE_CTX_get_ex_data(ctx, ssl_idx);
 if (!(dane = SSL_get_ex_data(ssl, dane_idx)) || !cert)
   return X509_verify_cert(ctx);
 
-    /* Reset for verification of a new chain, perhaps a renegotiation. */
+/* Reset for verification of a new chain, perhaps a renegotiation. */
 dane_reset(dane);
 
 if (dane->selectors[DANESSL_USAGE_DANE_EE])
diff --git a/src/src/tls-openssl.c b/src/src/tls-openssl.c
index 49347a2ae..7417790e7 100644
--- a/src/src/tls-openssl.c
+++ b/src/src/tls-openssl.c
@@ -345,15 +345,17 @@ May be called multiple times for different issues with a certificate, even
 for a given "depth" in the certificate chain.
 
 Arguments:
-  state      current yes/no state as 1/0
-  x509ctx    certificate information.
-  client     TRUE for client startup, FALSE for server startup
+  preverify_ok current yes/no state as 1/0
+  x509ctx      certificate information.
+  tlsp         per-direction (client vs. server) support data
+  calledp      has-been-called flag
+  optionalp    verification-is-optional flag
 
-Returns:     1 if verified, 0 if not
+Returns:     0 if verification should fail, otherwise 1
 */
 
 static int
-verify_callback(int state, X509_STORE_CTX *x509ctx,
+verify_callback(int preverify_ok, X509_STORE_CTX *x509ctx,
   tls_support *tlsp, BOOL *calledp, BOOL *optionalp)
 {
 X509 * cert = X509_STORE_CTX_get_current_cert(x509ctx);
@@ -363,7 +365,7 @@ uschar dn[256];
 X509_NAME_oneline(X509_get_subject_name(cert), CS dn, sizeof(dn));
 dn[sizeof(dn)-1] = '\0';
 
-if (state == 0)
+if (preverify_ok == 0)
   {
   log_write(0, LOG_MAIN, "[%s] SSL verify error: depth=%d error=%s cert=%s",
 	tlsp == &tls_out ? deliver_host_address : sender_host_address,
@@ -470,15 +472,17 @@ return 1;   /* accept, at least for this level */
 }
 
 static int
-verify_callback_client(int state, X509_STORE_CTX *x509ctx)
+verify_callback_client(int preverify_ok, X509_STORE_CTX *x509ctx)
 {
-return verify_callback(state, x509ctx, &tls_out, &client_verify_callback_called, &client_verify_optional);
+return verify_callback(preverify_ok, x509ctx, &tls_out,
+  &client_verify_callback_called, &client_verify_optional);
 }
 
 static int
-verify_callback_server(int state, X509_STORE_CTX *x509ctx)
+verify_callback_server(int preverify_ok, X509_STORE_CTX *x509ctx)
 {
-return verify_callback(state, x509ctx, &tls_in, &server_verify_callback_called, &server_verify_optional);
+return verify_callback(preverify_ok, x509ctx, &tls_in,
+  &server_verify_callback_called, &server_verify_optional);
 }
 
 
@@ -488,7 +492,7 @@ return verify_callback(state, x509ctx, &tls_in, &server_verify_callback_called,
 itself.
 */
 static int
-verify_callback_client_dane(int state, X509_STORE_CTX * x509ctx)
+verify_callback_client_dane(int preverify_ok, X509_STORE_CTX * x509ctx)
 {
 X509 * cert = X509_STORE_CTX_get_current_cert(x509ctx);
 uschar dn[256];
@@ -500,7 +504,8 @@ BOOL dummy_called, optional = FALSE;
 X509_NAME_oneline(X509_get_subject_name(cert), CS dn, sizeof(dn));
 dn[sizeof(dn)-1] = '\0';
 
-DEBUG(D_tls) debug_printf("verify_callback_client_dane: %s\n", dn);
+DEBUG(D_tls) debug_printf("verify_callback_client_dane: %s depth %d %s\n",
+  preverify_ok ? "ok":"BAD", depth, dn);
 
 #ifndef DISABLE_EVENT
   if (verify_event(&tls_out, cert, depth, dn,
@@ -508,10 +513,18 @@ DEBUG(D_tls) debug_printf("verify_callback_client_dane: %s\n", dn);
     return 0;				/* reject, with peercert set */
 #endif
 
-if (state == 1)
+if (preverify_ok == 1)
   tls_out.dane_verified =
   tls_out.certificate_verified = TRUE;
-return 1;
+else
+  {
+  int err = X509_STORE_CTX_get_error(x509ctx);
+  DEBUG(D_tls)
+    debug_printf(" - err %d '%s'\n", err, X509_verify_cert_error_string(err));
+  if (err = X509_V_ERR_APPLICATION_VERIFICATION)
+    preverify_ok = 1;
+  }
+return preverify_ok;
 }
 
 #endif	/*EXPERIMENTAL_DANE*/
@@ -1557,8 +1570,8 @@ if (expcerts != NULL && *expcerts != '\0')
       certificates are recognized, but the error message is still misleading (it
       says no certificate was supplied.) But this is better. */
 
-      if ((file == NULL || statbuf.st_size > 0) &&
-	    !SSL_CTX_load_verify_locations(sctx, CS file, CS dir))
+      if (  (!file || statbuf.st_size > 0)
+         && !SSL_CTX_load_verify_locations(sctx, CS file, CS dir))
 	return tls_error(US"SSL_CTX_load_verify_locations", host, NULL);
 
       /* Load the list of CAs for which we will accept certs, for sending
@@ -1572,10 +1585,11 @@ if (expcerts != NULL && *expcerts != '\0')
       Because of this, and that the dir variant is likely only used for
       the public-CA bundle (not for a private CA), not worth fixing.
       */
-      if (file != NULL)
+      if (file)
 	{
 	STACK_OF(X509_NAME) * names = SSL_load_client_CA_file(CS file);
-  DEBUG(D_tls) debug_printf("Added %d certificate authorities.\n",
+
+	DEBUG(D_tls) debug_printf("Added %d certificate authorities.\n",
 				    sk_X509_NAME_num(names));
 	SSL_CTX_set_client_CA_list(sctx, names);
 	}