wbrawner/openssl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix ticket callbacks in TLSv1.3

Browse source 
The return value from the ticket_key callback was not properly handled in
TLSv1.3, so that a ticket was *always* renewed even if the callback
requested that it should not be.

Also the ticket decrypt callback was not being called at all in TLSv1.3.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6198)
This commit is contained in:
Matt Caswell 2018-05-08 14:50:17 +01:00
parent 5fe3715707
commit c0638adeec
4 changed files with 78 additions and 81 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
ssl/statem/statem_lib.c
View file

@ -1042,6 +1042,7 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop)
s->renegotiate = 0;
s->new_session = 0;
s->statem.cleanuphand = 0;
s->ext.ticket_expected = 0;
ssl3_cleanup_key_block(s);

11
ssl/statem/statem_srvr.c
View file

@ -486,9 +486,16 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
* and give the application the opportunity to delay sending the
* session ticket?
*/
if (s->post_handshake_auth == SSL_PHA_REQUESTED)
s->post_handshake_auth = SSL_PHA_EXT_RECEIVED;
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) {
/*
* If we resumed and we're not going to renew the ticket then we
* just finish the handshake at this point.
*/
st->hand_state = TLS_ST_OK;
}
return WRITE_TRAN_CONTINUE;
case TLS_ST_SR_KEY_UPDATE:

143
ssl/t1_lib.c
View file

