Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

AkiSakurai
Copy link
Contributor

@AkiSakurai AkiSakurai commented Sep 1, 2024

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 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 source 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.
openssl/openssl@d8def79

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.
openssl/openssl@d8def79
@github-actions github-actions bot added tests CI Continuous Integration labels Sep 1, 2024
tests/libtest/lib3207.c Outdated Show resolved Hide resolved
tests/libtest/lib3207.c Outdated Show resolved Hide resolved
lib/curl_memory.h Outdated Show resolved Hide resolved
tests/libtest/lib3207.c Outdated Show resolved Hide resolved
Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>
@icing
Copy link
Contributor

icing commented Sep 2, 2024

I believe this PR makes a necessary change. Well done.

@vszakats vszakats added the TLS label Sep 2, 2024
@bagder bagder closed this in a2bcec0 Sep 2, 2024
@bagder
Copy link
Member

bagder commented Sep 2, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration tests TLS
Development

Successfully merging this pull request may close these issues.

4 participants