Fix logic around when to send an HRR based on cookies

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4435)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index b0d016a03d0083bf2589fd2d59f251a5d2735caf..4e2dae0ee89b095669f8b5ef747284461a101022 100644 (file)
--- 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 9247cf3530c16a4e1e51931de16ed12b51e1e704..00795776f8120dec18f20c3ef5862010ecba6989 100644 (file)
--- 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 6022c493dc03fbe5d7fccbaaa19111699c397931..335c9452ffaee0c68b95dcc879a34d7d6e066b1a 100644 (file)
--- 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 16f1b853672c4db607a6765fffdf3a8cacdf5aa6..83cd3633a6bc7d77c6f87e3276f22d7fda1e65a4 100644 (file)
--- 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 211a4ca14d9a5f8a28aa9aef2f7c6b5f255e5ab4..5651f6476d2dff9740827ea24e69af11f500ad48 100644 (file)
--- 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 */