Fix weak digest in TLS 1.2 with SNI.

1ce95f19601bbc6bfd24092c76c8f8105124e857 was incomplete and did not
handle the case when SSL_set_SSL_CTX was called from the cert_cb
callback rather than the SNI callback. The consequence is any server
using OpenSSL 1.0.2 and the cert_cb callback for SNI only ever signs a
weak digest, SHA-1, even when connecting to clients which use secure
ones.

Fix this and add regression tests for both this and the original issue.

Fixes #4554.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4577)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 8466da0e97292dd1eb99852bcb491dac2b56e036..3539f4b8d20a368bf552446cb400d61b32121cf7 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3180,6 +3180,7 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx)
 #endif
     ssl->cert = ssl_cert_dup(ctx->cert);
     if (ocert) {
+        int i;
         /* Preserve any already negotiated parameters */
         if (ssl->server) {
             ssl->cert->peer_sigalgs = ocert->peer_sigalgs;
@@ -3189,6 +3190,9 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx)
             ssl->cert->ciphers_rawlen = ocert->ciphers_rawlen;
             ocert->ciphers_raw = NULL;
         }
+        for (i = 0; i < SSL_PKEY_NUM; i++) {
+            ssl->cert->pkeys[i].digest = ocert->pkeys[i].digest;
+        }
 #ifndef OPENSSL_NO_TLSEXT
         ssl->cert->alpn_proposed = ocert->alpn_proposed;
         ssl->cert->alpn_proposed_len = ocert->alpn_proposed_len;

diff --git a/ssl/ssltest.c b/ssl/ssltest.c
index b75cac61fbdc4205b2e31c933405bdae573d1752..2d6141cd954e000387503208fe1758f84354c0f2 100644 (file)
--- a/ssl/ssltest.c
+++ b/ssl/ssltest.c
@@ -315,6 +315,9 @@ static int s_ticket1 = 0;
 static int s_ticket2 = 0;
 static int c_ticket = 0;
 static int ticket_expect = -1;
+static int sni_in_cert_cb = 0;
+static const char *client_sigalgs = NULL;
+static const char *server_digest_expect = NULL;
 
 static int servername_cb(SSL *s, int *ad, void *arg)
 {
@@ -355,6 +358,11 @@ static int verify_servername(SSL *client, SSL *server)
         BIO_printf(bio_stdout, "Servername: context is unknown\n");
     return -1;
 }
+static int cert_cb(SSL *ssl, void *arg)
+{
+    int unused;
+    return servername_cb(ssl, &unused, NULL) != SSL_TLSEXT_ERR_ALERT_FATAL;
+}
 
 static int verify_ticket(SSL* ssl)
 {
@@ -371,6 +379,20 @@ static int verify_ticket(SSL* ssl)
     return -1;
 }
 
