diff options
author | Matt Caswell <matt@openssl.org> | 2018-05-09 18:22:36 +0100 |
---|---|---|
committer | Matt Caswell <matt@openssl.org> | 2018-05-11 14:51:09 +0100 |
commit | 61fb59238dad6452a37ec14513fae617a4faef29 (patch) | |
tree | 5737eeba510f7a64792d3ac007f794d62a2dcb8a /ssl | |
parent | c20e3b282c26205f39a89a23664245475d4d7cbc (diff) |
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 <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6198)
Diffstat (limited to 'ssl')
-rw-r--r-- | ssl/ssl_locl.h | 4 | ||||
-rw-r--r-- | ssl/ssl_sess.c | 7 | ||||
-rw-r--r-- | ssl/statem/extensions_srvr.c | 17 | ||||
-rw-r--r-- | ssl/statem/statem_srvr.c | 6 | ||||
-rw-r--r-- | ssl/t1_lib.c | 130 |
5 files changed, 108 insertions, 56 deletions
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 */ |