luo980/curl
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

openssl: fix the data race when sharing an SSL session between threads

Browse Source 
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.
d8def79838

Closes #14751
This commit is contained in:
Aki 2024-08-31 11:48:18 +08:00 committed by Daniel Stenberg
parent 2c2292ecaf
commit a2bcec0ee0
No known key found for this signature in database
GPG Key ID: 5CC908FDB71E12C2
9 changed files with 499 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
.github/workflows/linux.yml vendored
View File

@ -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

2
lib/curl_threads.c
View File

@ -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"

68
lib/vtls/openssl.c
View File

@ -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);
}

2
tests/data/Makefile.am
View File

@ -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

175
tests/data/test3207 Normal file
View File

@ -0,0 +1,175 @@
<testcase>
<info>
<keywords>
HTTPS
</keywords>
</info>
# Server-side
<reply>
<data>
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!
</data>
<datacheck>
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!
</datacheck>
</reply>
# Client-side
<client>
<features>
SSL
OpenSSL
</features>
<server>
https
</server>
<name>
concurrent HTTPS GET using shared ssl session cache
</name>
<tool>
lib%TESTNUMBER
</tool>
# provide URL and ca-cert
<command>
https://localhost:%HTTPSPORT/%TESTNUMBER %SRCDIR/certs/EdelCurlRoot-ca.crt
</command>
</client>
# Verify data after the test has been "shot"
<verify>
</verify>
</testcase>

2
tests/libtest/CMakeLists.txt
View File

@ -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)

7
tests/libtest/Makefile.inc
View File

@ -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)

231
tests/libtest/lib3207.c Normal file
View File

@ -0,0 +1,231 @@
/***************************************************************************
* _ _ ____ _
* Project ___| | | | _ \| |
* / __| | | | |_) | |
* | (__| |_| | _ <| |___
* \___|\___/|_| \_\_____|
*
* Copyright (C) Daniel Stenberg, <daniel@haxx.se>, 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 <stdio.h>
#if defined(USE_THREADS_POSIX) || defined(USE_THREADS_WIN32)
#if defined(USE_THREADS_POSIX)
#include <pthread.h>
#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;
}

2
tests/server/sws.c
View File

@ -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));