data race in Curl_ossl_seed

[grrtrr]

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]

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]

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]

Problem occurred with boringssl

Which problem was that exactly?

[grrtrr]

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

[bagder]

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

[grrtrr]

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

[bagder]

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.

[grrtrr]

Thanks for looking into this.