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)

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;
 }

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)

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,12 +1238,18 @@ 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
+     *     IF
+     *         we have a suitable key_share
+     *     THEN
+     *         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
@@ -1253,15 +1260,37 @@ static int final_key_share(SSL *s, unsigned int context, int sent)
      *             a shared group exists
      *         THEN
      *             send a HelloRetryRequest
-     *     ELSE If
+     *         ELSE IF
      *             we are not resuming
      *             OR
      *             the kex_mode doesn't allow non key_share resumes
      *         THEN
-     *         fail;
+     *             fail
+     *         ELSE IF
+     *             we are stateless AND we have no cookie
+     *         THEN
+     *             send a HelloRetryRequest
      */
-    if (s->server && s->s3->peer_tmp == NULL) {
+    if (s->server) {
-        /* No suitable share */
+        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)
@@ -1276,11 +1305,14 @@ static int final_key_share(SSL *s, unsigned int context, int sent)
                 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 */
+                /*
+                 * 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))
+                    if (check_in_list(s, group_id, clntgroups, clnt_num_groups,
+                                      1))
                         break;
                 }
 
@@ -1294,27 +1326,47 @@ static int final_key_share(SSL *s, unsigned int context, int sent)
             if (!s->hit
                     || (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE) == 0) {
                 /* Nothing left we can do - just fail */
-            SSLfatal(s,
+                SSLfatal(s, sent ? SSL_AD_HANDSHAKE_FAILURE
-                     sent ? SSL_AD_HANDSHAKE_FAILURE : SSL_AD_MISSING_EXTENSION,
+                                 : SSL_AD_MISSING_EXTENSION,
                          SSL_F_FINAL_KEY_SHARE, SSL_R_NO_SUITABLE_KEY_SHARE);
                 return 0;
             }
+
+            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;
+            }
         }
 
-    /* We have a key_share so don't send any more HelloRetryRequest messages */
+        /*
-    if (s->server && s->hello_retry_request == SSL_HRR_PENDING)
+         * 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 && !s->server && !tls13_generate_handshake_secret(s, NULL, 0)) {
+        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;
         }
+    }
 
     return 1;
 }

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;
 }
 

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 */