Fixed various style issues in the key_share code

Numerous style issues as well as references to TLS1_3_VERSION instead of
SSL_IS_TLS13(s)

Reviewed-by: Rich Salz <rsalz@openssl.org>

5 files changed, 285 insertions, 283 deletions

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 4b4559d08e..235a53ccc8 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -19,6 +19,7 @@
 # define ERR_REASON(reason) ERR_PACK(ERR_LIB_SSL,0,reason)
 
 static ERR_STRING_DATA SSL_str_functs[] = {
+    {ERR_FUNC(SSL_F_ADD_CLIENT_KEY_SHARE_EXT), "add_client_key_share_ext"},
     {ERR_FUNC(SSL_F_CHECK_SUITEB_CIPHER_LIST), "check_suiteb_cipher_list"},
     {ERR_FUNC(SSL_F_CT_MOVE_SCTS), "ct_move_scts"},
     {ERR_FUNC(SSL_F_CT_STRICT), "ct_strict"},
@@ -61,6 +62,7 @@ static ERR_STRING_DATA SSL_str_functs[] = {
      "ossl_statem_server_construct_message"},
     {ERR_FUNC(SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION),
      "ossl_statem_server_read_transition"},
+    {ERR_FUNC(SSL_F_PROCESS_KEY_SHARE_EXT), "process_key_share_ext"},
     {ERR_FUNC(SSL_F_READ_STATE_MACHINE), "read_state_machine"},
     {ERR_FUNC(SSL_F_SSL3_CHANGE_CIPHER_STATE), "ssl3_change_cipher_state"},
     {ERR_FUNC(SSL_F_SSL3_CHECK_CERT_AND_ALGORITHM),
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index feda8d2119..f89d317ce7 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -112,15 +112,19 @@ static int key_exchange_expected(SSL *s)
  * server. The message type that the server has sent is provided in |mt|. The
  * current state is in |s->statem.hand_state|.
  *
- *  Return values are:
- *  1: Success (transition allowed)
- *  0: Error (transition not allowed)
+ * Return values are 1 for success (transition allowed) and  0 on error
+ * (transition not allowed)
  */
 static int ossl_statem_client13_read_transition(SSL *s, int mt)
 {
     OSSL_STATEM *st = &s->statem;
 
     /*
+     * TODO(TLS1.3): This is still based on the TLSv1.2 state machine. Over time
+     * we will update this to look more like real TLSv1.3
+     */
+
+    /*
      * Note: There is no case for TLS_ST_CW_CLNT_HELLO, because we haven't
      * yet negotiated TLSv1.3 at that point so that is handled by
      * ossl_statem_client_read_transition()
@@ -218,9 +222,8 @@ static int ossl_statem_client13_read_transition(SSL *s, int mt)
  * server. The message type that the server has sent is provided in |mt|. The
  * current state is in |s->statem.hand_state|.
  *
- *  Return values are:
- *  1: Success (transition allowed)
- *  0: Error (transition not allowed)
+ * Return values are 1 for success (transition allowed) and  0 on error
+ * (transition not allowed)
  */
 int ossl_statem_client_read_transition(SSL *s, int mt)
 {
@@ -387,17 +390,17 @@ int ossl_statem_client_read_transition(SSL *s, int mt)
  * ossl_statem_client13_write_transition() works out what handshake state to
  * move to next when the TLSv1.3 client is writing messages to be sent to the
  * server.
- *
- * Return values:
- * WRITE_TRAN_ERROR - an error occurred
- * WRITE_TRAN_CONTINUE - Successful transition, more writing to be done
- * WRITE_TRAN_FINISHED - Successful transition, no more writing to be done
  */
 static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s)
 {
     OSSL_STATEM *st = &s->statem;
 
     /*
+     * TODO(TLS1.3): This is still based on the TLSv1.2 state machine. Over time
+     * we will update this to look more like real TLSv1.3
+     */
+
+    /*
      * Note: There are no cases for TLS_ST_BEFORE or TLS_ST_CW_CLNT_HELLO,
      * because we haven't negotiated TLSv1.3 yet at that point. They are
      * handled by ossl_statem_client_write_transition().
@@ -408,18 +411,14 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s)
         return WRITE_TRAN_ERROR;
 
     case TLS_ST_CR_SRVR_DONE:
-        if (s->s3->tmp.cert_req)
-            st->hand_state = TLS_ST_CW_CERT;
-        else
-            st->hand_state = TLS_ST_CW_CHANGE;
+        st->hand_state = (s->s3->tmp.cert_req != 0) ? TLS_ST_CW_CERT
+                                                    : TLS_ST_CW_CHANGE;
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_CW_CERT:
         /* If a non-empty Certificate we also send CertificateVerify */
-        if (s->s3->tmp.cert_req == 1)
-            st->hand_state = TLS_ST_CW_CERT_VRFY;
-        else
-            st->hand_state = TLS_ST_CW_CHANGE;
+        st->hand_state = (s->s3->tmp.cert_req == 1) ? TLS_ST_CW_CERT_VRFY
+                                                    : TLS_ST_CW_CHANGE;
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_CW_CERT_VRFY:
@@ -435,30 +434,23 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s)
             st->hand_state = TLS_ST_OK;
             ossl_statem_set_in_init(s, 0);
             return WRITE_TRAN_CONTINUE;
-        } else {
-            return WRITE_TRAN_FINISHED;
         }
+        return WRITE_TRAN_FINISHED;
 
     case TLS_ST_CR_FINISHED:
         if (s->hit) {
             st->hand_state = TLS_ST_CW_CHANGE;
             return WRITE_TRAN_CONTINUE;
-        } else {
-            st->hand_state = TLS_ST_OK;
-            ossl_statem_set_in_init(s, 0);
-            return WRITE_TRAN_CONTINUE;
         }
+        st->hand_state = TLS_ST_OK;
+        ossl_statem_set_in_init(s, 0);
+        return WRITE_TRAN_CONTINUE;
     }
 }
 
 /*
  * ossl_statem_client_write_transition() works out what handshake state to
  * move to next when the client is writing messages to be sent to the server.
- *
- * Return values:
- * WRITE_TRAN_ERROR - an error occurred
- * WRITE_TRAN_CONTINUE - Successful transition, more writing to be done
- * WRITE_TRAN_FINISHED - Successful transition, no more writing to be done
  */
 WRITE_TRAN ossl_statem_client_write_transition(SSL *s)
 {
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index c8c0b8e17a..3c4d6ee768 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -73,15 +73,19 @@ static STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s,
  * the client. The message type that the client has sent is provided in |mt|.
  * The current state is in |s->statem.hand_state|.
  *
- *  Valid return values are:
- *  1: Success (transition allowed)
- *  0: Error (transition not allowed)
+ * Return values are 1 for success (transition allowed) and  0 on error
+ * (transition not allowed)
  */
 static int ossl_statem_server13_read_transition(SSL *s, int mt)
 {
     OSSL_STATEM *st = &s->statem;
 
     /*
+     * TODO(TLS1.3): This is still based on the TLSv1.2 state machine. Over time
+     * we will update this to look more like real TLSv1.3
+     */
+
+    /*
      * Note: There is no case for TLS_ST_BEFORE because at that stage we have
      * not negotiated TLSv1.3 yet, so that case is handled by
      * ossl_statem_server_read_transition()
@@ -153,9 +157,8 @@ static int ossl_statem_server13_read_transition(SSL *s, int mt)
  * client. The message type that the client has sent is provided in |mt|. The
  * current state is in |s->statem.hand_state|.
  *
- *  Valid return values are:
- *  1: Success (transition allowed)
- *  0: Error (transition not allowed)
+ * Return values are 1 for success (transition allowed) and  0 on error
+ * (transition not allowed)
  */
 int ossl_statem_server_read_transition(SSL *s, int mt)
 {
@@ -390,17 +393,17 @@ static int send_certificate_request(SSL *s)
  * ossl_statem_server13_write_transition() works out what handshake state to
  * move to next when a TLSv1.3 server is writing messages to be sent to the
  * client.
- *
- * Return values:
- * WRITE_TRAN_ERROR - an error occurred
- * WRITE_TRAN_CONTINUE - Successful transition, more writing to be done
- * WRITE_TRAN_FINISHED - Successful transition, no more writing to be done
  */
 static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
 {
     OSSL_STATEM *st = &s->statem;
 
     /*
+     * TODO(TLS1.3): This is still based on the TLSv1.2 state machine. Over time
+     * we will update this to look more like real TLSv1.3
+     */
+
+    /*
      * No case for TLS_ST_BEFORE, because at that stage we have not negotiated
      * TLSv1.3 yet, so that is handled by ossl_statem_server_write_transition()
      */
@@ -415,14 +418,12 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_SW_SRVR_HELLO:
-        if (s->hit) {
-            if (s->tlsext_ticket_expected)
-                st->hand_state = TLS_ST_SW_SESSION_TICKET;
-            else
-                st->hand_state = TLS_ST_SW_CHANGE;
-        } else {
+        if (s->hit)
+            st->hand_state = s->tlsext_ticket_expected
+                                ? TLS_ST_SW_SESSION_TICKET : TLS_ST_SW_CHANGE;
+        else
             st->hand_state = TLS_ST_SW_CERT;
-        }
+
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_SW_CERT:
@@ -451,11 +452,10 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
             st->hand_state = TLS_ST_OK;
             ossl_statem_set_in_init(s, 0);
             return WRITE_TRAN_CONTINUE;
-        } else if (s->tlsext_ticket_expected) {
-            st->hand_state = TLS_ST_SW_SESSION_TICKET;
-        } else {
-            st->hand_state = TLS_ST_SW_CHANGE;
         }
+
+        st->hand_state = s->tlsext_ticket_expected ? TLS_ST_SW_SESSION_TICKET
+                                                   : TLS_ST_SW_CHANGE;
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_SW_SESSION_TICKET:
@@ -467,9 +467,9 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_SW_FINISHED:
-        if (s->hit) {
+        if (s->hit)
             return WRITE_TRAN_FINISHED;
-        }
+
         st->hand_state = TLS_ST_OK;
         ossl_statem_set_in_init(s, 0);
         return WRITE_TRAN_CONTINUE;
@@ -479,11 +479,6 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
 /*
  * ossl_statem_server_write_transition() works out what handshake state to move
  * to next when the server is writing messages to be sent to the client.
- *
- * Return values:
- * WRITE_TRAN_ERROR - an error occurred
- * WRITE_TRAN_CONTINUE - Successful transition, more writing to be done
- * WRITE_TRAN_FINISHED - Successful transition, no more writing to be done
  */
 WRITE_TRAN ossl_statem_server_write_transition(SSL *s)
 {
@@ -1452,7 +1447,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
     /* Check we've got a key_share for TLSv1.3 */
     if (s->version == TLS1_3_VERSION && s->s3->peer_tmp == NULL && !s->hit) {
         /* No suitable share */
-        /* TODO(1.3): Send a HelloRetryRequest */
+        /* TODO(TLS1.3): Send a HelloRetryRequest */
         al = SSL_AD_HANDSHAKE_FAILURE;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_NO_SUITABLE_KEY_SHARE);
         goto f_err;
@@ -3123,7 +3118,7 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL *s, PACKET *pkt)
      * Freeze the handshake buffer. For <TLS1.3 we do this after the CKE
      * message
      */
-    if (s->version == TLS1_3_VERSION && !ssl3_digest_cached_records(s, 1)) {
+    if (SSL_IS_TLS13(s) && !ssl3_digest_cached_records(s, 1)) {
         al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_CERTIFICATE, ERR_R_INTERNAL_ERROR);
         goto f_err;
diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c
index df2ce37030..8cb6dd5a3b 100644
--- a/ssl/t1_enc.c
+++ b/ssl/t1_enc.c
@@ -480,7 +480,7 @@ int tls1_generate_master_secret(SSL *s, unsigned char *out, unsigned char *p,
      * handshake has). This will need to be removed later
      */
     if ((s->session->flags & SSL_SESS_FLAG_EXTMS)
-            && s->version != TLS1_3_VERSION) {
+            && SSL_IS_TLS13(s)) {
         unsigned char hash[EVP_MAX_MD_SIZE * 2];
         size_t hashlen;
         /*
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 6474c6dbc2..56b6f27e0a 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1048,7 +1048,7 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
                 break;
             }
         }
-    } else if (s->version >= TLS1_3_VERSION) {
+    } else if (SSL_IS_TLS13(s)) {
         /*
          * TODO(TLS1.3): We always use ECC for TLSv1.3 at the moment. This will
          * change if we implement DH key shares
@@ -1056,7 +1056,7 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
         using_ecc = 1;
     }
 #else
-    if (s->version >= TLS1_3_VERSION) {
+    if (SSL_IS_TLS13(s)) {
         /* Shouldn't happen! */
         SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
         return 0;
@@ -1423,57 +1423,57 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
          * now, just send one
          */
         for (i = 0; i < num_curves && sharessent < 1; i++, pcurves += 2) {
-            if (tls_curve_allowed(s, pcurves, SSL_SECOP_CURVE_SUPPORTED)) {
-                unsigned char *encodedPoint = NULL;
-                unsigned int curve_id = 0;
-                EVP_PKEY *key_share_key = NULL;
-                size_t encodedlen;
-
-                if (s->s3->tmp.pkey != NULL) {
-                    /* Shouldn't happen! */
-                    SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT,
-                           ERR_R_INTERNAL_ERROR);
-                    return 0;
-                }
-
-                /* Generate a key for this key_share */
-                curve_id = (pcurves[0] << 8) | pcurves[1];
-                key_share_key = ssl_generate_pkey_curve(curve_id);
-                if (key_share_key == NULL) {
-                    SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_EVP_LIB);
-                    return 0;
-                }
+            unsigned char *encodedPoint = NULL;
+            unsigned int curve_id = 0;
+            EVP_PKEY *key_share_key = NULL;
+            size_t encodedlen;
+
+            if (!tls_curve_allowed(s, pcurves, SSL_SECOP_CURVE_SUPPORTED))
+                continue;
+
+            if (s->s3->tmp.pkey != NULL) {
+                /* Shouldn't happen! */
+                SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT,
+                       ERR_R_INTERNAL_ERROR);
+                return 0;
+            }
 
-                /* Encode the public key. */
-                encodedlen = EVP_PKEY_get1_tls_encodedpoint(key_share_key,
-                                                            &encodedPoint);
-                if (encodedlen == 0) {
-                    SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_EC_LIB);
-                    EVP_PKEY_free(key_share_key);
-                    return 0;
-                }
+            /* Generate a key for this key_share */
+            curve_id = (pcurves[0] << 8) | pcurves[1];
+            key_share_key = ssl_generate_pkey_curve(curve_id);
+            if (key_share_key == NULL) {
+                SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_EVP_LIB);
+                return 0;
+            }
 
-                /* Create KeyShareEntry */
-                if (!WPACKET_put_bytes_u16(pkt, curve_id)
-                        || !WPACKET_sub_memcpy_u16(pkt, encodedPoint,
-                                                   encodedlen)) {
-                    SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT,
-                           ERR_R_INTERNAL_ERROR);
-                    EVP_PKEY_free(key_share_key);
-                    OPENSSL_free(encodedPoint);
-                    return 0;
-                }
+            /* Encode the public key. */
+            encodedlen = EVP_PKEY_get1_tls_encodedpoint(key_share_key,
+                                                        &encodedPoint);
+            if (encodedlen == 0) {
+                SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_EC_LIB);
+                EVP_PKEY_free(key_share_key);
+                return 0;
+            }
 
