From: Aki <75532970+AkiSakurai@users.noreply.github.com> Date: Sat, 31 Aug 2024 03:48:18 +0000 (+0800) Subject: openssl: fix the data race when sharing an SSL session between threads X-Git-Tag: curl-8_10_0~55 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a2bcec0ee0895c23b98aea8e72ad4e9278fa67c8;p=thirdparty%2Fcurl.git openssl: fix the data race when sharing an SSL session between threads The SSL_Session object is mutated during connection inside openssl, and it might not be thread-safe. Besides, according to documentation of openssl: ``` SSL_SESSION objects keep internal link information about the session cache list, when being inserted into one SSL_CTX object's session cache. One SSL_SESSION object, regardless of its reference count, must therefore only be used with one SSL_CTX object (and the SSL objects created from this SSL_CTX object). ``` If I understand correctly, it is not safe to share it even in a single thread. Instead, serialize the SSL_SESSION before adding it to the cache, and deserialize it after retrieving it from the cache, so that no concurrent write to the same object is infeasible. Also - add a ci test for thread sanitizer - add a test for sharing ssl sessions concurrently - avoid redefining memory functions when not building libcurl, but including the soruce in libtest - increase the concurrent connections limit in sws Notice that there are fix for a global data race for openssl which is not yet release. The fix is cherry pick for the ci test with thread sanitizer. https://github.com/openssl/openssl/commit/d8def79838cd0d5e7c21d217aa26edb5229f0ab4 Closes #14751 --- diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index 79aee4b38b..909960dea8 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -149,6 +149,16 @@ jobs: --with-openssl --enable-debug --enable-websockets singleuse: --unit + - name: thread-sanitizer + install_packages: zlib1g-dev clang libtsan2 + install_steps: pytest openssltsan3 + configure: > + CC=clang + CFLAGS="-fsanitize=thread -g" + LDFLAGS="-fsanitize=thread -Wl,-rpath,$HOME/openssl3/lib" + --with-openssl=$HOME/openssl3 --enable-debug --enable-websockets + singleuse: --unit + - name: memory-sanitizer install_packages: clang install_steps: @@ -310,6 +320,28 @@ jobs: ./config --prefix=$HOME/openssl3 --libdir=lib make -j1 install_sw + - name: cache openssltsan3 + if: contains(matrix.build.install_steps, 'openssltsan3') + uses: actions/cache@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4 + id: cache-openssltsan3 + env: + cache-name: cache-openssltsan3 + with: + path: /home/runner/openssl3 + key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.openssl3-version }}-d8def798 + + - name: 'install openssltsan3' + if: contains(matrix.build.install_steps, 'openssltsan3') && steps.cache-openssltsan3.outputs.cache-hit != 'true' + # There are global data race in openssl: + # Cherry-Pick the fix for testing https://github.com/openssl/openssl/pull/24782 + run: | + git clone --quiet --depth=1 -b ${{ env.openssl3-version }} https://github.com/openssl/openssl + cd openssl + git fetch --quiet --depth=2 origin d8def79838cd0d5e7c21d217aa26edb5229f0ab4 + git cherry-pick -n d8def79838cd0d5e7c21d217aa26edb5229f0ab4 + CC="clang" CFLAGS="-fsanitize=thread" LDFLAGS="-fsanitize=thread" ./config --prefix=$HOME/openssl3 --libdir=lib + make -j1 install_sw + - name: cache quictls if: contains(matrix.build.install_steps, 'quictls') uses: actions/cache@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4 diff --git a/lib/curl_threads.c b/lib/curl_threads.c index fb4af73d02..6d73273f78 100644 --- a/lib/curl_threads.c +++ b/lib/curl_threads.c @@ -35,7 +35,9 @@ #endif #include "curl_threads.h" +#ifdef BUILDING_LIBCURL #include "curl_memory.h" +#endif /* The last #include file should be: */ #include "memdebug.h" diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c index aacf68c59a..dc6ad84f54 100644 --- a/lib/vtls/openssl.c +++ b/lib/vtls/openssl.c @@ -2004,7 +2004,7 @@ static void ossl_session_free(void *sessionid, size_t idsize) { /* free the ID */ (void)idsize; - SSL_SESSION_free(sessionid); + free(sessionid); } /* @@ -2869,6 +2869,9 @@ CURLcode Curl_ossl_add_session(struct Curl_cfilter *cf, { const struct ssl_config_data *config; CURLcode result = CURLE_OK; + size_t der_session_size; + unsigned char *der_session_buf; + unsigned char *der_session_ptr; if(!cf || !data) goto out; @@ -2876,16 +2879,32 @@ CURLcode Curl_ossl_add_session(struct Curl_cfilter *cf, config = Curl_ssl_cf_get_config(cf, data); if(config->primary.cache_session) { + der_session_size = i2d_SSL_SESSION(session, NULL); + if(der_session_size == 0) { + result = CURLE_OUT_OF_MEMORY; + goto out; + } + + der_session_buf = der_session_ptr = malloc(der_session_size); + if(!der_session_buf) { + result = CURLE_OUT_OF_MEMORY; + goto out; + } + + der_session_size = i2d_SSL_SESSION(session, &der_session_ptr); + if(der_session_size == 0) { + result = CURLE_OUT_OF_MEMORY; + free(der_session_buf); + goto out; + } + Curl_ssl_sessionid_lock(data); - result = Curl_ssl_set_sessionid(cf, data, peer, session, 0, - ossl_session_free); - session = NULL; /* call has taken ownership */ + result = Curl_ssl_set_sessionid(cf, data, peer, der_session_buf, + der_session_size, ossl_session_free); Curl_ssl_sessionid_unlock(data); } out: - if(session) - ossl_session_free(session, 0); return result; } @@ -2902,7 +2921,7 @@ static int ossl_new_session_cb(SSL *ssl, SSL_SESSION *ssl_sessionid) connssl = cf? cf->ctx : NULL; data = connssl? CF_DATA_CURRENT(cf) : NULL; Curl_ossl_add_session(cf, data, &connssl->peer, ssl_sessionid); - return 1; + return 0; } static CURLcode load_cacert_from_memory(X509_STORE *store, @@ -3452,7 +3471,9 @@ CURLcode Curl_ossl_ctx_init(struct ossl_ctx *octx, const char *ciphers; SSL_METHOD_QUAL SSL_METHOD *req_method = NULL; ctx_option_t ctx_options = 0; - void *ssl_sessionid = NULL; + SSL_SESSION *ssl_session = NULL; + const unsigned char *der_sessionid = NULL; + size_t der_sessionid_size = 0; struct ssl_primary_config *conn_config = Curl_ssl_cf_get_primary_config(cf); struct ssl_config_data *ssl_config = Curl_ssl_cf_get_config(cf, data); const long int ssl_version_min = conn_config->version; @@ -3937,18 +3958,29 @@ CURLcode Curl_ossl_ctx_init(struct ossl_ctx *octx, octx->reused_session = FALSE; if(ssl_config->primary.cache_session && transport == TRNSPRT_TCP) { Curl_ssl_sessionid_lock(data); - if(!Curl_ssl_getsessionid(cf, data, peer, &ssl_sessionid, NULL)) { + if(!Curl_ssl_getsessionid(cf, data, peer, (void **)&der_sessionid, + &der_sessionid_size)) { /* we got a session id, use it! */ - if(!SSL_set_session(octx->ssl, ssl_sessionid)) { - Curl_ssl_sessionid_unlock(data); - failf(data, "SSL: SSL_set_session failed: %s", - ossl_strerror(ERR_get_error(), error_buffer, - sizeof(error_buffer))); - return CURLE_SSL_CONNECT_ERROR; + ssl_session = d2i_SSL_SESSION(NULL, &der_sessionid, + (long)der_sessionid_size); + if(ssl_session) { + if(!SSL_set_session(octx->ssl, ssl_session)) { + Curl_ssl_sessionid_unlock(data); + SSL_SESSION_free(ssl_session); + failf(data, "SSL: SSL_set_session failed: %s", + ossl_strerror(ERR_get_error(), error_buffer, + sizeof(error_buffer))); + return CURLE_SSL_CONNECT_ERROR; + } + SSL_SESSION_free(ssl_session); + /* Informational message */ + infof(data, "SSL reusing session ID"); + octx->reused_session = TRUE; + } + else { + Curl_ssl_sessionid_unlock(data); + return CURLE_SSL_CONNECT_ERROR; } - /* Informational message */ - infof(data, "SSL reusing session ID"); - octx->reused_session = TRUE; } Curl_ssl_sessionid_unlock(data); } diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 54efa41909..c691e610fa 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -268,6 +268,6 @@ test3024 test3025 test3026 test3027 test3028 test3029 test3030 \ \ test3100 test3101 test3102 test3103 \ test3200 \ -test3201 test3202 test3203 test3204 test3205 +test3201 test3202 test3203 test3204 test3205 test3207 EXTRA_DIST = $(TESTCASES) DISABLED diff --git a/tests/data/test3207 b/tests/data/test3207 new file mode 100644 index 0000000000..01b3353a3d --- /dev/null +++ b/tests/data/test3207 @@ -0,0 +1,175 @@ + + + +HTTPS + + + +# Server-side + + +HTTP/1.1 200 OK +Date: Tue, 09 Nov 2010 14:49:00 GMT +Server: test-server/fake +Content-Type: text/html +Content-Length: 29 + +run 1: foobar and so on fun! + + +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! +run 1: foobar and so on fun! + + + +# Client-side + + +SSL +OpenSSL + + +https + + +concurrent HTTPS GET using shared ssl session cache + + +lib%TESTNUMBER + +# provide URL and ca-cert + +https://localhost:%HTTPSPORT/%TESTNUMBER %SRCDIR/certs/EdelCurlRoot-ca.crt + + + +# Verify data after the test has been "shot" + + + diff --git a/tests/libtest/CMakeLists.txt b/tests/libtest/CMakeLists.txt index 73bc345034..965e3dffe6 100644 --- a/tests/libtest/CMakeLists.txt +++ b/tests/libtest/CMakeLists.txt @@ -35,7 +35,7 @@ foreach(_target IN LISTS noinst_PROGRAMS) if(LIB_SELECTED STREQUAL LIB_STATIC) # These are part of the libcurl static lib. Do not compile/link them again. - list(REMOVE_ITEM _sources ${WARNLESS} ${MULTIBYTE} ${TIMEDIFF}) + list(REMOVE_ITEM _sources ${WARNLESS} ${MULTIBYTE} ${TIMEDIFF} ${THREADS}) endif() string(TOUPPER ${_target} _upper_target) diff --git a/tests/libtest/Makefile.inc b/tests/libtest/Makefile.inc index c3e86d9367..d84b17643d 100644 --- a/tests/libtest/Makefile.inc +++ b/tests/libtest/Makefile.inc @@ -37,6 +37,8 @@ MULTIBYTE = ../../lib/curl_multibyte.c ../../lib/curl_multibyte.h TIMEDIFF = ../../lib/timediff.c ../../lib/timediff.h SUPPORTFILES = $(TIMEDIFF) first.c test.h +THREADS = ../../lib/curl_threads.c ../../lib/curl_threads.h + # These are all libcurl test programs noinst_PROGRAMS = libauthretry libntlmconnect libprereq \ lib500 lib501 lib502 lib503 lib504 lib505 lib506 lib507 lib508 lib509 \ @@ -78,7 +80,7 @@ noinst_PROGRAMS = libauthretry libntlmconnect libprereq \ lib2402 lib2404 lib2405 \ lib2502 \ lib3010 lib3025 lib3026 lib3027 \ - lib3100 lib3101 lib3102 lib3103 + lib3100 lib3101 lib3102 lib3103 lib3207 libntlmconnect_SOURCES = libntlmconnect.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS) libntlmconnect_LDADD = $(TESTUTIL_LIBS) @@ -716,3 +718,6 @@ lib3102_LDADD = $(TESTUTIL_LIBS) lib3103_SOURCES = lib3103.c $(SUPPORTFILES) lib3103_LDADD = $(TESTUTIL_LIBS) + +lib3207_SOURCES = lib3207.c $(SUPPORTFILES) $(TESTUTIL) $(THREADS) $(WARNLESS) $(MULTIBYTE) +lib3207_LDADD = $(TESTUTIL_LIBS) diff --git a/tests/libtest/lib3207.c b/tests/libtest/lib3207.c new file mode 100644 index 0000000000..669526f98d --- /dev/null +++ b/tests/libtest/lib3207.c @@ -0,0 +1,231 @@ +/*************************************************************************** + * _ _ ____ _ + * Project ___| | | | _ \| | + * / __| | | | |_) | | + * | (__| |_| | _ <| |___ + * \___|\___/|_| \_\_____| + * + * Copyright (C) Daniel Stenberg, , et al. + * + * This software is licensed as described in the file COPYING, which + * you should have received as part of this distribution. The terms + * are also available at https://curl.se/docs/copyright.html. + * + * You may opt to use, copy, modify, merge, publish, distribute and/or sell + * copies of the Software, and permit persons to whom the Software is + * furnished to do so, under the terms of the COPYING file. + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY + * KIND, either express or implied. + * + * SPDX-License-Identifier: curl + * + ***************************************************************************/ +#include "test.h" +#include "testutil.h" +#include "memdebug.h" + +#include + +#if defined(USE_THREADS_POSIX) || defined(USE_THREADS_WIN32) +#if defined(USE_THREADS_POSIX) +#include +#endif +#include "curl_threads.h" +#endif + +#define CAINFO libtest_arg2 +#define THREAD_SIZE 16 +#define PER_THREAD_SIZE 8 + +struct Ctx +{ + const char *URL; + CURLSH *share; + int result; + int thread_id; + struct curl_slist *contents; +}; + +static size_t write_memory_callback(void *contents, size_t size, + size_t nmemb, void *userp) { + /* append the data to contents */ + size_t realsize = size * nmemb; + struct Ctx *mem = (struct Ctx *)userp; + char *data = (char *)malloc(realsize + 1); + struct curl_slist *item_append = NULL; + if(!data) { + printf("not enough memory (malloc returned NULL)\n"); + return 0; + } + memcpy(data, contents, realsize); + data[realsize] = '\0'; + item_append = curl_slist_append(mem->contents, data); + free(data); + if(item_append) { + mem->contents = item_append; + } + else { + printf("not enough memory (curl_slist_append returned NULL)\n"); + return 0; + } + return realsize; +} + +static +#if defined(USE_THREADS_POSIX) || defined(USE_THREADS_WIN32) +#if defined(_WIN32_WCE) || defined(CURL_WINDOWS_APP) +DWORD +#else +unsigned int +#endif +CURL_STDCALL +#else +unsigned int +#endif +test_thread(void *ptr) +{ + struct Ctx *ctx = (struct Ctx *)ptr; + CURLcode res = CURLE_OK; + + int i; + + /* Loop the transfer and cleanup the handle properly every lap. This will + still reuse ssl session since the pool is in the shared object! */ + for(i = 0; i < PER_THREAD_SIZE; i++) { + CURL *curl = curl_easy_init(); + if(curl) { + curl_easy_setopt(curl, CURLOPT_URL, (char *)ctx->URL); + + /* use the share object */ + curl_easy_setopt(curl, CURLOPT_SHARE, ctx->share); + curl_easy_setopt(curl, CURLOPT_CAINFO, CAINFO); + + curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_memory_callback); + curl_easy_setopt(curl, CURLOPT_WRITEDATA, ptr); + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1); + + /* Perform the request, res will get the return code */ + res = curl_easy_perform(curl); + + /* always cleanup */ + curl_easy_cleanup(curl); + /* Check for errors */ + if(res != CURLE_OK) { + fprintf(stderr, "curl_easy_perform() failed: %s\n", + curl_easy_strerror(res)); + goto test_cleanup; + } + } + } + +test_cleanup: + ctx->result = (int)res; + return 0; +} + +#if defined(USE_THREADS_POSIX) || defined(USE_THREADS_WIN32) + +static void my_lock(CURL *handle, curl_lock_data data, + curl_lock_access laccess, void *useptr) +{ + curl_mutex_t *mutexes = (curl_mutex_t*) useptr; + (void)handle; + (void)laccess; + Curl_mutex_acquire(&mutexes[data]); +} + +static void my_unlock(CURL *handle, curl_lock_data data, void *useptr) +{ + curl_mutex_t *mutexes = (curl_mutex_t*) useptr; + (void)handle; + Curl_mutex_release(&mutexes[data]); +} + +static void execute(struct Curl_share *share, struct Ctx *ctx) +{ + int i; + curl_mutex_t mutexes[CURL_LOCK_DATA_LAST - 1]; + curl_thread_t thread[THREAD_SIZE]; + for(i = 0; i < CURL_LOCK_DATA_LAST - 1; i++) { + Curl_mutex_init(&mutexes[i]); + } + curl_share_setopt(share, CURLSHOPT_LOCKFUNC, my_lock); + curl_share_setopt(share, CURLSHOPT_UNLOCKFUNC, my_unlock); + curl_share_setopt(share, CURLSHOPT_USERDATA, (void *)mutexes); + curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_SSL_SESSION); + + for(i = 0; i < THREAD_SIZE; i++) { + thread[i] = Curl_thread_create(test_thread, (void *)&ctx[i]); + } + for(i = 0; i < THREAD_SIZE; i++) { + if(thread[i]) { + Curl_thread_join(&thread[i]); + Curl_thread_destroy(thread[i]); + } + } + curl_share_setopt(share, CURLSHOPT_LOCKFUNC, NULL); + curl_share_setopt(share, CURLSHOPT_UNLOCKFUNC, NULL); + for(i = 0; i < CURL_LOCK_DATA_LAST - 1; i++) { + Curl_mutex_destroy(&mutexes[i]); + } +} + +#else /* without pthread, run serially */ + +static void execute(struct Curl_share *share, struct Ctx *ctx) +{ + int i; + (void) share; + for(i = 0; i < THREAD_SIZE; i++) { + test_thread((void *)&ctx[i]); + } +} + +#endif + +CURLcode test(char *URL) +{ + int res = 0; + int i; + CURLSH* share; + struct Ctx ctx[THREAD_SIZE]; + + curl_global_init(CURL_GLOBAL_ALL); + + share = curl_share_init(); + if(!share) { + fprintf(stderr, "curl_share_init() failed\n"); + goto test_cleanup; + } + + for(i = 0; i < THREAD_SIZE; i++) { + ctx[i].share = share; + ctx[i].URL = URL; + ctx[i].thread_id = i; + ctx[i].result = 0; + ctx[i].contents = NULL; + } + + execute(share, ctx); + + for(i = 0; i < THREAD_SIZE; i++) { + if(ctx[i].result) { + res = ctx[i].result; + } + else { + struct curl_slist *item = ctx[i].contents; + while(item) { + printf("%s", item->data); + item = item->next; + } + } + curl_slist_free_all(ctx[i].contents); + } + +test_cleanup: + if(share) + curl_share_cleanup(share); + curl_global_cleanup(); + return (CURLcode)res; +} diff --git a/tests/server/sws.c b/tests/server/sws.c index 9468acce4c..56b4f4744c 100644 --- a/tests/server/sws.c +++ b/tests/server/sws.c @@ -2240,7 +2240,7 @@ int main(int argc, char *argv[]) protocol_type, socket_type, location_str); /* start accepting connections */ - rc = listen(sock, 5); + rc = listen(sock, 50); if(0 != rc) { error = SOCKERRNO; logmsg("listen() failed with error: (%d) %s", error, sstrerror(error));