From 8bddeb10c7d909171ee1a42f65f733e975b0ef43 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Mon, 5 Feb 2024 15:55:58 +0100 Subject: [PATCH] DTLS sequence number and cookie fixes - dtls: check that the cookie secret is not emtpy - Dtls13DoDowngrade -> Dtls13ClientDoDowngrade - dtls: generate both 1.2 and 1.3 cookie secrets in case we downgrade - dtls: setup sequence numbers for downgrade - add dtls downgrade sequence number check test Fixes ZD17314 --- src/dtls.c | 8 ++++++ src/internal.c | 13 ++++----- src/tls13.c | 21 +++++++++++--- tests/api.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 11 deletions(-) diff --git a/src/dtls.c b/src/dtls.c index fceeedbec..aecd2605a 100644 --- a/src/dtls.c +++ b/src/dtls.c @@ -114,6 +114,7 @@ int DtlsIgnoreError(int err) case SOCKET_ERROR_E: case WANT_READ: case WANT_WRITE: + case COOKIE_ERROR: return 0; default: return 1; @@ -207,6 +208,13 @@ static int CreateDtls12Cookie(const WOLFSSL* ssl, const WolfSSL_CH* ch, { int ret; Hmac cookieHmac; + + if (ssl->buffers.dtlsCookieSecret.buffer == NULL || + ssl->buffers.dtlsCookieSecret.length == 0) { + WOLFSSL_MSG("Missing DTLS 1.2 cookie secret"); + return COOKIE_ERROR; + } + ret = wc_HmacInit(&cookieHmac, ssl->heap, ssl->devId); if (ret == 0) { ret = wc_HmacSetKey(&cookieHmac, DTLS_COOKIE_TYPE, diff --git a/src/internal.c b/src/internal.c index c61db68e1..c123c8cec 100644 --- a/src/internal.c +++ b/src/internal.c @@ -7456,15 +7456,14 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) #if defined(WOLFSSL_DTLS) && !defined(NO_WOLFSSL_SERVER) if (ssl->options.dtls && ssl->options.side == WOLFSSL_SERVER_END) { - if (!IsAtLeastTLSv1_3(ssl->version)) { - ret = wolfSSL_DTLS_SetCookieSecret(ssl, NULL, 0); - if (ret != 0) { - WOLFSSL_MSG("DTLS Cookie Secret error"); - return ret; - } + /* Initialize both in case we allow downgrading. */ + ret = wolfSSL_DTLS_SetCookieSecret(ssl, NULL, 0); + if (ret != 0) { + WOLFSSL_MSG("DTLS Cookie Secret error"); + return ret; } #if defined(WOLFSSL_DTLS13) && defined(WOLFSSL_SEND_HRR_COOKIE) - else { + if (IsAtLeastTLSv1_3(ssl->version)) { ret = wolfSSL_send_hrr_cookie(ssl, NULL, 0); if (ret != WOLFSSL_SUCCESS) { WOLFSSL_MSG("DTLS1.3 Cookie secret error"); diff --git a/src/tls13.c b/src/tls13.c index 2f5b9071c..8a84fff81 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -3546,6 +3546,12 @@ int CreateCookieExt(const WOLFSSL* ssl, byte* hash, word16 hashSz, return BAD_FUNC_ARG; } + if (ssl->buffers.tls13CookieSecret.buffer == NULL || + ssl->buffers.tls13CookieSecret.length == 0) { + WOLFSSL_MSG("Missing DTLS 1.3 cookie secret"); + return COOKIE_ERROR; + } + /* Cookie Data = Hash Len | Hash | CS | KeyShare Group */ cookie[cookieSz++] = (byte)hashSz; XMEMCPY(cookie + cookieSz, hash, hashSz); @@ -4693,7 +4699,7 @@ int SendTls13ClientHello(WOLFSSL* ssl) } #if defined(WOLFSSL_DTLS13) && !defined(WOLFSSL_NO_CLIENT) -static int Dtls13DoDowngrade(WOLFSSL* ssl) +static int Dtls13ClientDoDowngrade(WOLFSSL* ssl) { int ret; if (ssl->dtls13ClientHello == NULL) @@ -5099,7 +5105,7 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, if (ssl->options.dtls) { ssl->chVersion.minor = DTLSv1_2_MINOR; ssl->version.minor = DTLSv1_2_MINOR; - ret = Dtls13DoDowngrade(ssl); + ret = Dtls13ClientDoDowngrade(ssl); if (ret != 0) return ret; } @@ -5193,7 +5199,7 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, if (ssl->options.dtls) { ssl->chVersion.minor = DTLSv1_2_MINOR; ssl->version.minor = DTLSv1_2_MINOR; - ret = Dtls13DoDowngrade(ssl); + ret = Dtls13ClientDoDowngrade(ssl); if (ret != 0) return ret; } @@ -5266,7 +5272,7 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #ifdef WOLFSSL_DTLS13 if (ssl->options.dtls) { - ret = Dtls13DoDowngrade(ssl); + ret = Dtls13ClientDoDowngrade(ssl); if (ret != 0) return ret; } @@ -6321,6 +6327,12 @@ int TlsCheckCookie(const WOLFSSL* ssl, const byte* cookie, word16 cookieSz) byte cookieType = 0; byte macSz = 0; + if (ssl->buffers.tls13CookieSecret.buffer == NULL || + ssl->buffers.tls13CookieSecret.length == 0) { + WOLFSSL_MSG("Missing DTLS 1.3 cookie secret"); + return COOKIE_ERROR; + } + #if !defined(NO_SHA) && defined(NO_SHA256) cookieType = SHA; macSz = WC_SHA_DIGEST_SIZE; @@ -6695,6 +6707,7 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, * wolfSSL_accept_TLSv13 when changing this one. */ if (IsDtlsNotSctpMode(ssl) && ssl->options.sendCookie && !ssl->options.dtlsStateful) { + DtlsSetSeqNumForReply(ssl); ret = DoClientHelloStateless(ssl, input + *inOutIdx, helloSz, 0, NULL); if (ret != 0 || !ssl->options.dtlsStateful) { *inOutIdx += helloSz; diff --git a/tests/api.c b/tests/api.c index ea2f55705..5341123ff 100644 --- a/tests/api.c +++ b/tests/api.c @@ -68547,6 +68547,81 @@ static int test_dtls_dropped_ccs(void) #endif return EXPECT_RESULT(); } + +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_DTLS) \ + && !defined(WOLFSSL_NO_TLS12) +static int test_dtls_seq_num_downgrade_check_num(byte* ioBuf, int ioBufLen, + byte seq_num) +{ + EXPECT_DECLS; + DtlsRecordLayerHeader* dtlsRH; + byte sequence_number[8]; + + XMEMSET(&sequence_number, 0, sizeof(sequence_number)); + + ExpectIntGE(ioBufLen, sizeof(*dtlsRH)); + dtlsRH = (DtlsRecordLayerHeader*)ioBuf; + ExpectIntEQ(dtlsRH->type, handshake); + ExpectIntEQ(dtlsRH->pvMajor, DTLS_MAJOR); + ExpectIntEQ(dtlsRH->pvMinor, DTLSv1_2_MINOR); + sequence_number[7] = seq_num; + ExpectIntEQ(XMEMCMP(sequence_number, dtlsRH->sequence_number, + sizeof(sequence_number)), 0); + + return EXPECT_RESULT(); +} +#endif + +/* + * Make sure that we send the correct sequence number after a HelloVerifyRequest + * and after a HelloRetryRequest. This is testing the server side as it is + * operating statelessly and should copy the sequence number of the ClientHello. + */ +static int test_dtls_seq_num_downgrade(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_DTLS) \ + && !defined(WOLFSSL_NO_TLS12) + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + struct test_memio_ctx test_ctx; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfDTLSv1_2_client_method, wolfDTLS_server_method), 0); + + /* CH1 */ + ExpectIntEQ(wolfSSL_negotiate(ssl_c), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + ExpectIntEQ(test_dtls_seq_num_downgrade_check_num(test_ctx.s_buff, + test_ctx.s_len, 0), TEST_SUCCESS); + /* HVR */ + ExpectIntEQ(wolfSSL_negotiate(ssl_s), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ); + ExpectIntEQ(test_dtls_seq_num_downgrade_check_num(test_ctx.c_buff, + test_ctx.c_len, 0), TEST_SUCCESS); + /* CH2 */ + ExpectIntEQ(wolfSSL_negotiate(ssl_c), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + ExpectIntEQ(test_dtls_seq_num_downgrade_check_num(test_ctx.s_buff, + test_ctx.s_len, 1), TEST_SUCCESS); + /* Server first flight */ + ExpectIntEQ(wolfSSL_negotiate(ssl_s), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ); + ExpectIntEQ(test_dtls_seq_num_downgrade_check_num(test_ctx.c_buff, + test_ctx.c_len, 1), TEST_SUCCESS); + + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + + wolfSSL_free(ssl_c); + wolfSSL_CTX_free(ctx_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + /** * Make sure we don't send RSA Signature Hash Algorithms in the * CertificateRequest when we don't have any such ciphers set. @@ -70649,6 +70724,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_dtls_client_hello_timeout_downgrade), TEST_DECL(test_dtls_client_hello_timeout), TEST_DECL(test_dtls_dropped_ccs), + TEST_DECL(test_dtls_seq_num_downgrade), TEST_DECL(test_certreq_sighash_algos), TEST_DECL(test_revoked_loaded_int_cert), TEST_DECL(test_dtls_frag_ch),