-                /*
-                 * TODO(TLS1.3): When changing to send more than one key_share
-                 * we're going to need to be able to save more than one EVP_PKEY
-                 * For now we reuse the existing tmp.pkey
-                 */
-                s->s3->group_id = curve_id;
-                s->s3->tmp.pkey = key_share_key;
-                sharessent++;
+            /* Create KeyShareEntry */
+            if (!WPACKET_put_bytes_u16(pkt, curve_id)
+                    || !WPACKET_sub_memcpy_u16(pkt, encodedPoint, encodedlen)) {
+                SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT,
+                       ERR_R_INTERNAL_ERROR);
+                EVP_PKEY_free(key_share_key);
                 OPENSSL_free(encodedPoint);
+                return 0;
             }
+
+            /*
+             * TODO(TLS1.3): When changing to send more than one key_share we're
+             * going to need to be able to save more than one EVP_PKEY. For now
+             * we reuse the existing tmp.pkey
+             */
+            s->s3->group_id = curve_id;
+            s->s3->tmp.pkey = key_share_key;
+            sharessent++;
+            OPENSSL_free(encodedPoint);
         }
         if (!WPACKET_close(pkt) || !WPACKET_close(pkt)) {
             SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
@@ -1516,6 +1516,59 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
     return 1;
 }
 
