From c8b20d90906f1a4bfe43048af0730df92cb4d693 Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Fri, 19 Feb 2016 10:10:32 -0700 Subject: [PATCH 1/9] Add support for dynamic session tickets, add openssl.test to testuiste --- scripts/include.am | 2 +- scripts/openssl.test | 45 +++++++++++++++++++++++++++++++++------ src/internal.c | 40 ++++++++++++++++++++++++++++------ src/ssl.c | 36 +++++++++++++++++++++++++++---- src/tls.c | 3 +++ wolfssl/internal.h | 2 ++ wolfssl/wolfcrypt/types.h | 3 ++- 7 files changed, 112 insertions(+), 19 deletions(-) diff --git a/scripts/include.am b/scripts/include.am index 0e1bffe52..1701ad97e 100644 --- a/scripts/include.am +++ b/scripts/include.am @@ -53,7 +53,7 @@ if BUILD_EXAMPLE_CLIENTS if !BUILD_IPV6 dist_noinst_SCRIPTS+= scripts/external.test dist_noinst_SCRIPTS+= scripts/google.test -#dist_noinst_SCRIPTS+= scripts/openssl.test +dist_noinst_SCRIPTS+= scripts/openssl.test endif endif diff --git a/scripts/openssl.test b/scripts/openssl.test index 8f068309c..c457c1281 100755 --- a/scripts/openssl.test +++ b/scripts/openssl.test @@ -3,7 +3,15 @@ #openssl.test # need a unique port since may run the same time as testsuite -openssl_port=11114 +generate_port() { + openssl_port=`tr -cd 0-9 /dev/null + then + echo "s_server started successfully on port $openssl_port" + found_free_port=1 + break + else + #port already started, try a different port + counter=$((counter+ 1)) + generate_port + fi +done + +if [ $found_free_port = 0 ] +then + echo -e "Couldn't find free port for server" + do_cleanup + exit 1 +fi # get wolfssl ciphers wolf_ciphers=`./examples/client/client -e` @@ -99,7 +130,7 @@ if [ $server_ready = 0 ] then echo -e "Couldn't verify openssl server is running, timeout error" do_cleanup - exit -1 + exit 1 fi OIFS=$IFS # store old seperator to reset diff --git a/src/internal.c b/src/internal.c index 06fa29237..4bfe3ffbc 100755 --- a/src/internal.c +++ b/src/internal.c @@ -2648,6 +2648,10 @@ void SSL_ResourceFree(WOLFSSL* ssl) #if defined(KEEP_PEER_CERT) || defined(GOAHEAD_WS) FreeX509(&ssl->peerCert); #endif +#ifdef HAVE_SESSION_TICKET + if (ssl->session.dynTicket) + XFREE(ssl->session.dynTicket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); +#endif } #ifdef WOLFSSL_TI_HASH @@ -11349,9 +11353,14 @@ static void PickHashSigAlgo(WOLFSSL* ssl, #ifdef HAVE_SESSION_TICKET if (ssl->options.resuming && ssl->session.ticketLen > 0) { SessionTicket* ticket; + byte* ticketData; + + ticketData = ssl->session.isDynamic ? + ssl->session.dynTicket : + ssl->session.ticket; ticket = TLSX_SessionTicket_Create(0, - ssl->session.ticket, ssl->session.ticketLen); + ticketData, ssl->session.ticketLen); if (ticket == NULL) return MEMORY_E; ret = TLSX_UseSessionTicket(&ssl->extensions, ticket); @@ -14285,8 +14294,16 @@ int DoSessionTicket(WOLFSSL* ssl, ato16(input + *inOutIdx, &length); *inOutIdx += OPAQUE16_LEN; - if (length > sizeof(ssl->session.ticket)) - return SESSION_TICKET_LEN_E; + if (length > sizeof(ssl->session.ticket)) { + ssl->session.isDynamic = 1; + + ssl->session.dynTicket = (byte*)XMALLOC( + length, ssl->heap, + DYNAMIC_TYPE_SESSION_TICK); + if (ssl->session.dynTicket == NULL) { + return MEMORY_E; + } + } if ((*inOutIdx - begin) + length > size) return BUFFER_ERROR; @@ -14294,7 +14311,11 @@ int DoSessionTicket(WOLFSSL* ssl, /* If the received ticket including its length is greater than * a length value, the save it. Otherwise, don't save it. */ if (length > 0) { - XMEMCPY(ssl->session.ticket, input + *inOutIdx, length); + if (ssl->session.isDynamic) + XMEMCPY(ssl->session.dynTicket, input + *inOutIdx, length); + else + XMEMCPY(ssl->session.ticket, input + *inOutIdx, length); + *inOutIdx += length; ssl->session.ticketLen = length; ssl->timeout = lifetime; @@ -14305,7 +14326,12 @@ int DoSessionTicket(WOLFSSL* ssl, } /* Create a fake sessionID based on the ticket, this will * supercede the existing session cache info. */ - ssl->options.haveSessionId = 1; + ssl->options.haveSessionId = 1; + + if (ssl->session.isDynamic) + XMEMCPY(ssl->arrays->sessionID, + ssl->session.dynTicket + length - ID_LEN, ID_LEN); + else XMEMCPY(ssl->arrays->sessionID, ssl->session.ticket + length - ID_LEN, ID_LEN); #ifndef NO_SESSION_CACHE @@ -16618,7 +16644,9 @@ int DoSessionTicket(WOLFSSL* ssl, static int CreateTicket(WOLFSSL* ssl) { InternalTicket it; - ExternalTicket* et = (ExternalTicket*)ssl->session.ticket; + ExternalTicket* et = ssl->session.isDynamic ? + (ExternalTicket*)ssl->session.dynTicket : + (ExternalTicket*)ssl->session.ticket; int encLen; int ret; byte zeros[WOLFSSL_TICKET_MAC_SZ]; /* biggest cmp size */ diff --git a/src/ssl.c b/src/ssl.c index 1dab84730..8f6ade691 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -1251,7 +1251,10 @@ WOLFSSL_API int wolfSSL_get_SessionTicket(WOLFSSL* ssl, return BAD_FUNC_ARG; if (ssl->session.ticketLen <= *bufSz) { - XMEMCPY(buf, ssl->session.ticket, ssl->session.ticketLen); + if (ssl->session.isDynamic) + XMEMCPY(buf, ssl->session.dynTicket, ssl->session.ticketLen); + else + XMEMCPY(buf, ssl->session.ticket, ssl->session.ticketLen); *bufSz = ssl->session.ticketLen; } else @@ -1262,12 +1265,17 @@ WOLFSSL_API int wolfSSL_get_SessionTicket(WOLFSSL* ssl, WOLFSSL_API int wolfSSL_set_SessionTicket(WOLFSSL* ssl, byte* buf, word32 bufSz) { - if (ssl == NULL || (buf == NULL && bufSz > 0)) + if (ssl == NULL || (buf == NULL && bufSz > 0) || bufSz > SESSION_TICKET_LEN) return BAD_FUNC_ARG; if (bufSz > 0) XMEMCPY(ssl->session.ticket, buf, bufSz); ssl->session.ticketLen = (word16)bufSz; + /* session ticket should only be size of static buffer. Delete dynamic buffer*/ + if (ssl->session.isDynamic) { + XFREE(ssl->session.dynTicket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + ssl->session.isDynamic = 0; + } return SSL_SUCCESS; } @@ -7067,9 +7075,29 @@ int AddSession(WOLFSSL* ssl) SessionCache[row].Sessions[idx].bornOn = LowResTimer(); #ifdef HAVE_SESSION_TICKET - SessionCache[row].Sessions[idx].ticketLen = ssl->session.ticketLen; - XMEMCPY(SessionCache[row].Sessions[idx].ticket, + if (ssl->session.isDynamic) { + if (!SessionCache[row].Sessions[idx].dynTicket) { + SessionCache[row].Sessions[idx].dynTicket = XMALLOC( + ssl->session.ticketLen, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + if (!SessionCache[row].Sessions[idx].dynTicket) + return MEMORY_E; + } else if (SessionCache[row].Sessions[idx].ticketLen < ssl->session.ticketLen) { + XFREE(SessionCache[row].Sessions[idx].dynTicket, + ssl->heap, DYNAMIC_TYPE_SESS_TICK); + SessionCache[row].Sessions[idx].dynTicket = XMALLOC( + ssl->session.ticketLen, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + if (!SessionCache[row].Sessions[idx].dynTicket) + return MEMORY_E; + } + XMEMCPY(SessionCache[row].Sessions[idx].dynTicket, + ssl->session.dynTicket, ssl->session.ticketLen); + SessionCache[row].Sessions[idx].isDynamic = 1; + } + else { + XMEMCPY(SessionCache[row].Sessions[idx].ticket, ssl->session.ticket, ssl->session.ticketLen); + } + SessionCache[row].Sessions[idx].ticketLen = ssl->session.ticketLen; #endif #ifdef SESSION_CERTS diff --git a/src/tls.c b/src/tls.c index 3b6e0a879..482c8f58d 100644 --- a/src/tls.c +++ b/src/tls.c @@ -3212,9 +3212,11 @@ int TLSX_UseSessionTicket(TLSX** extensions, SessionTicket* ticket) #define STK_GET_SIZE TLSX_SessionTicket_GetSize #define STK_WRITE TLSX_SessionTicket_Write #define STK_PARSE TLSX_SessionTicket_Parse +#define STK_FREE(stk) TLSX_SessionTicket_Free((SessionTicket*)stk) #else +#define STK_FREE(a) #define STK_VALIDATE_REQUEST(a) #define STK_GET_SIZE(a, b) 0 #define STK_WRITE(a, b, c) 0 @@ -3865,6 +3867,7 @@ void TLSX_FreeAll(TLSX* list) case TLSX_SESSION_TICKET: /* Nothing to do. */ + STK_FREE(extension->data); break; case TLSX_QUANTUM_SAFE_HYBRID: diff --git a/wolfssl/internal.h b/wolfssl/internal.h index eea8b2908..ca473bede 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -2181,6 +2181,8 @@ struct WOLFSSL_SESSION { #endif #ifdef HAVE_SESSION_TICKET word16 ticketLen; + byte *dynTicket; + byte isDynamic; byte ticket[SESSION_TICKET_LEN]; #endif #ifdef HAVE_STUNNEL diff --git a/wolfssl/wolfcrypt/types.h b/wolfssl/wolfcrypt/types.h index db9897a1d..b8237d6ec 100644 --- a/wolfssl/wolfcrypt/types.h +++ b/wolfssl/wolfcrypt/types.h @@ -302,7 +302,8 @@ DYNAMIC_TYPE_X509_CTX = 53, DYNAMIC_TYPE_URL = 54, DYNAMIC_TYPE_DTLS_FRAG = 55, - DYNAMIC_TYPE_DTLS_BUFFER = 56 + DYNAMIC_TYPE_DTLS_BUFFER = 56, + DYNAMIC_TYPE_SESSION_TICK = 57 }; /* max error buffer string size */ From 0eb59d5c35219c62ae166ce893b47efc6e8d72e3 Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Fri, 11 Mar 2016 09:50:46 -0700 Subject: [PATCH 2/9] Fix rand num generation on MacOS, Improve organization with tic storage --- scripts/openssl.test | 4 +-- src/internal.c | 68 ++++++++++++++++++++++---------------- src/ssl.c | 79 +++++++++++++++++++++++++++----------------- src/tls.c | 1 - wolfssl/internal.h | 5 +-- 5 files changed, 92 insertions(+), 65 deletions(-) diff --git a/scripts/openssl.test b/scripts/openssl.test index c457c1281..f93f95dda 100755 --- a/scripts/openssl.test +++ b/scripts/openssl.test @@ -4,8 +4,8 @@ # need a unique port since may run the same time as testsuite generate_port() { - openssl_port=`tr -cd 0-9 sessionSecretCb = NULL; ssl->sessionSecretCtx = NULL; #endif + +#ifdef HAVE_SESSION_TICKET + ssl->session.ticket = ssl->session.staticTicket; + ssl->session.isDynamic = 0; + ssl->session.dynTicket = NULL; + ssl->session.ticketLen = 0; +#endif return 0; } @@ -2649,8 +2656,12 @@ void SSL_ResourceFree(WOLFSSL* ssl) FreeX509(&ssl->peerCert); #endif #ifdef HAVE_SESSION_TICKET - if (ssl->session.dynTicket) + if (ssl->session.dynTicket) { XFREE(ssl->session.dynTicket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + ssl->session.dynTicket = NULL; + ssl->session.isDynamic = 0; + ssl->session.ticket = ssl->session.staticTicket; + } #endif } @@ -11353,14 +11364,9 @@ static void PickHashSigAlgo(WOLFSSL* ssl, #ifdef HAVE_SESSION_TICKET if (ssl->options.resuming && ssl->session.ticketLen > 0) { SessionTicket* ticket; - byte* ticketData; - - ticketData = ssl->session.isDynamic ? - ssl->session.dynTicket : - ssl->session.ticket; ticket = TLSX_SessionTicket_Create(0, - ticketData, ssl->session.ticketLen); + ssl->session.ticket, ssl->session.ticketLen); if (ticket == NULL) return MEMORY_E; ret = TLSX_UseSessionTicket(&ssl->extensions, ticket); @@ -14294,15 +14300,30 @@ int DoSessionTicket(WOLFSSL* ssl, ato16(input + *inOutIdx, &length); *inOutIdx += OPAQUE16_LEN; - if (length > sizeof(ssl->session.ticket)) { - ssl->session.isDynamic = 1; - - ssl->session.dynTicket = (byte*)XMALLOC( - length, ssl->heap, - DYNAMIC_TYPE_SESSION_TICK); - if (ssl->session.dynTicket == NULL) { - return MEMORY_E; + if (length > sizeof(ssl->session.staticTicket)) { + /* Free old dynamic ticket if we already had one */ + if (ssl->session.dynTicket) { + XFREE(ssl->session.dynTicket, ssl->heap, + DYNAMIC_TYPE_SESSION_TICK); } + + ssl->session.dynTicket = + (byte*)XMALLOC(length, ssl->heap, + DYNAMIC_TYPE_SESSION_TICK); + + if (ssl->session.dynTicket == NULL) + return MEMORY_E; + + ssl->session.isDynamic = 1; + ssl->session.ticket = ssl->session.dynTicket; + } else { + if(ssl->session.dynTicket) { + XFREE(ssl->session.dynTicket, ssl->heap, + DYNAMIC_TYPE_SESSION_TICK); + ssl->session.dynTicket = NULL; + } + ssl->session.isDynamic = 0; + ssl->session.ticket = ssl->session.staticTicket; } if ((*inOutIdx - begin) + length > size) @@ -14311,11 +14332,7 @@ int DoSessionTicket(WOLFSSL* ssl, /* If the received ticket including its length is greater than * a length value, the save it. Otherwise, don't save it. */ if (length > 0) { - if (ssl->session.isDynamic) - XMEMCPY(ssl->session.dynTicket, input + *inOutIdx, length); - else - XMEMCPY(ssl->session.ticket, input + *inOutIdx, length); - + XMEMCPY(ssl->session.ticket, input + *inOutIdx, length); *inOutIdx += length; ssl->session.ticketLen = length; ssl->timeout = lifetime; @@ -14326,12 +14343,7 @@ int DoSessionTicket(WOLFSSL* ssl, } /* Create a fake sessionID based on the ticket, this will * supercede the existing session cache info. */ - ssl->options.haveSessionId = 1; - - if (ssl->session.isDynamic) - XMEMCPY(ssl->arrays->sessionID, - ssl->session.dynTicket + length - ID_LEN, ID_LEN); - else + ssl->options.haveSessionId = 1; XMEMCPY(ssl->arrays->sessionID, ssl->session.ticket + length - ID_LEN, ID_LEN); #ifndef NO_SESSION_CACHE @@ -16644,9 +16656,7 @@ int DoSessionTicket(WOLFSSL* ssl, static int CreateTicket(WOLFSSL* ssl) { InternalTicket it; - ExternalTicket* et = ssl->session.isDynamic ? - (ExternalTicket*)ssl->session.dynTicket : - (ExternalTicket*)ssl->session.ticket; + ExternalTicket* et = (ExternalTicket*)ssl->session.ticket; int encLen; int ret; byte zeros[WOLFSSL_TICKET_MAC_SZ]; /* biggest cmp size */ diff --git a/src/ssl.c b/src/ssl.c index 8f6ade691..986dae3ab 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -1251,10 +1251,7 @@ WOLFSSL_API int wolfSSL_get_SessionTicket(WOLFSSL* ssl, return BAD_FUNC_ARG; if (ssl->session.ticketLen <= *bufSz) { - if (ssl->session.isDynamic) - XMEMCPY(buf, ssl->session.dynTicket, ssl->session.ticketLen); - else - XMEMCPY(buf, ssl->session.ticket, ssl->session.ticketLen); + XMEMCPY(buf, ssl->session.ticket, ssl->session.ticketLen); *bufSz = ssl->session.ticketLen; } else @@ -1268,15 +1265,19 @@ WOLFSSL_API int wolfSSL_set_SessionTicket(WOLFSSL* ssl, byte* buf, word32 bufSz) if (ssl == NULL || (buf == NULL && bufSz > 0) || bufSz > SESSION_TICKET_LEN) return BAD_FUNC_ARG; - if (bufSz > 0) - XMEMCPY(ssl->session.ticket, buf, bufSz); + ssl->session.ticket = ssl->session.staticTicket; ssl->session.ticketLen = (word16)bufSz; - /* session ticket should only be size of static buffer. Delete dynamic buffer*/ - if (ssl->session.isDynamic) { - XFREE(ssl->session.dynTicket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); - ssl->session.isDynamic = 0; + if (bufSz > 0) { + XMEMCPY(ssl->session.ticket, buf, bufSz); } + /* session ticket should only be size of static buffer. Delete dynamic buffer*/ + if (ssl->session.dynTicket) { + XFREE(ssl->session.dynTicket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + ssl->session.dynTicket = NULL; + } + ssl->session.isDynamic = 0; + return SSL_SUCCESS; } @@ -7039,6 +7040,9 @@ int AddSession(WOLFSSL* ssl) { word32 row, idx; int error = 0; +#ifdef HAVE_SESSION_TICKET + byte* tmpBuff = NULL; +#endif if (ssl->options.sessionCacheOff) return 0; @@ -7057,8 +7061,22 @@ int AddSession(WOLFSSL* ssl) return error; } - if (LockMutex(&session_mutex) != 0) +#ifdef HAVE_SESSION_TICKET + /* Alloc Memory here so if Malloc fails can exit outside of lock */ + if(ssl->session.ticketLen > SESSION_TICKET_LEN) { + tmpBuff = XMALLOC(ssl->session.ticketLen, ssl->heap, + DYNAMIC_TYPE_SESSION_TICK); + if(!tmpBuff) + return MEMORY_E; + } +#endif + + if (LockMutex(&session_mutex) != 0) { +#ifdef HAVE_SESSION_TICKET + XFREE(tmpBuff, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); +#endif return BAD_MUTEX_E; + } idx = SessionCache[row].nextIdx++; #ifdef SESSION_INDEX @@ -7075,29 +7093,28 @@ int AddSession(WOLFSSL* ssl) SessionCache[row].Sessions[idx].bornOn = LowResTimer(); #ifdef HAVE_SESSION_TICKET - if (ssl->session.isDynamic) { - if (!SessionCache[row].Sessions[idx].dynTicket) { - SessionCache[row].Sessions[idx].dynTicket = XMALLOC( - ssl->session.ticketLen, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); - if (!SessionCache[row].Sessions[idx].dynTicket) - return MEMORY_E; - } else if (SessionCache[row].Sessions[idx].ticketLen < ssl->session.ticketLen) { - XFREE(SessionCache[row].Sessions[idx].dynTicket, - ssl->heap, DYNAMIC_TYPE_SESS_TICK); - SessionCache[row].Sessions[idx].dynTicket = XMALLOC( - ssl->session.ticketLen, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); - if (!SessionCache[row].Sessions[idx].dynTicket) - return MEMORY_E; - } - XMEMCPY(SessionCache[row].Sessions[idx].dynTicket, - ssl->session.dynTicket, ssl->session.ticketLen); + /* Cleanup cache row's old Dynamic buff if exists */ + if(SessionCache[row].Sessions[idx].dynTicket) { + XFREE(SessionCache[row].Sessions[idx].dynTicket, + ssl->heap, DYNAMIC_TYPE_SESS_TICK); + } + + /* If too large to store in static buffer, use dyn buffer */ + if (ssl->session.ticketLen > SESSION_TICKET_LEN) { + SessionCache[row].Sessions[idx].dynTicket = tmpBuff; SessionCache[row].Sessions[idx].isDynamic = 1; + SessionCache[row].Sessions[idx].ticket = + SessionCache[row].Sessions[idx].dynTicket; + } else { + SessionCache[row].Sessions[idx].dynTicket = NULL; + SessionCache[row].Sessions[idx].isDynamic = 0; + SessionCache[row].Sessions[idx].ticket = + SessionCache[row].Sessions[idx].staticTicket; } - else { - XMEMCPY(SessionCache[row].Sessions[idx].ticket, - ssl->session.ticket, ssl->session.ticketLen); - } + SessionCache[row].Sessions[idx].ticketLen = ssl->session.ticketLen; + XMEMCPY(SessionCache[row].Sessions[idx].ticket, + ssl->session.ticket, ssl->session.ticketLen); #endif #ifdef SESSION_CERTS diff --git a/src/tls.c b/src/tls.c index 482c8f58d..b274e0932 100644 --- a/src/tls.c +++ b/src/tls.c @@ -3866,7 +3866,6 @@ void TLSX_FreeAll(TLSX* list) break; case TLSX_SESSION_TICKET: - /* Nothing to do. */ STK_FREE(extension->data); break; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index ca473bede..60651e19d 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -2181,9 +2181,10 @@ struct WOLFSSL_SESSION { #endif #ifdef HAVE_SESSION_TICKET word16 ticketLen; - byte *dynTicket; + byte* dynTicket; byte isDynamic; - byte ticket[SESSION_TICKET_LEN]; + byte staticTicket[SESSION_TICKET_LEN]; + byte* ticket; #endif #ifdef HAVE_STUNNEL void* ex_data[MAX_EX_DATA]; From f27aca0956ccd4fdfb73d90b333a8b4ede4ff96e Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Fri, 25 Mar 2016 13:31:55 -0600 Subject: [PATCH 3/9] Remove redundant dynTicket pointer. Reorder struct for packing/alignment --- src/internal.c | 49 ++++++++++++++++++++++------------------------ src/ssl.c | 48 ++++++++++++++++++++++++++------------------- wolfssl/internal.h | 7 +++---- 3 files changed, 54 insertions(+), 50 deletions(-) diff --git a/src/internal.c b/src/internal.c index 03b45ba66..ab2ec279e 100755 --- a/src/internal.c +++ b/src/internal.c @@ -2482,9 +2482,6 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx) #ifdef HAVE_SESSION_TICKET ssl->session.ticket = ssl->session.staticTicket; - ssl->session.isDynamic = 0; - ssl->session.dynTicket = NULL; - ssl->session.ticketLen = 0; #endif return 0; } @@ -2656,11 +2653,11 @@ void SSL_ResourceFree(WOLFSSL* ssl) FreeX509(&ssl->peerCert); #endif #ifdef HAVE_SESSION_TICKET - if (ssl->session.dynTicket) { - XFREE(ssl->session.dynTicket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); - ssl->session.dynTicket = NULL; - ssl->session.isDynamic = 0; + if (ssl->session.isDynamic) { + XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); ssl->session.ticket = ssl->session.staticTicket; + ssl->session.isDynamic = 0; + ssl->session.ticketLen = 0; } #endif } @@ -2800,6 +2797,15 @@ void FreeHandshakeResources(WOLFSSL* ssl) #ifdef HAVE_QSH QSH_FreeAll(ssl); #endif +#ifdef HAVE_SESSION_TICKET + if (ssl->session.isDynamic) { + XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + ssl->session.ticket = ssl->session.staticTicket; + ssl->session.isDynamic = 0; + ssl->session.ticketLen = 0; + } +#endif + } @@ -14300,35 +14306,26 @@ int DoSessionTicket(WOLFSSL* ssl, ato16(input + *inOutIdx, &length); *inOutIdx += OPAQUE16_LEN; + if ((*inOutIdx - begin) + length > size) + return BUFFER_ERROR; + if (length > sizeof(ssl->session.staticTicket)) { /* Free old dynamic ticket if we already had one */ - if (ssl->session.dynTicket) { - XFREE(ssl->session.dynTicket, ssl->heap, - DYNAMIC_TYPE_SESSION_TICK); - } - - ssl->session.dynTicket = - (byte*)XMALLOC(length, ssl->heap, - DYNAMIC_TYPE_SESSION_TICK); - - if (ssl->session.dynTicket == NULL) + if (ssl->session.isDynamic) + XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + ssl->session.ticket = + (byte*)XMALLOC(length, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + if (ssl->session.ticket == NULL) return MEMORY_E; - ssl->session.isDynamic = 1; - ssl->session.ticket = ssl->session.dynTicket; } else { - if(ssl->session.dynTicket) { - XFREE(ssl->session.dynTicket, ssl->heap, - DYNAMIC_TYPE_SESSION_TICK); - ssl->session.dynTicket = NULL; + if(ssl->session.isDynamic) { + XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); } ssl->session.isDynamic = 0; ssl->session.ticket = ssl->session.staticTicket; } - if ((*inOutIdx - begin) + length > size) - return BUFFER_ERROR; - /* If the received ticket including its length is greater than * a length value, the save it. Otherwise, don't save it. */ if (length > 0) { diff --git a/src/ssl.c b/src/ssl.c index 986dae3ab..d120f4bae 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -1262,22 +1262,32 @@ WOLFSSL_API int wolfSSL_get_SessionTicket(WOLFSSL* ssl, WOLFSSL_API int wolfSSL_set_SessionTicket(WOLFSSL* ssl, byte* buf, word32 bufSz) { - if (ssl == NULL || (buf == NULL && bufSz > 0) || bufSz > SESSION_TICKET_LEN) + if (ssl == NULL || (buf == NULL && bufSz > 0)) return BAD_FUNC_ARG; - ssl->session.ticket = ssl->session.staticTicket; - ssl->session.ticketLen = (word16)bufSz; - if (bufSz > 0) { + /* Ticket will fit into static ticket */ + if(bufSz <= SESSION_TICKET_LEN) { + if (ssl->session.isDynamic) { + XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + ssl->session.isDynamic = 0; + } + + ssl->session.ticket = ssl->session.staticTicket; + ssl->session.ticketLen = (word16)bufSz; XMEMCPY(ssl->session.ticket, buf, bufSz); + } else { /* Ticket requires dynamic ticket storage */ + /*Not big enough space, need to alloc */ + if (!(ssl->session.ticketLen >= bufSz)) { + if(ssl->session.isDynamic) + XFREE(ssl->session.ticket); + ssl->session.ticket = XMALLOC(bufSz, ssl->heap, DYNAMIC_TYPE_TICK); + if(!ssl->session.ticket) + return MEMORY_ERROR; + ssl->session.isDynamic = 1; + } + XMEMCPY(ssl->session.ticket, buf, bufSz); + ssl->session.ticketLen = bufSz; } - - /* session ticket should only be size of static buffer. Delete dynamic buffer*/ - if (ssl->session.dynTicket) { - XFREE(ssl->session.dynTicket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); - ssl->session.dynTicket = NULL; - } - ssl->session.isDynamic = 0; - return SSL_SUCCESS; } @@ -7073,7 +7083,7 @@ int AddSession(WOLFSSL* ssl) if (LockMutex(&session_mutex) != 0) { #ifdef HAVE_SESSION_TICKET - XFREE(tmpBuff, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + XFREE(tmpBuff, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); #endif return BAD_MUTEX_E; } @@ -7094,22 +7104,20 @@ int AddSession(WOLFSSL* ssl) #ifdef HAVE_SESSION_TICKET /* Cleanup cache row's old Dynamic buff if exists */ - if(SessionCache[row].Sessions[idx].dynTicket) { - XFREE(SessionCache[row].Sessions[idx].dynTicket, + if(SessionCache[row].Sessions[idx].isDynamic) { + XFREE(SessionCache[row].Sessions[idx].ticket, ssl->heap, DYNAMIC_TYPE_SESS_TICK); + SessionCache[row].Sessions[idx].ticket = NULL; } /* If too large to store in static buffer, use dyn buffer */ if (ssl->session.ticketLen > SESSION_TICKET_LEN) { - SessionCache[row].Sessions[idx].dynTicket = tmpBuff; + SessionCache[row].Sessions[idx].ticket = tmpBuff; SessionCache[row].Sessions[idx].isDynamic = 1; - SessionCache[row].Sessions[idx].ticket = - SessionCache[row].Sessions[idx].dynTicket; } else { - SessionCache[row].Sessions[idx].dynTicket = NULL; - SessionCache[row].Sessions[idx].isDynamic = 0; SessionCache[row].Sessions[idx].ticket = SessionCache[row].Sessions[idx].staticTicket; + SessionCache[row].Sessions[idx].isDynamic = 0; } SessionCache[row].Sessions[idx].ticketLen = ssl->session.ticketLen; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 60651e19d..b7e66a86d 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -2180,11 +2180,10 @@ struct WOLFSSL_SESSION { byte serverID[SERVER_ID_LEN]; /* for easier client lookup */ #endif #ifdef HAVE_SESSION_TICKET - word16 ticketLen; - byte* dynTicket; - byte isDynamic; - byte staticTicket[SESSION_TICKET_LEN]; byte* ticket; + word16 ticketLen; + byte staticTicket[SESSION_TICKET_LEN]; + byte isDynamic; #endif #ifdef HAVE_STUNNEL void* ex_data[MAX_EX_DATA]; From 5f9c1ffca688f5bf96b7a6530017bf5f97b9c2c9 Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Fri, 8 Apr 2016 11:14:00 -0600 Subject: [PATCH 4/9] Initial support for deep copying of session --- src/internal.c | 12 +-- src/sniffer.c | 4 +- src/ssl.c | 149 ++++++++++++++++++++++++++------ wolfcrypt/src/error.c | 3 + wolfssl/internal.h | 26 +++--- wolfssl/ssl.h | 1 + wolfssl/wolfcrypt/error-crypt.h | 2 + 7 files changed, 149 insertions(+), 48 deletions(-) diff --git a/src/internal.c b/src/internal.c index ab2ec279e..a50bb8cac 100755 --- a/src/internal.c +++ b/src/internal.c @@ -2652,6 +2652,7 @@ void SSL_ResourceFree(WOLFSSL* ssl) #if defined(KEEP_PEER_CERT) || defined(GOAHEAD_WS) FreeX509(&ssl->peerCert); #endif + #ifdef HAVE_SESSION_TICKET if (ssl->session.isDynamic) { XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); @@ -2797,6 +2798,7 @@ void FreeHandshakeResources(WOLFSSL* ssl) #ifdef HAVE_QSH QSH_FreeAll(ssl); #endif + #ifdef HAVE_SESSION_TICKET if (ssl->session.isDynamic) { XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); @@ -15966,7 +15968,7 @@ int DoSessionTicket(WOLFSSL* ssl, if (ssl->options.resuming) { /* let's try */ int ret = -1; WOLFSSL_SESSION* session = GetSession(ssl, - ssl->arrays->masterSecret); + ssl->arrays->masterSecret, 1); #ifdef HAVE_SESSION_TICKET if (ssl->options.useTicket == 1) { session = &ssl->session; @@ -15981,9 +15983,6 @@ int DoSessionTicket(WOLFSSL* ssl, WOLFSSL_MSG("Unsupported cipher suite, OldClientHello"); return UNSUPPORTED_SUITE; } - #ifdef SESSION_CERTS - ssl->session = *session; /* restore session certs. */ - #endif ret = wc_RNG_GenerateBlock(ssl->rng, ssl->arrays->serverRandom, RAN_LEN); @@ -16361,7 +16360,7 @@ int DoSessionTicket(WOLFSSL* ssl, if (ssl->options.resuming) { int ret = -1; WOLFSSL_SESSION* session = GetSession(ssl, - ssl->arrays->masterSecret); + ssl->arrays->masterSecret, 1); #ifdef HAVE_SESSION_TICKET if (ssl->options.useTicket == 1) { session = &ssl->session; @@ -16377,9 +16376,6 @@ int DoSessionTicket(WOLFSSL* ssl, WOLFSSL_MSG("Unsupported cipher suite, ClientHello"); return UNSUPPORTED_SUITE; } - #ifdef SESSION_CERTS - ssl->session = *session; /* restore session certs. */ - #endif ret = wc_RNG_GenerateBlock(ssl->rng, ssl->arrays->serverRandom, RAN_LEN); diff --git a/src/sniffer.c b/src/sniffer.c index 4a9f18570..3194183ae 100644 --- a/src/sniffer.c +++ b/src/sniffer.c @@ -1560,7 +1560,7 @@ static int ProcessServerHello(const byte* input, int* sslBytes, if (doResume ) { int ret = 0; SSL_SESSION* resume = GetSession(session->sslServer, - session->sslServer->arrays->masterSecret); + session->sslServer->arrays->masterSecret, 0); if (resume == NULL) { SetError(BAD_SESSION_RESUME_STR, error, session, FATAL_ERROR_STATE); return -1; @@ -1825,7 +1825,7 @@ static int ProcessFinished(const byte* input, int size, int* sslBytes, if (ret == 0 && session->flags.cached == 0) { if (session->sslServer->options.haveSessionId) { - WOLFSSL_SESSION* sess = GetSession(session->sslServer, NULL); + WOLFSSL_SESSION* sess = GetSession(session->sslServer, NULL, 0); if (sess == NULL) AddSession(session->sslServer); /* don't re add */ session->flags.cached = 1; diff --git a/src/ssl.c b/src/ssl.c index d120f4bae..0c8a7c201 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -1265,29 +1265,34 @@ WOLFSSL_API int wolfSSL_set_SessionTicket(WOLFSSL* ssl, byte* buf, word32 bufSz) if (ssl == NULL || (buf == NULL && bufSz > 0)) return BAD_FUNC_ARG; - /* Ticket will fit into static ticket */ - if(bufSz <= SESSION_TICKET_LEN) { - if (ssl->session.isDynamic) { - XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); - ssl->session.isDynamic = 0; - } + if (bufSz > 0) { + /* Ticket will fit into static ticket */ + if(bufSz <= SESSION_TICKET_LEN) { + if (ssl->session.isDynamic) { + XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + ssl->session.isDynamic = 0; + } - ssl->session.ticket = ssl->session.staticTicket; - ssl->session.ticketLen = (word16)bufSz; - XMEMCPY(ssl->session.ticket, buf, bufSz); - } else { /* Ticket requires dynamic ticket storage */ - /*Not big enough space, need to alloc */ - if (!(ssl->session.ticketLen >= bufSz)) { - if(ssl->session.isDynamic) - XFREE(ssl->session.ticket); - ssl->session.ticket = XMALLOC(bufSz, ssl->heap, DYNAMIC_TYPE_TICK); - if(!ssl->session.ticket) - return MEMORY_ERROR; - ssl->session.isDynamic = 1; + ssl->session.ticket = ssl->session.staticTicket; + ssl->session.ticketLen = (word16)bufSz; + XMEMCPY(ssl->session.ticket, buf, bufSz); + } else { /* Ticket requires dynamic ticket storage */ + /*Not big enough space, need to alloc */ + if (!(ssl->session.ticketLen >= bufSz)) { + if(ssl->session.isDynamic) + XFREE(ssl->session.ticket, ssl->heap, + DYNAMIC_TYPE_SESSION_TICK); + ssl->session.ticket = XMALLOC(bufSz, ssl->heap, + DYNAMIC_TYPE_SESSION_TICK); + if(!ssl->session.ticket) + return MEMORY_ERROR; + ssl->session.isDynamic = 1; + } + XMEMCPY(ssl->session.ticket, buf, bufSz); } - XMEMCPY(ssl->session.ticket, buf, bufSz); - ssl->session.ticketLen = bufSz; } + ssl->session.ticketLen = (word16)bufSz; + return SSL_SUCCESS; } @@ -5201,7 +5206,7 @@ WOLFSSL_SESSION* wolfSSL_get_session(WOLFSSL* ssl) { WOLFSSL_ENTER("SSL_get_session"); if (ssl) - return GetSession(ssl, 0); + return GetSession(ssl, 0, 0); return NULL; } @@ -6949,7 +6954,8 @@ WOLFSSL_SESSION* GetSessionClient(WOLFSSL* ssl, const byte* id, int len) #endif /* NO_CLIENT_CACHE */ -WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret) +WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret, + byte restoreSessionCerts) { WOLFSSL_SESSION* ret = 0; const byte* id = NULL; @@ -6958,6 +6964,8 @@ WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret) int count; int error = 0; + (void) restoreSessionCerts; + if (ssl->options.sessionCacheOff) return NULL; @@ -7005,6 +7013,17 @@ WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret) ret = current; if (masterSecret) XMEMCPY(masterSecret, current->masterSecret, SECRET_LEN); +#ifdef SESSION_CERTS + /* If set, we should copy the session certs from the ssl object + * to the session we are returning so we can resume */ + if (restoreSessionCerts) { + ret->chain = ssl->session.chain; + ret->version = ssl->session.version; + ret->cipherSuite0 = ssl->session.cipherSuite0; + ret->cipherSuite = ssl->session.cipherSuite; + } +#endif /* SESSION_CERTS */ + } else { WOLFSSL_MSG("Session timed out"); } @@ -7020,13 +7039,92 @@ WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret) } +int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) +{ + int ticketLen; + int doDynamicCopy; + WOLFSSL_SESSION* copyInto = &ssl->session; + void* tmpBuff = NULL; + int ret = SSL_SUCCESS; + + (void)ticketLen; + (void)doDynamicCopy; + (void)tmpBuff; + + if (!ssl || !copyFrom) + return BAD_FUNC_ARG; + + if (LockMutex(&session_mutex) != 0) + return BAD_MUTEX_E; + +#ifdef HAVE_SESSION_TICKET + /* Free old dynamic ticket if we had one to avoid leak */ + if (copyInto->isDynamic) { + XFREE(copyInto->ticket, ssl->heap, DYNAMIC_TYPE_SESS_TICK); + copyInto->ticket = copyInto->staticTicket; + copyInto->isDynamic = 0; + } + /* Size of ticket to alloc if needed; Use later for alloc outside lock */ + if (copyFrom) { + doDynamicCopy = 1; + ticketLen = copyFrom->ticketLen; + } +#endif + + *copyInto = *copyFrom; + if (UnLockMutex(&session_mutex) != 0) { + return BAD_MUTEX_E; + } + +#ifdef HAVE_SESSION_TICKET + /* If doing dynamic copy, need to alloc outside lock, then inside a lock + * confirm the size still matches and memcpy */ + if (doDynamicCopy) { + tmpBuff = XMALLOC(ticketLen, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + if (!tmpBuff) + return MEMORY_ERROR; + + if (LockMutex(&session_mutex) != 0) { + XFREE(tmpBuff, ssl->heap, DYNAMIC_TYPE_SESS_TICK); + return BAD_MUTEX_E; + } + + if (ticketLen != copyFrom->ticketLen) { + /* Another thread modified the ssl-> session ticket during alloc. + * Treat as error, as ticket different than when copy requested */ + ret = VAR_STATE_CHANGE_E; + } + + if (ret == SSL_SUCCESS) { + copyInto->ticket = tmpBuff; + XMEMCPY(copyInto->ticket, copyFrom->ticket, ticketLen); + } + } + + if (UnLockMutex(&session_mutex) != 0) { + if (ret == SSL_SUCCESS) + ret = BAD_MUTEX_E; + } + + if (ret != SSL_SUCCESS) { + /* cleanup */ + if (tmpBuff) + XFREE(tmpBuff, ssl->heap, DYNAMIC_TYPE_SESS_TICK); + copyInto->ticket = copyInto->staticTicket; + copyInto->isDynamic = 0; + } +#endif /* HAVE_SESSION_TICKET */ + return ret; +} + + int SetSession(WOLFSSL* ssl, WOLFSSL_SESSION* session) { if (ssl->options.sessionCacheOff) return SSL_FAILURE; if (LowResTimer() < (session->bornOn + session->timeout)) { - ssl->session = *session; + GetDeepCopySession(ssl, session); ssl->options.resuming = 1; #ifdef SESSION_CERTS @@ -7034,7 +7132,6 @@ int SetSession(WOLFSSL* ssl, WOLFSSL_SESSION* session) ssl->options.cipherSuite0 = session->cipherSuite0; ssl->options.cipherSuite = session->cipherSuite; #endif - return SSL_SUCCESS; } return SSL_FAILURE; /* session timed out */ @@ -7397,10 +7494,12 @@ int wolfSSL_get_session_stats(word32* active, word32* total, word32* peak, #else /* NO_SESSION_CACHE */ /* No session cache version */ -WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret) +WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret, + byte restoreSessionCerts) { (void)ssl; (void)masterSecret; + (void)restoreSessionCerts; return NULL; } diff --git a/wolfcrypt/src/error.c b/wolfcrypt/src/error.c index 5ad5d77a5..c3f252d90 100644 --- a/wolfcrypt/src/error.c +++ b/wolfcrypt/src/error.c @@ -101,6 +101,9 @@ const char* wc_GetErrorString(int error) case MEMORY_E : return "out of memory error"; + case VAR_STATE_CHANGE_E : + return "Variable state modified by different thread"; + case RSA_WRONG_TYPE_E : return "RSA wrong block type for RSA function"; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index b7e66a86d..4f8d47e4b 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -2164,11 +2164,11 @@ struct WOLFSSL_X509_CHAIN { /* wolfSSL session type */ struct WOLFSSL_SESSION { - word32 bornOn; /* create time in seconds */ - word32 timeout; /* timeout in seconds */ - byte sessionID[ID_LEN]; /* id for protocol */ - byte sessionIDSz; - byte masterSecret[SECRET_LEN]; /* stored secret */ + word32 bornOn; /* create time in seconds */ + word32 timeout; /* timeout in seconds */ + byte sessionID[ID_LEN]; /* id for protocol */ + byte sessionIDSz; + byte masterSecret[SECRET_LEN]; /* stored secret */ #ifdef SESSION_CERTS WOLFSSL_X509_CHAIN chain; /* peer cert chain, static */ ProtocolVersion version; /* which version was used */ @@ -2176,23 +2176,23 @@ struct WOLFSSL_SESSION { byte cipherSuite; /* 2nd byte, actual suite */ #endif #ifndef NO_CLIENT_CACHE - word16 idLen; /* serverID length */ - byte serverID[SERVER_ID_LEN]; /* for easier client lookup */ + word16 idLen; /* serverID length */ + byte serverID[SERVER_ID_LEN]; /* for easier client lookup */ #endif #ifdef HAVE_SESSION_TICKET - byte* ticket; - word16 ticketLen; - byte staticTicket[SESSION_TICKET_LEN]; - byte isDynamic; + byte* ticket; + word16 ticketLen; + byte staticTicket[SESSION_TICKET_LEN]; + byte isDynamic; #endif #ifdef HAVE_STUNNEL - void* ex_data[MAX_EX_DATA]; + void* ex_data[MAX_EX_DATA]; #endif }; WOLFSSL_LOCAL -WOLFSSL_SESSION* GetSession(WOLFSSL*, byte*); +WOLFSSL_SESSION* GetSession(WOLFSSL*, byte*, byte); WOLFSSL_LOCAL int SetSession(WOLFSSL*, WOLFSSL_SESSION*); diff --git a/wolfssl/ssl.h b/wolfssl/ssl.h index 72434bd63..8e6c5cc82 100644 --- a/wolfssl/ssl.h +++ b/wolfssl/ssl.h @@ -287,6 +287,7 @@ WOLFSSL_API void wolfSSL_set_quiet_shutdown(WOLFSSL*, int); WOLFSSL_API int wolfSSL_get_error(WOLFSSL*, int); WOLFSSL_API int wolfSSL_get_alert_history(WOLFSSL*, WOLFSSL_ALERT_HISTORY *); +WOLFSSL_API int GetDeepCopySession(WOLFSSL*, WOLFSSL_SESSION*); WOLFSSL_API int wolfSSL_set_session(WOLFSSL* ssl,WOLFSSL_SESSION* session); WOLFSSL_API long wolfSSL_SSL_SESSION_set_timeout(WOLFSSL_SESSION* session, long t); WOLFSSL_API WOLFSSL_SESSION* wolfSSL_get_session(WOLFSSL* ssl); diff --git a/wolfssl/wolfcrypt/error-crypt.h b/wolfssl/wolfcrypt/error-crypt.h index fe1aef3e5..91c229f18 100644 --- a/wolfssl/wolfcrypt/error-crypt.h +++ b/wolfssl/wolfcrypt/error-crypt.h @@ -59,6 +59,8 @@ enum { MP_ZERO_E = -121, /* got a mp zero result, not expected */ MEMORY_E = -125, /* out of memory error */ + VAR_STATE_CHANGE_E = -126, /* var state modified by different thread */ + RSA_WRONG_TYPE_E = -130, /* RSA wrong block type for RSA function */ RSA_BUFFER_E = -131, /* RSA buffer error, output too small or From 5f12b4c2aec863ef67436c3d4819a9228bf6c1aa Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Fri, 22 Apr 2016 11:21:00 -0600 Subject: [PATCH 5/9] Add check to see if thread modified session in AddSession --- src/ssl.c | 132 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 75 insertions(+), 57 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 0c8a7c201..f08ab457a 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -1271,14 +1271,12 @@ WOLFSSL_API int wolfSSL_set_SessionTicket(WOLFSSL* ssl, byte* buf, word32 bufSz) if (ssl->session.isDynamic) { XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); ssl->session.isDynamic = 0; + ssl->session.ticket = ssl->session.staticTicket; } - ssl->session.ticket = ssl->session.staticTicket; - ssl->session.ticketLen = (word16)bufSz; XMEMCPY(ssl->session.ticket, buf, bufSz); } else { /* Ticket requires dynamic ticket storage */ - /*Not big enough space, need to alloc */ - if (!(ssl->session.ticketLen >= bufSz)) { + if (ssl->session.ticketLen < bufSz) { if(ssl->session.isDynamic) XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); @@ -7014,13 +7012,13 @@ WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret, if (masterSecret) XMEMCPY(masterSecret, current->masterSecret, SECRET_LEN); #ifdef SESSION_CERTS - /* If set, we should copy the session certs from the ssl object - * to the session we are returning so we can resume */ + /* If set, we should copy the session certs into the ssl object + * from the session we are returning so we can resume */ if (restoreSessionCerts) { - ret->chain = ssl->session.chain; - ret->version = ssl->session.version; - ret->cipherSuite0 = ssl->session.cipherSuite0; - ret->cipherSuite = ssl->session.cipherSuite; + ssl->session.chain = ret->chain; + ssl->session.version = ret->version; + ssl->session.cipherSuite0 = ret->cipherSuite0; + ssl->session.cipherSuite = ret->cipherSuite; } #endif /* SESSION_CERTS */ @@ -7065,10 +7063,10 @@ int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) copyInto->isDynamic = 0; } /* Size of ticket to alloc if needed; Use later for alloc outside lock */ - if (copyFrom) { + if (copyFrom->isDynamic) { doDynamicCopy = 1; - ticketLen = copyFrom->ticketLen; } + ticketLen = copyFrom->ticketLen; #endif *copyInto = *copyFrom; @@ -7091,7 +7089,7 @@ int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) if (ticketLen != copyFrom->ticketLen) { /* Another thread modified the ssl-> session ticket during alloc. - * Treat as error, as ticket different than when copy requested */ + * Treat as error,si ticket different than when copy requested */ ret = VAR_STATE_CHANGE_E; } @@ -7149,6 +7147,7 @@ int AddSession(WOLFSSL* ssl) int error = 0; #ifdef HAVE_SESSION_TICKET byte* tmpBuff = NULL; + int ticLen = 0; #endif if (ssl->options.sessionCacheOff) @@ -7169,9 +7168,10 @@ int AddSession(WOLFSSL* ssl) } #ifdef HAVE_SESSION_TICKET + ticLen = ssl->session.ticketLen; /* Alloc Memory here so if Malloc fails can exit outside of lock */ - if(ssl->session.ticketLen > SESSION_TICKET_LEN) { - tmpBuff = XMALLOC(ssl->session.ticketLen, ssl->heap, + if(ticLen > SESSION_TICKET_LEN) { + tmpBuff = XMALLOC(ticLen, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); if(!tmpBuff) return MEMORY_E; @@ -7209,60 +7209,78 @@ int AddSession(WOLFSSL* ssl) /* If too large to store in static buffer, use dyn buffer */ if (ssl->session.ticketLen > SESSION_TICKET_LEN) { - SessionCache[row].Sessions[idx].ticket = tmpBuff; - SessionCache[row].Sessions[idx].isDynamic = 1; + /* A different thread modified ssl's ticket since alloc */ + if (ticLen != ssl->session.ticketLen) { + error = VAR_STATE_CHANGE_E; + } else { + SessionCache[row].Sessions[idx].ticket = tmpBuff; + SessionCache[row].Sessions[idx].isDynamic = 1; + } } else { SessionCache[row].Sessions[idx].ticket = SessionCache[row].Sessions[idx].staticTicket; SessionCache[row].Sessions[idx].isDynamic = 0; } - SessionCache[row].Sessions[idx].ticketLen = ssl->session.ticketLen; - XMEMCPY(SessionCache[row].Sessions[idx].ticket, + if (error == 0) { + SessionCache[row].Sessions[idx].ticketLen = ssl->session.ticketLen; + XMEMCPY(SessionCache[row].Sessions[idx].ticket, ssl->session.ticket, ssl->session.ticketLen); + } else { + SessionCache[row].Sessions[idx].ticket = + SessionCache[row].Sessions[idx].staticTicket; + SessionCache[row].Sessions[idx].isDynamic = 0; + SessionCache[row].Sessions[idx].ticketLen = 0; + } #endif #ifdef SESSION_CERTS - SessionCache[row].Sessions[idx].chain.count = ssl->session.chain.count; - XMEMCPY(SessionCache[row].Sessions[idx].chain.certs, - ssl->session.chain.certs, sizeof(x509_buffer) * MAX_CHAIN_DEPTH); + if (error == 0) { + SessionCache[row].Sessions[idx].chain.count = ssl->session.chain.count; + XMEMCPY(SessionCache[row].Sessions[idx].chain.certs, + ssl->session.chain.certs, sizeof(x509_buffer) * MAX_CHAIN_DEPTH); - SessionCache[row].Sessions[idx].version = ssl->version; - SessionCache[row].Sessions[idx].cipherSuite0 = ssl->options.cipherSuite0; - SessionCache[row].Sessions[idx].cipherSuite = ssl->options.cipherSuite; -#endif /* SESSION_CERTS */ - - SessionCache[row].totalCount++; - if (SessionCache[row].nextIdx == SESSIONS_PER_ROW) - SessionCache[row].nextIdx = 0; - -#ifndef NO_CLIENT_CACHE - if (ssl->options.side == WOLFSSL_CLIENT_END && ssl->session.idLen) { - word32 clientRow, clientIdx; - - WOLFSSL_MSG("Adding client cache entry"); - - SessionCache[row].Sessions[idx].idLen = ssl->session.idLen; - XMEMCPY(SessionCache[row].Sessions[idx].serverID, ssl->session.serverID, - ssl->session.idLen); - - clientRow = HashSession(ssl->session.serverID, ssl->session.idLen, - &error) % SESSION_ROWS; - if (error != 0) { - WOLFSSL_MSG("Hash session failed"); - } else { - clientIdx = ClientCache[clientRow].nextIdx++; - - ClientCache[clientRow].Clients[clientIdx].serverRow = (word16)row; - ClientCache[clientRow].Clients[clientIdx].serverIdx = (word16)idx; - - ClientCache[clientRow].totalCount++; - if (ClientCache[clientRow].nextIdx == SESSIONS_PER_ROW) - ClientCache[clientRow].nextIdx = 0; - } + SessionCache[row].Sessions[idx].version = ssl->version; + SessionCache[row].Sessions[idx].cipherSuite0 = ssl->options.cipherSuite0; + SessionCache[row].Sessions[idx].cipherSuite = ssl->options.cipherSuite; + } +#endif /* SESSION_CERTS */ + if (error == 0) { + SessionCache[row].totalCount++; + if (SessionCache[row].nextIdx == SESSIONS_PER_ROW) + SessionCache[row].nextIdx = 0; + } +#ifndef NO_CLIENT_CACHE + if (error == 0) { + if (ssl->options.side == WOLFSSL_CLIENT_END && ssl->session.idLen) { + word32 clientRow, clientIdx; + + WOLFSSL_MSG("Adding client cache entry"); + + SessionCache[row].Sessions[idx].idLen = ssl->session.idLen; + XMEMCPY(SessionCache[row].Sessions[idx].serverID, + ssl->session.serverID, ssl->session.idLen); + + clientRow = HashSession(ssl->session.serverID, ssl->session.idLen, + &error) % SESSION_ROWS; + if (error != 0) { + WOLFSSL_MSG("Hash session failed"); + } else { + clientIdx = ClientCache[clientRow].nextIdx++; + + ClientCache[clientRow].Clients[clientIdx].serverRow = + (word16)row; + ClientCache[clientRow].Clients[clientIdx].serverIdx = + (word16)idx; + + ClientCache[clientRow].totalCount++; + if (ClientCache[clientRow].nextIdx == SESSIONS_PER_ROW) + ClientCache[clientRow].nextIdx = 0; + } + } + else + SessionCache[row].Sessions[idx].idLen = 0; } - else - SessionCache[row].Sessions[idx].idLen = 0; #endif /* NO_CLIENT_CACHE */ #if defined(WOLFSSL_SESSION_STATS) && defined(WOLFSSL_PEAK_SESSIONS) From 1c9bf483ec671780ed9e78c9e2e8a26052d37fd8 Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Mon, 25 Apr 2016 10:19:21 -0600 Subject: [PATCH 6/9] Reorder check for thread modified in addSession. Make sure tick assigned correctly in non dynamic case --- src/internal.c | 5 +++- src/ssl.c | 65 +++++++++++++++++++++++++++++--------------------- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/src/internal.c b/src/internal.c index a50bb8cac..c5fbaa1fb 100755 --- a/src/internal.c +++ b/src/internal.c @@ -14317,8 +14317,11 @@ int DoSessionTicket(WOLFSSL* ssl, XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); ssl->session.ticket = (byte*)XMALLOC(length, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); - if (ssl->session.ticket == NULL) + if (ssl->session.ticket == NULL) { + /* Set to static ticket to avoid null pointer error */ + ssl->session.ticket = ssl->session.staticTicket; return MEMORY_E; + } ssl->session.isDynamic = 1; } else { if(ssl->session.isDynamic) { diff --git a/src/ssl.c b/src/ssl.c index f08ab457a..b633c0d19 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -1282,8 +1282,10 @@ WOLFSSL_API int wolfSSL_set_SessionTicket(WOLFSSL* ssl, byte* buf, word32 bufSz) DYNAMIC_TYPE_SESSION_TICK); ssl->session.ticket = XMALLOC(bufSz, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); - if(!ssl->session.ticket) + if(!ssl->session.ticket) { + ssl->session.ticket = ssl->session.staticTicket; return MEMORY_ERROR; + } ssl->session.isDynamic = 1; } XMEMCPY(ssl->session.ticket, buf, bufSz); @@ -7039,10 +7041,10 @@ WOLFSSL_SESSION* GetSession(WOLFSSL* ssl, byte* masterSecret, int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) { - int ticketLen; - int doDynamicCopy; WOLFSSL_SESSION* copyInto = &ssl->session; void* tmpBuff = NULL; + int ticketLen; + int doDynamicCopy; int ret = SSL_SUCCESS; (void)ticketLen; @@ -7089,7 +7091,7 @@ int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) if (ticketLen != copyFrom->ticketLen) { /* Another thread modified the ssl-> session ticket during alloc. - * Treat as error,si ticket different than when copy requested */ + * Treat as error, since ticket different than when copy requested */ ret = VAR_STATE_CHANGE_E; } @@ -7097,6 +7099,10 @@ int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) copyInto->ticket = tmpBuff; XMEMCPY(copyInto->ticket, copyFrom->ticket, ticketLen); } + } else { + /* Need to ensure ticket pointer gets updated to own buffer + * and is not pointing to buff of session copied from */ + copyInto->ticket = copyInto->staticTicket; } if (UnLockMutex(&session_mutex) != 0) { @@ -7130,6 +7136,7 @@ int SetSession(WOLFSSL* ssl, WOLFSSL_SESSION* session) ssl->options.cipherSuite0 = session->cipherSuite0; ssl->options.cipherSuite = session->cipherSuite; #endif + return SSL_SUCCESS; } return SSL_FAILURE; /* session timed out */ @@ -7200,37 +7207,41 @@ int AddSession(WOLFSSL* ssl) SessionCache[row].Sessions[idx].bornOn = LowResTimer(); #ifdef HAVE_SESSION_TICKET - /* Cleanup cache row's old Dynamic buff if exists */ - if(SessionCache[row].Sessions[idx].isDynamic) { - XFREE(SessionCache[row].Sessions[idx].ticket, - ssl->heap, DYNAMIC_TYPE_SESS_TICK); - SessionCache[row].Sessions[idx].ticket = NULL; - } - - /* If too large to store in static buffer, use dyn buffer */ - if (ssl->session.ticketLen > SESSION_TICKET_LEN) { - /* A different thread modified ssl's ticket since alloc */ - if (ticLen != ssl->session.ticketLen) { - error = VAR_STATE_CHANGE_E; - } else { - SessionCache[row].Sessions[idx].ticket = tmpBuff; - SessionCache[row].Sessions[idx].isDynamic = 1; - } - } else { - SessionCache[row].Sessions[idx].ticket = - SessionCache[row].Sessions[idx].staticTicket; - SessionCache[row].Sessions[idx].isDynamic = 0; + /* Check if another thread modified ticket since alloc */ + if (ticLen != ssl->session.ticketLen) { + error = VAR_STATE_CHANGE_E; } if (error == 0) { - SessionCache[row].Sessions[idx].ticketLen = ssl->session.ticketLen; + /* Cleanup cache row's old Dynamic buff if exists */ + if(SessionCache[row].Sessions[idx].isDynamic) { + XFREE(SessionCache[row].Sessions[idx].ticket, + ssl->heap, DYNAMIC_TYPE_SESS_TICK); + SessionCache[row].Sessions[idx].ticket = NULL; + } + + /* If too large to store in static buffer, use dyn buffer */ + if (ticLen > SESSION_TICKET_LEN) { + SessionCache[row].Sessions[idx].ticket = tmpBuff; + SessionCache[row].Sessions[idx].isDynamic = 1; + } else { + SessionCache[row].Sessions[idx].ticket = + SessionCache[row].Sessions[idx].staticTicket; + SessionCache[row].Sessions[idx].isDynamic = 0; + } + } + + if (error == 0) { + SessionCache[row].Sessions[idx].ticketLen = ticLen; XMEMCPY(SessionCache[row].Sessions[idx].ticket, - ssl->session.ticket, ssl->session.ticketLen); - } else { + ssl->session.ticket, ticLen); + } else { /* cleanup, reset state */ SessionCache[row].Sessions[idx].ticket = SessionCache[row].Sessions[idx].staticTicket; SessionCache[row].Sessions[idx].isDynamic = 0; SessionCache[row].Sessions[idx].ticketLen = 0; + if (tmpBuff) + XFREE(tmpBuff, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); } #endif From ccee49978bce60f9859a48e9123e6b2ed4e74efc Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Mon, 25 Apr 2016 10:32:41 -0600 Subject: [PATCH 7/9] Fix scan-build warning --- src/ssl.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index b633c0d19..4600db7f1 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -7044,7 +7044,7 @@ int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) WOLFSSL_SESSION* copyInto = &ssl->session; void* tmpBuff = NULL; int ticketLen; - int doDynamicCopy; + int doDynamicCopy = 0; int ret = SSL_SUCCESS; (void)ticketLen; @@ -7065,9 +7065,7 @@ int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) copyInto->isDynamic = 0; } /* Size of ticket to alloc if needed; Use later for alloc outside lock */ - if (copyFrom->isDynamic) { - doDynamicCopy = 1; - } + doDynamicCopy = copyFrom->isDynamic; ticketLen = copyFrom->ticketLen; #endif From 00737d1e82e9973db1d14422682fcc0c57e8a3b8 Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Fri, 29 Apr 2016 09:45:44 -0600 Subject: [PATCH 8/9] Ensure that tmpBuff gets assigned null after free. --- src/ssl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ssl.c b/src/ssl.c index 4600db7f1..e21935202 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -7238,8 +7238,10 @@ int AddSession(WOLFSSL* ssl) SessionCache[row].Sessions[idx].staticTicket; SessionCache[row].Sessions[idx].isDynamic = 0; SessionCache[row].Sessions[idx].ticketLen = 0; - if (tmpBuff) + if (tmpBuff) { XFREE(tmpBuff, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); + tmpBuff = NULL; + } } #endif From ecba5161acbe445a92a22ace3573f3d336b335e2 Mon Sep 17 00:00:00 2001 From: Nickolas Lapp Date: Fri, 6 May 2016 13:15:21 -0600 Subject: [PATCH 9/9] default copyInto static instead of dynamic --- src/ssl.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/ssl.c b/src/ssl.c index e21935202..84582581d 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -7070,6 +7070,13 @@ int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) #endif *copyInto = *copyFrom; + + /* Default ticket to non dynamic. This will avoid crash if we fail below */ +#ifdef HAVE_SESSION_TICKET + copyInto->ticket = copyInto->staticTicket; + copyInto->isDynamic = 0; +#endif + if (UnLockMutex(&session_mutex) != 0) { return BAD_MUTEX_E; } @@ -7095,6 +7102,7 @@ int GetDeepCopySession(WOLFSSL* ssl, WOLFSSL_SESSION* copyFrom) if (ret == SSL_SUCCESS) { copyInto->ticket = tmpBuff; + copyInto->isDynamic = 1; XMEMCPY(copyInto->ticket, copyFrom->ticket, ticketLen); } } else {