From d6548b6b889e65cbaa866577f414e8275fb50c7d Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Wed, 17 Aug 2022 11:46:48 +0200 Subject: [PATCH 1/6] 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); From 3918a2e29a9102e2e27d341d4f97baa6894f4281 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Wed, 17 Aug 2022 17:01:18 +0200 Subject: [PATCH 2/6] Renaming the named_curve parameter to curve_id to avoid shadowing. --- src/ssl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 342db4b9d..3bdb4ebd8 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -33905,18 +33905,18 @@ 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) +int wolfSSL_CTX_curve_is_disabled(WOLFSSL_CTX* ctx, word16 curve_id) { - return (named_curve <= WOLFSSL_ECC_MAX && + return (curve_id <= WOLFSSL_ECC_MAX && ctx->disabledCurves && - ctx->disabledCurves & (1 << named_curve)); + ctx->disabledCurves & (1 << curve_id)); } -int wolfSSL_curve_is_disabled(WOLFSSL* ssl, word16 named_curve) +int wolfSSL_curve_is_disabled(WOLFSSL* ssl, word16 curve_id) { /* 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); + return wolfSSL_CTX_curve_is_disabled(ssl->ctx, curve_id); } #endif From 6316e26bdcbf7671c33786db58556de1afb0b85f Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Thu, 18 Aug 2022 10:24:18 +0200 Subject: [PATCH 3/6] Adding the forgotten wolfSSL_CTX_free() at the end of the new test_quic_key_share case. --- tests/quic.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/quic.c b/tests/quic.c index 5e43c30cf..a6d6a0dcd 100644 --- a/tests/quic.c +++ b/tests/quic.c @@ -1178,6 +1178,8 @@ static int test_quic_key_share(int verbose) { QuicTestContext_free(&tclient); QuicTestContext_free(&tserver); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); printf(" test_quic_key_share: %s\n", (ret == 0)? passed : failed); return ret; } From a66516d3a571052193b96750bebcce521100a40d Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Fri, 19 Aug 2022 09:02:28 +0200 Subject: [PATCH 4/6] Extending quic resumption tests. --- tests/quic.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 5 deletions(-) diff --git a/tests/quic.c b/tests/quic.c index a6d6a0dcd..df4256c5e 100644 --- a/tests/quic.c +++ b/tests/quic.c @@ -1186,10 +1186,13 @@ static int test_quic_key_share(int verbose) { static int test_quic_resumption(int verbose) { WOLFSSL_CTX *ctx_c, *ctx_s; - WOLFSSL_SESSION *session; + WOLFSSL_SESSION *session, *session_restored; int ret = 0; QuicTestContext tclient, tserver; QuicConversation conv; + unsigned char session_buffer[16 * 1024], *session_data; + const unsigned char *session_data2; + unsigned int session_size; AssertNotNull(ctx_c = wolfSSL_CTX_new(wolfTLSv1_3_client_method())); AssertNotNull(ctx_s = wolfSSL_CTX_new(wolfTLSv1_3_server_method())); @@ -1206,13 +1209,22 @@ static int test_quic_resumption(int verbose) { /* what have we seen? */ AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Certificate:CertificateVerify:Finished:Finished:SessionTicket"); - /* Should have received a session ticket, save the session */ + /* Should have received a session ticket, save the session + * and also make a serialized/deserialized copy to check that persisting + * a session works. */ AssertTrue(tclient.ticket_len > 0); AssertNotNull(session = wolfSSL_get1_session(tclient.ssl)); + AssertTrue((session_size = wolfSSL_i2d_SSL_SESSION(session, NULL)) > 0); + AssertTrue((size_t)session_size < sizeof(session_buffer)); + session_data2 = session_data = session_buffer; + session_size = wolfSSL_i2d_SSL_SESSION(session, &session_data); + session_restored = wolfSSL_d2i_SSL_SESSION(NULL, &session_data2, session_size); + AssertNotNull(session_restored); + QuicTestContext_free(&tserver); QuicTestContext_free(&tclient); - /* Do a Session resumption with the ticket */ + /* Do a Session resumption with the session object */ QuicTestContext_init(&tserver, ctx_s, "server", verbose); QuicTestContext_init(&tclient, ctx_c, "client_resume", verbose); AssertIntEQ(wolfSSL_set_session(tclient.ssl, session), WOLFSSL_SUCCESS); @@ -1221,11 +1233,43 @@ static int test_quic_resumption(int verbose) { QuicConversation_do(&conv); /* this is what should happen. Look Ma, no certificate! */ AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Finished:Finished:SessionTicket"); - QuicTestContext_free(&tclient); QuicTestContext_free(&tserver); + /* Do a Session resumption with the restored session object */ + QuicTestContext_init(&tserver, ctx_s, "server", verbose); + QuicTestContext_init(&tclient, ctx_c, "client_resume_restored", verbose); + AssertIntEQ(wolfSSL_set_session(tclient.ssl, session_restored), WOLFSSL_SUCCESS); + /* let them talk */ + QuicConversation_init(&conv, &tclient, &tserver); + QuicConversation_do(&conv); + /* this is what should happen. Look Ma, no certificate! */ + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Finished:Finished:SessionTicket"); + QuicTestContext_free(&tclient); + QuicTestContext_free(&tserver); + + if (/*disables code*/(0)) { + /* FIXME: this fails with a RSA Padding error in DoTls13CertificateVerify */ + /* Do a Session resumption with a new server ctx */ + WOLFSSL_CTX *ctx_s2; + AssertNotNull(ctx_s2 = wolfSSL_CTX_new(wolfTLSv1_3_server_method())); + AssertTrue(wolfSSL_CTX_use_certificate_file(ctx_s2, eccCertFile, WOLFSSL_FILETYPE_PEM)); + AssertTrue(wolfSSL_CTX_use_PrivateKey_file(ctx_s2, eccKeyFile, WOLFSSL_FILETYPE_PEM)); + + QuicTestContext_init(&tserver, ctx_s2, "server2", verbose); + QuicTestContext_init(&tclient, ctx_c, "client_resume2", verbose); + AssertIntEQ(wolfSSL_set_session(tclient.ssl, session_restored), WOLFSSL_SUCCESS); + /* let them talk */ + QuicConversation_init(&conv, &tclient, &tserver); + QuicConversation_do(&conv); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:XEncryptedExtension:Finished:Finished:SessionTicket"); + QuicTestContext_free(&tclient); + QuicTestContext_free(&tserver); + wolfSSL_CTX_free(ctx_s2); + } + wolfSSL_SESSION_free(session); + wolfSSL_SESSION_free(session_restored); wolfSSL_CTX_free(ctx_c); wolfSSL_CTX_free(ctx_s); @@ -1422,7 +1466,7 @@ int QuicTest(void) 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; + if ((ret = test_quic_resumption(verbose || 1)) != 0) goto leave; #ifdef WOLFSSL_EARLY_DATA if ((ret = test_quic_early_data(verbose)) != 0) goto leave; #endif /* WOLFSSL_EARLY_DATA */ From 6cb0caa0a070070a7c9aa9166e93d03613ad47d4 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Fri, 19 Aug 2022 11:03:23 +0200 Subject: [PATCH 5/6] Adding `disabledCurves` as a member of WOLFSSL in the OPENSSL_EXTRA case. - inheriting from WOLFSSL_CTX on creation - enabling on WOLFSSL only when wolfSSL_set1_curves_list() is called --- src/internal.c | 2 ++ src/ssl.c | 53 +++++++++++++++++++++++++--------------------- wolfssl/internal.h | 3 +-- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/src/internal.c b/src/internal.c index 1add79966..77fd88f60 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6797,6 +6797,8 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) if (ctx->protoMsgCb != NULL) { ssl->toInfoOn = 1; } + + ssl->disabledCurves = ctx->disabledCurves; #endif InitCiphers(ssl); diff --git a/src/ssl.c b/src/ssl.c index 3bdb4ebd8..93e614f19 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -33905,36 +33905,25 @@ 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 curve_id) -{ - return (curve_id <= WOLFSSL_ECC_MAX && - ctx->disabledCurves && - ctx->disabledCurves & (1 << curve_id)); -} - int wolfSSL_curve_is_disabled(WOLFSSL* ssl, word16 curve_id) { - /* 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, curve_id); + return (curve_id <= WOLFSSL_ECC_MAX && + ssl->disabledCurves && + ssl->disabledCurves & (1 << curve_id)); } #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) +static int set_curves_list(WOLFSSL* ssl, WOLFSSL_CTX *ctx, const char* names) { int idx, start = 0, len; word16 curve; + word32 disabled; char name[MAX_CURVE_NAME_SZ]; - if (ctx == NULL || names == NULL) { - WOLFSSL_MSG("ctx or names was NULL"); - return WOLFSSL_FAILURE; - } - /* Disable all curves so that only the ones the user wants are enabled. */ - ctx->disabledCurves = 0xFFFFFFFFUL; + disabled = 0xFFFFFFFFUL; for (idx = 1; names[idx-1] != '\0'; idx++) { if (names[idx] != ':' && names[idx] != '\0') continue; @@ -34008,28 +33997,44 @@ int wolfSSL_CTX_set1_curves_list(WOLFSSL_CTX* ctx, const char* names) #if defined(HAVE_SUPPORTED_CURVES) && !defined(NO_WOLFSSL_CLIENT) /* set the supported curve so client TLS extension contains only the * desired curves */ - if (wolfSSL_CTX_UseSupportedCurve(ctx, curve) != WOLFSSL_SUCCESS) { + if ((ssl + && wolfSSL_UseSupportedCurve(ssl, curve) != WOLFSSL_SUCCESS) + || (ctx + && wolfSSL_CTX_UseSupportedCurve(ctx, curve) != WOLFSSL_SUCCESS)) { WOLFSSL_MSG("Unable to set supported curve"); return WOLFSSL_FAILURE; } #endif /* Switch the bit to off and therefore is enabled. */ - ctx->disabledCurves &= ~(1U << curve); + disabled &= ~(1U << curve); start = idx + 1; } + if (ssl) + ssl->disabledCurves = disabled; + else + ctx->disabledCurves = disabled; + return WOLFSSL_SUCCESS; } -int wolfSSL_set1_curves_list(WOLFSSL* ssl, const char* names) +int wolfSSL_CTX_set1_curves_list(WOLFSSL_CTX* ctx, const char* names) { - if (ssl == NULL) { + if (ctx == NULL || names == NULL) { + WOLFSSL_MSG("ctx or names was 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); + return set_curves_list(NULL, ctx, names); +} + +int wolfSSL_set1_curves_list(WOLFSSL* ssl, const char* names) +{ + if (ssl == NULL || names == NULL) { + WOLFSSL_MSG("ssl or names was NULL"); + return WOLFSSL_FAILURE; + } + return set_curves_list(ssl, NULL, names); } #endif /* OPENSSL_EXTRA && (HAVE_ECC || HAVE_CURVE25519 || HAVE_CURVE448) */ diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 6ded466c5..2c3b59f49 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -4668,6 +4668,7 @@ struct WOLFSSL { WOLFSSL_BIO* biowr; /* socket bio write to free/close */ byte sessionCtx[ID_LEN]; /* app session context ID */ WOLFSSL_X509_VERIFY_PARAM* param; /* verification parameters*/ + word32 disabledCurves; /* curves disabled by user */ #endif #if defined(OPENSSL_EXTRA) || defined(OPENSSL_EXTRA_X509_SMALL) unsigned long peerVerifyRet; @@ -5251,10 +5252,8 @@ 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 From a7c0c4649e3639a5cf808a0f63ca419cd0379973 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Fri, 19 Aug 2022 15:56:20 +0200 Subject: [PATCH 6/6] Fixing Handshake Hash update when Preshared Keys offered by client, but none of them was accepted. - This applies to TLSv1.3 and QUIC - QUIC test case to trigger the bug enabled --- src/tls13.c | 16 ++++++++++++++-- tests/quic.c | 36 ++++++++++++++++++++++-------------- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/src/tls13.c b/src/tls13.c index 88ccf2b92..2556367fc 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -4803,6 +4803,8 @@ static int CheckPreSharedKeys(WOLFSSL* ssl, const byte* input, word32 helloSz, #ifdef WOLFSSL_EARLY_DATA ssl->earlyData = no_early_data; #endif + if (usingPSK) + *usingPSK = 0; /* Hash data up to binders for deriving binders in PSK extension. */ ret = HashInput(ssl, input, helloSz); return ret; @@ -4860,8 +4862,18 @@ static int CheckPreSharedKeys(WOLFSSL* ssl, const byte* input, word32 helloSz, return ret; #endif - /* Hash the rest of the ClientHello. */ - ret = HashRaw(ssl, input + helloSz - bindersLen, bindersLen); + if (*usingPSK) { + /* While verifying the selected PSK, we updated the + * handshake hash up to the binder bytes in the PSK extensions. + * Continuing, we need the rest of the ClientHello hashed as well. + */ + ret = HashRaw(ssl, input + helloSz - bindersLen, bindersLen); + } + else { + /* No suitable PSK found, Hash the complete ClientHello, + * as caller expect it after we return */ + ret = HashInput(ssl, input, helloSz); + } if (ret != 0) return ret; diff --git a/tests/quic.c b/tests/quic.c index df4256c5e..8c0f6da2d 100644 --- a/tests/quic.c +++ b/tests/quic.c @@ -1037,7 +1037,8 @@ static int test_quic_client_hello(int verbose) { /* Set transport params, expect both extensions */ QuicTestContext_init(&tctx, ctx, "client", verbose); #ifdef HAVE_SNI - wolfSSL_UseSNI(tctx.ssl, WOLFSSL_SNI_HOST_NAME, "wolfssl.com", sizeof("wolfssl.com")-1); + wolfSSL_UseSNI(tctx.ssl, WOLFSSL_SNI_HOST_NAME, + "wolfssl.com", sizeof("wolfssl.com")-1); #endif AssertTrue(wolfSSL_connect(tctx.ssl) != 0); AssertIntEQ(wolfSSL_get_error(tctx.ssl, 0), SSL_ERROR_WANT_READ); @@ -1106,9 +1107,11 @@ static int test_quic_server_hello(int verbose) { AssertIntEQ(tserver.output.len, 0); /* what have we seen? */ #ifdef HAVE_SESSION_TICKET - AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Certificate:CertificateVerify:Finished:Finished:SessionTicket"); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:" + "Certificate:CertificateVerify:Finished:Finished:SessionTicket"); #else - AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Certificate:CertificateVerify:Finished:Finished"); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:" + "Certificate:CertificateVerify:Finished:Finished"); #endif /* we are at application encryption level */ AssertTrue(wolfSSL_quic_read_level(tclient.ssl) == wolfssl_encryption_application); @@ -1158,8 +1161,8 @@ static int test_quic_key_share(int 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"); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:" + "Certificate:CertificateVerify:Finished:Finished:SessionTicket"); QuicTestContext_free(&tclient); QuicTestContext_free(&tserver); @@ -1174,7 +1177,8 @@ static int test_quic_key_share(int verbose) { QuicConversation_init(&conv, &tclient, &tserver); QuicConversation_do(&conv); AssertStrEQ(conv.rec_log, - "ClientHello:ServerHello:ClientHello:ServerHello:EncryptedExtension:Certificate:CertificateVerify:Finished:Finished:SessionTicket"); + "ClientHello:ServerHello:ClientHello:ServerHello:EncryptedExtension:" + "Certificate:CertificateVerify:Finished:Finished:SessionTicket"); QuicTestContext_free(&tclient); QuicTestContext_free(&tserver); @@ -1207,7 +1211,8 @@ static int test_quic_resumption(int verbose) { /* run till end */ QuicConversation_do(&conv); /* what have we seen? */ - AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Certificate:CertificateVerify:Finished:Finished:SessionTicket"); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:" + "Certificate:CertificateVerify:Finished:Finished:SessionTicket"); /* Should have received a session ticket, save the session * and also make a serialized/deserialized copy to check that persisting @@ -1232,7 +1237,8 @@ static int test_quic_resumption(int verbose) { QuicConversation_init(&conv, &tclient, &tserver); QuicConversation_do(&conv); /* this is what should happen. Look Ma, no certificate! */ - AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Finished:Finished:SessionTicket"); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:" + "Finished:Finished:SessionTicket"); QuicTestContext_free(&tclient); QuicTestContext_free(&tserver); @@ -1244,12 +1250,12 @@ static int test_quic_resumption(int verbose) { QuicConversation_init(&conv, &tclient, &tserver); QuicConversation_do(&conv); /* this is what should happen. Look Ma, no certificate! */ - AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Finished:Finished:SessionTicket"); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:" + "Finished:Finished:SessionTicket"); QuicTestContext_free(&tclient); QuicTestContext_free(&tserver); - if (/*disables code*/(0)) { - /* FIXME: this fails with a RSA Padding error in DoTls13CertificateVerify */ + { /* Do a Session resumption with a new server ctx */ WOLFSSL_CTX *ctx_s2; AssertNotNull(ctx_s2 = wolfSSL_CTX_new(wolfTLSv1_3_server_method())); @@ -1262,7 +1268,8 @@ static int test_quic_resumption(int verbose) { /* let them talk */ QuicConversation_init(&conv, &tclient, &tserver); QuicConversation_do(&conv); - AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:XEncryptedExtension:Finished:Finished:SessionTicket"); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:" + "Certificate:CertificateVerify:Finished:Finished:SessionTicket"); QuicTestContext_free(&tclient); QuicTestContext_free(&tserver); wolfSSL_CTX_free(ctx_s2); @@ -1306,7 +1313,8 @@ static int test_quic_early_data(int verbose) { /* run till end */ QuicConversation_do(&conv); /* what have we seen? */ - AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:Certificate:CertificateVerify:Finished:Finished:SessionTicket"); + AssertStrEQ(conv.rec_log, "ClientHello:ServerHello:EncryptedExtension:" + "Certificate:CertificateVerify:Finished:Finished:SessionTicket"); /* Should have received a session ticket, save the session */ AssertTrue(tclient.ticket_len > 0); @@ -1466,7 +1474,7 @@ int QuicTest(void) 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 || 1)) != 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; #endif /* WOLFSSL_EARLY_DATA */