Fix session ticket and SNI

When session tickets are used, it's possible that SNI might swtich the
SSL_CTX on an SSL. Normally, this is not a problem, because the
initial_ctx/session_ctx are used for all session ticket/id processes.

However, when the SNI callback occurs, it's possible that the callback
may update the options in the SSL from the SSL_CTX, and this could
cause SSL_OP_NO_TICKET to be set. If this occurs, then two bad things
can happen:

1. The session ticket TLSEXT may not be written when the ticket expected
flag is set. The state machine transistions to writing the ticket, and
the client responds with an error as its not expecting a ticket.
2. When creating the session ticket, if the ticket key cb returns 0
the crypto/hmac contexts are not initialized, and the code crashes when
trying to encrypt the session ticket.

To fix 1, if the ticket TLSEXT is not written out, clear the expected
ticket flag.
To fix 2, consider a return of 0 from the ticket key cb a recoverable
error, and write a 0 length ticket and continue. The client-side code
can explicitly handle this case.

Fix these two cases, and add unit test code to validate ticket behavior.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1065)

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index becfccce13a143e0621030cb97e055830a66085a..82ea0c002c46a331a023546b58e2905c048ff063 100644 (file)
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -3468,8 +3468,22 @@ int ssl3_send_newsession_ticket(SSL *s)
          * all the work otherwise use generated values from parent ctx.
          */
         if (tctx->tlsext_ticket_key_cb) {
-            if (tctx->tlsext_ticket_key_cb(s, key_name, iv, &ctx,
-                                           &hctx, 1) < 0)
+            /* if 0 is returned, write en empty ticket */
+            int ret = tctx->tlsext_ticket_key_cb(s, key_name, iv, &ctx,
+                                                 &hctx, 1);
+
+            if (ret == 0) {
+                l2n(0, p); /* timeout */
+                s2n(0, p); /* length */
+                ssl_set_handshake_header(s, SSL3_MT_NEWSESSION_TICKET,
+                                         p - ssl_handshake_start(s));
+                s->state = SSL3_ST_SW_SESSION_TICKET_B;
+                OPENSSL_free(senc);
+                EVP_CIPHER_CTX_cleanup(&ctx);
+                HMAC_CTX_cleanup(&hctx);
+                return ssl_do_write(s);
+            }
+            if (ret < 0)
                 goto err;
         } else {
             if (RAND_bytes(iv, 16) <= 0)

diff --git a/ssl/ssltest.c b/ssl/ssltest.c
index 890e47685350bbf9c13392281403ba3af07d33d3..8bac2bb87e918fafc30067b4f0176db2057d7366 100644 (file)
--- a/ssl/ssltest.c
+++ b/ssl/ssltest.c
@@ -311,6 +311,10 @@ static const char *sn_client;
 static const char *sn_server1;
 static const char *sn_server2;
 static int sn_expect = 0;
+static int s_ticket1 = 0;
+static int s_ticket2 = 0;
+static int c_ticket = 0;
+static int ticket_expect = -1;
 
 static int servername_cb(SSL *s, int *ad, void *arg)
 {
@@ -325,6 +329,9 @@ static int servername_cb(SSL *s, int *ad, void *arg)
             !strcasecmp(servername, sn_server2)) {
             BIO_printf(bio_stdout, "Switching server context.\n");
             SSL_set_SSL_CTX(s, s_ctx2);
+            /* Copy over all the SSL_CTX options */
+            SSL_clear_options(s, 0xFFFFFFFFL);
+            SSL_set_options(s, SSL_CTX_get_options(s_ctx2));
         }
     }
     return SSL_TLSEXT_ERR_OK;
@@ -349,6 +356,21 @@ static int verify_servername(SSL *client, SSL *server)
     return -1;
 }
 
+static int verify_ticket(SSL* ssl)
+{
+    if (ticket_expect == -1)
+        return 0;
+    if (ticket_expect == 0 &&
+        (ssl->session->tlsext_tick == NULL ||
+         ssl->session->tlsext_ticklen == 0))
+        return 1;
+    if (ticket_expect == 1 &&
+        (ssl->session->tlsext_tick != NULL &&
+         ssl->session->tlsext_ticklen != 0))
+        return 1;
+    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.
@@ -477,6 +499,42 @@ static int verify_alpn(SSL *client, SSL *server)
     return -1;
 }
 