+/*
+ * Add the key_share extension.
+ *
+ * Returns 1 on success or 0 on failure.
+ */
+static int add_client_key_share_ext(SSL *s, WPACKET *pkt, int *al)
+{
+    unsigned char *encodedPoint;
+    size_t encoded_pt_len = 0;
+    EVP_PKEY *ckey = s->s3->peer_tmp, *skey = NULL;
+
+    if (ckey == NULL) {
+        SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR);
+        return 0;
+    }
+
+    if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_key_share)
+            || !WPACKET_start_sub_packet_u16(pkt)
+            || !WPACKET_put_bytes_u16(pkt, s->s3->group_id)) {
+        SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR);
+        return 0;
+    }
+
+    skey = ssl_generate_pkey(ckey);
+
+    /* Generate encoding of server key */
+    encoded_pt_len = EVP_PKEY_get1_tls_encodedpoint(skey, &encodedPoint);
+    if (encoded_pt_len == 0) {
+        SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_EC_LIB);
+        EVP_PKEY_free(skey);
+        return 0;
+    }
+
+    if (!WPACKET_sub_memcpy_u16(pkt, encodedPoint, encoded_pt_len)
+            || !WPACKET_close(pkt)) {
+        SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR);
+        EVP_PKEY_free(skey);
+        OPENSSL_free(encodedPoint);
+        return 0;
+    }
+    OPENSSL_free(encodedPoint);
+
+    /* This causes the crypto state to be updated based on the derived keys */
+    s->s3->tmp.pkey = skey;
+    if (ssl_derive(s, skey, ckey, 1) == 0) {
+        *al = SSL_AD_INTERNAL_ERROR;
+        SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR);
+        return 0;
+    }
+
+    return 1;
+}
+
 int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al)
 {
 #ifndef OPENSSL_NO_NEXTPROTONEG
@@ -1649,51 +1702,8 @@ int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al)
     }
 #endif
 