@ -1199,15 +1199,6 @@ int tls1_set_server_sigalgs(SSL *s)
* 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.
*
* Returns:
* -1: fatal error, either from parsing or decrypting the ticket.
* 0: no ticket was found (or was ignored, based on settings).
* 1: a zero length extension was found, indicating that the client supports
* session tickets but doesn't currently have one to offer.
* 2: either s->tls_session_secret_cb was set, or a ticket was offered but
* couldn't be decrypted because of a non-fatal error.
* 3: a ticket was successfully decrypted and *ret was set.
*
* 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
@ -1219,7 +1210,6 @@ int tls1_set_server_sigalgs(SSL *s)
SSL_TICKET_RETURN tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello,
SSL_SESSION **ret)
{
int retv;
size_t size;
RAW_EXTENSION *ticketext;
@ -1257,47 +1247,8 @@ SSL_TICKET_RETURN tls_get_ticket_from_client(SSL *s, CLIENTHELLO_MSG *hello,
return SSL_TICKET_NO_DECRYPT;
}
retv = tls_decrypt_ticket(s, PACKET_data(&ticketext->data), size,
return tls_decrypt_ticket(s, PACKET_data(&ticketext->data), size,
hello->session_id, hello->session_id_len, ret);
/*
* If set, the decrypt_ticket_cb() is always called regardless of the
* return from tls_decrypt_ticket(). The callback is responsible for
* checking |retv| before it performs any action
*/
if (s->session_ctx->decrypt_ticket_cb != NULL) {
size_t keyname_len = size;
if (keyname_len > TLSEXT_KEYNAME_LENGTH)
keyname_len = TLSEXT_KEYNAME_LENGTH;
retv = s->session_ctx->decrypt_ticket_cb(s, *ret,
PACKET_data(&ticketext->data),
keyname_len,
retv, s->session_ctx->ticket_cb_data);
}
switch (retv) {
case SSL_TICKET_NO_DECRYPT:
s->ext.ticket_expected = 1;
return SSL_TICKET_NO_DECRYPT;
case SSL_TICKET_SUCCESS:
return SSL_TICKET_SUCCESS;
case SSL_TICKET_SUCCESS_RENEW:
s->ext.ticket_expected = 1;
return SSL_TICKET_SUCCESS;
case SSL_TICKET_EMPTY:
s->ext.ticket_expected = 1;
return SSL_TICKET_EMPTY;
case SSL_TICKET_NONE:
return SSL_TICKET_NONE;
default:
return SSL_TICKET_FATAL_ERR_OTHER;
}
}
/*-
@ -1328,28 +1279,32 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
/* Need at least keyname + iv */
if (eticklen < TLSEXT_KEYNAME_LENGTH + EVP_MAX_IV_LENGTH) {
ret = SSL_TICKET_NO_DECRYPT;
goto err;
goto end;
}
/* Initialize session ticket encryption and HMAC contexts */
hctx = HMAC_CTX_new();
if (hctx == NULL)
return SSL_TICKET_FATAL_ERR_MALLOC;
if (hctx == NULL) {
ret = SSL_TICKET_FATAL_ERR_MALLOC;
goto end;
}
ctx = EVP_CIPHER_CTX_new();
if (ctx == NULL) {
ret = SSL_TICKET_FATAL_ERR_MALLOC;
goto err;
goto end;
}
if (tctx->ext.ticket_key_cb) {
unsigned char *nctick = (unsigned char *)etick;
int rv = tctx->ext.ticket_key_cb(s, nctick,
nctick + TLSEXT_KEYNAME_LENGTH,
ctx, hctx, 0);
if (rv < 0)
goto err;
if (rv < 0) {
ret = SSL_TICKET_FATAL_ERR_OTHER;
goto end;
}
if (rv == 0) {
ret = SSL_TICKET_NO_DECRYPT;
goto err;
goto end;
}
if (rv == 2)
renew_ticket = 1;
@ -1358,7 +1313,7 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
if (memcmp(etick, tctx->ext.tick_key_name,
TLSEXT_KEYNAME_LENGTH) != 0) {
ret = SSL_TICKET_NO_DECRYPT;
goto err;
goto end;
}
if (HMAC_Init_ex(hctx, tctx->ext.secure->tick_hmac_key,
sizeof(tctx->ext.secure->tick_hmac_key),
@ -1366,8 +1321,11 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
|| EVP_DecryptInit_ex(ctx, EVP_aes_256_cbc(), NULL,
tctx->ext.secure->tick_aes_key,
etick + TLSEXT_KEYNAME_LENGTH) <= 0) {
goto err;
ret = SSL_TICKET_FATAL_ERR_OTHER;
goto end;
}
if (SSL_IS_TLS13(s))
renew_ticket = 1;
}
/*
* Attempt to process session ticket, first conduct sanity and integrity
@ -1375,24 +1333,27 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
*/
mlen = HMAC_size(hctx);
if (mlen == 0) {
goto err;
ret = SSL_TICKET_FATAL_ERR_OTHER;
goto end;
}
/* Sanity check ticket length: must exceed keyname + IV + HMAC */
if (eticklen <=
TLSEXT_KEYNAME_LENGTH + EVP_CIPHER_CTX_iv_length(ctx) + mlen) {
ret = SSL_TICKET_NO_DECRYPT;
goto err;
goto end;
}
eticklen -= mlen;
/* Check HMAC of encrypted ticket */
if (HMAC_Update(hctx, etick, eticklen) <= 0
|| HMAC_Final(hctx, tick_hmac, NULL) <= 0) {
goto err;
ret = SSL_TICKET_FATAL_ERR_OTHER;
goto end;
}
HMAC_CTX_free(hctx);
if (CRYPTO_memcmp(tick_hmac, etick + eticklen, mlen)) {
EVP_CIPHER_CTX_free(ctx);
return SSL_TICKET_NO_DECRYPT;
ret = SSL_TICKET_NO_DECRYPT;
goto end;
}
/* Attempt to decrypt session data */
/* Move p after IV to start of encrypted ticket, update length */
@ -1401,18 +1362,16 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
sdec = OPENSSL_malloc(eticklen);
if (sdec == NULL || EVP_DecryptUpdate(ctx, sdec, &slen, p,
(int)eticklen) <= 0) {
EVP_CIPHER_CTX_free(ctx);
OPENSSL_free(sdec);
return SSL_TICKET_FATAL_ERR_OTHER;
ret = SSL_TICKET_FATAL_ERR_OTHER;
goto end;
}
if (EVP_DecryptFinal(ctx, sdec + slen, &declen) <= 0) {
EVP_CIPHER_CTX_free(ctx);
OPENSSL_free(sdec);
return SSL_TICKET_NO_DECRYPT;
ret = SSL_TICKET_NO_DECRYPT;
goto end;
}
slen += declen;
EVP_CIPHER_CTX_free(ctx);
ctx = NULL;
p = sdec;
sess = d2i_SSL_SESSION(NULL, &p, slen);
@ -1422,7 +1381,8 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
/* Some additional consistency checks */
if (slen != 0) {
SSL_SESSION_free(sess);
return SSL_TICKET_NO_DECRYPT;
ret = SSL_TICKET_NO_DECRYPT;
goto end;
}
/*
* The session ID, if non-empty, is used by some clients to detect
@ -1436,19 +1396,48 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
}
*psess = sess;
if (renew_ticket)
return SSL_TICKET_SUCCESS_RENEW;
ret = SSL_TICKET_SUCCESS_RENEW;
else
return SSL_TICKET_SUCCESS;
ret = SSL_TICKET_SUCCESS;
goto end;
}
ERR_clear_error();
/*
* For session parse failure, indicate that we need to send a new ticket.
*/
return SSL_TICKET_NO_DECRYPT;
err:
ret = SSL_TICKET_NO_DECRYPT;
end:
EVP_CIPHER_CTX_free(ctx);
HMAC_CTX_free(hctx);
return ret;
/*
* 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 (s->session_ctx->decrypt_ticket_cb != NULL) {
size_t keyname_len = eticklen;
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);
}
switch (ret) {
case SSL_TICKET_NO_DECRYPT:
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;
}
/* Check to see if a signature algorithm is allowed */

4
test/sslapitest.c
View file

@ -2204,9 +2204,9 @@ static int test_early_data_not_sent(int idx)
/*
* Should block due to the NewSessionTicket arrival unless we're using
* read_ahead
* read_ahead, or PSKs
*/
if (idx != 1) {
if (idx != 1 && idx != 2) {
if (!TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &readbytes)))
goto end;
}