From 617eda9d449a3755e9791ae8a8ce26ff43a7304b Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Mon, 14 Feb 2022 19:13:08 +0100 Subject: [PATCH] Fix misc memory issues - Make `InternalTicket` memory alignment independent --- configure.ac | 2 +- src/internal.c | 86 ++++++++++++++++++++-------------------------- src/ssl.c | 27 +++++++++++---- wolfssl/internal.h | 8 +++++ 4 files changed, 68 insertions(+), 55 deletions(-) diff --git a/configure.ac b/configure.ac index b7d5e9cb5..38d953982 100644 --- a/configure.ac +++ b/configure.ac @@ -6829,7 +6829,7 @@ AS_CASE(["$CFLAGS $CPPFLAGS $AM_CFLAGS"],[*'OPENSSL_COMPATIBLE_DEFAULTS'*], [ENABLED_OPENSSL_COMPATIBLE_DEFAULTS=yes]) if test "x$ENABLED_OPENSSL_COMPATIBLE_DEFAULTS" = "xyes" then - AM_CFLAGS="$AM_CFLAGS -DWOLFSSL_TRUST_PEER_CERT -DWOLFSSL_TLS13_MIDDLEBOX_COMPAT" + AM_CFLAGS="$AM_CFLAGS -DWOLFSSL_TRUST_PEER_CERT" AM_CFLAGS="$AM_CFLAGS -DNO_SESSION_CACHE_REF" ENABLED_TRUSTED_PEER_CERT=yes fi diff --git a/src/internal.c b/src/internal.c index ba8291d76..327ce501b 100644 --- a/src/internal.c +++ b/src/internal.c @@ -20206,12 +20206,8 @@ const char* wolfSSL_ERR_reason_error_string(unsigned long e) case SIDE_ERROR : return "wrong client/server type"; - case NO_PEER_CERT : -#ifndef OPENSSL_EXTRA - return "peer didn't send cert"; -#else + case NO_PEER_CERT : /* OpenSSL compatibility expects this exact text */ return "peer did not return a certificate"; -#endif case UNKNOWN_HANDSHAKE_TYPE : return "weird handshake type"; @@ -20525,12 +20521,8 @@ const char* wolfSSL_ERR_reason_error_string(unsigned long e) case MISSING_HANDSHAKE_DATA: return "The handshake message is missing required data"; - case BAD_BINDER: -#ifndef OPENSSL_EXTRA - return "Binder value does not match value server calculated"; -#else + case BAD_BINDER: /* OpenSSL compatibility expects this exact text */ return "binder does not verify"; -#endif case EXT_NOT_ALLOWED: return "Extension type not allowed in handshake message type"; @@ -29955,15 +29947,13 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #endif #endif - ret = MatchSuite(ssl, &clSuites); #ifdef OPENSSL_EXTRA /* Give user last chance to provide a cert for cipher selection */ - if (ret == 0) - ret = CertSetupCbWrapper(ssl); - /* Call again in case user changes affect cipher selection */ if (ret == 0 && ssl->ctx->certSetupCb != NULL) - ret = MatchSuite(ssl, &clSuites); + ret = CertSetupCbWrapper(ssl); #endif + if (ret == 0) + ret = MatchSuite(ssl, &clSuites); #ifdef WOLFSSL_EXTRA_ALERTS if (ret == BUFFER_ERROR) @@ -30504,19 +30494,21 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, WOLFSSL_TICKET_IV_SZ + WOLFSSL_TICKET_MAC_SZ + LENGTH_SZ) #define WOLFSSL_TICKET_ENC_SZ (SESSION_TICKET_LEN - WOLFSSL_TICKET_FIXED_SZ) - /* our ticket format */ + /* Our ticket format. All members need to be a byte or array of byte to + * avoid alignment issues */ typedef struct InternalTicket { ProtocolVersion pv; /* version when ticket created */ byte suite[SUITE_LEN]; /* cipher suite when created */ byte msecret[SECRET_LEN]; /* master secret */ - word32 timestamp; /* born on */ - word16 haveEMS; /* have extended master secret */ + byte timestamp[TIMESTAMP_LEN]; /* born on */ + byte haveEMS; /* have extended master secret */ #ifdef WOLFSSL_TLS13 - word32 ageAdd; /* Obfuscation of age */ - word16 namedGroup; /* Named group used */ + byte ageAdd[AGEADD_LEN]; /* Obfuscation of age */ + byte namedGroup[NAMEDGREOUP_LEN]; /* Named group used */ TicketNonce ticketNonce; /* Ticket nonce */ #ifdef WOLFSSL_EARLY_DATA - word32 maxEarlyDataSz; /* Max size of early data */ + byte maxEarlyDataSz[MAXEARLYDATASZ_LEN]; /* Max size of + * early data */ #endif #endif #ifdef WOLFSSL_TICKET_HAVE_ID @@ -30581,7 +30573,7 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, it.suite[1] = ssl->options.cipherSuite; #ifdef WOLFSSL_EARLY_DATA - it.maxEarlyDataSz = ssl->options.maxEarlyDataSz; + c32toa(ssl->options.maxEarlyDataSz, it.maxEarlyDataSz); #endif if (!ssl->options.tls1_3) { @@ -30598,9 +30590,9 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, sizeof(it.ageAdd)); if (ret != 0) return BAD_TICKET_ENCRYPT; - ssl->session->ticketAdd = it.ageAdd; - it.namedGroup = ssl->session->namedGroup; - it.timestamp = TimeNowInMilliseconds(); + ato32(it.ageAdd, &ssl->session->ticketAdd); + c16toa(ssl->session->namedGroup, it.namedGroup); + c32toa(TimeNowInMilliseconds(), it.timestamp); /* Resumption master secret. */ XMEMCPY(it.msecret, ssl->session->masterSecret, SECRET_LEN); XMEMCPY(&it.ticketNonce, &ssl->session->ticketNonce, @@ -30721,7 +30713,7 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, int DoClientTicket(WOLFSSL* ssl, const byte* input, word32 len) { ExternalTicket* et; - InternalTicket it; + InternalTicket* it; int ret; int outLen; word16 inLen; @@ -30767,19 +30759,17 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, return BAD_TICKET_KEY_CB_SZ; } - /* copy the decrypted ticket to avoid alignment issues */ - XMEMCPY(&it, et->enc_ticket, sizeof(InternalTicket)); - ForceZero(et->enc_ticket, sizeof(it)); + it = (InternalTicket*)et->enc_ticket; /* get master secret */ if (ret == WOLFSSL_TICKET_RET_OK || ret == WOLFSSL_TICKET_RET_CREATE) { - if (ssl->version.minor < it.pv.minor) { + if (ssl->version.minor < it->pv.minor) { ForceZero(&it, sizeof(it)); WOLFSSL_MSG("Ticket has greater version"); return VERSION_ERROR; } - else if (ssl->version.minor > it.pv.minor) { - if (IsAtLeastTLSv1_3(it.pv) != IsAtLeastTLSv1_3(ssl->version)) { + else if (ssl->version.minor > it->pv.minor) { + if (IsAtLeastTLSv1_3(it->pv) != IsAtLeastTLSv1_3(ssl->version)) { ForceZero(&it, sizeof(it)); WOLFSSL_MSG("Tickets cannot be shared between " "TLS 1.3 and TLS 1.2 and lower"); @@ -30794,17 +30784,17 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, WOLFSSL_MSG("Downgrading protocol due to ticket"); - if (it.pv.minor < ssl->options.minDowngrade) { + if (it->pv.minor < ssl->options.minDowngrade) { ForceZero(&it, sizeof(it)); return VERSION_ERROR; } - ssl->version.minor = it.pv.minor; + ssl->version.minor = it->pv.minor; } #ifdef WOLFSSL_TICKET_HAVE_ID { ssl->session->haveAltSessionID = 1; - XMEMCPY(ssl->session->altSessionID, it.id, ID_LEN); + XMEMCPY(ssl->session->altSessionID, it->id, ID_LEN); if (wolfSSL_GetSession(ssl, NULL, 1) != NULL) { WOLFSSL_MSG("Found session matching the session id" " found in the ticket"); @@ -30817,31 +30807,31 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #endif if (!IsAtLeastTLSv1_3(ssl->version)) { - XMEMCPY(ssl->arrays->masterSecret, it.msecret, SECRET_LEN); + XMEMCPY(ssl->arrays->masterSecret, it->msecret, SECRET_LEN); /* Copy the haveExtendedMasterSecret property from the ticket to * the saved session, so the property may be checked later. */ - ssl->session->haveEMS = it.haveEMS; - ato32((const byte*)&it.timestamp, &ssl->session->bornOn); + ssl->session->haveEMS = it->haveEMS; + ato32((const byte*)&it->timestamp, &ssl->session->bornOn); #ifndef NO_RESUME_SUITE_CHECK - ssl->session->cipherSuite0 = it.suite[0]; - ssl->session->cipherSuite = it.suite[1]; + ssl->session->cipherSuite0 = it->suite[0]; + ssl->session->cipherSuite = it->suite[1]; #endif } else { #ifdef WOLFSSL_TLS13 /* Restore information to renegotiate. */ - ssl->session->ticketSeen = it.timestamp; - ssl->session->ticketAdd = it.ageAdd; - ssl->session->cipherSuite0 = it.suite[0]; - ssl->session->cipherSuite = it.suite[1]; + ato32(it->timestamp, &ssl->session->ticketSeen); + ato32(it->ageAdd, &ssl->session->ticketAdd); + ssl->session->cipherSuite0 = it->suite[0]; + ssl->session->cipherSuite = it->suite[1]; #ifdef WOLFSSL_EARLY_DATA - ssl->session->maxEarlyDataSz = it.maxEarlyDataSz; + ato32(it->maxEarlyDataSz, &ssl->session->maxEarlyDataSz); #endif /* Resumption master secret. */ - XMEMCPY(ssl->session->masterSecret, it.msecret, SECRET_LEN); - XMEMCPY(&ssl->session->ticketNonce, &it.ticketNonce, + XMEMCPY(ssl->session->masterSecret, it->msecret, SECRET_LEN); + XMEMCPY(&ssl->session->ticketNonce, &it->ticketNonce, sizeof(TicketNonce)); - ssl->session->namedGroup = it.namedGroup; + ato16(it->namedGroup, &ssl->session->namedGroup); #endif } } diff --git a/src/ssl.c b/src/ssl.c index 40eadf10d..ebe0b3ac8 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -15980,6 +15980,8 @@ int AddSessionToCache(WOLFSSL_SESSION* addSession, const byte* id, byte idSz, WOLFSSL_X509* peer = NULL; #endif #ifdef HAVE_SESSION_TICKET + byte* cacheTicBuff = NULL; + byte ticBuffUsed = 0; byte* ticBuff = NULL; int ticLen = 0; #endif @@ -16004,7 +16006,7 @@ int AddSessionToCache(WOLFSSL_SESSION* addSession, const byte* id, byte idSz, ticLen = addSession->ticketLen; /* Alloc Memory here to avoid syscalls during lock */ if (ticLen > SESSION_TICKET_LEN) { - ticBuff = (byte*)XMALLOC(ticLen, addSession->heap, + ticBuff = (byte*)XMALLOC(ticLen, NULL, DYNAMIC_TYPE_SESSION_TICK); if (ticBuff == NULL) { return MEMORY_E; @@ -16016,7 +16018,7 @@ int AddSessionToCache(WOLFSSL_SESSION* addSession, const byte* id, byte idSz, if (ret != 0) { WOLFSSL_MSG("Hash session failed"); #ifdef HAVE_SESSION_TICKET - XFREE(ticBuff, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + XFREE(ticBuff, NULL, DYNAMIC_TYPE_SESSION_TICK); #endif return ret; } @@ -16024,7 +16026,7 @@ int AddSessionToCache(WOLFSSL_SESSION* addSession, const byte* id, byte idSz, sessRow = &SessionCache[row]; if (SESSION_ROW_LOCK(sessRow) != 0) { #ifdef HAVE_SESSION_TICKET - XFREE(ticBuff, addSession->heap, DYNAMIC_TYPE_SESSION_TICK); + XFREE(ticBuff, NULL, DYNAMIC_TYPE_SESSION_TICK); #endif WOLFSSL_MSG("Session row lock failed"); return BAD_MUTEX_E; @@ -16058,7 +16060,12 @@ int AddSessionToCache(WOLFSSL_SESSION* addSession, const byte* id, byte idSz, cacheSession->peer = NULL; #endif #ifdef HAVE_SESSION_TICKET - if (ticBuff != NULL) { + /* If we can re-use the existing buffer in cacheSession then we won't touch + * ticBuff at all making it a very cheap malloc/free. The page on a modern + * OS will most likely not even be allocated to the process. */ + if (ticBuff != NULL && cacheSession->ticketLenAlloc < ticLen) { + cacheTicBuff = cacheSession->ticket; + ticBuffUsed = 1; cacheSession->ticket = ticBuff; cacheSession->ticketLenAlloc = ticLen; } @@ -16073,6 +16080,7 @@ int AddSessionToCache(WOLFSSL_SESSION* addSession, const byte* id, byte idSz, sizeof(x509_buffer) * cacheSession->chain.count); } #endif /* SESSION_CERTS */ + cacheSession->heap = NULL; /* Copy data into the cache object */ ret = wolfSSL_DupSession(addSession, cacheSession, 1) == WOLFSSL_FAILURE; @@ -16089,7 +16097,7 @@ int AddSessionToCache(WOLFSSL_SESSION* addSession, const byte* id, byte idSz, } } #ifdef HAVE_SESSION_TICKET - else if (ticBuff != NULL) { + else if (ticBuffUsed) { /* Error occured. Need to clean up the ticket buffer. */ cacheSession->ticket = cacheSession->_staticTicket; cacheSession->ticketLenAlloc = 0; @@ -16109,6 +16117,13 @@ int AddSessionToCache(WOLFSSL_SESSION* addSession, const byte* id, byte idSz, } #endif +#ifdef HAVE_SESSION_TICKET + if (ticBuff != NULL && !ticBuffUsed) + XFREE(ticBuff, NULL, DYNAMIC_TYPE_SESSION_TICK); + if (cacheTicBuff != NULL) + XFREE(cacheTicBuff, NULL, DYNAMIC_TYPE_SESSION_TICK); +#endif + #if defined(SESSION_CERTS) && defined(OPENSSL_EXTRA) if (peer != NULL) { wolfSSL_X509_free(peer); @@ -16203,7 +16218,7 @@ void AddSession(WOLFSSL* ssl) WC_RNG* rng = NULL; if (ssl->rng != NULL) rng = ssl->rng; -#ifdef HAVE_GLOBAL_RNG +#if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA) else if (initGlobalRNG == 1 || wolfSSL_RAND_Init() == WOLFSSL_SUCCESS) { rng = &globalRNG; } diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 56a501126..fa0b911f6 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1185,6 +1185,14 @@ enum Misc { HELLO_EXT_EXTMS = 0x0017, /* ID for the extended master secret ext */ SECRET_LEN = WOLFSSL_MAX_MASTER_KEY_LENGTH, /* pre RSA and all master */ + TIMESTAMP_LEN = 4, /* timestamp size in ticket */ +#ifdef WOLFSSL_TLS13 + AGEADD_LEN = 4, /* ageAdd size in ticket */ + NAMEDGREOUP_LEN = 2, /* namedGroup size in ticket */ +#ifdef WOLFSSL_EARLY_DATA + MAXEARLYDATASZ_LEN = 4, /* maxEarlyDataSz size in ticket */ +#endif +#endif #ifdef HAVE_PQC ENCRYPT_LEN = 1500, /* allow 1500 bit static buffer for falcon */ #else