+#ifndef OPENSSL_NO_TLSEXT
+
+static int cb_ticket0(SSL* s, unsigned char* key_name, unsigned char *iv, EVP_CIPHER_CTX *ctx, HMAC_CTX *hctx, int enc)
+{
+    return 0;
+}
+
+static int cb_ticket1(SSL* s, unsigned char* key_name, unsigned char *iv, EVP_CIPHER_CTX *ctx, HMAC_CTX *hctx, int enc)
+{
+    static unsigned char key[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16 };
+    static char name[] = "ticket11ticket11";
+    if (SSL_get_options(s) & SSL_OP_NO_TICKET)
+        return 0;
+    if (enc) {
+        RAND_pseudo_bytes(iv, EVP_MAX_IV_LENGTH);
+        EVP_EncryptInit_ex(ctx, EVP_aes_128_cbc(), NULL, key, iv);
+        HMAC_Init_ex(hctx, key, sizeof(key), EVP_sha1(), NULL);
+        memcpy(key_name, name, 16);
+        return 1;
+    } else {
+        if (memcmp(key_name, name, 16) == 0) {
+            EVP_DecryptInit_ex(ctx, EVP_aes_128_cbc(), NULL, key, iv);
+            HMAC_Init_ex(hctx, key, sizeof(key), EVP_sha1(), NULL);
+            return 1;
+        }
+    }
+    return 0;
+}
+
+static int cb_ticket2(SSL* s, unsigned char* key_name, unsigned char *iv, EVP_CIPHER_CTX *ctx, HMAC_CTX *hctx, int enc)
+{
+    fprintf(stderr, "ticket callback for SNI context should never be called\n");
+    EXIT(1);
+}
+#endif
+
 #define SCT_EXT_TYPE 18
 
 /*
@@ -820,6 +878,12 @@ static void sv_usage(void)
     fprintf(stderr, " -sn_server2 <string> - have server context 2 respond to this servername\n");
     fprintf(stderr, " -sn_expect1          - expected server 1\n");
     fprintf(stderr, " -sn_expect2          - expected server 2\n");
+#ifndef OPENSSL_NO_TLSEXT
+    fprintf(stderr, " -s_ticket1 <yes|no|broken> - enable/disable session tickets on context 1\n");
+    fprintf(stderr, " -s_ticket2 <yes|no>        - enable/disable session tickets on context 2\n");
+    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
 }
 
 static void print_details(SSL *c_ssl, const char *prefix)
@@ -1241,6 +1305,36 @@ int main(int argc, char *argv[])
             sn_expect = 1;
         } else if (strcmp(*argv, "-sn_expect2") == 0) {
             sn_expect = 2;
+#ifndef OPENSSL_NO_TLSEXT
+        } else if (strcmp(*argv, "-s_ticket1") == 0) {
+            if (--argc < 1)
+                goto bad;
+            argv++;
+            if (strcmp(*argv, "yes") == 0)
+                s_ticket1 = 1;
+            if (strcmp(*argv, "broken") == 0)
+                s_ticket1 = 2;
+        } else if (strcmp(*argv, "-s_ticket2") == 0) {
+            if (--argc < 1)
+                goto bad;
+            argv++;
+            if (strcmp(*argv, "yes") == 0)
+                s_ticket2 = 1;
+        } else if (strcmp(*argv, "-c_ticket") == 0) {
+            if (--argc < 1)
+                goto bad;
+            argv++;
+            if (strcmp(*argv, "yes") == 0)
+                c_ticket = 1;
+        } else if (strcmp(*argv, "-ticket_expect") == 0) {
+            if (--argc < 1)
+                goto bad;
+            argv++;
+            if (strcmp(*argv, "yes") == 0)
+                ticket_expect = 1;
+            else if (strcmp(*argv, "no") == 0)
+                ticket_expect = 0;
+#endif
         } else {
             fprintf(stderr, "unknown option %s\n", *argv);
             badop = 1;
@@ -1679,6 +1773,24 @@ int main(int argc, char *argv[])
     if (sn_server1 || sn_server2)
         SSL_CTX_set_tlsext_servername_callback(s_ctx, servername_cb);
 
+#ifndef OPENSSL_NO_TLSEXT
+    if (s_ticket1 == 0)
+        SSL_CTX_set_options(s_ctx, SSL_OP_NO_TICKET);
+    /* always set the callback */
+    if (s_ticket1 == 2)
+        SSL_CTX_set_tlsext_ticket_key_cb(s_ctx, cb_ticket0);
+    else
+        SSL_CTX_set_tlsext_ticket_key_cb(s_ctx, cb_ticket1);
+
+    if (!s_ticket2)
+        SSL_CTX_set_options(s_ctx2, SSL_OP_NO_TICKET);
+    /* always set the callback - this should never be called */
+    SSL_CTX_set_tlsext_ticket_key_cb(s_ctx2, cb_ticket2);
+
+    if (!c_ticket)
+        SSL_CTX_set_options(c_ctx, SSL_OP_NO_TICKET);
+#endif
+
     c_ssl = SSL_new(c_ctx);
     s_ssl = SSL_new(s_ctx);
 
