From 61fb59238dad6452a37ec14513fae617a4faef29 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 9 May 2018 18:22:36 +0100 Subject: [PATCH] Rework the decrypt ticket callback Don't call the decrypt ticket callback if we've already encountered a fatal error. Do call it if we have an empty ticket present. Change the return code to have 5 distinct returns codes and separate it from the input status value. Reviewed-by: Viktor Dukhovni (Merged from https://github.com/openssl/openssl/pull/6198) --- doc/man3/SSL_CTX_set_session_ticket_cb.pod | 131 ++++++++++++------- include/openssl/ssl.h | 21 ++- ssl/ssl_locl.h | 4 +- ssl/ssl_sess.c | 7 +- ssl/statem/extensions_srvr.c | 17 ++- ssl/statem/statem_srvr.c | 6 +- ssl/t1_lib.c | 130 ++++++++++++------- test/handshake_helper.c | 22 +++- test/sslapitest.c | 143 ++++++++++++++++----- 9 files changed, 336 insertions(+), 145 deletions(-) diff --git a/doc/man3/SSL_CTX_set_session_ticket_cb.pod b/doc/man3/SSL_CTX_set_session_ticket_cb.pod index 3066534223..8f98c6f1c9 100644 --- a/doc/man3/SSL_CTX_set_session_ticket_cb.pod +++ b/doc/man3/SSL_CTX_set_session_ticket_cb.pod @@ -16,7 +16,7 @@ SSL_CTX_decrypt_session_ticket_fn - manage session ticket application data typedef SSL_TICKET_RETURN (*SSL_CTX_decrypt_session_ticket_fn)(SSL *s, SSL_SESSION *ss, const unsigned char *keyname, size_t keyname_len, - SSL_TICKET_RETURN retv, + SSL_TICKET_STATUS status, void *arg); int SSL_CTX_set_session_ticket_cb(SSL_CTX *ctx, SSL_CTX_generate_session_ticket_fn gen_cb, @@ -39,13 +39,13 @@ is the same as that given to SSL_CTX_set_session_ticket_cb(). The B callback is defined as type B. B is the application defined callback invoked after session ticket -decryption has been attempted and any session ticket application data is available. -The application can call SSL_SESSION_get_ticket_appdata() at this time to retrieve -the application data. The value of B is the same as that given to -SSL_CTX_set_session_ticket_cb(). The B argument is the result of the ticket -decryption. The B and B identify the key used to decrypt the -session ticket. The B callback is defined as type -B. +decryption has been attempted and any session ticket application data is +available. If ticket decryption was successful then the B argument contains +the session data. The B and B arguments identify the key +used to decrypt the session ticket. The B argument is the result of the +ticket decryption. See the L section below for further details. The value +of B is the same as that given to SSL_CTX_set_session_ticket_cb(). The +B callback is defined as type B. SSL_SESSION_set1_ticket_appdata() sets the application data specified by B and B into B which is then placed into any generated session @@ -66,73 +66,110 @@ application that a session ticket has just been decrypted. =head1 NOTES When the B callback is invoked, the SSL_SESSION B has not yet been -assigned to the SSL B. The B indicates the result of the ticket -decryption which can be modified by the callback before being returned. The -callback must check the B value before performing any action, as it's -called even if ticket decryption fails. +assigned to the SSL B. The B indicates the result of the ticket +decryption. The callback must check the B value before performing any +action, as it is called even if ticket decryption fails. The B and B arguments to B may be used to identify the key that was used to encrypt the session ticket. -When the B callback is invoked, the SSL_get_session() function can be -used to retrieve the SSL_SESSION for SSL_SESSION_set1_ticket_appdata(). +The B argument can be any of these values: -By default, in TLSv1.2 and below, a new session ticket is not issued on a -successful resumption and therefore B will not be called. In TLSv1.3 the -default behaviour is to always issue a new ticket on resumption. In both cases -this behaviour can be changed if a ticket key callback is in use (see -L). +=over 4 -=head1 RETURN VALUES +=item SSL_TICKET_EMPTY -The SSL_CTX_set_session_ticket_cb(), SSL_SESSION_set1_ticket_appdata() and -SSL_SESSION_get0_ticket_appdata() functions return 1 on success and 0 on -failure. +Empty ticket present. No ticket data will be used and a new ticket should be +sent to the client. This only occurs in TLSv1.2 or below. In TLSv1.3 it is not +valid for a client to send an empty ticket. -The B callback must return 1 to continue the connection. A return of 0 -will terminate the connection with an INTERNAL_ERROR alert. +=item SSL_TICKET_NO_DECRYPT -The B callback must return one of the following B -values. Under normal circumstances the B value is returned unmodified, -but the callback can change the behavior of the post-ticket decryption code -by returning something different. The B callback must check the B -value before performing any action. +The ticket couldn't be decrypted. No ticket data will be used and a new ticket +should be sent to the client. - typedef int SSL_TICKET_RETURN; +=item SSL_TICKET_SUCCESS -=over 4 +A ticket was successfully decrypted, any session ticket application data should +be available. A new ticket should not be sent to the client. -=item SSL_TICKET_FATAL_ERR_MALLOC +=item SSL_TICKET_SUCCESS_RENEW -Fatal error, malloc failure. +Same as B, but a new ticket should be sent to the client. -=item SSL_TICKET_FATAL_ERR_OTHER +=back -Fatal error, either from parsing or decrypting the ticket. +The return value can be any of these values: -=item SSL_TICKET_NONE +=over 4 -No ticket present. +=item SSL_TICKET_RETURN_ABORT -=item SSL_TICKET_EMPTY +The handshake should be aborted, either because of an error or because of some +policy. Note that in TLSv1.3 a client may send more than one ticket in a single +handshake. Therefore just because one ticket is unacceptable it does not mean +that all of them are. For this reason this option should be used with caution. -Empty ticket present. +=item SSL_TICKET_RETURN_IGNORE -=item SSL_TICKET_NO_DECRYPT +Do not use a ticket (if one was available). Do not send a renewed ticket to the +client. -The ticket couldn't be decrypted. +=item SSL_TICKET_RETURN_IGNORE_RENEW -=item SSL_TICKET_SUCCESS +Do not use a ticket (if one was available). Send a renewed ticket to the client. -A ticket was successfully decrypted, any session ticket application data should -be available. +If the callback does not wish to change the default ticket behaviour then it +should return this value if B is B or +B. -=item TICKET_SUCCESS_RENEW +=item SSL_TICKET_RETURN_USE -Same as B, but the ticket needs to be renewed. +Use the ticket. Do not send a renewed ticket to the client. It is an error for +the callback to return this value if B has a value other than +B or B. + +If the callback does not wish to change the default ticket behaviour then it +should return this value if B is B. + +=item SSL_TICKET_RETURN_USE_RENEW + +Use the ticket. Send a renewed ticket to the client. It is an error for the +callback to return this value if B has a value other than +B or B. + +If the callback does not wish to change the default ticket behaviour then it +should return this value if B is B. =back +If B has the value B or B then +no session data will be available and the callback must not use the B +argument. If B has the value B or +B then the application can call +SSL_SESSION_get0_ticket_appdata() using the session provided in the B +argument to retrieve the application data. + +When the B callback is invoked, the SSL_get_session() function can be +used to retrieve the SSL_SESSION for SSL_SESSION_set1_ticket_appdata(). + +By default, in TLSv1.2 and below, a new session ticket is not issued on a +successful resumption and therefore B will not be called. In TLSv1.3 the +default behaviour is to always issue a new ticket on resumption. In both cases +this behaviour can be changed if a ticket key callback is in use (see +L). + +=head1 RETURN VALUES + +The SSL_CTX_set_session_ticket_cb(), SSL_SESSION_set1_ticket_appdata() and +SSL_SESSION_get0_ticket_appdata() functions return 1 on success and 0 on +failure. + +The B callback must return 1 to continue the connection. A return of 0 +will terminate the connection with an INTERNAL_ERROR alert. + +The B callback must return a value as described in L above. + =head1 SEE ALSO L, diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 4bd53fc24c..1f4f2616b6 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2330,8 +2330,9 @@ __owur const struct openssl_ssl_test_functions *SSL_test_functions(void); __owur int SSL_free_buffers(SSL *ssl); __owur int SSL_alloc_buffers(SSL *ssl); -/* Return codes for tls_get_ticket_from_client() and tls_decrypt_ticket() */ -typedef int SSL_TICKET_RETURN; +/* Status codes passed to the decrypt session ticket callback. Some of these + * are for internal use only and are never passed to the callback. */ +typedef int SSL_TICKET_STATUS; /* Support for ticket appdata */ /* fatal error, malloc failure */ @@ -2349,11 +2350,25 @@ typedef int SSL_TICKET_RETURN; /* same as above but the ticket needs to be renewed */ # define SSL_TICKET_SUCCESS_RENEW 6 +/* Return codes for the decrypt session ticket callback */ +typedef int SSL_TICKET_RETURN; + +/* An error occurred */ +#define SSL_TICKET_RETURN_ABORT 0 +/* Do not use the ticket, do not send a renewed ticket to the client */ +#define SSL_TICKET_RETURN_IGNORE 1 +/* Do not use the ticket, send a renewed ticket to the client */ +#define SSL_TICKET_RETURN_IGNORE_RENEW 2 +/* Use the ticket, do not send a renewed ticket to the client */ +#define SSL_TICKET_RETURN_USE 3 +/* Use the ticket, send a renewed ticket to the client */ +#define SSL_TICKET_RETURN_USE_RENEW 4 + typedef int (*SSL_CTX_generate_session_ticket_fn)(SSL *s, void *arg); typedef SSL_TICKET_RETURN (*SSL_CTX_decrypt_session_ticket_fn)(SSL *s, SSL_SESSION *ss, const unsigned char *keyname, size_t keyname_length, - SSL_TICKET_RETURN retv, + SSL_TICKET_STATUS status, void *arg); int SSL_CTX_set_session_ticket_cb(SSL_CTX *ctx, SSL_CTX_generate_session_ticket_fn gen_cb, diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index b32b23bedf..c066e14b70 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2473,9 +2473,9 @@ void tls1_get_supported_groups(SSL *s, const uint16_t **pgroups, __owur int tls1_set_server_sigalgs(SSL *s); -__owur SSL_TICKET_RETURN tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello, +__owur SSL_TICKET_STATUS tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello, SSL_SESSION **ret); -__owur SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick, +__owur SSL_TICKET_STATUS tls_decrypt_ticket(SSL *s, const unsigned char *etick, size_t eticklen, const unsigned char *sess_id, size_t sesslen, SSL_SESSION **psess); diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index f936cb687f..541f82a851 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -485,9 +485,14 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello) SSL_SESSION *ret = NULL; int fatal = 0, discard; int try_session_cache = 0; - SSL_TICKET_RETURN r; + SSL_TICKET_STATUS r; if (SSL_IS_TLS13(s)) { + /* + * By default we will send a new ticket. This can be overridden in the + * ticket processing. + */ + s->ext.ticket_expected = 1; if (!tls_parse_extension(s, TLSEXT_IDX_psk_kex_modes, SSL_EXT_CLIENT_HELLO, hello->pre_proc_exts, NULL, 0) diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index adf63d80bf..ec4e1b8139 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -1030,6 +1030,7 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x, return 0; } + s->ext.ticket_expected = 0; for (id = 0; PACKET_remaining(&identities) != 0; id++) { PACKET identity; unsigned long ticket_agel; @@ -1127,9 +1128,17 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x, s->ext.early_data_ok = 1; } else { uint32_t ticket_age = 0, now, agesec, agems; - int ret = tls_decrypt_ticket(s, PACKET_data(&identity), - PACKET_remaining(&identity), NULL, 0, - &sess); + int ret; + + ret = tls_decrypt_ticket(s, PACKET_data(&identity), + PACKET_remaining(&identity), NULL, 0, + &sess); + + if (ret == SSL_TICKET_EMPTY) { + SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PARSE_CTOS_PSK, + SSL_R_BAD_EXTENSION); + return 0; + } if (ret == SSL_TICKET_FATAL_ERR_MALLOC || ret == SSL_TICKET_FATAL_ERR_OTHER) { @@ -1137,7 +1146,7 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x, SSL_F_TLS_PARSE_CTOS_PSK, ERR_R_INTERNAL_ERROR); return 0; } - if (ret == SSL_TICKET_NO_DECRYPT) + if (ret == SSL_TICKET_NONE || ret == SSL_TICKET_NO_DECRYPT) continue; /* Check for replay */ diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 3f56ff1389..22786bed13 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -489,10 +489,10 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) st->hand_state = TLS_ST_SW_SESSION_TICKET; if (s->post_handshake_auth == SSL_PHA_REQUESTED) { s->post_handshake_auth = SSL_PHA_EXT_RECEIVED; - } else if (s->hit && !s->ext.ticket_expected) { + } else if (!s->ext.ticket_expected) { /* - * If we resumed and we're not going to renew the ticket then we - * just finish the handshake at this point. + * If we're not going to renew the ticket then we just finish the + * handshake at this point. */ st->hand_state = TLS_ST_OK; } diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 2e8de7a09c..b312a14fab 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1194,20 +1194,8 @@ int tls1_set_server_sigalgs(SSL *s) * hello: The parsed ClientHello data * ret: (output) on return, if a ticket was decrypted, then this is set to * point to the resulting session. - * - * If s->tls_session_secret_cb is set then we are expecting a pre-shared key - * ciphersuite, in which case we have no use for session tickets and one will - * never be decrypted, nor will s->ext.ticket_expected be set to 1. - * - * Side effects: - * Sets s->ext.ticket_expected to 1 if the server will have to issue - * a new session ticket to the client because the client indicated support - * (and s->tls_session_secret_cb is NULL) but the client either doesn't have - * a session ticket or we couldn't use the one it gave us, or if - * s->ctx->ext.ticket_key_cb asked to renew the client's ticket. - * Otherwise, s->ext.ticket_expected is set to 0. */ -SSL_TICKET_RETURN tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello, +SSL_TICKET_STATUS tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello, SSL_SESSION **ret) { size_t size; @@ -1229,23 +1217,6 @@ SSL_TICKET_RETURN tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello, return SSL_TICKET_NONE; size = PACKET_remaining(&ticketext->data); - if (size == 0) { - /* - * The client will accept a ticket but doesn't currently have - * one. - */ - s->ext.ticket_expected = 1; - return SSL_TICKET_EMPTY; - } - if (s->ext.session_secret_cb) { - /* - * Indicate that the ticket couldn't be decrypted rather than - * generating the session from ticket now, trigger - * abbreviated handshake based on external mechanism to - * calculate the master secret later. - */ - return SSL_TICKET_NO_DECRYPT; - } return tls_decrypt_ticket(s, PACKET_data(&ticketext->data), size, hello->session_id, hello->session_id_len, ret); @@ -1254,6 +1225,19 @@ SSL_TICKET_RETURN tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello, /*- * tls_decrypt_ticket attempts to decrypt a session ticket. * + * If s->tls_session_secret_cb is set and we're not doing TLSv1.3 then we are + * expecting a pre-shared key ciphersuite, in which case we have no use for + * session tickets and one will never be decrypted, nor will + * s->ext.ticket_expected be set to 1. + * + * Side effects: + * Sets s->ext.ticket_expected to 1 if the server will have to issue + * a new session ticket to the client because the client indicated support + * (and s->tls_session_secret_cb is NULL) but the client either doesn't have + * a session ticket or we couldn't use the one it gave us, or if + * s->ctx->ext.ticket_key_cb asked to renew the client's ticket. + * Otherwise, s->ext.ticket_expected is set to 0. + * * etick: points to the body of the session ticket extension. * eticklen: the length of the session tickets extension. * sess_id: points at the session ID. @@ -1261,21 +1245,40 @@ SSL_TICKET_RETURN tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello, * psess: (output) on return, if a ticket was decrypted, then this is set to * point to the resulting session. */ -SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick, +SSL_TICKET_STATUS tls_decrypt_ticket(SSL *s, const unsigned char *etick, size_t eticklen, const unsigned char *sess_id, size_t sesslen, SSL_SESSION **psess) { - SSL_SESSION *sess; + SSL_SESSION *sess = NULL; unsigned char *sdec; const unsigned char *p; int slen, renew_ticket = 0, declen; - SSL_TICKET_RETURN ret = SSL_TICKET_FATAL_ERR_OTHER; + SSL_TICKET_STATUS ret = SSL_TICKET_FATAL_ERR_OTHER; size_t mlen; unsigned char tick_hmac[EVP_MAX_MD_SIZE]; HMAC_CTX *hctx = NULL; EVP_CIPHER_CTX *ctx = NULL; SSL_CTX *tctx = s->session_ctx; + if (eticklen == 0) { + /* + * The client will accept a ticket but doesn't currently have + * one (TLSv1.2 and below), or treated as a fatal error in TLSv1.3 + */ + ret = SSL_TICKET_EMPTY; + goto end; + } + if (!SSL_IS_TLS13(s) && s->ext.session_secret_cb) { + /* + * Indicate that the ticket couldn't be decrypted rather than + * generating the session from ticket now, trigger + * abbreviated handshake based on external mechanism to + * calculate the master secret later. + */ + ret = SSL_TICKET_NO_DECRYPT; + goto end; + } + /* Need at least keyname + iv */ if (eticklen < TLSEXT_KEYNAME_LENGTH + EVP_MAX_IV_LENGTH) { ret = SSL_TICKET_NO_DECRYPT; @@ -1394,7 +1397,6 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick, memcpy(sess->session_id, sess_id, sesslen); sess->session_id_length = sesslen; } - *psess = sess; if (renew_ticket) ret = SSL_TICKET_SUCCESS_RENEW; else @@ -1412,18 +1414,56 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick, HMAC_CTX_free(hctx); /* - * If set, the decrypt_ticket_cb() is always called regardless of the - * return value determined above. The callback is responsible for checking - * |ret| before it performs any action + * If set, the decrypt_ticket_cb() is called unless a fatal error was + * detected above. The callback is responsible for checking |ret| before it + * performs any action */ - if (s->session_ctx->decrypt_ticket_cb != NULL) { + if (s->session_ctx->decrypt_ticket_cb != NULL + && (ret == SSL_TICKET_EMPTY + || ret == SSL_TICKET_NO_DECRYPT + || ret == SSL_TICKET_SUCCESS + || ret == SSL_TICKET_SUCCESS_RENEW)) { size_t keyname_len = eticklen; + int retcb; if (keyname_len > TLSEXT_KEYNAME_LENGTH) keyname_len = TLSEXT_KEYNAME_LENGTH; - ret = s->session_ctx->decrypt_ticket_cb(s, *psess, etick, keyname_len, - ret, - s->session_ctx->ticket_cb_data); + retcb = s->session_ctx->decrypt_ticket_cb(s, sess, etick, keyname_len, + ret, + s->session_ctx->ticket_cb_data); + switch (retcb) { + case SSL_TICKET_RETURN_ABORT: + ret = SSL_TICKET_FATAL_ERR_OTHER; + break; + + case SSL_TICKET_RETURN_IGNORE: + ret = SSL_TICKET_NONE; + SSL_SESSION_free(sess); + sess = NULL; + break; + + case SSL_TICKET_RETURN_IGNORE_RENEW: + if (ret != SSL_TICKET_EMPTY && ret != SSL_TICKET_NO_DECRYPT) + ret = SSL_TICKET_NO_DECRYPT; + /* else the value of |ret| will already do the right thing */ + SSL_SESSION_free(sess); + sess = NULL; + break; + + case SSL_TICKET_RETURN_USE: + case SSL_TICKET_RETURN_USE_RENEW: + if (ret != SSL_TICKET_SUCCESS + && ret != SSL_TICKET_SUCCESS_RENEW) + ret = SSL_TICKET_FATAL_ERR_OTHER; + else if (retcb == SSL_TICKET_RETURN_USE) + ret = SSL_TICKET_SUCCESS; + else + ret = SSL_TICKET_SUCCESS_RENEW; + break; + + default: + ret = SSL_TICKET_FATAL_ERR_OTHER; + } } switch (ret) { @@ -1431,13 +1471,11 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick, case SSL_TICKET_SUCCESS_RENEW: case SSL_TICKET_EMPTY: s->ext.ticket_expected = 1; - /* Fall through */ - case SSL_TICKET_SUCCESS: - case SSL_TICKET_NONE: - return ret; } - return SSL_TICKET_FATAL_ERR_OTHER; + *psess = sess; + + return ret; } /* Check to see if a signature algorithm is allowed */ diff --git a/test/handshake_helper.c b/test/handshake_helper.c index fc5fcd6f4f..b3d94bb1ee 100644 --- a/test/handshake_helper.c +++ b/test/handshake_helper.c @@ -469,12 +469,24 @@ static int generate_session_ticket_cb(SSL *s, void *arg) return SSL_SESSION_set1_ticket_appdata(ss, app_data, strlen(app_data)); } -static SSL_TICKET_RETURN decrypt_session_ticket_cb(SSL *s, SSL_SESSION *ss, - const unsigned char *keyname, - size_t keyname_len, - SSL_TICKET_RETURN retv, void *arg) +static int decrypt_session_ticket_cb(SSL *s, SSL_SESSION *ss, + const unsigned char *keyname, + size_t keyname_len, + SSL_TICKET_STATUS status, + void *arg) { - return retv; + switch (status) { + case SSL_TICKET_EMPTY: + case SSL_TICKET_NO_DECRYPT: + return SSL_TICKET_RETURN_IGNORE_RENEW; + case SSL_TICKET_SUCCESS: + return SSL_TICKET_RETURN_USE; + case SSL_TICKET_SUCCESS_RENEW: + return SSL_TICKET_RETURN_USE_RENEW; + default: + break; + } + return SSL_TICKET_RETURN_ABORT; } /* diff --git a/test/sslapitest.c b/test/sslapitest.c index efce84d2d9..fc7288d57d 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -4596,7 +4596,9 @@ static int test_ssl_get_shared_ciphers(int tst) } static const char *appdata = "Hello World"; -static int gen_tick_called, dec_tick_called; +static int gen_tick_called, dec_tick_called, tick_key_cb_called; +static int tick_key_renew = 0; +static SSL_TICKET_RETURN tick_dec_ret = SSL_TICKET_RETURN_ABORT; static int gen_tick_cb(SSL *s, void *arg) { @@ -4609,26 +4611,46 @@ static int gen_tick_cb(SSL *s, void *arg) static SSL_TICKET_RETURN dec_tick_cb(SSL *s, SSL_SESSION *ss, const unsigned char *keyname, size_t keyname_length, - SSL_TICKET_RETURN retv, void *arg) + SSL_TICKET_STATUS status, + void *arg) { void *tickdata; size_t tickdlen; dec_tick_called = 1; - if (!TEST_true(retv == SSL_TICKET_SUCCESS - || retv == SSL_TICKET_SUCCESS_RENEW)) - return retv; + if (status == SSL_TICKET_EMPTY) + return SSL_TICKET_RETURN_IGNORE_RENEW; - if (!TEST_true(SSL_SESSION_get0_ticket_appdata(ss, &tickdata, &tickdlen)) + if (!TEST_true(status == SSL_TICKET_SUCCESS + || status == SSL_TICKET_SUCCESS_RENEW)) + return SSL_TICKET_RETURN_ABORT; + + if (!TEST_true(SSL_SESSION_get0_ticket_appdata(ss, &tickdata, + &tickdlen)) || !TEST_size_t_eq(tickdlen, strlen(appdata)) || !TEST_int_eq(memcmp(tickdata, appdata, tickdlen), 0)) - return SSL_TICKET_FATAL_ERR_OTHER; + return SSL_TICKET_RETURN_ABORT; - return retv; -} + if (tick_key_cb_called) { + /* Don't change what the ticket key callback wanted to do */ + switch (status) { + case SSL_TICKET_NO_DECRYPT: + return SSL_TICKET_RETURN_IGNORE_RENEW; + + case SSL_TICKET_SUCCESS: + return SSL_TICKET_RETURN_USE; -static int tick_renew = 0; + case SSL_TICKET_SUCCESS_RENEW: + return SSL_TICKET_RETURN_USE_RENEW; + + default: + return SSL_TICKET_RETURN_ABORT; + } + } + return tick_dec_ret; + +} static int tick_key_cb(SSL *s, unsigned char key_name[16], unsigned char iv[EVP_MAX_IV_LENGTH], EVP_CIPHER_CTX *ctx, @@ -4637,6 +4659,7 @@ static int tick_key_cb(SSL *s, unsigned char key_name[16], const unsigned char tick_aes_key[16] = "0123456789abcdef"; const unsigned char tick_hmac_key[16] = "0123456789abcdef"; + tick_key_cb_called = 1; memset(iv, 0, AES_BLOCK_SIZE); memset(key_name, 0, 16); if (!EVP_CipherInit_ex(ctx, EVP_aes_128_cbc(), NULL, tick_aes_key, iv, enc) @@ -4644,17 +4667,23 @@ static int tick_key_cb(SSL *s, unsigned char key_name[16], EVP_sha256(), NULL)) return -1; - return tick_renew ? 2 : 1; + return tick_key_renew ? 2 : 1; } /* * Test the various ticket callbacks - * Test 0: TLSv1.2, no ticket key callback (default no ticket renewal) - * Test 1: TLSv1.3, no ticket key callback (default ticket renewal) - * Test 2: TLSv1.2, ticket key callback, no ticket renewal - * Test 3: TLSv1.3, ticket key callback, no ticket renewal - * Test 4: TLSv1.2, ticket key callback, ticket renewal - * Test 5: TLSv1.3, ticket key callback, ticket renewal + * Test 0: TLSv1.2, no ticket key callback, no ticket, no renewal + * Test 1: TLSv1.3, no ticket key callback, no ticket, no renewal + * Test 2: TLSv1.2, no ticket key callback, no ticket, renewal + * Test 3: TLSv1.3, no ticket key callback, no ticket, renewal + * Test 4: TLSv1.2, no ticket key callback, ticket, no renewal + * Test 5: TLSv1.3, no ticket key callback, ticket, no renewal + * Test 6: TLSv1.2, no ticket key callback, ticket, renewal + * Test 7: TLSv1.3, no ticket key callback, ticket, renewal + * Test 8: TLSv1.2, ticket key callback, ticket, no renewal + * Test 9: TLSv1.3, ticket key callback, ticket, no renewal + * Test 10: TLSv1.2, ticket key callback, ticket, renewal + * Test 11: TLSv1.3, ticket key callback, ticket, renewal */ static int test_ticket_callbacks(int tst) { @@ -4672,27 +4701,60 @@ static int test_ticket_callbacks(int tst) return 1; #endif - gen_tick_called = dec_tick_called = 0; + gen_tick_called = dec_tick_called = tick_key_cb_called = 0; /* Which tests the ticket key callback should request renewal for */ - if (tst == 4 || tst == 5) - tick_renew = 1; + if (tst == 10 || tst == 11) + tick_key_renew = 1; else - tick_renew = 0; + tick_key_renew = 0; + + /* Which tests the decrypt ticket callback should request renewal for */ + switch (tst) { + case 0: + case 1: + tick_dec_ret = SSL_TICKET_RETURN_IGNORE; + break; + + case 2: + case 3: + tick_dec_ret = SSL_TICKET_RETURN_IGNORE_RENEW; + break; + + case 4: + case 5: + tick_dec_ret = SSL_TICKET_RETURN_USE; + break; + + case 6: + case 7: + tick_dec_ret = SSL_TICKET_RETURN_USE_RENEW; + break; + + default: + tick_dec_ret = SSL_TICKET_RETURN_ABORT; + } if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(), TLS_client_method(), TLS1_VERSION, - tst == 0 ? TLS1_2_VERSION - : TLS1_3_VERSION, + ((tst % 2) == 0) ? TLS1_2_VERSION + : TLS1_3_VERSION, &sctx, &cctx, cert, privkey))) goto end; + /* + * We only want sessions to resume from tickets - not the session cache. So + * switch the cache off. + */ + if (!TEST_true(SSL_CTX_set_session_cache_mode(sctx, SSL_SESS_CACHE_OFF))) + goto end; + if (!TEST_true(SSL_CTX_set_session_ticket_cb(sctx, gen_tick_cb, dec_tick_cb, NULL))) goto end; - if (tst >= 2 + if (tst >= 8 && !TEST_true(SSL_CTX_set_tlsext_ticket_key_cb(sctx, tick_key_cb))) goto end; @@ -4702,8 +4764,15 @@ static int test_ticket_callbacks(int tst) SSL_ERROR_NONE))) goto end; + /* + * The decrypt ticket key callback in TLSv1.2 should be called even though + * we have no ticket yet, because it gets called with a status of + * SSL_TICKET_EMPTY (the client indicates support for tickets but does not + * actually send any ticket data). This does not happen in TLSv1.3 because + * it is not valid to send empty ticket data in TLSv1.3. + */ if (!TEST_int_eq(gen_tick_called, 1) - || !TEST_int_eq(dec_tick_called, 0)) + || !TEST_int_eq(dec_tick_called, ((tst % 2) == 0) ? 1 : 0)) goto end; gen_tick_called = dec_tick_called = 0; @@ -4720,17 +4789,23 @@ static int test_ticket_callbacks(int tst) NULL)) || !TEST_true(SSL_set_session(clientssl, clntsess)) || !TEST_true(create_ssl_connection(serverssl, clientssl, - SSL_ERROR_NONE)) - || !TEST_true(SSL_session_reused(clientssl))) + SSL_ERROR_NONE))) goto end; - /* - * In TLSv1.2 we default to not renewing the ticket everytime. In TLSv1.3 - * we default to renewing. The defaults are overridden if a ticket key - * callback is in place. - */ + if (tick_dec_ret == SSL_TICKET_RETURN_IGNORE + || tick_dec_ret == SSL_TICKET_RETURN_IGNORE_RENEW) { + if (!TEST_false(SSL_session_reused(clientssl))) + goto end; + } else { + if (!TEST_true(SSL_session_reused(clientssl))) + goto end; + } + if (!TEST_int_eq(gen_tick_called, - (tst == 0 || tst == 2 || tst == 3) ? 0 : 1) + (tick_key_renew + || tick_dec_ret == SSL_TICKET_RETURN_IGNORE_RENEW + || tick_dec_ret == SSL_TICKET_RETURN_USE_RENEW) + ? 1 : 0) || !TEST_int_eq(dec_tick_called, 1)) goto end; @@ -4839,7 +4914,7 @@ int setup_tests(void) ADD_ALL_TESTS(test_info_callback, 6); ADD_ALL_TESTS(test_ssl_pending, 2); ADD_ALL_TESTS(test_ssl_get_shared_ciphers, OSSL_NELEM(shared_ciphers_data)); - ADD_ALL_TESTS(test_ticket_callbacks, 6); + ADD_ALL_TESTS(test_ticket_callbacks, 12); return 1; } -- 2.25.1