+static int verify_server_digest(SSL* ssl)
+{
+    int nid = NID_undef;
+
+    if (server_digest_expect == NULL)
+        return 0;
+    SSL_get_peer_signature_nid(ssl, &nid);
+    if (strcmp(server_digest_expect, OBJ_nid2sn(nid)) == 0)
+        return 1;
+    BIO_printf(bio_stdout, "Expected server digest %s, got %s.\n",
+               server_digest_expect, OBJ_nid2sn(nid));
+    return -1;
+}
+
 /*-
  * next_protos_parse parses a comma separated list of strings into a string
  * in a format suitable for passing to SSL_CTX_set_next_protos_advertised.
@@ -831,6 +853,7 @@ static void sv_usage(void)
 #endif
 #ifndef OPENSSL_NO_TLS1
     fprintf(stderr, " -tls1         - use TLSv1\n");
+    fprintf(stderr, " -tls12        - use TLSv1.2\n");
 #endif
 #ifndef OPENSSL_NO_DTLS
     fprintf(stderr, " -dtls1        - use DTLSv1\n");
@@ -884,6 +907,9 @@ static void sv_usage(void)
     fprintf(stderr, " -c_ticket <yes|no>         - enable/disable session tickets on the client\n");
     fprintf(stderr, " -ticket_expect <yes|no>    - indicate that the client should (or should not) have a ticket\n");
 #endif
+    fprintf(stderr, " -sni_in_cert_cb           - have the server handle SNI in the certificate callback\n");
+    fprintf(stderr, " -client_sigalgs arg       - the signature algorithms to configure on the client\n");
+    fprintf(stderr, " -server_digest_expect arg - the expected server signing digest\n");
 }
 
 static void print_details(SSL *c_ssl, const char *prefix)
@@ -1010,7 +1036,7 @@ int main(int argc, char *argv[])
     int badop = 0;
     int bio_pair = 0;
     int force = 0;
-    int dtls1 = 0, dtls12 = 0, tls1 = 0, ssl2 = 0, ssl3 = 0, ret = 1;
+    int dtls1 = 0, dtls12 = 0, tls1 = 0, tls12 = 0, ssl2 = 0, ssl3 = 0, ret = 1;
     int client_auth = 0;
     int server_auth = 0, i;
     struct app_verify_arg app_verify_arg =
@@ -1164,6 +1190,11 @@ int main(int argc, char *argv[])
             no_protocol = 1;
 #endif
             tls1 = 1;
+        } else if (strcmp(*argv, "-tls12") == 0) {
+#ifdef OPENSSL_NO_TLS1
+            no_protocol = 1;
+#endif
+            tls12 = 1;
         } else if (strcmp(*argv, "-ssl3") == 0) {
 #ifdef OPENSSL_NO_SSL3_METHOD
             no_protocol = 1;
@@ -1343,6 +1374,16 @@ int main(int argc, char *argv[])
             else if (strcmp(*argv, "no") == 0)
                 ticket_expect = 0;
 #endif
+        } else if (strcmp(*argv, "-sni_in_cert_cb") == 0) {
+            sni_in_cert_cb = 1;
+        } else if (strcmp(*argv, "-client_sigalgs") == 0) {
+            if (--argc < 1)
+                goto bad;
+            client_sigalgs = *(++argv);
+        } else if (strcmp(*argv, "-server_digest_expect") == 0) {
+            if (--argc < 1)
+                goto bad;
+            server_digest_expect = *(++argv);
         } else {
             fprintf(stderr, "unknown option %s\n", *argv);
             badop = 1;
@@ -1373,9 +1414,9 @@ int main(int argc, char *argv[])
         goto end;
     }
 
-    if (ssl2 + ssl3 + tls1 + dtls1 + dtls12 > 1) {
-        fprintf(stderr, "At most one of -ssl2, -ssl3, -tls1, -dtls1 or -dtls12 should "
-                "be requested.\n");
+    if (ssl2 + ssl3 + tls1 + tls12 + dtls1 + dtls12 > 1) {
+        fprintf(stderr, "At most one of -ssl2, -ssl3, -tls1, -tls12, -dtls1 or "
+                "-dtls12 should be requested.\n");
         EXIT(1);
     }
 
@@ -1391,10 +1432,11 @@ int main(int argc, char *argv[])
         goto end;
     }
 
-    if (!ssl2 && !ssl3 && !tls1 && !dtls1 && !dtls12 && number > 1 && !reuse && !force) {
+    if (!ssl2 && !ssl3 && !tls1 && !tls12 && !dtls1 && !dtls12 && number > 1
+            && !reuse && !force) {
         fprintf(stderr, "This case cannot work.  Use -f to perform "
                 "the test anyway (and\n-d to see what happens), "
-                "or add one of ssl2, -ssl3, -tls1, -dtls1, -dtls12, -reuse\n"
+                "or add one of ssl2, -ssl3, -tls1, -tls12, -dtls1, -dtls12, -reuse\n"
                 "to avoid protocol mismatch.\n");
         EXIT(1);
     }
@@ -1458,7 +1500,7 @@ int main(int argc, char *argv[])
 #endif
 
     /*
-     * At this point, ssl2/ssl3/tls1 is only set if the protocol is
+     * At this point, ssl2/ssl3/tls1/tls12 is only set if the protocol is
      * available. (Otherwise we exit early.) However the compiler doesn't
      * know this, so we ifdef.
      */
