From 2847cbfbad41246f173757fd376d3361cd991335 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Tue, 22 Oct 2024 10:55:15 +0200 Subject: [PATCH 1/3] Simplify TLS 1.2 session ID logic Optimize entropy use. Only generate the exact amount of random data that we will actually keep. Refactor done as part of work on ZD18822 --- src/internal.c | 87 ++++++++++++++++++++++++-------------------------- 1 file changed, 42 insertions(+), 45 deletions(-) diff --git a/src/internal.c b/src/internal.c index 005075f88d..8bc4bc5a43 100644 --- a/src/internal.c +++ b/src/internal.c @@ -34510,6 +34510,29 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #ifndef WOLFSSL_NO_TLS12 + static int getSessionID(WOLFSSL* ssl) + { + int sessIdSz = 0; + (void)ssl; +#ifndef NO_SESSION_CACHE + /* if no session cache don't send a session ID */ + if (!ssl->options.sessionCacheOff) + sessIdSz = ID_LEN; +#endif +#ifdef HAVE_SESSION_TICKET + /* we may be echoing an ID as part of session tickets */ + if (ssl->options.useTicket) { + /* echo session id sz can be 0,32 or bogus len in between */ + sessIdSz = ssl->arrays->sessionIDSz; + if (sessIdSz > ID_LEN) { + WOLFSSL_MSG("Bad bogus session id len"); + return BUFFER_ERROR; + } + } +#endif /* HAVE_SESSION_TICKET */ + return sessIdSz; + } + /* handle generation of server_hello (2) */ int SendServerHello(WOLFSSL* ssl) { @@ -34518,17 +34541,18 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, word16 length; word32 idx = RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ; int sendSz; - byte sessIdSz = ID_LEN; - #if defined(HAVE_TLS_EXTENSIONS) && defined(HAVE_SESSION_TICKET) - byte echoId = 0; /* ticket echo id flag */ - #endif - byte cacheOff = 0; /* session cache off flag */ + byte sessIdSz; WOLFSSL_START(WC_FUNC_SERVER_HELLO_SEND); WOLFSSL_ENTER("SendServerHello"); + ret = getSessionID(ssl); + if (ret < 0) + return ret; + sessIdSz = (byte)ret; + length = VERSION_SZ + RAN_LEN - + ID_LEN + ENUM_LEN + + ENUM_LEN + sessIdSz + SUITE_LEN + ENUM_LEN; @@ -34536,45 +34560,12 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, ret = TLSX_GetResponseSize(ssl, server_hello, &length); if (ret != 0) return ret; - #ifdef HAVE_SESSION_TICKET - if (ssl->options.useTicket) { - /* echo session id sz can be 0,32 or bogus len in between */ - sessIdSz = ssl->arrays->sessionIDSz; - if (sessIdSz > ID_LEN) { - WOLFSSL_MSG("Bad bogus session id len"); - return BUFFER_ERROR; - } - if (!IsAtLeastTLSv1_3(ssl->version)) - length -= (ID_LEN - sessIdSz); /* adjust ID_LEN assumption */ - echoId = 1; - } - #endif /* HAVE_SESSION_TICKET */ #else if (ssl->options.haveEMS) { length += HELLO_EXT_SZ_SZ + HELLO_EXT_SZ; } #endif - /* is the session cache off at build or runtime */ -#ifdef NO_SESSION_CACHE - cacheOff = 1; -#else - if (ssl->options.sessionCacheOff == 1) { - cacheOff = 1; - } -#endif - - /* if no session cache don't send a session ID unless we're echoing - * an ID as part of session tickets */ - if (cacheOff == 1 - #if defined(HAVE_TLS_EXTENSIONS) && defined(HAVE_SESSION_TICKET) - && echoId == 0 - #endif - ) { - length -= ID_LEN; /* adjust ID_LEN assumption */ - sessIdSz = 0; - } - sendSz = length + HANDSHAKE_HEADER_SZ + RECORD_HEADER_SZ; #ifdef WOLFSSL_DTLS if (ssl->options.dtls) { @@ -34605,11 +34596,7 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, /* then random and session id */ if (!ssl->options.resuming) { - /* generate random part and session id */ - ret = wc_RNG_GenerateBlock(ssl->rng, output + idx, - RAN_LEN + sizeof(sessIdSz) + sessIdSz); - if (ret != 0) - return ret; + word32 genRanLen = RAN_LEN; #ifdef WOLFSSL_TLS13 if (TLSv1_3_Capable(ssl)) { @@ -34617,6 +34604,7 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, XMEMCPY(output + idx + RAN_LEN - (TLS13_DOWNGRADE_SZ + 1), tls13Downgrade, TLS13_DOWNGRADE_SZ); output[idx + RAN_LEN - 1] = (byte)IsAtLeastTLSv1_2(ssl); + genRanLen -= TLS13_DOWNGRADE_SZ + 1; } else #endif @@ -34628,12 +34616,21 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, XMEMCPY(output + idx + RAN_LEN - (TLS13_DOWNGRADE_SZ + 1), tls13Downgrade, TLS13_DOWNGRADE_SZ); output[idx + RAN_LEN - 1] = 0; + genRanLen -= TLS13_DOWNGRADE_SZ + 1; } - /* store info in SSL for later */ + /* generate random part */ + ret = wc_RNG_GenerateBlock(ssl->rng, output + idx, genRanLen); + if (ret != 0) + return ret; XMEMCPY(ssl->arrays->serverRandom, output + idx, RAN_LEN); idx += RAN_LEN; + + /* generate session id */ output[idx++] = sessIdSz; + ret = wc_RNG_GenerateBlock(ssl->rng, output + idx, sessIdSz); + if (ret != 0) + return ret; XMEMCPY(ssl->arrays->sessionID, output + idx, sessIdSz); ssl->arrays->sessionIDSz = sessIdSz; } From 031656ee7a40bc82bbd705b39fb878148348dfd9 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Wed, 23 Oct 2024 12:32:10 +0200 Subject: [PATCH 2/3] Send a new ticket when rejecting a ticket and tickets enabled --- src/tls.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/tls.c b/src/tls.c index 48161c6da8..8441acf522 100644 --- a/src/tls.c +++ b/src/tls.c @@ -5905,14 +5905,25 @@ static int TLSX_SessionTicket_Parse(WOLFSSL* ssl, const byte* input, /* SERVER: ticket is peer auth. */ ssl->options.peerAuthGood = 1; } - } else if (ret == WOLFSSL_TICKET_RET_REJECT) { + } else if (ret == WOLFSSL_TICKET_RET_REJECT || + ret == WC_NO_ERR_TRACE(VERSION_ERROR)) { WOLFSSL_MSG("Process client ticket rejected, not using"); - ssl->options.rejectTicket = 1; + if (ret == WC_NO_ERR_TRACE(VERSION_ERROR)) + WOLFSSL_MSG("\tbad TLS version"); ret = 0; /* not fatal */ - } else if (ret == WC_NO_ERR_TRACE(VERSION_ERROR)) { - WOLFSSL_MSG("Process client ticket rejected, bad TLS version"); + ssl->options.rejectTicket = 1; - ret = 0; /* not fatal */ + /* If we have session tickets enabled then send a new ticket */ + if (!TLSX_CheckUnsupportedExtension(ssl, TLSX_SESSION_TICKET)) { + ret = TLSX_UseSessionTicket(&ssl->extensions, NULL, + ssl->heap); + if (ret == WOLFSSL_SUCCESS) { + ret = 0; + TLSX_SetResponse(ssl, TLSX_SESSION_TICKET); + ssl->options.createTicket = 1; + ssl->options.useTicket = 1; + } + } } else if (ret == WOLFSSL_TICKET_RET_FATAL) { WOLFSSL_MSG("Process client ticket fatal error, not using"); } else if (ret < 0) { From 25e32c2539b480d2b7595b06afed1e75d9f69ca2 Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 23 Oct 2024 14:53:00 -0700 Subject: [PATCH 3/3] Fix for TLS v1.2 session resumption with tickets where the server decides to do a full handshake. The wrong sessionIDSz was being checked and should be the arrays one since it get set from the server_hello. --- src/internal.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index 8bc4bc5a43..a152022bb8 100644 --- a/src/internal.c +++ b/src/internal.c @@ -17471,6 +17471,18 @@ int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, case certificate_request: case server_hello_done: if (ssl->options.resuming) { + /* Client requested resumption, but server is doing a + * full handshake */ + + /* The server's decision to resume isn't known until after the + * "server_hello". If subsequent handshake messages like + * "certificate" or "server_key_exchange" are recevied then we + * are doing a full handshake */ + + /* If the server included a session id then we + * treat this as a fatal error, since the server said it was + * doing resumption, but did not. */ + /* https://www.rfc-editor.org/rfc/rfc5077.html#section-3.4 * Alternatively, the client MAY include an empty Session ID * in the ClientHello. In this case, the client ignores the @@ -17479,7 +17491,7 @@ int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, * messages. */ #ifndef WOLFSSL_WPAS - if (ssl->session->sessionIDSz != 0) { + if (ssl->arrays->sessionIDSz != 0) { /* Fatal error. Only try to send an alert. RFC 5246 does not * allow for reverting back to a full handshake after the * server has indicated the intention to do a resumption. */