From d6548b6b889e65cbaa866577f414e8275fb50c7d Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Wed, 17 Aug 2022 11:46:48 +0200 Subject: [PATCH] Small refactoring of named group handling for readability and de-duplicating of code. - add wolfSSL_CTX_curve_is_disabled() and wolfSSL_curve_is_disabled() to have common checks on wether a curve has been disabled by user - add macros returning 0 for above function when OPENSSL_EXTRA is not defined, enabling use without #fidef check - add macros for checking if named groups are in a certain range WOLFSSL_NAMED_GROUP_IS_FFHDE() WOLFSSL_NAMED_GROUP_IS_PQC() Fixed QuicTransportParam_free() use without case when compiling with c++. --- src/ssl.c | 18 ++++++++++++++ src/tls.c | 61 ++++++++++++++++++---------------------------- src/tls13.c | 3 +-- tests/quic.c | 43 +++++++++++++++++++++++++++++++- wolfssl/internal.h | 17 +++++++++++++ 5 files changed, 102 insertions(+), 40 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index dc629deed..342db4b9d 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -33904,6 +33904,22 @@ void wolfSSL_get0_next_proto_negotiated(const WOLFSSL *s, const unsigned char ** #endif /* WOLFSSL_NGINX / WOLFSSL_HAPROXY */ +#ifdef OPENSSL_EXTRA +int wolfSSL_CTX_curve_is_disabled(WOLFSSL_CTX* ctx, word16 named_curve) +{ + return (named_curve <= WOLFSSL_ECC_MAX && + ctx->disabledCurves && + ctx->disabledCurves & (1 << named_curve)); +} + +int wolfSSL_curve_is_disabled(WOLFSSL* ssl, word16 named_curve) +{ + /* FIXME: see wolfSSL_set1_curves_list() below on why + * this dependency on ssl->ctx alone is insufficient. */ + return wolfSSL_CTX_curve_is_disabled(ssl->ctx, named_curve); +} +#endif + #if defined(OPENSSL_EXTRA) && (defined(HAVE_ECC) || \ defined(HAVE_CURVE25519) || defined(HAVE_CURVE448)) int wolfSSL_CTX_set1_curves_list(WOLFSSL_CTX* ctx, const char* names) @@ -34011,6 +34027,8 @@ int wolfSSL_set1_curves_list(WOLFSSL* ssl, const char* names) if (ssl == NULL) { return WOLFSSL_FAILURE; } + /* FIXME: this manipulates the context from a WOLFSSL* and + * will lead to surprises for some. */ return wolfSSL_CTX_set1_curves_list(ssl->ctx, names); } #endif /* OPENSSL_EXTRA && (HAVE_ECC || HAVE_CURVE25519 || HAVE_CURVE448) */ diff --git a/src/tls.c b/src/tls.c index 6bd814a72..046b768d8 100644 --- a/src/tls.c +++ b/src/tls.c @@ -4183,8 +4183,7 @@ static int tlsx_ffdhe_find_group(WOLFSSL* ssl, SupportedCurve* clientGroup, const DhParams* params = NULL; for (; serverGroup != NULL; serverGroup = serverGroup->next) { - if (serverGroup->name < MIN_FFHDE_GROUP || - serverGroup->name > MAX_FFHDE_GROUP) + if (!WOLFSSL_NAMED_GROUP_IS_FFHDE(serverGroup->name)) continue; for (group = clientGroup; group != NULL; group = group->next) { @@ -4261,8 +4260,7 @@ static int tlsx_ffdhe_find_group(WOLFSSL* ssl, SupportedCurve* clientGroup, word32 p_len; for (; serverGroup != NULL; serverGroup = serverGroup->next) { - if (serverGroup->name < MIN_FFHDE_GROUP || - serverGroup->name > MAX_FFHDE_GROUP) + if (!WOLFSSL_NAMED_GROUP_IS_FFHDE(serverGroup->name)) continue; for (group = clientGroup; group != NULL; group = group->next) { @@ -4367,7 +4365,7 @@ int TLSX_SupportedFFDHE_Set(WOLFSSL* ssl) return 0; clientGroup = (SupportedCurve*)extension->data; for (group = clientGroup; group != NULL; group = group->next) { - if (group->name >= MIN_FFHDE_GROUP && group->name <= MAX_FFHDE_GROUP) { + if (WOLFSSL_NAMED_GROUP_IS_FFHDE(group->name)) { found = 1; break; } @@ -4495,11 +4493,10 @@ int TLSX_ValidateSupportedCurves(WOLFSSL* ssl, byte first, byte second) { curve = curve->next) { #ifdef OPENSSL_EXTRA - /* skip if name is not in supported ECC range */ - if (curve->name > WOLFSSL_ECC_X448) - continue; - /* skip if curve is disabled by user */ - if (ssl->ctx->disabledCurves & (1 << curve->name)) + /* skip if name is not in supported ECC range + * or disabled by user */ + if (curve->name > WOLFSSL_ECC_MAX || + wolfSSL_curve_is_disabled(ssl, curve->name)) continue; #endif @@ -7356,14 +7353,14 @@ static int TLSX_KeyShare_GenKey(WOLFSSL *ssl, KeyShareEntry *kse) { int ret; /* Named FFDHE groups have a bit set to identify them. */ - if (kse->group >= MIN_FFHDE_GROUP && kse->group <= MAX_FFHDE_GROUP) + if (WOLFSSL_NAMED_GROUP_IS_FFHDE(kse->group)) ret = TLSX_KeyShare_GenDhKey(ssl, kse); else if (kse->group == WOLFSSL_ECC_X25519) ret = TLSX_KeyShare_GenX25519Key(ssl, kse); else if (kse->group == WOLFSSL_ECC_X448) ret = TLSX_KeyShare_GenX448Key(ssl, kse); #ifdef HAVE_PQC - else if (kse->group >= WOLFSSL_PQC_MIN && kse->group <= WOLFSSL_PQC_MAX) + else if (WOLFSSL_NAMED_GROUP_IS_PQC(kse->group)) ret = TLSX_KeyShare_GenPqcKey(ssl, kse); #endif else @@ -7385,8 +7382,7 @@ static void TLSX_KeyShare_FreeAll(KeyShareEntry* list, void* heap) while ((current = list) != NULL) { list = current->next; - if (current->group >= MIN_FFHDE_GROUP && - current->group <= MAX_FFHDE_GROUP) { + if (WOLFSSL_NAMED_GROUP_IS_FFHDE(current->group)) { #ifndef NO_DH wc_FreeDhKey((DhKey*)current->key); #endif @@ -7402,8 +7398,7 @@ static void TLSX_KeyShare_FreeAll(KeyShareEntry* list, void* heap) #endif } #ifdef HAVE_PQC - else if (current->group >= WOLFSSL_PQC_MIN && - current->group <= WOLFSSL_PQC_MAX && + else if (WOLFSSL_NAMED_GROUP_IS_PQC(current->group) && current->key != NULL) { ForceZero((byte*)current->key, current->keyLen); } @@ -8251,16 +8246,14 @@ static int TLSX_KeyShare_Process(WOLFSSL* ssl, KeyShareEntry* keyShareEntry) ssl->arrays->preMasterSz = ENCRYPT_LEN; /* Use Key Share Data from server. */ - if (keyShareEntry->group >= MIN_FFHDE_GROUP && - keyShareEntry->group <= MAX_FFHDE_GROUP) + if (WOLFSSL_NAMED_GROUP_IS_FFHDE(keyShareEntry->group)) ret = TLSX_KeyShare_ProcessDh(ssl, keyShareEntry); else if (keyShareEntry->group == WOLFSSL_ECC_X25519) ret = TLSX_KeyShare_ProcessX25519(ssl, keyShareEntry); else if (keyShareEntry->group == WOLFSSL_ECC_X448) ret = TLSX_KeyShare_ProcessX448(ssl, keyShareEntry); #ifdef HAVE_PQC - else if (keyShareEntry->group >= WOLFSSL_PQC_MIN && - keyShareEntry->group <= WOLFSSL_PQC_MAX) + else if (WOLFSSL_NAMED_GROUP_IS_PQC(keyShareEntry->group)) ret = TLSX_KeyShare_ProcessPqc(ssl, keyShareEntry); #endif else @@ -8311,8 +8304,7 @@ static int TLSX_KeyShareEntry_Parse(WOLFSSL* ssl, const byte* input, return BUFFER_ERROR; #ifdef HAVE_PQC - if (group >= WOLFSSL_PQC_MIN && - group <= WOLFSSL_PQC_MAX && + if (WOLFSSL_NAMED_GROUP_IS_PQC(group) && ssl->options.side == WOLFSSL_SERVER_END) { /* For KEMs, the public key is not stored. Casting away const because * we know for KEMs, it will be read-only.*/ @@ -8526,7 +8518,7 @@ static int TLSX_KeyShare_Parse(WOLFSSL* ssl, const byte* input, word16 length, #ifdef HAVE_PQC /* For post-quantum groups, do this in TLSX_PopulateExtensions(). */ - if (group < WOLFSSL_PQC_MIN || group > WOLFSSL_PQC_MAX) + if (!WOLFSSL_NAMED_GROUP_IS_PQC(group)) #endif ret = TLSX_KeyShare_Use(ssl, group, 0, NULL, NULL); } @@ -8907,8 +8899,7 @@ int TLSX_KeyShare_Use(WOLFSSL* ssl, word16 group, word16 len, byte* data, #ifdef HAVE_PQC - if (group >= WOLFSSL_PQC_MIN && - group <= WOLFSSL_PQC_MAX && + if (WOLFSSL_NAMED_GROUP_IS_PQC(group) && ssl->options.side == WOLFSSL_SERVER_END) { ret = server_generate_pqc_ciphertext(ssl, keyShareEntry, data, len); @@ -9261,6 +9252,8 @@ static int TLSX_KeyShare_SetSupported(WOLFSSL* ssl) for (; curve != NULL; curve = curve->next) { if (!TLSX_KeyShare_IsSupported(curve->name)) continue; + if (wolfSSL_curve_is_disabled(ssl, curve->name)) + continue; rank = TLSX_KeyShare_GroupRank(ssl, curve->name); if (rank == -1) @@ -9355,21 +9348,16 @@ int TLSX_KeyShare_Establish(WOLFSSL *ssl, int* doHelloRetry) if (!TLSX_SupportedGroups_Find(ssl, clientKSE->group)) continue; - if (clientKSE->group < MIN_FFHDE_GROUP || - clientKSE->group > MAX_FFHDE_GROUP) { + if (!WOLFSSL_NAMED_GROUP_IS_FFHDE(clientKSE->group)) { /* Check max value supported. */ if (clientKSE->group > WOLFSSL_ECC_MAX) { #ifdef HAVE_PQC - if (clientKSE->group < WOLFSSL_PQC_MIN || - clientKSE->group > WOLFSSL_PQC_MAX ) + if (!WOLFSSL_NAMED_GROUP_IS_PQC(clientKSE->group)) #endif continue; } - #ifdef OPENSSL_EXTRA - /* Check if server supports group. */ - if (ssl->ctx->disabledCurves & ((word32)1 << clientKSE->group)) + if (wolfSSL_curve_is_disabled(ssl, clientKSE->group)) continue; - #endif } if (!TLSX_KeyShare_IsSupported(clientKSE->group)) continue; @@ -9402,8 +9390,7 @@ int TLSX_KeyShare_Establish(WOLFSSL *ssl, int* doHelloRetry) if (clientKSE->key == NULL) { #ifdef HAVE_PQC - if (clientKSE->group >= WOLFSSL_PQC_MIN && - clientKSE->group <= WOLFSSL_PQC_MAX ) { + if (WOLFSSL_NAMED_GROUP_IS_PQC(clientKSE->group)) { /* Going to need the public key (AKA ciphertext). */ serverKSE->pubKey = clientKSE->pubKey; clientKSE->pubKey = NULL; @@ -10446,7 +10433,7 @@ int TLSX_QuicTP_Use(WOLFSSL* ssl, TLSX_Type ext_type, int is_response) } } if (extension->data) { - QuicTransportParam_free(extension->data, ssl->heap); + QuicTransportParam_free((QuicTransportParam*)extension->data, ssl->heap); extension->data = NULL; } extension->resp = is_response; @@ -11476,7 +11463,7 @@ int TLSX_PopulateExtensions(WOLFSSL* ssl, byte isServer) if (namedGroup > 0) { #ifdef HAVE_PQC /* For KEMs, the key share has already been generated. */ - if (namedGroup < WOLFSSL_PQC_MIN || namedGroup > WOLFSSL_PQC_MAX) + if (!WOLFSSL_NAMED_GROUP_IS_PQC(namedGroup)) #endif ret = TLSX_KeyShare_Use(ssl, namedGroup, 0, NULL, NULL); if (ret != 0) diff --git a/src/tls13.c b/src/tls13.c index ec48af32c..88ccf2b92 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -10606,8 +10606,7 @@ int wolfSSL_UseKeyShare(WOLFSSL* ssl, word16 group) #endif #ifdef HAVE_PQC - if (group >= WOLFSSL_PQC_MIN && - group <= WOLFSSL_PQC_MAX) { + if (WOLFSSL_NAMED_GROUP_IS_PQC(group)) { if (ssl->ctx != NULL && ssl->ctx->method != NULL && ssl->ctx->method->version.minor != TLSv1_3_MINOR) { diff --git a/tests/quic.c b/tests/quic.c index abc34e277..5e43c30cf 100644 --- a/tests/quic.c +++ b/tests/quic.c @@ -218,7 +218,7 @@ static void dump_buffer(const char *name, const byte *p, size_t len, int indent) while((p != NULL) && (i < len)) { if((i % 0x10) == 0) { printf("\n"); - printf("%*s %04X - ", indent, " ", (int)(i / 0x10)); + printf("%*s %04X - ", indent, " ", (int)i); } else if((i % 0x08) == 0) { printf(" "); @@ -1142,6 +1142,46 @@ static int test_quic_server_hello(int verbose) { #ifdef HAVE_SESSION_TICKET +static int test_quic_key_share(int verbose) { + WOLFSSL_CTX *ctx_c, *ctx_s; + int ret = 0; + QuicTestContext tclient, tserver; + QuicConversation conv; + + AssertNotNull(ctx_c = wolfSSL_CTX_new(wolfTLSv1_3_client_method())); + AssertNotNull(ctx_s = wolfSSL_CTX_new(wolfTLSv1_3_server_method())); + AssertTrue(wolfSSL_CTX_use_certificate_file(ctx_s, svrCertFile, WOLFSSL_FILETYPE_PEM)); + AssertTrue(wolfSSL_CTX_use_PrivateKey_file(ctx_s, svrKeyFile, WOLFSSL_FILETYPE_PEM)); + + /* setup & handshake defaults */ + QuicTestContext_init(&tclient, ctx_c, "client", verbose); + QuicTestContext_init(&tserver, ctx_s, "server", verbose); + QuicConversation_init(&conv, &tclient, &tserver); + QuicConversation_do(&conv); + AssertStrEQ(conv.rec_log, + "ClientHello:ServerHello:EncryptedExtension:Certificate:CertificateVerify:Finished:Finished:SessionTicket"); + QuicTestContext_free(&tclient); + QuicTestContext_free(&tserver); + + /* setup & handshake, restricted groups, will trigger a + * HelloRetryRequest(ServerHello) and a new ClientHello */ + QuicTestContext_init(&tclient, ctx_c, "client", verbose); + QuicTestContext_init(&tserver, ctx_s, "server", verbose); + AssertTrue(wolfSSL_set1_curves_list(tclient.ssl, "X25519:P-256") + == WOLFSSL_SUCCESS); + AssertTrue(wolfSSL_set1_curves_list(tserver.ssl, "X25519") + == WOLFSSL_SUCCESS); + QuicConversation_init(&conv, &tclient, &tserver); + QuicConversation_do(&conv); + AssertStrEQ(conv.rec_log, + "ClientHello:ServerHello:ClientHello:ServerHello:EncryptedExtension:Certificate:CertificateVerify:Finished:Finished:SessionTicket"); + QuicTestContext_free(&tclient); + QuicTestContext_free(&tserver); + + printf(" test_quic_key_share: %s\n", (ret == 0)? passed : failed); + return ret; +} + static int test_quic_resumption(int verbose) { WOLFSSL_CTX *ctx_c, *ctx_s; WOLFSSL_SESSION *session; @@ -1379,6 +1419,7 @@ int QuicTest(void) if ((ret = test_quic_client_hello(verbose)) != 0) goto leave; if ((ret = test_quic_server_hello(verbose)) != 0) goto leave; #ifdef HAVE_SESSION_TICKET + if ((ret = test_quic_key_share(verbose)) != 0) goto leave; if ((ret = test_quic_resumption(verbose)) != 0) goto leave; #ifdef WOLFSSL_EARLY_DATA if ((ret = test_quic_early_data(verbose)) != 0) goto leave; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 39744f996..6ded466c5 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1570,6 +1570,15 @@ enum Misc { READ_PROTO = 0 /* reading a protocol message */ }; +#define WOLFSSL_NAMED_GROUP_IS_FFHDE(group) \ + (MIN_FFHDE_GROUP <= (group) && (group) <= MAX_FFHDE_GROUP) +#ifdef HAVE_PQC +#define WOLFSSL_NAMED_GROUP_IS_PQC(group) \ + (WOLFSSL_PQC_MIN <= (group) && (group) <= WOLFSSL_PQC_MAX) +#else +#define WOLFSSL_NAMED_GROUP_IS_PQC(group) ((void)(group), 0) +#endif /* HAVE_PQC */ + /* minimum Downgrade Minor version */ #ifndef WOLFSSL_MIN_DOWNGRADE #ifndef NO_OLD_TLS @@ -5241,6 +5250,14 @@ WOLFSSL_LOCAL int SetECKeyInternal(WOLFSSL_EC_KEY* eckey); WOLFSSL_LOCAL int SetECKeyExternal(WOLFSSL_EC_KEY* eckey); #endif +#if defined(OPENSSL_EXTRA) +WOLFSSL_LOCAL int wolfSSL_CTX_curve_is_disabled(WOLFSSL_CTX* ctx, word16 named_curve); +WOLFSSL_LOCAL int wolfSSL_curve_is_disabled(WOLFSSL* ssl, word16 named_curve); +#else +#define wolfSSL_CTX_curve_is_disabled(ctx, c) ((void)(ctx), (void)(c), 0) +#define wolfSSL_curve_is_disabled(ssl, c) ((void)(ssl), (void)(c), 0) +#endif + WOLFSSL_LOCAL WC_RNG* WOLFSSL_RSA_GetRNG(WOLFSSL_RSA *rsa, WC_RNG **tmpRNG, int *initTmpRng);