-    if (s->version == TLS1_3_VERSION && !s->hit) {
-        unsigned char *encodedPoint;
-        size_t encoded_pt_len = 0;
-        EVP_PKEY *ckey = NULL, *skey = NULL;
-
-        ckey = s->s3->peer_tmp;
-        if (ckey == NULL) {
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            return 0;
-        }
-
-        if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_key_share)
-                || !WPACKET_start_sub_packet_u16(pkt)
-                || !WPACKET_put_bytes_u16(pkt, s->s3->group_id)) {
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            return 0;
-        }
-
-        skey = ssl_generate_pkey(ckey);
-
-        /* Generate encoding of server key */
-        encoded_pt_len = EVP_PKEY_get1_tls_encodedpoint(skey, &encodedPoint);
-        if (encoded_pt_len == 0) {
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_EC_LIB);
-            EVP_PKEY_free(skey);
-            return 0;
-        }
-
-        if (!WPACKET_sub_memcpy_u16(pkt, encodedPoint, encoded_pt_len)
-                || !WPACKET_close(pkt)) {
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            EVP_PKEY_free(skey);
-            OPENSSL_free(encodedPoint);
-            return 0;
-        }
-        OPENSSL_free(encodedPoint);
-
-        s->s3->tmp.pkey = skey;
-
-        if (ssl_derive(s, skey, ckey, 1) == 0) {
-            *al = SSL_AD_INTERNAL_ERROR;
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            return 0;
-        }
-    }
+    if (SSL_IS_TLS13(s) && !s->hit && !add_client_key_share_ext(s, pkt, al))
+        return 0;
 
     if (!custom_ext_add(s, 1, pkt, al)) {
         SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
@@ -1890,9 +1900,7 @@ static void ssl_check_for_safari(SSL *s, const CLIENTHELLO_MSG *hello)
  * Process the supported_groups extension if present. Returns success if the
  * extension is absent, or if it has been successfully processed.
  *
- * Returns
- * 1 on success
- * 0 on failure
+ * Returns 1 on success or 0 on failure
  */
 static int tls_process_supported_groups(SSL *s, CLIENTHELLO_MSG *hello)
 {
@@ -1926,11 +1934,8 @@ static int tls_process_supported_groups(SSL *s, CLIENTHELLO_MSG *hello)
 /*
  * Checks a list of |groups| to determine if the |group_id| is in it. If it is
  * and |checkallow| is 1 then additionally check if the group is allowed to be
- * used.
- *
- * Returns:
- * 1 if the group is in the list (and allowed if |checkallow| is 1)
- * 0 otherwise
+ * used. Returns 1 if the group is in the list (and allowed if |checkallow| is
+ * 1) or 0 otherwise.
  */
 static int check_in_list(SSL *s, unsigned int group_id,
                          const unsigned char *groups, size_t num_groups,
@@ -1943,6 +1948,7 @@ static int check_in_list(SSL *s, unsigned int group_id,
 
     for (i = 0; i < num_groups; i++, groups += 2) {
         unsigned int share_id = (groups[0] << 8) | (groups[1]);
+
         if (group_id == share_id
                 && (!checkallow || tls_curve_allowed(s, groups,
                                                      SSL_SECOP_CURVE_CHECK))) {
@@ -1950,11 +1956,130 @@ static int check_in_list(SSL *s, unsigned int group_id,
         }
     }
 
-    if (i == num_groups) {
-        /* Not in list */
+    /* If i == num_groups then not in the list */
+    return i < num_groups;
+}
+
+/*
+ * Process a key_share extension received in the ClientHello. |pkt| contains
+ * the raw PACKET data for the extension. Returns 1 on success or 0 on failure.
+ * If a failure occurs then |*al| is set to an appropriate alert value.
+ */
+static int process_key_share_ext(SSL *s, PACKET *pkt, int *al)
+{
+    unsigned int group_id;
+    PACKET key_share_list, encoded_pt;
+    const unsigned char *curves;
+    size_t num_curves;
+    int group_nid, found = 0;
+    unsigned int curve_flags;
+
+    /* Sanity check */
+    if (s->s3->peer_tmp != NULL) {
+        *al = SSL_AD_INTERNAL_ERROR;
+        SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR);
+        return 0;
+    }
+
+    if (!PACKET_as_length_prefixed_2(pkt, &key_share_list)) {
+        *al = SSL_AD_HANDSHAKE_FAILURE;
+        SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT,
+               SSL_R_LENGTH_MISMATCH);
         return 0;
     }
 
+    while (PACKET_remaining(&key_share_list) > 0) {
+        if (!PACKET_get_net_2(&key_share_list, &group_id)
+                || !PACKET_get_length_prefixed_2(&key_share_list, &encoded_pt)
+                || PACKET_remaining(&encoded_pt) == 0) {
+            *al = SSL_AD_HANDSHAKE_FAILURE;
+            SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT,
+                   SSL_R_LENGTH_MISMATCH);
+            return 0;
+        }
+
+        /*
+         * If we already found a suitable key_share we loop through the
+         * rest to verify the structure, but don't process them.
+         */
+        if (found)
+            continue;
+
+        /* Check if this share is in supported_groups sent from client */
+        if (!tls1_get_curvelist(s, 1, &curves, &num_curves)) {
+            *al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT,
+                   ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
+        if (!check_in_list(s, group_id, curves, num_curves, 0)) {
+            *al = SSL_AD_HANDSHAKE_FAILURE;
+            SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT,
+                   SSL_R_BAD_KEY_SHARE);
+            return 0;
+        }
+
+        /* Check if this share is for a group we can use */
+        if (!tls1_get_curvelist(s, 0, &curves, &num_curves)) {
+            *al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT,
+                   ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
+        if (!check_in_list(s, group_id, curves, num_curves, 1)) {
+            /* Share not suitable */
+            continue;
+        }
+
+        group_nid = tls1_ec_curve_id2nid(group_id, &curve_flags);
+
+        if (group_nid == 0) {
+            *al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT,
+                   SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS);
+            return 0;
+        }
+
+        if ((curve_flags & TLS_CURVE_TYPE) == TLS_CURVE_CUSTOM) {
+            /* Can happen for some curves, e.g. X25519 */
+            EVP_PKEY *key = EVP_PKEY_new();
+
+            if (key == NULL || !EVP_PKEY_set_type(key, group_nid)) {
+                *al = SSL_AD_INTERNAL_ERROR;
+                SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, ERR_R_EVP_LIB);
+                EVP_PKEY_free(key);
+                return 0;
+            }
+            s->s3->peer_tmp = key;
+        } else {
+            /* Set up EVP_PKEY with named curve as parameters */
+            EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_EC, NULL);
+            if (pctx == NULL
+                    || EVP_PKEY_paramgen_init(pctx) <= 0
+                    || EVP_PKEY_CTX_set_ec_paramgen_curve_nid(pctx,
+                                                              group_nid) <= 0
+                    || EVP_PKEY_paramgen(pctx, &s->s3->peer_tmp) <= 0) {
+                *al = SSL_AD_INTERNAL_ERROR;
+                SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, ERR_R_EVP_LIB);
+                EVP_PKEY_CTX_free(pctx);
+                return 0;
+            }
+            EVP_PKEY_CTX_free(pctx);
+            pctx = NULL;
+        }
+        s->s3->group_id = group_id;
+
+        if (!EVP_PKEY_set1_tls_encodedpoint(s->s3->peer_tmp,
+                PACKET_data(&encoded_pt),
+                PACKET_remaining(&encoded_pt))) {
+            *al = SSL_AD_DECODE_ERROR;
+            SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, SSL_R_BAD_ECPOINT);
+            return 0;
+        }
+
+        found = 1;
+    }
+
     return 1;
 }
 
