From dfa603e502b98d417e9e639e804d007209342e46 Mon Sep 17 00:00:00 2001 From: Daniel Pouzzner Date: Wed, 26 Oct 2022 11:32:06 -0500 Subject: [PATCH] fixes for warnings and defects around QUIC and ALPN -- fixes for clang-diagnostic-gnu-zero-variadic-macro-arguments, clang-analyzer-deadcode.DeadStores, clang-analyzer-core.UndefinedBinaryOperatorResult, clang-analyzer-security.insecureAPI.strcpy, and an overrun prevention assert in wolfSSL_ALPN_GetPeerProtocol(). --- src/ssl.c | 13 ++++++++++--- src/tls.c | 12 ++++++++---- tests/quic.c | 28 ++++++++++++++++------------ wolfssl/wolfcrypt/logging.h | 4 ++-- 4 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index e15652e95..a8258703c 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -3130,13 +3130,20 @@ int wolfSSL_ALPN_GetPeerProtocol(WOLFSSL* ssl, char **list, word16 *listSz) if (p == NULL) return MEMORY_ERROR; - for (i = 0, s = ssl->alpn_peer_requested, len = 0; + for (i = 0, s = ssl->alpn_peer_requested; i < ssl->alpn_peer_requested_length; - p += len, i += len) { + p += len, i += len) + { if (i) *p++ = ','; len = s[i++]; - XSTRNCPY(p, (char *)(s + i), len); + /* guard against bad length bytes. */ + if (i + len > ssl->alpn_peer_requested_length) { + XFREE(*list, ssl->heap, DYNAMIC_TYPE_TLSX); + *list = NULL; + return WOLFSSL_FAILURE; + } + XMEMCPY(p, s + i, len); } *p = 0; diff --git a/src/tls.c b/src/tls.c index a5eecd526..04af303ad 100644 --- a/src/tls.c +++ b/src/tls.c @@ -1594,11 +1594,15 @@ static int ALPN_find_match(WOLFSSL *ssl, TLSX **pextension, TLSX_APPLICATION_LAYER_PROTOCOL); /* No ALPN configured here */ - if (extension == NULL || extension->data == NULL) + if (extension == NULL || extension->data == NULL) { + *pextension = NULL; + *psel = NULL; + *psel_len = 0; return 0; + } list = (ALPN*)extension->data; - for (s = alpn_val, wlen = 0; + for (s = alpn_val; (s - alpn_val) < alpn_val_len; s += wlen) { wlen = *s++; /* bounds already checked on save */ @@ -1687,7 +1691,6 @@ static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, { word16 size = 0, offset = 0, wlen; int r = BUFFER_ERROR; - TLSX *extension; const byte *s; if (OPAQUE16_LEN > length) @@ -1701,7 +1704,7 @@ static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, return BUFFER_ERROR; /* validating length of entries before accepting */ - for (s = input + offset, wlen = 0; (s - input) < size; s += wlen) { + for (s = input + offset; (s - input) < size; s += wlen) { wlen = *s++; if (wlen == 0 || (s + wlen - input) > length) return BUFFER_ERROR; @@ -1726,6 +1729,7 @@ static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length, /* a response, we should find the value in our config */ const byte *sel = NULL; byte sel_len = 0; + TLSX *extension = NULL; r = ALPN_find_match(ssl, &extension, &sel, &sel_len, input + offset, size); if (r != 0) diff --git a/tests/quic.c b/tests/quic.c index 77e185926..a9c25d921 100644 --- a/tests/quic.c +++ b/tests/quic.c @@ -1186,13 +1186,17 @@ static int test_quic_server_hello(int verbose) { #endif #ifdef REALLY_HAVE_ALPN_AND_SNI +struct stripe_buffer { + char stripe[256]; +}; + static int inspect_SNI(WOLFSSL *ssl, int *ad, void *baton) { - char *stripe = baton; + struct stripe_buffer *stripe = (struct stripe_buffer *)baton; (void)ssl; *ad = 0; - strcat(stripe, "S"); + XSTRLCAT(stripe->stripe, "S", sizeof(stripe->stripe)); return 0; } @@ -1203,14 +1207,14 @@ static int select_ALPN(WOLFSSL *ssl, unsigned int inlen, void *baton) { - char *stripe = baton; + struct stripe_buffer *stripe = (struct stripe_buffer *)baton; (void)ssl; (void)inlen; /* just select the first */ *out = in + 1; *outlen = in[0]; - strcat(stripe, "A"); + XSTRLCAT(stripe->stripe, "A", sizeof(stripe->stripe)); return 0; } @@ -1219,7 +1223,7 @@ static int test_quic_alpn(int verbose) { int ret = 0; QuicTestContext tclient, tserver; QuicConversation conv; - char stripe[256]; + struct stripe_buffer stripe; unsigned char alpn_protos[256]; AssertNotNull(ctx_c = wolfSSL_CTX_new(wolfTLSv1_3_client_method())); @@ -1227,10 +1231,10 @@ static int test_quic_alpn(int verbose) { 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'; + stripe.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); + 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); @@ -1243,16 +1247,16 @@ static int test_quic_alpn(int verbose) { /* 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); + XSTRLCPY((char*)(alpn_protos + 1), "test", sizeof(alpn_protos)); + alpn_protos[0] = strlen("test"); + wolfSSL_set_alpn_protos(tclient.ssl, alpn_protos, 1 + strlen("test")); 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"); + AssertStrEQ(stripe.stripe, "SA"); QuicTestContext_free(&tclient); QuicTestContext_free(&tserver); diff --git a/wolfssl/wolfcrypt/logging.h b/wolfssl/wolfcrypt/logging.h index 1a756ced9..66518e137 100644 --- a/wolfssl/wolfcrypt/logging.h +++ b/wolfssl/wolfcrypt/logging.h @@ -166,7 +166,7 @@ WOLFSSL_API void wolfSSL_Debugging_OFF(void); WOLFSSL_API void WOLFSSL_MSG_EX(const char* fmt, ...); #define HAVE_WOLFSSL_MSG_EX #else - #define WOLFSSL_MSG_EX(m, ...) + #define WOLFSSL_MSG_EX(...) #endif WOLFSSL_API void WOLFSSL_MSG(const char* msg); WOLFSSL_API void WOLFSSL_BUFFER(const byte* buffer, word32 length); @@ -178,7 +178,7 @@ WOLFSSL_API void wolfSSL_Debugging_OFF(void); #define WOLFSSL_STUB(m) #define WOLFSSL_IS_DEBUG_ON() 0 - #define WOLFSSL_MSG_EX(m, ...) do{} while(0) + #define WOLFSSL_MSG_EX(...) do{} while(0) #define WOLFSSL_MSG(m) do{} while(0) #define WOLFSSL_BUFFER(b, l) do{} while(0)