@@ -1742,6 +1854,8 @@ int main(int argc, char *argv[])
         ret = 1;
     if (verify_servername(c_ssl, s_ssl) < 0)
         ret = 1;
+    if (verify_ticket(c_ssl) < 0)
+        ret = 1;
 
     SSL_free(s_ssl);
     SSL_free(c_ssl);

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 5355f0ee110e5014af51fad8e52e35fabac8bb88..d07a9f0e837043f4db2ead8b90bf5cc11a89ced1 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1769,6 +1769,9 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
             return NULL;
         s2n(TLSEXT_TYPE_session_ticket, ret);
         s2n(0, ret);
+    } else {
+        /* if we don't add the above TLSEXT, we can't add a session ticket later */
+        s->tlsext_ticket_expected = 0;
     }
 
     if (s->tlsext_status_expected) {

diff --git a/test/testssl b/test/testssl
index a6f9fa770b364749ab1f13fa6b2328d45c0f19d5..d7dda6e9a5af4cb9c2b2e5850b079cd8bcb74253 100644 (file)
--- a/test/testssl
+++ b/test/testssl
@@ -292,4 +292,25 @@ if [ -z "$extra" -a `uname -m` = "x86_64" ]; then
   $ssltest -cipher AES128-SHA256 -bytes 8m     || exit 1
 fi
 
+
+$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
+$ssltest -bio_pair -sn_client alice -sn_server1 alice -sn_server2 bob -s_ticket1 no -s_ticket2 yes -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 yes -c_ticket yes -ticket_expect no || exit 1
+$ssltest -bio_pair -sn_client alice -sn_server1 alice -sn_server2 bob -s_ticket1 yes -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 yes -s_ticket2 no -c_ticket yes -ticket_expect yes || exit 1
+$ssltest -bio_pair -sn_client alice -sn_server1 alice -sn_server2 bob -s_ticket1 yes -s_ticket2 yes -c_ticket no -ticket_expect no || exit 1
+$ssltest -bio_pair -sn_client alice -sn_server1 alice -sn_server2 bob -s_ticket1 yes -s_ticket2 yes -c_ticket yes -ticket_expect yes || exit 1
+
+$ssltest -bio_pair -sn_client bob -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 bob -sn_server1 alice -sn_server2 bob -s_ticket1 no -s_ticket2 no -c_ticket yes -ticket_expect no || exit 1
+$ssltest -bio_pair -sn_client bob -sn_server1 alice -sn_server2 bob -s_ticket1 no -s_ticket2 yes -c_ticket no -ticket_expect no || exit 1
+$ssltest -bio_pair -sn_client bob -sn_server1 alice -sn_server2 bob -s_ticket1 no -s_ticket2 yes -c_ticket yes -ticket_expect no || exit 1
+$ssltest -bio_pair -sn_client bob -sn_server1 alice -sn_server2 bob -s_ticket1 yes -s_ticket2 no -c_ticket no -ticket_expect no || exit 1
+$ssltest -bio_pair -sn_client bob -sn_server1 alice -sn_server2 bob -s_ticket1 yes -s_ticket2 no -c_ticket yes -ticket_expect no || exit 1
+$ssltest -bio_pair -sn_client bob -sn_server1 alice -sn_server2 bob -s_ticket1 yes -s_ticket2 yes -c_ticket no -ticket_expect no || exit 1
+$ssltest -bio_pair -sn_client bob -sn_server1 alice -sn_server2 bob -s_ticket1 yes -s_ticket2 yes -c_ticket yes -ticket_expect yes || exit 1
+
+$ssltest -bio_pair -s_ticket1 broken -c_ticket yes -ticket_expect no || exit 1
+
 exit 0