Commit 5c753de6 authored by Todd Short's avatar Todd Short Committed by Rich Salz
Browse files

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: default avatarEmilia Käsper <emilia@openssl.org>
Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1098)
parent 2a7de0fd

ssl/statem/statem_srvr.c

+15 −1
Original line number Diff line number Diff line
@@ -2950,7 +2950,21 @@ int tls_construct_new_session_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 an 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 */

            if (!ssl_set_handshake_header(s, SSL3_MT_NEWSESSION_TICKET, p - ssl_handshake_start(s)))

                goto err;

            OPENSSL_free(senc);

            EVP_CIPHER_CTX_free(ctx);

            HMAC_CTX_free(hctx);

            return 1;

        }

        if (ret < 0)

            goto err;

        iv_len = EVP_CIPHER_CTX_iv_length(ctx);

    } else {

ssl/t1_lib.c

+3 −0
Original line number Diff line number Diff line
@@ -1502,6 +1502,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) {

test/README.ssltest.md

+14 −0
Original line number Diff line number Diff line
@@ -64,6 +64,16 @@ The test section supports the following options: 
  - AcceptAll - accepts all certificates.

  - RejectAll - rejects all certificates.



* ServerName - the server the client is expected to successfully connect to

  - server1 - the initial context (default)

  - server2 - the secondary context



* SessionTicketExpected - whether or not a session ticket is expected

  - Ignore - do not check for a session ticket (default)

  - Yes - a session ticket is expected

  - No - a session ticket is not expected

  - Broken - a special test case where the session ticket callback does not initialize crypto



## Configuring the client and server



The client and server configurations can be any valid `SSL_CTX`
@@ -78,6 +88,10 @@ server => { 
}

```



A server2 section may optionally be defined to configure a secondary

context that is selected via the ServerName test option. If the server2

section is not configured, then the configuration matches server.



### Default server and client configurations



The default server certificate and CA files are added to the configurations

test/generate_ssl_tests.pl

+6 −0
Original line number Diff line number Diff line
@@ -43,6 +43,12 @@ sub print_templates { 
    # Add the implicit base configuration.

    foreach my $test (@ssltests::tests) {

        $test->{"server"} = { (%ssltests::base_server, %{$test->{"server"}}) };

	# use server values if server2 is not defined

	if (defined $test->{"server2"}) {

	    $test->{"server2"} = { (%ssltests::base_server, %{$test->{"server2"}}) };

	} else {

	    $test->{"server2"} = { (%ssltests::base_server, %{$test->{"server"}}) };

	}

        $test->{"client"} = { (%ssltests::base_client, %{$test->{"client"}}) };

    }

test/handshake_helper.c

+47 −3
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ 
typedef struct handshake_ex_data {

    int alert_sent;

    int alert_received;

    int session_ticket_do_not_call;

} HANDSHAKE_EX_DATA;



static int ex_data_idx;
@@ -49,11 +50,26 @@ static int verify_accept_callback(X509_STORE_CTX *ctx, void *arg) { 
    return 1;

}



static int broken_session_ticket_callback(SSL* s, unsigned char* key_name, unsigned char *iv,

                                          EVP_CIPHER_CTX *ctx, HMAC_CTX *hctx, int enc)

{

    return 0;

}



int do_not_call_session_ticket_callback(SSL* s, unsigned char* key_name, unsigned char *iv,

                                        EVP_CIPHER_CTX *ctx, HMAC_CTX *hctx, int enc)

{

    HANDSHAKE_EX_DATA *ex_data =

        (HANDSHAKE_EX_DATA*)(SSL_get_ex_data(s, ex_data_idx));

    ex_data->session_ticket_do_not_call = 1;

    return 0;

}



/*

 * Configure callbacks and other properties that can't be set directly

 * in the server/client CONF.

 */

static void configure_handshake(SSL_CTX *server_ctx, SSL_CTX *client_ctx,

static void configure_handshake_ctx(SSL_CTX *server_ctx, SSL_CTX *client_ctx,

                                    const SSL_TEST_CTX *test_ctx)

{

    switch (test_ctx->client_verify_callback) {
@@ -68,6 +84,19 @@ static void configure_handshake(SSL_CTX *server_ctx, SSL_CTX *client_ctx, 
    default:

        break;

    }

    if (test_ctx->session_ticket_expected == SSL_TEST_SESSION_TICKET_BROKEN) {

        SSL_CTX_set_tlsext_ticket_key_cb(server_ctx, broken_session_ticket_callback);

    }

}



/*

 * Configure callbacks and other properties that can't be set directly

 * in the server/client CONF.

 */

static void configure_handshake_ssl(SSL *server, SSL *client,

                                    const SSL_TEST_CTX *test_ctx)

{

    SSL_set_tlsext_host_name(client, ssl_servername_name(test_ctx->servername));

}
@@ -180,13 +209,18 @@ HANDSHAKE_RESULT do_handshake(SSL_CTX *server_ctx, SSL_CTX *client_ctx, 
    int client_turn = 1;

    peer_status_t client_status = PEER_RETRY, server_status = PEER_RETRY;

    handshake_status_t status = HANDSHAKE_RETRY;

    unsigned char* tick = NULL;

    size_t len = 0;

    SSL_SESSION* sess = NULL;



    configure_handshake(server_ctx, client_ctx, test_ctx);

    configure_handshake_ctx(server_ctx, client_ctx, test_ctx);



    server = SSL_new(server_ctx);

    client = SSL_new(client_ctx);

    OPENSSL_assert(server != NULL && client != NULL);



    configure_handshake_ssl(server, client, test_ctx);



    memset(&server_ex_data, 0, sizeof(server_ex_data));

    memset(&client_ex_data, 0, sizeof(client_ex_data));

    memset(&ret, 0, sizeof(ret));
@@ -266,6 +300,16 @@ HANDSHAKE_RESULT do_handshake(SSL_CTX *server_ctx, SSL_CTX *client_ctx, 
    ret.client_alert_received = server_ex_data.alert_received;

    ret.server_protocol = SSL_version(server);

    ret.client_protocol = SSL_version(client);

    ret.servername = ((SSL_get_SSL_CTX(server) == server_ctx)

                      ? SSL_TEST_SERVERNAME_SERVER1

                      : SSL_TEST_SERVERNAME_SERVER2);

    if ((sess = SSL_get0_session(client)) != NULL)

        SSL_SESSION_get0_ticket(sess, &tick, &len);

    if (tick == NULL || len == 0)

        ret.session_ticket = SSL_TEST_SESSION_TICKET_NO;

    else

        ret.session_ticket = SSL_TEST_SESSION_TICKET_YES;

    ret.session_ticket_do_not_call = server_ex_data.session_ticket_do_not_call;



    SSL_free(server);

    SSL_free(client);