@@ -2313,120 +2438,9 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al)
                  && !(s->options & SSL_OP_NO_ENCRYPT_THEN_MAC)) {
             s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;
         } else if (currext->type == TLSEXT_TYPE_key_share
-                   && s->version == TLS1_3_VERSION && !s->hit) {
-            unsigned int group_id;
-            PACKET key_share_list, encoded_pt;
-            const unsigned char *curves;
-            size_t num_curves;
-            int group_nid, found = 0;
-            unsigned int curve_flags;
-
-            /* Sanity check */
-            if (s->s3->peer_tmp != NULL) {
-                *al = SSL_AD_INTERNAL_ERROR;
-                SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-                return 0;
-            }
-
-            if (!PACKET_as_length_prefixed_2(&currext->data, &key_share_list)) {
-                *al = SSL_AD_HANDSHAKE_FAILURE;
-                SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT,
-                       SSL_R_LENGTH_MISMATCH);
-                return 0;
-            }
-
-            while (PACKET_remaining(&key_share_list) > 0) {
-                if (!PACKET_get_net_2(&key_share_list, &group_id)
-                        || !PACKET_get_length_prefixed_2(&key_share_list,
-                                                         &encoded_pt)
-                        || PACKET_remaining(&encoded_pt) == 0) {
-                    *al = SSL_AD_HANDSHAKE_FAILURE;
-                    SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT,
-                           SSL_R_LENGTH_MISMATCH);
-                    return 0;
-                }
-
-                /*
-                 * If we already found a suitable key_share we loop through the
-                 * rest to verify the structure, but don't process them.
-                 */
-                if (found)
-                    continue;
-
-                /* Check this share is in supported_groups */
-                if (!tls1_get_curvelist(s, 1, &curves, &num_curves)) {
-                    *al = SSL_AD_INTERNAL_ERROR;
-                    SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT,
-                           ERR_R_INTERNAL_ERROR);
-                    return 0;
-                }
-                if (!check_in_list(s, group_id, curves, num_curves, 0)) {
-                    *al = SSL_AD_HANDSHAKE_FAILURE;
-                    SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT,
-                           SSL_R_BAD_KEY_SHARE);
-                    return 0;
-                }
-
-                /* Find a share that we can use */
-                if (!tls1_get_curvelist(s, 0, &curves, &num_curves)) {
-                    *al = SSL_AD_INTERNAL_ERROR;
-                    SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT,
-                           ERR_R_INTERNAL_ERROR);
-                    return 0;
-                }
-                if (!check_in_list(s, group_id, curves, num_curves, 1)) {
-                    /* Share not suitable */
-                    continue;
-                }
-
-                group_nid = tls1_ec_curve_id2nid(group_id, &curve_flags);
-
-                if (group_nid == 0) {
-                    *al = SSL_AD_INTERNAL_ERROR;
-                    SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT,
-                           SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS);
-                    return 0;
-                }
-
-                if ((curve_flags & TLS_CURVE_TYPE) == TLS_CURVE_CUSTOM) {
-                    /* Can happen for some curves, e.g. X25519 */
-                    EVP_PKEY *key = EVP_PKEY_new();
-
-                    if (key == NULL || !EVP_PKEY_set_type(key, group_nid)) {
-                        *al = SSL_AD_INTERNAL_ERROR;
-                        SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, ERR_R_EVP_LIB);
-                        EVP_PKEY_free(key);
-                        return 0;
-                    }
-                    s->s3->peer_tmp = key;
-                } else {
-                    /* Set up EVP_PKEY with named curve as parameters */
-                    EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_EC, NULL);
-                    if (pctx == NULL
-                        || EVP_PKEY_paramgen_init(pctx) <= 0
-                        || EVP_PKEY_CTX_set_ec_paramgen_curve_nid(pctx,
-                                                                  group_nid) <= 0
-                        || EVP_PKEY_paramgen(pctx, &s->s3->peer_tmp) <= 0) {
-                        *al = SSL_AD_INTERNAL_ERROR;
-                        SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, ERR_R_EVP_LIB);
-                        EVP_PKEY_CTX_free(pctx);
-                        return 0;
-                    }
-                    EVP_PKEY_CTX_free(pctx);
-                    pctx = NULL;
-                }
-                s->s3->group_id = group_id;
-
-                if (!EVP_PKEY_set1_tls_encodedpoint(s->s3->peer_tmp,
-                        PACKET_data(&encoded_pt),
-                        PACKET_remaining(&encoded_pt))) {
-                    *al = SSL_AD_DECODE_ERROR;
-                    SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, SSL_R_BAD_ECPOINT);
-                    return 0;
-                }
-
-                found = 1;
-            }
+                   && SSL_IS_TLS13(s) && !s->hit
+                   && !process_key_share_ext(s, &currext->data, al)) {
+            return 0;
         }
         /*
          * Note: extended master secret extension handled in
@@ -2717,12 +2731,12 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al)
                 && s->s3->tmp.new_cipher->algorithm_enc != SSL_RC4)
                 s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;
         } else if (type == TLSEXT_TYPE_extended_master_secret &&
-                (SSL_IS_DTLS(s) || s->version < TLS1_3_VERSION)) {
+                (SSL_IS_DTLS(s) || !SSL_IS_TLS13(s))) {
             s->s3->flags |= TLS1_FLAGS_RECEIVED_EXTMS;
             if (!s->hit)
                 s->session->flags |= SSL_SESS_FLAG_EXTMS;
         } else if (type == TLSEXT_TYPE_key_share
-                && s->version == TLS1_3_VERSION) {
+                && SSL_IS_TLS13(s)) {
             unsigned int group_id;
             PACKET encoded_pt;
             EVP_PKEY *ckey = s->s3->tmp.pkey, *skey = NULL;
@@ -2752,8 +2766,6 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al)
                 return 0;
             }
 
-            skey = ssl_generate_pkey(ckey);
-
             if (!PACKET_as_length_prefixed_2(&spkt, &encoded_pt)
                     || PACKET_remaining(&encoded_pt) == 0) {
                 *al = SSL_AD_DECODE_ERROR;
@@ -2762,6 +2774,7 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al)
                 return 0;
             }
 
+            skey = ssl_generate_pkey(ckey);
             if (!EVP_PKEY_set1_tls_encodedpoint(skey, PACKET_data(&encoded_pt),
                                                 PACKET_remaining(&encoded_pt))) {
                 *al = SSL_AD_DECODE_ERROR;