@@ -1482,6 +1524,8 @@ int main(int argc, char *argv[])
 #ifndef OPENSSL_NO_TLS1
     if (tls1)
         meth = TLSv1_method();
+    else if (tls12)
+        meth = TLSv1_2_method();
     else
 #endif
         meth = SSLv23_method();
@@ -1778,8 +1822,12 @@ int main(int argc, char *argv[])
         OPENSSL_free(alpn);
     }
 
-    if (sn_server1 || sn_server2)
-        SSL_CTX_set_tlsext_servername_callback(s_ctx, servername_cb);
+    if (sn_server1 || sn_server2) {
+        if (sni_in_cert_cb)
+            SSL_CTX_set_cert_cb(s_ctx, cert_cb, NULL);
+        else
+            SSL_CTX_set_tlsext_servername_callback(s_ctx, servername_cb);
+    }
 
 #ifndef OPENSSL_NO_TLSEXT
     if (s_ticket1 == 0)
@@ -1799,6 +1847,9 @@ int main(int argc, char *argv[])
         SSL_CTX_set_options(c_ctx, SSL_OP_NO_TICKET);
 #endif
 
+    if (client_sigalgs != NULL)
+        SSL_CTX_set1_sigalgs_list(c_ctx, client_sigalgs);
+
     c_ssl = SSL_new(c_ctx);
     s_ssl = SSL_new(s_ctx);
 
@@ -1864,6 +1915,8 @@ int main(int argc, char *argv[])
         ret = 1;
     if (verify_ticket(c_ssl) < 0)
         ret = 1;
+    if (verify_server_digest(c_ssl) < 0)
+        ret = 1;
 
     SSL_free(s_ssl);
     SSL_free(c_ssl);

diff --git a/test/testssl b/test/testssl
index d7dda6e9a5af4cb9c2b2e5850b079cd8bcb74253..21bc4d81400e90656b094f41e6ee4e1ac9495237 100644 (file)
--- a/test/testssl
+++ b/test/testssl
@@ -292,6 +292,15 @@ if [ -z "$extra" -a `uname -m` = "x86_64" ]; then
   $ssltest -cipher AES128-SHA256 -bytes 8m     || exit 1
 fi
 
+#############################################################################
+# Signature algorithms + SNI
+
+$ssltest -tls12 -sn_client server1 -sn_server1 server1 -sn_server2 server2 -sn_expect1 -client_sigalgs RSA+SHA256 -server_digest_expect SHA256 || exit 1
+$ssltest -tls12 -sn_client server1 -sn_server1 server1 -sn_server2 server2 -sn_expect1 -client_sigalgs RSA+SHA256 -server_digest_expect SHA256 -sni_in_cert_cb || exit 1
+# Switching SSL_CTX on SNI must not break signature algorithm negotiation.
+$ssltest -tls12 -sn_client server2 -sn_server1 server1 -sn_server2 server2 -sn_expect2 -client_sigalgs RSA+SHA256 -server_digest_expect SHA256 || exit 1
+$ssltest -tls12 -sn_client server2 -sn_server1 server1 -sn_server2 server2 -sn_expect2 -client_sigalgs RSA+SHA256 -server_digest_expect SHA256 -sni_in_cert_cb || exit 1
+
 
 $ssltest -bio_pair -sn_client alice -sn_server1 alice -sn_server2 bob -s_ticket1 no -s_ticket2 no -c_ticket no -ticket_expect no || exit 1
 $ssltest -bio_pair -sn_client alice -sn_server1 alice -sn_server2 bob -s_ticket1 no -s_ticket2 no -c_ticket yes -ticket_expect no || exit 1