SM2 KeyShareEntry MUST be included in ClientHello when enable TLS1.3 …

…+ SM strictly

Fixed #522

Re-order curveSM2 to the first supported group when enable_sm_tls13_strict is set,
so that the key_share extension will include a KeyShareEntry for the "curveSM2"
group because only one KeyShareEntry is sent now.

ssl/statem/extensions_clnt.c

@@ -217,6 +217,56 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL *s, WPACKET *pkt,
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         return EXT_RETURN_FAIL;
     }
+
+#ifndef OPENSSL_NO_SM2
+    /*
+     * RFC 8998 requires that:
+     * For the key_share extension, a KeyShareEntry for the "curveSM2" group
+     * MUST be included. We re-order curveSM2 to the first supported group when
+     * enable_sm_tls13_strict so that the key_share extension will include a
+     * KeyShareEntry for the "curveSM2" group because only one KeyShareEntry is
+     * sent now.
+     */
+    if (!SSL_IS_DTLS(s) && max_version >= TLS1_3_VERSION
+        && s->enable_sm_tls13_strict == 1) {
+        int sm2_idx = -1;
+
+        for (i = 0; i < num_groups; i++) {
+            if (pgroups[i] == TLSEXT_curve_SM2) {
+                sm2_idx = i;
+                break;
+            }
+        }
+
+        if (sm2_idx > 0) {
+            int *groups = OPENSSL_malloc(sizeof(int) * num_groups);
+            if (groups == NULL) {
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+                return EXT_RETURN_FAIL;
+            }
+
+            for (i = 0; i < num_groups; i++)
+                groups[i] = tls1_group_id2nid(pgroups[i], 1);
+
+            for (i = sm2_idx; i > 0; i--)
+                groups[i] = groups[i - 1];
+
+            groups[0] = NID_sm2;
+
+            if (!tls1_set_groups(&s->ext.supportedgroups,
+                                 &s->ext.supportedgroups_len,
+                                 groups, num_groups)) {
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+                OPENSSL_free(groups);
+                return EXT_RETURN_FAIL;
+            }
+
+            OPENSSL_free(groups);
+            tls1_get_supported_groups(s, &pgroups, &num_groups);
+        }
+    }
+#endif
+
     /* Copy group ID if supported */
     for (i = 0; i < num_groups; i++) {
         uint16_t ctmp = pgroups[i];

ssl/t1_lib.c

@@ -677,13 +677,7 @@ int tls1_set_groups(uint16_t **pext, size_t *pextlen,
 {
     uint16_t *glist;
     size_t i;
-    /*
-     * Bitmap of groups included to detect duplicates: two variables are added
-     * to detect duplicates as some values are more than 32.
-     */
-    unsigned long *dup_list = NULL;
-    unsigned long dup_list_egrp = 0;
-    unsigned long dup_list_dhgrp = 0;
+    uint8_t bitmap[64] = { 0 };
 
     if (ngroups == 0) {
         ERR_raise(ERR_LIB_SSL, SSL_R_BAD_LENGTH);
@@ -694,20 +688,19 @@ int tls1_set_groups(uint16_t **pext, size_t *pextlen,
         return 0;
     }
     for (i = 0; i < ngroups; i++) {
-        unsigned long idmask;
         uint16_t id;
         id = tls1_nid2group_id(groups[i]);
         if (ngroups == 1) {
             glist[i] = id;
             break;
         }
-        if ((id & 0x00FF) >= (sizeof(unsigned long) * 8))
+        if (id == 0 || id >= sizeof(bitmap) * 8)
             goto err;
-        idmask = 1L << (id & 0x00FF);
-        dup_list = (id < 0x100) ? &dup_list_egrp : &dup_list_dhgrp;
-        if (!id || ((*dup_list) & idmask))
+
+        if (bitmap[id / 8] & (1 << (id % 8)))
             goto err;
-        *dup_list |= idmask;
+
+        bitmap[id / 8] |= 1 << (id % 8);
         glist[i] = id;
     }
     OPENSSL_free(*pext);

test/helpers/handshake.c

@@ -1716,6 +1716,9 @@ static HANDSHAKE_RESULT *do_handshake_internal(
     SSL_get_peer_signature_type_nid(client.ssl, &ret->server_sign_type);
     SSL_get_peer_signature_type_nid(server.ssl, &ret->client_sign_type);
 
+    if (SSL_IS_TLS13(client.ssl) && client.ssl->s3.did_kex)
+        ret->client_key_share = SSL_get_negotiated_group(client.ssl);
+
     names = SSL_get0_peer_CA_list(client.ssl);
     if (names == NULL)
         ret->client_ca_names = NULL;

test/helpers/handshake.h

@@ -70,6 +70,8 @@ typedef struct handshake_result {
     int client_sign_hash;
     /* client signature type */
     int client_sign_type;
+    /* client key share */
+    int client_key_share;
     /* Client CA names */
     STACK_OF(X509_NAME) *client_ca_names;
     /* Session id status */

test/helpers/ssl_test_ctx.c

@@ -13,6 +13,7 @@
 #include <openssl/crypto.h>
 
 #include "internal/nelem.h"
+#include "../../ssl/ssl_local.h"
 #include "ssl_test_ctx.h"
 #include "../testutil.h"
 
@@ -636,6 +637,22 @@ __owur static int parse_expected_sign_hash(int *ptype, const char *value)
     return 1;
 }
 
+__owur static int parse_expected_key_share(int *ptype, const char *value)
+{
+    int nid;
+
+    if (value == NULL)
+        return 0;
+    nid = OBJ_sn2nid(value);
+    if (nid == NID_undef)
+        nid = OBJ_ln2nid(value);
+    if (nid == NID_undef)
+        return 0;
+
+    *ptype = nid;
+    return 1;
+}
+
 __owur static int parse_expected_server_sign_hash(SSL_TEST_CTX *test_ctx,
                                                   const char *value)
 {
@@ -650,6 +667,13 @@ __owur static int parse_expected_client_sign_hash(SSL_TEST_CTX *test_ctx,
                                     value);
 }
 
+__owur static int parse_expected_client_key_share(SSL_TEST_CTX *test_ctx,
+                                                  const char *value)
+{
+    return parse_expected_key_share(&test_ctx->expected_client_key_share,
+                                    value);
+}
+
 __owur static int parse_expected_ca_names(STACK_OF(X509_NAME) **pnames,
                                           const char *value,
                                           OSSL_LIB_CTX *libctx)
@@ -737,6 +761,7 @@ static const ssl_test_ctx_option ssl_test_ctx_options[] = {
     { "ExpectedClientSignHash", &parse_expected_client_sign_hash },
     { "ExpectedClientSignType", &parse_expected_client_sign_type },
     { "ExpectedClientCANames", &parse_expected_client_ca_names },
+    { "ExpectedClientKeyShare", &parse_expected_client_key_share },
     { "UseSCTP", &parse_test_use_sctp },
     { "EnableClientSCTPLabelBug", &parse_test_enable_client_sctp_label_bug },
     { "EnableServerSCTPLabelBug", &parse_test_enable_server_sctp_label_bug },

test/helpers/ssl_test_ctx.h

@@ -233,6 +233,8 @@ typedef struct {
     int expected_client_sign_type;
     /* Expected CA names for client auth */
     STACK_OF(X509_NAME) *expected_client_ca_names;
+    /* Expected client key share */
+    int expected_client_key_share;
     /* Whether to use SCTP for the transport */
     int use_sctp;
     /* Enable SSL_MODE_DTLS_SCTP_LABEL_LENGTH_BUG on client side */