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

Jump to bottom

data race in Curl_ossl_seed #7296

Closed
grrtrr opened this issue Jun 24, 2021 · 8 comments
Closed

data race in Curl_ossl_seed #7296

grrtrr opened this issue Jun 24, 2021 · 8 comments
Labels
TLS

Comments

@grrtrr
Copy link

grrtrr commented Jun 24, 2021

Thread sanitizer found a data race in seeding openssl state.

I did this

Ran AWS SDK with curl as its client, using the thead sanitizer.

curl/libcurl version

7.67.0

operating system

5.4.0-1049-aws #51~18.04.1-Ubuntu SMP Fri May 14 18:38:46 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Problem description

The thread sanitizer produced the following trace:

WARNING: ThreadSanitizer: data race (pid=28103)
  Read of size 1 at 0x7fecd59c1280 by thread T1:
    #0 Curl_ossl_seed <null> (libexternal_Scurl_Slibcurl.so+0x69a35)
    #1 ossl_connect_step1 <null> (libexternal_Scurl_Slibcurl.so+0x6d7b3)
    #2 ossl_connect_common <null> (libexternal_Scurl_Slibcurl.so+0x6f8bb)
    #3 Curl_ossl_connect_nonblocking <null> (libexternal_Scurl_Slibcurl.so+0x6fcc5)
    #4 Curl_ssl_connect_nonblocking <null> (libexternal_Scurl_Slibcurl.so+0x7144b)
    #5 https_connecting <null> (libexternal_Scurl_Slibcurl.so+0x27fb2)
    #6 Curl_http_connect <null> (libexternal_Scurl_Slibcurl.so+0x2a5cf)
    #7 multi_runsingle <null> (libexternal_Scurl_Slibcurl.so+0x3e173)
    #8 curl_multi_perform <null> (libexternal_Scurl_Slibcurl.so+0x3f335)
    #9 curl_easy_perform <null> (libexternal_Scurl_Slibcurl.so+0x202d7)
    #10 Aws::Http::CurlHttpClient::MakeRequest(std::shared_ptr<Aws::Http::HttpRequest> const&, Aws::Utils::RateLimits::RateLimiterInterface*, Aws::Utils::RateLimits::RateLimiterInterface*) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-core_Slibaws-core.so+0x138ae3)
    #11 Aws::Client::AWSClient::AttemptOneRequest(std::shared_ptr<Aws::Http::HttpRequest> const&, Aws::AmazonWebServiceRequest const&, char const*, char const*, char const*) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-core_Slibaws-core.so+0xc0819)
    #12 Aws::Client::AWSClient::AttemptExhaustively(Aws::Http::URI const&, Aws::AmazonWebServiceRequest const&, Aws::Http::HttpMethod, char const*, char const*, char const*) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-core_Slibaws-core.so+0xc2e8e)
    #13 Aws::Client::AWSXMLClient::MakeRequest(Aws::Http::URI const&, Aws::AmazonWebServiceRequest const&, Aws::Http::HttpMethod, char const*, char const*, char const*) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-core_Slibaws-core.so+0xc689e)
    #14 Aws::S3::S3Client::ListObjectsV2(Aws::S3::Model::ListObjectsV2Request const&) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-s3_Slibaws-s3.so+0x1c12af)

  Previous write of size 1 at 0x7fecd59c1280 by thread T2:
    #0 Curl_ossl_seed <null> (libexternal_Scurl_Slibcurl.so+0x69af6)
    #1 ossl_connect_step1 <null> (libexternal_Scurl_Slibcurl.so+0x6d7b3)
    #2 ossl_connect_common <null> (libexternal_Scurl_Slibcurl.so+0x6f8bb)
    #3 Curl_ossl_connect_nonblocking <null> (libexternal_Scurl_Slibcurl.so+0x6fcc5)
    #4 Curl_ssl_connect_nonblocking <null> (libexternal_Scurl_Slibcurl.so+0x7144b)
    #5 https_connecting <null> (libexternal_Scurl_Slibcurl.so+0x27fb2)
    #6 Curl_http_connect <null> (libexternal_Scurl_Slibcurl.so+0x2a5cf)
    #7 multi_runsingle <null> (libexternal_Scurl_Slibcurl.so+0x3e173)
    #8 curl_multi_perform <null> (libexternal_Scurl_Slibcurl.so+0x3f335)
    #9 curl_easy_perform <null> (libexternal_Scurl_Slibcurl.so+0x202d7)
    #10 Aws::Http::CurlHttpClient::MakeRequest(std::shared_ptr<Aws::Http::HttpRequest> const&, Aws::Utils::RateLimits::RateLimiterInterface*, Aws::Utils::RateLimits::RateLimiterInterface*) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-core_Slibaws-core.so+0x138ae3)
    #11 Aws::Client::AWSClient::AttemptOneRequest(std::shared_ptr<Aws::Http::HttpRequest> const&, Aws::AmazonWebServiceRequest const&, char const*, char const*, char const*) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-core_Slibaws-core.so+0xc0819)
    #12 Aws::Client::AWSClient::AttemptExhaustively(Aws::Http::URI const&, Aws::AmazonWebServiceRequest const&, Aws::Http::HttpMethod, char const*, char const*, char const*) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-core_Slibaws-core.so+0xc2e8e)
    #13 Aws::Client::AWSXMLClient::MakeRequest(Aws::Http::URI const&, Aws::AmazonWebServiceRequest const&, Aws::Http::HttpMethod, char const*, char const*, char const*) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-core_Slibaws-core.so+0xc689e)
    #14 Aws::S3::S3Client::ListObjectsV2(Aws::S3::Model::ListObjectsV2Request const&) const <null> (libthird_Uparty_Saws-cpp-sdk_Saws-s3_Slibaws-s3.so+0x1c12af)

  Location is global 'ssl_seeded.23703' of size 1 at 0x7fecd59c1280 (libexternal_Scurl_Slibcurl.so+0x000000089280)

