From 5947c9ae8c435d0976dc550bebcd7ef8b73aba8f Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Fri, 21 Jul 2023 14:22:55 +0200 Subject: [PATCH 1/3] TLSX_CA_Names_Parse: Verify the length of the extension --- src/tls.c | 5 +++++ tests/api.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/tls.c b/src/tls.c index db15e0ae9..3ee94ba8a 100644 --- a/src/tls.c +++ b/src/tls.c @@ -6634,6 +6634,9 @@ static int TLSX_CA_Names_Parse(WOLFSSL *ssl, const byte* input, if (ssl->client_ca_names == NULL) return MEMORY_ERROR; + if (length < OPAQUE16_LEN) + return BUFFER_ERROR; + ato16(input, &extLen); input += OPAQUE16_LEN; length -= OPAQUE16_LEN; @@ -6655,6 +6658,8 @@ static int TLSX_CA_Names_Parse(WOLFSSL *ssl, const byte* input, DecodedCert cert[1]; #endif + if (length < OPAQUE16_LEN) + return BUFFER_ERROR; ato16(input, &extLen); idx += OPAQUE16_LEN; diff --git a/tests/api.c b/tests/api.c index d11dc59da..ebf23447a 100644 --- a/tests/api.c +++ b/tests/api.c @@ -62942,6 +62942,53 @@ static int test_dtls_no_extensions(void) return EXPECT_RESULT(); } +static int test_TLSX_CA_NAMES_bad_extension(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ + !defined(NO_CERTS) && !defined(WOLFSSL_NO_CA_NAMES) && \ + defined(OPENSSL_EXTRA) + /* This test should only fail (with BUFFER_ERROR) when we actually try to + * parse the CA Names extension. Otherwise it will return other non-related + * errors. If CA Names will be parsed in more configurations, that should + * be reflected in the macro guard above. */ + WOLFSSL *ssl_c = NULL; + WOLFSSL_CTX *ctx_c = NULL; + struct test_memio_ctx test_ctx; + const byte shBadCaNamesExt[] = { + 0x16, 0x03, 0x04, 0x00, 0x3f, 0x02, 0x00, 0x00, 0x3b, 0x03, 0x03, 0xcf, + 0x21, 0xad, 0x74, 0xe5, 0x9a, 0x61, 0x11, 0xbe, 0x1d, 0x8c, 0x02, 0x1e, + 0x65, 0xb8, 0x91, 0xc2, 0xa2, 0x11, 0x16, 0x7a, 0xbb, 0x8c, 0x5e, 0x07, + 0x9e, 0x09, 0xe2, 0xc8, 0xa8, 0x33, 0x9c, 0x00, 0x13, 0x03, 0x00, 0x00, + 0x13, 0x94, 0x7e, 0x00, 0x03, 0x0b, 0xf7, 0x03, 0x00, 0x2b, 0x00, 0x02, + 0x03, 0x04, 0x00, 0x33, 0x00, 0x02, 0x00, 0x19, 0x16, 0x03, 0x03, 0x00, + 0x5c, 0x02, 0x00, 0x00, 0x3b, 0x03, 0x03, 0x03, 0xcf, 0x21, 0xad, 0x74, + 0x00, 0x00, 0x83, 0x3f, 0x3b, 0x80, 0x01, 0xac, 0x65, 0x8c, 0x19, 0x2a, + 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x02, 0x00, 0x9e, 0x09, 0x1c, 0xe8, + 0xa8, 0x09, 0x9c, 0x00, 0xc0, 0xb5, 0x00, 0x00, 0x11, 0x8f, 0x00, 0x00, + 0x03, 0x3f, 0x00, 0x0c, 0x00, 0x2b, 0x00, 0x02, 0x03, 0x04, 0x13, 0x05, + 0x00, 0x00, 0x08, 0x00, 0x00, 0x06, 0x00, 0x04, 0x00, 0x09, 0x00, 0x00, + 0x0d, 0x00, 0x00, 0x11, 0x00, 0x00, 0x0d, 0x00, 0x2f, 0x00, 0x01, 0xff, + 0xff, 0xff, 0xff, 0xfa, 0x0d, 0x00, 0x00, 0x00, 0xad, 0x02 + }; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, NULL, &ssl_c, NULL, + wolfTLSv1_3_client_method, NULL), 0); + + XMEMCPY(test_ctx.c_buff, shBadCaNamesExt, sizeof(shBadCaNamesExt)); + test_ctx.c_len = sizeof(shBadCaNamesExt); + + ExpectIntEQ(wolfSSL_connect(ssl_c), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), BUFFER_ERROR); + + wolfSSL_free(ssl_c); + wolfSSL_CTX_free(ctx_c); +#endif + return EXPECT_RESULT(); +} + /*----------------------------------------------------------------------------* | Main *----------------------------------------------------------------------------*/ @@ -64192,6 +64239,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_dtls_ipv6_check), TEST_DECL(test_wolfSSL_SCR_after_resumption), TEST_DECL(test_dtls_no_extensions), + TEST_DECL(test_TLSX_CA_NAMES_bad_extension), /* This test needs to stay at the end to clean up any caches allocated. */ TEST_DECL(test_wolfSSL_Cleanup) }; From a495bb4e7f14696b71d5dc50d05260da5b4d88c6 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Wed, 26 Jul 2023 13:44:04 +0200 Subject: [PATCH 2/3] TLSX_CA_Names_Parse: make sure to do cleanup when smallstack is on --- src/tls.c | 24 ++++++++++++++---------- tests/api.c | 4 +++- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/tls.c b/src/tls.c index 3ee94ba8a..698c8cb41 100644 --- a/src/tls.c +++ b/src/tls.c @@ -6647,6 +6647,7 @@ static int TLSX_CA_Names_Parse(WOLFSSL *ssl, const byte* input, word32 idx = 0; WOLFSSL_X509_NAME* name = NULL; int ret = 0; + int didInit = FALSE; /* Use a DecodedCert struct to get access to GetName to * parse DN name */ #ifdef WOLFSSL_SMALL_STACK @@ -6664,24 +6665,27 @@ static int TLSX_CA_Names_Parse(WOLFSSL *ssl, const byte* input, idx += OPAQUE16_LEN; if (extLen > length) - return BUFFER_ERROR; + ret = BUFFER_ERROR; - InitDecodedCert(cert, input + idx, extLen, ssl->heap); - idx += extLen; - - ret = GetName(cert, SUBJECT, extLen); + if (ret == 0) { + InitDecodedCert(cert, input + idx, extLen, ssl->heap); + didInit = TRUE; + idx += extLen; + ret = GetName(cert, SUBJECT, extLen); + } if (ret == 0 && (name = wolfSSL_X509_NAME_new()) == NULL) ret = MEMORY_ERROR; - if (ret == 0) + if (ret == 0) { CopyDecodedName(name, cert, SUBJECT); - - if (ret == 0 && wolfSSL_sk_X509_NAME_push(ssl->client_ca_names, name) - == WOLFSSL_FAILURE) + if (wolfSSL_sk_X509_NAME_push(ssl->client_ca_names, name) + == WOLFSSL_FAILURE) ret = MEMORY_ERROR; + } - FreeDecodedCert(cert); + if (didInit) + FreeDecodedCert(cert); #ifdef WOLFSSL_SMALL_STACK XFREE(cert, ssl->heap, DYNAMIC_TYPE_DCERT); diff --git a/tests/api.c b/tests/api.c index ebf23447a..88c6f71f9 100644 --- a/tests/api.c +++ b/tests/api.c @@ -62947,7 +62947,8 @@ static int test_TLSX_CA_NAMES_bad_extension(void) EXPECT_DECLS; #if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ !defined(NO_CERTS) && !defined(WOLFSSL_NO_CA_NAMES) && \ - defined(OPENSSL_EXTRA) + defined(OPENSSL_EXTRA) && defined(WOLFSSL_SHA384) && \ + defined(HAVE_NULL_CIPHER) /* This test should only fail (with BUFFER_ERROR) when we actually try to * parse the CA Names extension. Otherwise it will return other non-related * errors. If CA Names will be parsed in more configurations, that should @@ -62955,6 +62956,7 @@ static int test_TLSX_CA_NAMES_bad_extension(void) WOLFSSL *ssl_c = NULL; WOLFSSL_CTX *ctx_c = NULL; struct test_memio_ctx test_ctx; + /* HRR + SH using TLS_DHE_PSK_WITH_NULL_SHA384 */ const byte shBadCaNamesExt[] = { 0x16, 0x03, 0x04, 0x00, 0x3f, 0x02, 0x00, 0x00, 0x3b, 0x03, 0x03, 0xcf, 0x21, 0xad, 0x74, 0xe5, 0x9a, 0x61, 0x11, 0xbe, 0x1d, 0x8c, 0x02, 0x1e, From 854ae0dcdb2391ddd6d6737ddd4dd78dbac13800 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Mon, 31 Jul 2023 15:16:59 +0200 Subject: [PATCH 3/3] Code review --- src/tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tls.c b/src/tls.c index 698c8cb41..3a9ef7a57 100644 --- a/src/tls.c +++ b/src/tls.c @@ -6681,7 +6681,7 @@ static int TLSX_CA_Names_Parse(WOLFSSL *ssl, const byte* input, CopyDecodedName(name, cert, SUBJECT); if (wolfSSL_sk_X509_NAME_push(ssl->client_ca_names, name) == WOLFSSL_FAILURE) - ret = MEMORY_ERROR; + ret = MEMORY_ERROR; } if (didInit)