From 79d85f6c136c63b66ab5acc8015c5d6e9c7c9c48 Mon Sep 17 00:00:00 2001 From: Sean Parkinson Date: Wed, 14 Sep 2022 09:16:16 +1000 Subject: [PATCH] TLS cipher suite: improvements wolfSSL_clear: check return from InitSSL_Suites() call. TLS13: check ClientHello cipher suite length is even. Silently remove duplicate cipher suites from user input. Add tests of duplicate cipher suite removal. --- src/internal.c | 29 +++++++++++++++++++++++++++++ src/ssl.c | 3 ++- src/tls13.c | 15 +++++++++++---- tests/api.c | 41 +++++++++++++++++++++++++++++++++-------- 4 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/internal.c b/src/internal.c index f9a8cdabc..6cc3427a5 100644 --- a/src/internal.c +++ b/src/internal.c @@ -24208,6 +24208,8 @@ int SetCipherList(WOLFSSL_CTX* ctx, Suites* suites, const char* list) #endif /* OPENSSL_EXTRA */ for (i = 0; i < suiteSz; i++) { + int j; + if (XSTRNCMP(name, cipher_names[i].name, sizeof(name)) == 0 #ifndef NO_ERROR_STRINGS || XSTRNCMP(name, cipher_names[i].name_iana, sizeof(name)) == 0 @@ -24225,6 +24227,17 @@ int SetCipherList(WOLFSSL_CTX* ctx, Suites* suites, const char* list) } #endif /* WOLFSSL_DTLS */ + for (j = 0; j < idx; j += 2) { + if ((suites->suites[j+0] == cipher_names[i].cipherSuite0) && + (suites->suites[j+1] == cipher_names[i].cipherSuite)) { + break; + } + } + /* Silently drop duplicates from list. */ + if (j != idx) { + break; + } + if (idx + 1 >= WOLFSSL_MAX_SUITE_SZ) { WOLFSSL_MSG("WOLFSSL_MAX_SUITE_SZ set too low"); return 0; /* suites buffer not large enough, error out */ @@ -24341,10 +24354,15 @@ int SetCipherListFromBytes(WOLFSSL_CTX* ctx, Suites* suites, const byte* list, return 0; } + if ((listSz % 2) != 0) { + return 0; + } + for (i = 0; (i + 1) < listSz; i += 2) { const byte firstByte = list[i]; const byte secondByte = list[i + 1]; const char* name = NULL; + int j; name = GetCipherNameInternal(firstByte, secondByte); if (XSTRCMP(name, "None") == 0) { @@ -24362,6 +24380,17 @@ int SetCipherListFromBytes(WOLFSSL_CTX* ctx, Suites* suites, const byte* list, } #endif /* WOLFSSL_DTLS */ + for (j = 0; j < idx; j += 2) { + if ((suites->suites[j+0] == firstByte) && + (suites->suites[j+1] == secondByte)) { + break; + } + } + /* Silently drop duplicates from list. */ + if (j != idx) { + continue; + } + if (idx + 1 >= WOLFSSL_MAX_SUITE_SZ) { WOLFSSL_MSG("WOLFSSL_MAX_SUITE_SZ set too low"); return 0; /* suites buffer not large enough, error out */ diff --git a/src/ssl.c b/src/ssl.c index 7778a501a..0b6fbab88 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -18718,7 +18718,8 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out, ssl->keys.encryptionOn = 0; XMEMSET(&ssl->msgsReceived, 0, sizeof(ssl->msgsReceived)); - InitSSL_Suites(ssl); + if (InitSSL_Suites(ssl) != WOLFSSL_SUCCESS) + return WOLFSSL_FAILURE; if (InitHandshakeHashes(ssl) != 0) return WOLFSSL_FAILURE; diff --git a/src/tls13.c b/src/tls13.c index f75437bb5..b56559af3 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -4764,26 +4764,30 @@ static void RefineSuites(WOLFSSL* ssl, Suites* peerSuites) /* Server order refining. */ for (i = 0; i < ssl->suites->suiteSz; i += 2) { for (j = 0; j < peerSuites->suiteSz; j += 2) { - if (ssl->suites->suites[i+0] == peerSuites->suites[j+0] && - ssl->suites->suites[i+1] == peerSuites->suites[j+1]) { + if ((ssl->suites->suites[i+0] == peerSuites->suites[j+0]) && + (ssl->suites->suites[i+1] == peerSuites->suites[j+1])) { suites[suiteSz++] = peerSuites->suites[j+0]; suites[suiteSz++] = peerSuites->suites[j+1]; break; } } + if (suiteSz == WOLFSSL_MAX_SUITE_SZ) + break; } } else { /* Client order refining. */ for (j = 0; j < peerSuites->suiteSz; j += 2) { for (i = 0; i < ssl->suites->suiteSz; i += 2) { - if (ssl->suites->suites[i+0] == peerSuites->suites[j+0] && - ssl->suites->suites[i+1] == peerSuites->suites[j+1]) { + if ((ssl->suites->suites[i+0] == peerSuites->suites[j+0]) && + (ssl->suites->suites[i+1] == peerSuites->suites[j+1])) { suites[suiteSz++] = peerSuites->suites[j+0]; suites[suiteSz++] = peerSuites->suites[j+1]; break; } } + if (suiteSz == WOLFSSL_MAX_SUITE_SZ) + break; } } @@ -5788,6 +5792,9 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, ERROR_OUT(BUFFER_ERROR, exit_dch); ato16(&input[args->idx], &args->clSuites->suiteSz); args->idx += OPAQUE16_LEN; + if ((args->clSuites->suiteSz % 2) != 0) { + ERROR_OUT(INVALID_PARAMETER, exit_dch); + } /* suites and compression length check */ if ((args->idx - args->begin) + args->clSuites->suiteSz + OPAQUE8_LEN > helloSz) ERROR_OUT(BUFFER_ERROR, exit_dch); diff --git a/tests/api.c b/tests/api.c index ff024a890..cce198927 100644 --- a/tests/api.c +++ b/tests/api.c @@ -51033,6 +51033,19 @@ static int test_tls13_cipher_suites(void) const int csOff = 78; /* Server cipher list. */ const char* serverCs = "TLS13-AES256-GCM-SHA384:TLS13-AES128-GCM-SHA256"; + /* Suite list with duplicates. */ + const char* dupCs = "TLS13-AES128-GCM-SHA256:" + "TLS13-AES128-GCM-SHA256:" + "TLS13-AES256-GCM-SHA384:" + "TLS13-AES256-GCM-SHA384:" + "TLS13-AES128-GCM-SHA256"; +#if defined(OPENSSL_EXTRA) || defined(WOLFSSL_SET_CIPHER_BYTES) + const byte dupCsBytes[] = { TLS13_BYTE, TLS_AES_256_GCM_SHA384, + TLS13_BYTE, TLS_AES_256_GCM_SHA384, + TLS13_BYTE, TLS_AES_128_GCM_SHA256, + TLS13_BYTE, TLS_AES_128_GCM_SHA256, + TLS13_BYTE, TLS_AES_256_GCM_SHA384 }; +#endif printf(testingFmt, "test_tls13_cipher_suites"); @@ -51076,14 +51089,11 @@ static int test_tls13_cipher_suites(void) msg.length = (unsigned int)sizeof(clientHello); wolfSSL_SetIOReadCtx(ssl, &msg); /* Server order: TLS13-AES256-GCM-SHA384:TLS13-AES128-GCM-SHA256 */ -#ifdef OPENSSL_EXTRA AssertIntEQ(wolfSSL_set_cipher_list(ssl, serverCs), WOLFSSL_SUCCESS); -#else - AssertIntEQ(wolfSSL_set_cipher_list(ssl, serverCs), 1); -#endif /* Negotiate cipher suites in server order: TLS13-AES256-GCM-SHA384 */ wolfSSL_accept_TLSv13(ssl); /* Check refined order - server order. */ + AssertIntEQ(ssl->suites->suiteSz, 4); AssertIntEQ(ssl->suites->suites[0], TLS13_BYTE); AssertIntEQ(ssl->suites->suites[1], TLS_AES_256_GCM_SHA384); AssertIntEQ(ssl->suites->suites[2], TLS13_BYTE); @@ -51096,21 +51106,36 @@ static int test_tls13_cipher_suites(void) msg.length = (unsigned int)sizeof(clientHello); wolfSSL_SetIOReadCtx(ssl, &msg); /* Server order: TLS13-AES256-GCM-SHA384:TLS13-AES128-GCM-SHA256 */ -#ifdef OPENSSL_EXTRA AssertIntEQ(wolfSSL_set_cipher_list(ssl, serverCs), WOLFSSL_SUCCESS); -#else - AssertIntEQ(wolfSSL_set_cipher_list(ssl, serverCs), 1); -#endif AssertIntEQ(wolfSSL_UseClientSuites(ssl), 0); /* Negotiate cipher suites in client order: TLS13-AES128-GCM-SHA256 */ wolfSSL_accept_TLSv13(ssl); /* Check refined order - client order. */ + AssertIntEQ(ssl->suites->suiteSz, 4); AssertIntEQ(ssl->suites->suites[0], TLS13_BYTE); AssertIntEQ(ssl->suites->suites[1], TLS_AES_128_GCM_SHA256); AssertIntEQ(ssl->suites->suites[2], TLS13_BYTE); AssertIntEQ(ssl->suites->suites[3], TLS_AES_256_GCM_SHA384); wolfSSL_free(ssl); + /* Check duplicate detection is working. */ + AssertIntEQ(wolfSSL_CTX_set_cipher_list(ctx, dupCs), WOLFSSL_SUCCESS); + AssertIntEQ(ctx->suites->suiteSz, 4); + AssertIntEQ(ctx->suites->suites[0], TLS13_BYTE); + AssertIntEQ(ctx->suites->suites[1], TLS_AES_128_GCM_SHA256); + AssertIntEQ(ctx->suites->suites[2], TLS13_BYTE); + AssertIntEQ(ctx->suites->suites[3], TLS_AES_256_GCM_SHA384); + +#if defined(OPENSSL_EXTRA) || defined(WOLFSSL_SET_CIPHER_BYTES) + AssertIntEQ(wolfSSL_CTX_set_cipher_list_bytes(ctx, dupCsBytes, + sizeof(dupCsBytes)), WOLFSSL_SUCCESS); + AssertIntEQ(ctx->suites->suiteSz, 4); + AssertIntEQ(ctx->suites->suites[0], TLS13_BYTE); + AssertIntEQ(ctx->suites->suites[1], TLS_AES_256_GCM_SHA384); + AssertIntEQ(ctx->suites->suites[2], TLS13_BYTE); + AssertIntEQ(ctx->suites->suites[3], TLS_AES_128_GCM_SHA256); +#endif + wolfSSL_CTX_free(ctx); printf(resultFmt, passed);