SUMMARY: ThreadSanitizer: data race (...) in Curl_ossl_seed

This points to the following code:

// curl/lib/vtls/openssl.c 
static CURLcode Curl_ossl_seed(struct Curl_easy *data)
{
  /* we have the "SSL is seeded" boolean static to prevent multiple
     time-consuming seedings in vain */
  static bool ssl_seeded = FALSE;  // <=== HERE (size=1)
  char fname[256];
  
  if(ssl_seeded)  // <== 
    return CURLE_OK;
  
  if(rand_enough()) {
    /* OpenSSL 1.1.0+ will return here */
    ssl_seeded = TRUE;  // <==
    return CURLE_OK;
  }

Either of the following fixes the problem (the first needs to be cleaned up, since it only works on Linux):

 --- lib/vtls/openssl.c.orig     2021-06-24 12:32:32.394071075 -0400
+++ lib/vtls/openssl.c  2021-06-24 12:32:16.545889072 -0400
@@ -30,6 +30,7 @@
 #ifdef USE_OPENSSL

 #include <limits.h>
+#include <pthread.h>

 #include "urldata.h"
 #include "sendf.h"
@@ -452,15 +453,22 @@ static CURLcode Curl_ossl_seed(struct Cu
 { 
   /* we have the "SSL is seeded" boolean static to prevent multiple
      time-consuming seedings in vain */
+  static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
   static bool ssl_seeded = FALSE;
   char fname[256];

-  if(ssl_seeded)
+  pthread_mutex_lock(&lock);
+  if(ssl_seeded) {
+    pthread_mutex_unlock(&lock);
     return CURLE_OK;
+  }
+  pthread_mutex_unlock(&lock);

   if(rand_enough()) {
     /* OpenSSL 1.1.0+ will return here */
+    pthread_mutex_lock(&lock);
     ssl_seeded = TRUE;
+    pthread_mutex_unlock(&lock);
     return CURLE_OK;
   }

Without static variable:

--- lib/vtls/openssl.c
+++ lib/vtls/openssl.c
@@ -450,9 +450,7 @@
 
 static CURLcode Curl_ossl_seed(struct Curl_easy *data)
 {
-  /* we have the "SSL is seeded" boolean static to prevent multiple
-     time-consuming seedings in vain */
-  static bool ssl_seeded = FALSE;
+  bool ssl_seeded = FALSE;
   char fname[256];
 
   if(ssl_seeded)
@bagder
Copy link
Member

bagder commented Jun 24, 2021 •
edited

We can not unconditionally use pthreads, its not a dependency we always use/have.

Is this race really harmful? Does it actually matter?

I believe the seeding isn't necessary for OpenSSL >= 1.1.1. If that's true, maybe we should just #ifdef the code accordingly...

@grrtrr
Copy link
Author

grrtrr commented Jun 24, 2021

Yes, that was a quick hack, I saw that there is an internal alias for mutexes.

Problem occurred with boringssl (version is a few years old, don't have the version at hand at the moment).

@bagder
Copy link
Member

bagder commented Jun 26, 2021

Problem occurred with boringssl

Which problem was that exactly?

@grrtrr
Copy link
Author

grrtrr commented Jun 28, 2021

The problem is described above. There is a race condition since the variable ssl_seeded is accessible by multiple threads.

@bagder
Copy link
Member

bagder commented Jun 28, 2021

Yes, but the result of the race is just that it gets seeded twice, isn't it?

@grrtrr
Copy link
Author

grrtrr commented Jun 28, 2021 •
edited

Ok, how about always seeding it? If I understand you correctly, the condition is not happening frequently anyway?

@bagder
Copy link
Member

bagder commented Jun 28, 2021

Ok, how about always seeding it?

We should make it so, but probably only once per handle.

If I understand you correctly, the condition is not happening frequently anyway?

The race or the seeding? The seeding will be done once per process (that uses libcurl) as it works right now.

bagder added a commit that referenced this issue Jun 28, 2021
@bagder
openssl: avoid static variable for seed flag
efd29b8 
Avoid the race condition risk by instead storing the "seeded" flag in
the multi handle. Modern OpenSSL versions handle the seeding itself so
doing the seeding once per multi-handle instead of once per process is
less of an issue.

Reported-by: Gerrit Renker
Fixes #7296
@bagder bagder mentioned this issue Jun 28, 2021
Closed
@grrtrr
Copy link
Author

grrtrr commented Jun 28, 2021

Thanks for looking into this.

@jay jay added the TLS label Jun 29, 2021
bagder added a commit that referenced this issue Jun 29, 2021
@bagder
openssl: avoid static variable for seed flag
fa1be64 
Avoid the race condition risk by instead storing the "seeded" flag in
the multi handle. Modern OpenSSL versions handle the seeding itself so
doing the seeding once per multi-handle instead of once per process is
less of an issue.

Reported-by: Gerrit Renker
Fixes #7296
@bagder bagder closed this as completed in 4aed7a1 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
TLS
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

openssl: avoid static variable for seed flag curl/curl
3 participants
@bagder @jay @grrtrr