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

mbedtls: properly cleanup the thread-shared entropy #13071

Closed
wants to merge 2 commits into from

Conversation

jay
Copy link
Member

@jay jay commented Mar 7, 2024

  • Store the state of the thread-shared entropy for global init/cleanup.

  • Use curl's thread support of mbedtls for all Windows builds instead of just when the threaded resolver is used via USE_THREADS_WIN32.

Prior to this change on global cleanup curl builds that have curl thread support for mbedtls freed the entropy (8b1d229) but failed to mark that it had been freed, which caused problems on subsequent init + transfer.

Bug: #11919 (comment)
Reported-by: awesomekosm@users.noreply.github.com

Closes #xxxxx

- Store the state of the thread-shared entropy for global init/cleanup.

- Use curl's thread support of mbedtls for all Windows builds instead of
  just when the threaded resolver is used via USE_THREADS_WIN32.

Prior to this change on global cleanup curl builds that have curl thread
support for mbedtls freed the entropy (8b1d229) but failed to mark that
it had been freed, which caused problems on subsequent init + transfer.

Bug: curl#11919 (comment)
Reported-by: awesomekosm@users.noreply.github.com

Closes #xxxxx
@jay jay added the TLS label Mar 7, 2024
@jay
Copy link
Member Author

jay commented Mar 7, 2024

The mbedtls threading support in curl is from like 10 years ago and was before mbedtls supported threading properly. However I notice they still do not enable threading support in their library by default. Currently the way curl works is it only uses its separate threading support (mbedtls_threadlock.c) for mbedtls if USE_THREADS_POSIX or USE_THREADS_WIN32 (as in, use the threaded resolver).

I changed it for Windows to always use it when _WIN32. Someone familiar with mbedtls will have to suggest what else should be done, like if we should just revert to relying on mbedtls to be built with thread support. But that's outside the scope of this PR.

@bagder
Copy link
Member

bagder commented Mar 7, 2024

mbedTLS is simply not a modern TLS library and we cannot recommend using it. The lack of threading support by default, the lack of TLS 1.3 support (at least in curl) etc.

@wyattoday
Copy link
Contributor

wyattoday commented Mar 8, 2024

@bagder

I have a PR in progress that addresses both threading and TLS 1.3 with mbedTLS. It also addresses and cleans up other issues. The PR is well documented with rationale for a couple of the larger changes.

It amounts to a full audit of the mbedTLS support in curl. Yeah, the code needed some love.

I’ll post a draft of it in a week or 2.

@jay jay closed this in 942896f Mar 12, 2024
@jay jay deleted the mbedtls_reinit_fix branch March 12, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants