From c36001c3a89691e21dc4940425fc880c89c57ffc Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 13 Sep 2017 14:50:49 +0100 Subject: [PATCH] Fix logic around when to send an HRR based on cookies Reviewed-by: Ben Kaduk (Merged from https://github.com/openssl/openssl/pull/4435) --- ssl/ssl_lib.c | 3 + ssl/ssl_locl.h | 3 + ssl/statem/extensions.c | 182 ++++++++++++++++++++++------------- ssl/statem/extensions_srvr.c | 2 + ssl/statem/statem_srvr.c | 3 +- 5 files changed, 127 insertions(+), 66 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index b0d016a03d..4e2dae0ee8 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -5310,5 +5310,8 @@ int SSL_stateless(SSL *s) ret = SSL_accept(s); s->s3->flags &= ~TLS1_FLAGS_STATELESS; + if (s->ext.cookieok) + return 1; + return ret; } diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 9247cf3530..00795776f8 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -1276,6 +1276,9 @@ struct ssl_st { /* May be sent by a server in HRR. Must be echoed back in ClientHello */ unsigned char *tls13_cookie; size_t tls13_cookie_len; + /* Have we received a cookie from the client? */ + int cookieok; + /* * Maximum Fragment Length as per RFC 4366. * If this member contains one of the allowed values (1-4) diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 6022c493dc..335c9452ff 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -12,6 +12,7 @@ #include "internal/cryptlib.h" #include "../ssl_locl.h" #include "statem_locl.h" +#include "internal/cryptlib.h" static int final_renegotiate(SSL *s, unsigned int context, int sent); static int init_server_name(SSL *s, unsigned int context); @@ -1237,85 +1238,136 @@ static int final_key_share(SSL *s, unsigned int context, int sent) return 0; } /* - * If + * IF * we are a server - * AND - * we have no key_share * THEN - * If - * we didn't already send a HelloRetryRequest - * AND - * the client sent a key_share extension - * AND - * (we are not resuming - * OR the kex_mode allows key_share resumes) - * AND - * a shared group exists - * THEN - * send a HelloRetryRequest - * ELSE If - * we are not resuming - * OR - * the kex_mode doesn't allow non key_share resumes + * IF + * we have a suitable key_share * THEN - * fail; + * IF + * we are stateless AND we have no cookie + * THEN + * send a HelloRetryRequest + * ELSE + * IF + * we didn't already send a HelloRetryRequest + * AND + * the client sent a key_share extension + * AND + * (we are not resuming + * OR the kex_mode allows key_share resumes) + * AND + * a shared group exists + * THEN + * send a HelloRetryRequest + * ELSE IF + * we are not resuming + * OR + * the kex_mode doesn't allow non key_share resumes + * THEN + * fail + * ELSE IF + * we are stateless AND we have no cookie + * THEN + * send a HelloRetryRequest */ - if (s->server && s->s3->peer_tmp == NULL) { - /* No suitable share */ - if (s->hello_retry_request == SSL_HRR_NONE && sent - && (!s->hit - || (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE_DHE) - != 0)) { - const uint16_t *pgroups, *clntgroups; - size_t num_groups, clnt_num_groups, i; - unsigned int group_id = 0; - - /* Check if a shared group exists */ - - /* Get the clients list of supported groups. */ - tls1_get_peer_groups(s, &clntgroups, &clnt_num_groups); - tls1_get_supported_groups(s, &pgroups, &num_groups); - - /* Find the first group we allow that is also in client's list */ - for (i = 0; i < num_groups; i++) { - group_id = pgroups[i]; - - if (check_in_list(s, group_id, clntgroups, clnt_num_groups, 1)) - break; + if (s->server) { + if (s->s3->peer_tmp != NULL) { + /* We have a suitable key_share */ + if ((s->s3->flags & TLS1_FLAGS_STATELESS) != 0 + && !s->ext.cookieok) { + if (!ossl_assert(s->hello_retry_request == SSL_HRR_NONE)) { + /* + * If we are stateless then we wouldn't know about any + * previously sent HRR - so how can this be anything other + * than 0? + */ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_FINAL_KEY_SHARE, + ERR_R_INTERNAL_ERROR); + return 0; + } + s->hello_retry_request = SSL_HRR_PENDING; + return 1; + } + } else { + /* No suitable key_share */ + if (s->hello_retry_request == SSL_HRR_NONE && sent + && (!s->hit + || (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE_DHE) + != 0)) { + const uint16_t *pgroups, *clntgroups; + size_t num_groups, clnt_num_groups, i; + unsigned int group_id = 0; + + /* Check if a shared group exists */ + + /* Get the clients list of supported groups. */ + tls1_get_peer_groups(s, &clntgroups, &clnt_num_groups); + tls1_get_supported_groups(s, &pgroups, &num_groups); + + /* + * Find the first group we allow that is also in client's list + */ + for (i = 0; i < num_groups; i++) { + group_id = pgroups[i]; + + if (check_in_list(s, group_id, clntgroups, clnt_num_groups, + 1)) + break; + } + + if (i < num_groups) { + /* A shared group exists so send a HelloRetryRequest */ + s->s3->group_id = group_id; + s->hello_retry_request = SSL_HRR_PENDING; + return 1; + } + } + if (!s->hit + || (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE) == 0) { + /* Nothing left we can do - just fail */ + SSLfatal(s, sent ? SSL_AD_HANDSHAKE_FAILURE + : SSL_AD_MISSING_EXTENSION, + SSL_F_FINAL_KEY_SHARE, SSL_R_NO_SUITABLE_KEY_SHARE); + return 0; } - if (i < num_groups) { - /* A shared group exists so send a HelloRetryRequest */ - s->s3->group_id = group_id; + if ((s->s3->flags & TLS1_FLAGS_STATELESS) != 0 + && !s->ext.cookieok) { + if (!ossl_assert(s->hello_retry_request == SSL_HRR_NONE)) { + /* + * If we are stateless then we wouldn't know about any + * previously sent HRR - so how can this be anything other + * than 0? + */ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_FINAL_KEY_SHARE, + ERR_R_INTERNAL_ERROR); + return 0; + } s->hello_retry_request = SSL_HRR_PENDING; return 1; } } - if (!s->hit - || (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE) == 0) { - /* Nothing left we can do - just fail */ - SSLfatal(s, - sent ? SSL_AD_HANDSHAKE_FAILURE : SSL_AD_MISSING_EXTENSION, - SSL_F_FINAL_KEY_SHARE, SSL_R_NO_SUITABLE_KEY_SHARE); + + /* + * We have a key_share so don't send any more HelloRetryRequest + * messages + */ + if (s->hello_retry_request == SSL_HRR_PENDING) + s->hello_retry_request = SSL_HRR_COMPLETE; + } else { + /* + * For a client side resumption with no key_share we need to generate + * the handshake secret (otherwise this is done during key_share + * processing). + */ + if (!sent && !tls13_generate_handshake_secret(s, NULL, 0)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_FINAL_KEY_SHARE, + ERR_R_INTERNAL_ERROR); return 0; } } - /* We have a key_share so don't send any more HelloRetryRequest messages */ - if (s->server && s->hello_retry_request == SSL_HRR_PENDING) - s->hello_retry_request = SSL_HRR_COMPLETE; - - /* - * For a client side resumption with no key_share we need to generate - * the handshake secret (otherwise this is done during key_share - * processing). - */ - if (!sent && !s->server && !tls13_generate_handshake_secret(s, NULL, 0)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_FINAL_KEY_SHARE, - ERR_R_INTERNAL_ERROR); - return 0; - } - return 1; } #endif diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 16f1b85367..83cd3633a6 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -895,6 +895,8 @@ int tls_parse_ctos_cookie(SSL *s, PACKET *pkt, unsigned int context, X509 *x, /* Act as if this ClientHello came after a HelloRetryRequest */ s->hello_retry_request = 1; + s->ext.cookieok = 1; + return 1; } diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 211a4ca14d..5651f6476d 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -687,7 +687,8 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst) return WORK_FINISHED_CONTINUE; case TLS_ST_EARLY_DATA: - if (s->early_data_state != SSL_EARLY_DATA_ACCEPTING) + if (s->early_data_state != SSL_EARLY_DATA_ACCEPTING + && (s->s3->flags & TLS1_FLAGS_STATELESS) == 0) return WORK_FINISHED_CONTINUE; /* Fall through */ -- 2.34.1