From f1cf96846af951abe8b977bd27c2d6d3ab50eb54 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Wed, 19 Oct 2022 15:25:52 +0200 Subject: [PATCH 1/5] Changing ALPN selection to a deterministic point in the handshake. --- src/internal.c | 14 ++- src/ssl.c | 28 ++++-- src/tls.c | 244 ++++++++++++++++++++++++++++----------------- src/tls13.c | 6 ++ tests/quic.c | 85 ++++++++++++++++ wolfssl/internal.h | 8 +- 6 files changed, 277 insertions(+), 108 deletions(-) diff --git a/src/internal.c b/src/internal.c index ae7c9bf93..2d83cdbbb 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6829,7 +6829,8 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) ssl->max_fragment = MAX_RECORD_SIZE; #endif #ifdef HAVE_ALPN - ssl->alpn_client_list = NULL; + ssl->alpn_peer_requested = NULL; + ssl->alpn_peer_requested_length = 0; #if defined(OPENSSL_ALL) || defined(WOLFSSL_NGINX) || defined(WOLFSSL_HAPROXY) ssl->alpnSelect = ctx->alpnSelect; ssl->alpnSelectArg = ctx->alpnSelectArg; @@ -7659,9 +7660,10 @@ void SSL_ResourceFree(WOLFSSL* ssl) TLSX_FreeAll(ssl->extensions, ssl->heap); #endif /* !NO_TLS */ #ifdef HAVE_ALPN - if (ssl->alpn_client_list != NULL) { - XFREE(ssl->alpn_client_list, ssl->heap, DYNAMIC_TYPE_ALPN); - ssl->alpn_client_list = NULL; + if (ssl->alpn_peer_requested != NULL) { + XFREE(ssl->alpn_peer_requested, ssl->heap, DYNAMIC_TYPE_ALPN); + ssl->alpn_peer_requested = NULL; + ssl->alpn_peer_requested_length = 0; } #endif #endif /* HAVE_TLS_EXTENSIONS */ @@ -33305,6 +33307,10 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, if((ret=SNI_Callback(ssl))) goto out; #endif + #ifdef HAVE_ALPN + if((ret=ALPN_Select(ssl))) + goto out; + #endif i += totalExtSz; #else diff --git a/src/ssl.c b/src/ssl.c index 9771cac1b..4b678e767 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -3108,22 +3108,32 @@ int wolfSSL_ALPN_GetProtocol(WOLFSSL* ssl, char **protocol_name, word16 *size) int wolfSSL_ALPN_GetPeerProtocol(WOLFSSL* ssl, char **list, word16 *listSz) { + int i, len; + char *p; + byte *s; + if (list == NULL || listSz == NULL) return BAD_FUNC_ARG; - if (ssl->alpn_client_list == NULL) + if (ssl->alpn_peer_requested == NULL + || ssl->alpn_peer_requested_length == 0) return BUFFER_ERROR; - *listSz = (word16)XSTRLEN(ssl->alpn_client_list); - if (*listSz == 0) - return BUFFER_ERROR; - - *list = (char *)XMALLOC((*listSz)+1, ssl->heap, DYNAMIC_TYPE_TLSX); - if (*list == NULL) + *listSz = ssl->alpn_peer_requested_length -1; + *list = p = (char *)XMALLOC(ssl->alpn_peer_requested_length, ssl->heap, + DYNAMIC_TYPE_TLSX); + if (p == NULL) return MEMORY_ERROR; - XSTRNCPY(*list, ssl->alpn_client_list, (*listSz)+1); - (*list)[*listSz] = 0; + for (i = 0, s = ssl->alpn_peer_requested, len = 0; + i < ssl->alpn_peer_requested_length; + p += len, i += len) { + if (i) + *p++ = ','; + len = s[i++]; + XSTRNCPY(p, (char *)(s + i), len); + } + *p = 0; return WOLFSSL_SUCCESS; } diff --git a/src/tls.c b/src/tls.c index d7e2a9f00..837a2341f 100644 --- a/src/tls.c +++ b/src/tls.c @@ -1579,134 +1579,190 @@ static int TLSX_SetALPN(TLSX** extensions, const void* data, word16 size, return WOLFSSL_SUCCESS; } -/** Parses a buffer of ALPN extensions and set the first one matching - * client and server requirements */ -static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, - byte isRequest) +static int ALPN_find_match(WOLFSSL *ssl, TLSX **pextension, + const byte **psel, byte *psel_len, + const byte *alpn_val, word16 alpn_val_len) { - word16 size = 0, offset = 0, idx = 0; - int r = BUFFER_ERROR; - byte match = 0; TLSX *extension; - ALPN *alpn = NULL, *list; - - if (OPAQUE16_LEN > length) - return BUFFER_ERROR; - - ato16(input, &size); - offset += OPAQUE16_LEN; - - if (size == 0) - return BUFFER_ERROR; + ALPN *alpn, *list; + int r = 0; + const byte *sel = NULL, *s; + byte sel_len = 0, wlen; extension = TLSX_Find(ssl->extensions, TLSX_APPLICATION_LAYER_PROTOCOL); if (extension == NULL) extension = TLSX_Find(ssl->ctx->extensions, TLSX_APPLICATION_LAYER_PROTOCOL); -#if defined(OPENSSL_ALL) || defined(WOLFSSL_NGINX) || defined(WOLFSSL_HAPROXY) - if (ssl->alpnSelect != NULL && ssl->options.side == WOLFSSL_SERVER_END) { - const byte* out; - unsigned char outLen; - - if (ssl->alpnSelect(ssl, &out, &outLen, input + offset, size, - ssl->alpnSelectArg) == 0) { - WOLFSSL_MSG("ALPN protocol match"); - /* clears out all current ALPN extensions set */ - TLSX_Remove(&ssl->extensions, TLSX_APPLICATION_LAYER_PROTOCOL, ssl->heap); - extension = NULL; - if (TLSX_UseALPN(&ssl->extensions, (char*)out, outLen, 0, ssl->heap) - == WOLFSSL_SUCCESS) { - extension = TLSX_Find(ssl->extensions, - TLSX_APPLICATION_LAYER_PROTOCOL); - } - } - } -#endif - + /* No ALPN configured here */ if (extension == NULL || extension->data == NULL) { - return isRequest ? 0 - : TLSX_HandleUnsupportedExtension(ssl); + extension = NULL; + goto cleanup; } - /* validating alpn list length */ - if (length != OPAQUE16_LEN + size) - return BUFFER_ERROR; - list = (ALPN*)extension->data; - - /* keep the list sent by client */ - if (isRequest) { - if (ssl->alpn_client_list != NULL) - XFREE(ssl->alpn_client_list, ssl->heap, DYNAMIC_TYPE_ALPN); - - ssl->alpn_client_list = (char *)XMALLOC(size, ssl->heap, - DYNAMIC_TYPE_ALPN); - if (ssl->alpn_client_list == NULL) - return MEMORY_ERROR; - } - - for (size = 0; offset < length; offset += size) { - - size = input[offset++]; - if (offset + size > length || size == 0) - return BUFFER_ERROR; - - if (isRequest) { - XMEMCPY(ssl->alpn_client_list+idx, (char*)input + offset, size); - idx += size; - ssl->alpn_client_list[idx++] = ','; - } - - if (!match) { - alpn = TLSX_ALPN_Find(list, (char*)input + offset, size); - if (alpn != NULL) { - WOLFSSL_MSG("ALPN protocol match"); - match = 1; - - /* skip reading other values if not required */ - if (!isRequest) - break; - } + for (s = alpn_val, wlen = 0; + (s - alpn_val) < alpn_val_len; + s += wlen) { + wlen = *s++; /* bounds already checked on save */ + alpn = TLSX_ALPN_Find(list, (char*)s, wlen); + if (alpn != NULL) { + WOLFSSL_MSG("ALPN protocol match"); + sel = s, + sel_len = wlen; + break; } } - if (isRequest) - ssl->alpn_client_list[idx-1] = 0; - - if (!match) { + if (sel == NULL) { WOLFSSL_MSG("No ALPN protocol match"); /* do nothing if no protocol match between client and server and option is set to continue (like OpenSSL) */ if (list->options & WOLFSSL_ALPN_CONTINUE_ON_MISMATCH) { WOLFSSL_MSG("Continue on mismatch"); - return 0; + goto cleanup; } SendAlert(ssl, alert_fatal, no_application_protocol); WOLFSSL_ERROR_VERBOSE(UNKNOWN_ALPN_PROTOCOL_NAME_E); - return UNKNOWN_ALPN_PROTOCOL_NAME_E; + r = UNKNOWN_ALPN_PROTOCOL_NAME_E; + goto cleanup; } - /* set the matching negotiated protocol */ - r = TLSX_SetALPN(&ssl->extensions, - alpn->protocol_name, - (word16)XSTRLEN(alpn->protocol_name), - ssl->heap); - if (r != WOLFSSL_SUCCESS) { - WOLFSSL_MSG("TLSX_SetALPN failed"); - return BUFFER_ERROR; +cleanup: + *pextension = extension; + *psel = sel; + *psel_len = sel_len; + return r; +} + +int ALPN_Select(WOLFSSL *ssl) +{ + TLSX *extension; + const byte *sel = NULL; + byte sel_len = 0; + int r = 0; + + WOLFSSL_ENTER("ALPN_Select"); + if (ssl->alpn_peer_requested == NULL) { + goto cleanup; } - /* reply to ALPN extension sent from client */ - if (isRequest) { +#if defined(OPENSSL_ALL) || defined(WOLFSSL_NGINX) || defined(WOLFSSL_HAPROXY) + if (ssl->alpnSelect != NULL && ssl->options.side == WOLFSSL_SERVER_END) { + if (ssl->alpnSelect(ssl, &sel, &sel_len, ssl->alpn_peer_requested, + ssl->alpn_peer_requested_length, + ssl->alpnSelectArg) == 0) { + WOLFSSL_MSG_EX("ALPN protocol match"); + } + else { + sel = NULL; + sel_len = 0; + } + } +#endif + + if (sel == NULL) { + r = ALPN_find_match(ssl, &extension, &sel, &sel_len, + ssl->alpn_peer_requested, + ssl->alpn_peer_requested_length); + if (r != 0) + goto cleanup; + } + + if (sel != NULL) { + /* set the matching negotiated protocol */ + r = TLSX_SetALPN(&ssl->extensions, sel, sel_len, ssl->heap); + if (r != WOLFSSL_SUCCESS) { + WOLFSSL_MSG("TLSX_SetALPN failed"); + r = BUFFER_ERROR; + goto cleanup; + } + /* reply to ALPN extension sent from peer */ #ifndef NO_WOLFSSL_SERVER TLSX_SetResponse(ssl, TLSX_APPLICATION_LAYER_PROTOCOL); #endif } + r = 0; - return 0; +cleanup: + WOLFSSL_LEAVE("ALPN_Select", r); + return r; +} + +/** Parses a buffer of ALPN extensions and set the first one matching + * client and server requirements */ +static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, + byte isRequest) +{ + word16 size = 0, offset = 0, wlen; + int r = BUFFER_ERROR; + TLSX *extension; + const byte *s; + + if (OPAQUE16_LEN > length) + goto cleanup; + + ato16(input, &size); + offset += OPAQUE16_LEN; + + /* validating alpn list length */ + if (size == 0 || length != OPAQUE16_LEN + size) + goto cleanup; + /* validating length of entries before accepting */ + for (s = input + offset, wlen = 0; (s - input) < size; s += wlen) { + wlen = *s++; + if (wlen == 0 || (s + wlen - input) > length) + goto cleanup; + } + + if (isRequest) { + /* keep the list sent by peer, if this is from a request. We + * use it later in ALPN_Select() for evaluation. */ + if (ssl->alpn_peer_requested != NULL) + XFREE(ssl->alpn_peer_requested, ssl->heap, DYNAMIC_TYPE_ALPN); + + ssl->alpn_peer_requested = (byte *)XMALLOC(size, ssl->heap, + DYNAMIC_TYPE_ALPN); + if (ssl->alpn_peer_requested == NULL) { + r = MEMORY_ERROR; + goto cleanup; + } + ssl->alpn_peer_requested_length = size; + XMEMCPY(ssl->alpn_peer_requested, (char*)input + offset, size); + } + else { + /* a response, we should find the value in our config */ + const byte *sel = NULL; + byte sel_len = 0; + + r = ALPN_find_match(ssl, &extension, &sel, &sel_len, input + offset, size); + if (r != 0) + goto cleanup; + + if (sel != NULL) { + /* set the matching negotiated protocol */ + r = TLSX_SetALPN(&ssl->extensions, sel, sel_len, ssl->heap); + if (r != WOLFSSL_SUCCESS) { + WOLFSSL_MSG("TLSX_SetALPN failed"); + r = BUFFER_ERROR; + goto cleanup; + } + } + /* If we had nothing configured, the response is unexpected */ + else if (extension == NULL) { + r = TLSX_HandleUnsupportedExtension(ssl); + goto cleanup; + } + else { + /* have sth configured, but did not match. no error returned + * means we accepted that. */ + } + } + r = 0; +cleanup: + return r; } /** Add a protocol name to the list of accepted usable ones */ diff --git a/src/tls13.c b/src/tls13.c index b18df6d19..bb9b2ac0e 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -5950,6 +5950,12 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #endif } +#ifdef HAVE_ALPN + /* With PSK and all other things validated, it's time to + * select the ALPN protocol, if so requested */ + if ((ret = ALPN_Select(ssl)) != 0) + return ret; +#endif /* Advance state and proceed */ ssl->options.asyncState = TLS_ASYNC_BUILD; } /* case TLS_ASYNC_BEGIN */ diff --git a/tests/quic.c b/tests/quic.c index 261ba14fc..4290b0fe8 100644 --- a/tests/quic.c +++ b/tests/quic.c @@ -1174,6 +1174,88 @@ static int test_quic_server_hello(int verbose) { return ret; } +#if defined(HAVE_ALPN) && defined(HAVE_SNI) + +static int inspect_SNI(WOLFSSL *ssl, int *ad, void *baton) +{ + char *stripe = baton; + + (void)ssl; + *ad = 0; + strcat(stripe, "S"); + return 0; +} + +static int select_ALPN(WOLFSSL *ssl, + const unsigned char **out, + unsigned char *outlen, + const unsigned char *in, + unsigned int inlen, + void *baton) +{ + char *stripe = baton; + + (void)ssl; + (void)inlen; + /* just select the first */ + *out = in + 1; + *outlen = in[0]; + strcat(stripe, "A"); + return 0; +} + +static int test_quic_alpn(int verbose) { + WOLFSSL_CTX *ctx_c, *ctx_s; + int ret = 0; + QuicTestContext tclient, tserver; + QuicConversation conv; + char stripe[256]; + unsigned char alpn_protos[256]; + + 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)); + + stripe[0] = '\0'; + wolfSSL_CTX_set_servername_callback(ctx_s, inspect_SNI); + wolfSSL_CTX_set_servername_arg(ctx_s, stripe); + wolfSSL_CTX_set_alpn_select_cb(ctx_s, select_ALPN, stripe); + + /* setup ssls */ + QuicTestContext_init(&tclient, ctx_c, "client", verbose); + QuicTestContext_init(&tserver, ctx_s, "server", verbose); + + /* set SNI and ALPN callbacks on server side, + * provide values on client side */ + wolfSSL_UseSNI(tclient.ssl, WOLFSSL_SNI_HOST_NAME, + "wolfssl.com", sizeof("wolfssl.com")-1); + /* connect */ + QuicConversation_init(&conv, &tclient, &tserver); + + strcpy((char*)(alpn_protos + 1), "test"); + alpn_protos[0] = 4; + wolfSSL_set_alpn_protos(tclient.ssl, alpn_protos, 5); + + QuicConversation_do(&conv); + AssertIntEQ(tclient.output.len, 0); + AssertIntEQ(tserver.output.len, 0); + + /* SNI callback needs to be called before ALPN callback */ + AssertStrEQ(stripe, "SA"); + + QuicTestContext_free(&tclient); + QuicTestContext_free(&tserver); + + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); + printf(" test_quic_alpn: %s\n", (ret == 0)? passed : failed); + + return ret; +} +#endif /* defined(HAVE_ALPN) && defined(HAVE_SNI) */ + + #ifdef HAVE_SESSION_TICKET static int test_quic_key_share(int verbose) { @@ -1536,6 +1618,9 @@ int QuicTest(void) if ((ret = test_quic_crypt()) != 0) goto leave; if ((ret = test_quic_client_hello(verbose)) != 0) goto leave; if ((ret = test_quic_server_hello(verbose)) != 0) goto leave; +#if defined(HAVE_ALPN) && defined(HAVE_SNI) + if ((ret = test_quic_alpn(verbose)) != 0) goto leave; +#endif /* defined(HAVE_ALPN) && defined(HAVE_SNI) */ #ifdef HAVE_SESSION_TICKET if ((ret = test_quic_key_share(verbose)) != 0) goto leave; if ((ret = test_quic_resumption(verbose)) != 0) goto leave; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index c2858b62c..ad102b236 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1975,6 +1975,10 @@ WOLFSSL_LOCAL int SNI_Callback(WOLFSSL* ssl); #endif #endif +#ifdef HAVE_ALPN +WOLFSSL_LOCAL int ALPN_Select(WOLFSSL* ssl); +#endif + WOLFSSL_LOCAL int ChachaAEADEncrypt(WOLFSSL* ssl, byte* out, const byte* input, word16 sz); /* needed by sniffer */ @@ -5080,7 +5084,9 @@ struct WOLFSSL { SecureRenegotiation* secure_renegotiation; /* valid pointer indicates */ #endif /* user turned on */ #ifdef HAVE_ALPN - char* alpn_client_list; /* keep the client's list */ + byte *alpn_peer_requested; /* the ALPN bytes requested by peer, sequence + * of length byte + chars */ + word16 alpn_peer_requested_length; /* number of bytes total */ #if defined(OPENSSL_ALL) || defined(WOLFSSL_NGINX) || \ defined(WOLFSSL_HAPROXY) || defined(WOLFSSL_QUIC) CallbackALPNSelect alpnSelect; From 057fdd30d38f599d097386de7de6785fa9425289 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Thu, 20 Oct 2022 09:21:36 +0200 Subject: [PATCH 2/5] Properly check the defined() combinations that make ALPN and SNI available for testing. --- tests/quic.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/quic.c b/tests/quic.c index 4290b0fe8..77e185926 100644 --- a/tests/quic.c +++ b/tests/quic.c @@ -1174,8 +1174,18 @@ static int test_quic_server_hello(int verbose) { return ret; } -#if defined(HAVE_ALPN) && defined(HAVE_SNI) +/* This has gotten a bit out of hand. */ +#if (defined(OPENSSL_ALL) || (defined(OPENSSL_EXTRA) && \ + (defined(HAVE_STUNNEL) || defined(WOLFSSL_NGINX) || \ + defined(HAVE_LIGHTY) || defined(WOLFSSL_HAPROXY) || \ + defined(WOLFSSL_OPENSSH) || defined(HAVE_SBLIM_SFCB)))) \ + && defined(HAVE_ALPN) && defined(HAVE_SNI) +#define REALLY_HAVE_ALPN_AND_SNI +#else +#undef REALLY_HAVE_ALPN_AND_SNI +#endif +#ifdef REALLY_HAVE_ALPN_AND_SNI static int inspect_SNI(WOLFSSL *ssl, int *ad, void *baton) { char *stripe = baton; @@ -1253,7 +1263,7 @@ static int test_quic_alpn(int verbose) { return ret; } -#endif /* defined(HAVE_ALPN) && defined(HAVE_SNI) */ +#endif /* REALLY_HAVE_ALPN_AND_SNI */ #ifdef HAVE_SESSION_TICKET @@ -1618,9 +1628,9 @@ int QuicTest(void) if ((ret = test_quic_crypt()) != 0) goto leave; if ((ret = test_quic_client_hello(verbose)) != 0) goto leave; if ((ret = test_quic_server_hello(verbose)) != 0) goto leave; -#if defined(HAVE_ALPN) && defined(HAVE_SNI) +#ifdef REALLY_HAVE_ALPN_AND_SNI if ((ret = test_quic_alpn(verbose)) != 0) goto leave; -#endif /* defined(HAVE_ALPN) && defined(HAVE_SNI) */ +#endif /* REALLY_HAVE_ALPN_AND_SNI */ #ifdef HAVE_SESSION_TICKET if ((ret = test_quic_key_share(verbose)) != 0) goto leave; if ((ret = test_quic_resumption(verbose)) != 0) goto leave; From 02d37f08fc7152a5eec774f8f2667633ceb9e824 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Thu, 20 Oct 2022 12:33:08 +0200 Subject: [PATCH 3/5] Do not direclty return but goto exit label for cleanup of allocated resources in case ALPN selection or SNI callback fails. --- src/tls13.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tls13.c b/src/tls13.c index bb9b2ac0e..7cbe43cc5 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -5863,7 +5863,7 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #ifdef HAVE_SNI if ((ret = SNI_Callback(ssl)) != 0) - return ret; + goto exit_dch; ssl->options.side = WOLFSSL_SERVER_END; #endif @@ -5954,7 +5954,7 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, /* With PSK and all other things validated, it's time to * select the ALPN protocol, if so requested */ if ((ret = ALPN_Select(ssl)) != 0) - return ret; + goto exit_dch; #endif /* Advance state and proceed */ ssl->options.asyncState = TLS_ASYNC_BUILD; From a1203917c5a3a0c7629f1a73b55dbcefef0dd33c Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Mon, 24 Oct 2022 10:17:29 +0200 Subject: [PATCH 4/5] Update after review by haydenroche5. --- src/ssl.c | 9 ++++++-- src/tls.c | 63 ++++++++++++++++++++----------------------------------- 2 files changed, 30 insertions(+), 42 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 4b678e767..c37a8a40e 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -3112,14 +3112,19 @@ int wolfSSL_ALPN_GetPeerProtocol(WOLFSSL* ssl, char **list, word16 *listSz) char *p; byte *s; - if (list == NULL || listSz == NULL) + if (ssl == NULL || list == NULL || listSz == NULL) return BAD_FUNC_ARG; if (ssl->alpn_peer_requested == NULL || ssl->alpn_peer_requested_length == 0) return BUFFER_ERROR; - *listSz = ssl->alpn_peer_requested_length -1; + /* ssl->alpn_peer_requested are the original bytes sent in a ClientHello, + * formatted as (len-byte chars+)+. To turn n protocols into a + * comma-separated C string, one needs (n-1) commas and a final 0 byte + * which has the same length as the original. + * The returned length is the strlen() of the C string, so -1 of that. */ + *listSz = ssl->alpn_peer_requested_length-1; *list = p = (char *)XMALLOC(ssl->alpn_peer_requested_length, ssl->heap, DYNAMIC_TYPE_TLSX); if (p == NULL) diff --git a/src/tls.c b/src/tls.c index 837a2341f..9bfc7dbdb 100644 --- a/src/tls.c +++ b/src/tls.c @@ -1585,7 +1585,6 @@ static int ALPN_find_match(WOLFSSL *ssl, TLSX **pextension, { TLSX *extension; ALPN *alpn, *list; - int r = 0; const byte *sel = NULL, *s; byte sel_len = 0, wlen; @@ -1595,10 +1594,8 @@ static int ALPN_find_match(WOLFSSL *ssl, TLSX **pextension, TLSX_APPLICATION_LAYER_PROTOCOL); /* No ALPN configured here */ - if (extension == NULL || extension->data == NULL) { - extension = NULL; - goto cleanup; - } + if (extension == NULL || extension->data == NULL) + return 0; list = (ALPN*)extension->data; for (s = alpn_val, wlen = 0; @@ -1621,20 +1618,18 @@ static int ALPN_find_match(WOLFSSL *ssl, TLSX **pextension, is set to continue (like OpenSSL) */ if (list->options & WOLFSSL_ALPN_CONTINUE_ON_MISMATCH) { WOLFSSL_MSG("Continue on mismatch"); - goto cleanup; } - - SendAlert(ssl, alert_fatal, no_application_protocol); - WOLFSSL_ERROR_VERBOSE(UNKNOWN_ALPN_PROTOCOL_NAME_E); - r = UNKNOWN_ALPN_PROTOCOL_NAME_E; - goto cleanup; + else { + SendAlert(ssl, alert_fatal, no_application_protocol); + WOLFSSL_ERROR_VERBOSE(UNKNOWN_ALPN_PROTOCOL_NAME_E); + return UNKNOWN_ALPN_PROTOCOL_NAME_E; + } } -cleanup: *pextension = extension; *psel = sel; *psel_len = sel_len; - return r; + return 0; } int ALPN_Select(WOLFSSL *ssl) @@ -1645,9 +1640,8 @@ int ALPN_Select(WOLFSSL *ssl) int r = 0; WOLFSSL_ENTER("ALPN_Select"); - if (ssl->alpn_peer_requested == NULL) { - goto cleanup; - } + if (ssl->alpn_peer_requested == NULL) + return 0; #if defined(OPENSSL_ALL) || defined(WOLFSSL_NGINX) || defined(WOLFSSL_HAPROXY) if (ssl->alpnSelect != NULL && ssl->options.side == WOLFSSL_SERVER_END) { @@ -1668,7 +1662,7 @@ int ALPN_Select(WOLFSSL *ssl) ssl->alpn_peer_requested, ssl->alpn_peer_requested_length); if (r != 0) - goto cleanup; + return r; } if (sel != NULL) { @@ -1676,19 +1670,14 @@ int ALPN_Select(WOLFSSL *ssl) r = TLSX_SetALPN(&ssl->extensions, sel, sel_len, ssl->heap); if (r != WOLFSSL_SUCCESS) { WOLFSSL_MSG("TLSX_SetALPN failed"); - r = BUFFER_ERROR; - goto cleanup; + return BUFFER_ERROR; } /* reply to ALPN extension sent from peer */ #ifndef NO_WOLFSSL_SERVER TLSX_SetResponse(ssl, TLSX_APPLICATION_LAYER_PROTOCOL); #endif } - r = 0; - -cleanup: - WOLFSSL_LEAVE("ALPN_Select", r); - return r; + return 0; } /** Parses a buffer of ALPN extensions and set the first one matching @@ -1702,19 +1691,20 @@ static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, const byte *s; if (OPAQUE16_LEN > length) - goto cleanup; + return BUFFER_ERROR; ato16(input, &size); offset += OPAQUE16_LEN; /* validating alpn list length */ if (size == 0 || length != OPAQUE16_LEN + size) - goto cleanup; + return BUFFER_ERROR; + /* validating length of entries before accepting */ for (s = input + offset, wlen = 0; (s - input) < size; s += wlen) { wlen = *s++; if (wlen == 0 || (s + wlen - input) > length) - goto cleanup; + return BUFFER_ERROR; } if (isRequest) { @@ -1726,8 +1716,7 @@ static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, ssl->alpn_peer_requested = (byte *)XMALLOC(size, ssl->heap, DYNAMIC_TYPE_ALPN); if (ssl->alpn_peer_requested == NULL) { - r = MEMORY_ERROR; - goto cleanup; + return MEMORY_ERROR; } ssl->alpn_peer_requested_length = size; XMEMCPY(ssl->alpn_peer_requested, (char*)input + offset, size); @@ -1739,30 +1728,24 @@ static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, r = ALPN_find_match(ssl, &extension, &sel, &sel_len, input + offset, size); if (r != 0) - goto cleanup; + return r; if (sel != NULL) { /* set the matching negotiated protocol */ r = TLSX_SetALPN(&ssl->extensions, sel, sel_len, ssl->heap); if (r != WOLFSSL_SUCCESS) { WOLFSSL_MSG("TLSX_SetALPN failed"); - r = BUFFER_ERROR; - goto cleanup; + return BUFFER_ERROR; } } /* If we had nothing configured, the response is unexpected */ else if (extension == NULL) { r = TLSX_HandleUnsupportedExtension(ssl); - goto cleanup; - } - else { - /* have sth configured, but did not match. no error returned - * means we accepted that. */ + if (r != 0) + return r; } } - r = 0; -cleanup: - return r; + return 0; } /** Add a protocol name to the list of accepted usable ones */ From 879f788bb9ba68a99296b475801c2223eeab087d Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Mon, 24 Oct 2022 10:27:16 +0200 Subject: [PATCH 5/5] Setting ssl->alpn_peer_requested_length to 0 when freeing ssl->alpn_peer_requested. --- src/tls.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tls.c b/src/tls.c index 9bfc7dbdb..a5eecd526 100644 --- a/src/tls.c +++ b/src/tls.c @@ -1710,9 +1710,10 @@ static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, if (isRequest) { /* keep the list sent by peer, if this is from a request. We * use it later in ALPN_Select() for evaluation. */ - if (ssl->alpn_peer_requested != NULL) + if (ssl->alpn_peer_requested != NULL) { XFREE(ssl->alpn_peer_requested, ssl->heap, DYNAMIC_TYPE_ALPN); - + ssl->alpn_peer_requested_length = 0; + } ssl->alpn_peer_requested = (byte *)XMALLOC(size, ssl->heap, DYNAMIC_TYPE_ALPN); if (ssl->alpn_peer_requested == NULL) {