[Bug]: wolfSSL reads/writes to global variables on first

[alexsn]

Contact Details

[email protected]

Version

5.7.0

Description

We found multiple locations where wolfSSL reads/writes to global variables without locks cause which is not thread safe.

The following locations were detected by tsan:

1. sha512.c uses static int transform_check which is read from and written to by Sha512_SetTransform which can be called from multiple threads.
2. sha256.c also has the same code but is actually called from wolfSSL_Init so it doesn't suffer from this issue.
3. aes.c has static int checkedAESNI = 0; static int haveAESNI = 0; both of which are read from and written to by wc_AesSetKeyLocal

Reproduction steps

Those issues are obvious by looking at the code links above, I expect all global inits to occur on wolfSSL_Init and not on first use.

Relevant log output

WARNING: ThreadSanitizer: data race (pid=12652)
  Read of size 1 at 0x000003184840 by thread T2:
    #0 Sha512_SetTransform third-party/wolfssl/wolfcrypt/src/sha512.c:438:13 (camera_helper_test+0xfa0625) (BuildId: b0389c65bba4749e)
    #1 wc_InitSha384_ex third-party/wolfssl/wolfcrypt/src/sha512.c:1465:5 (camera_helper_test+0xfa0625)
    #2 InitHandshakeHashes third-party/wolfssl/src/internal.c:6886:11 (camera_helper_test+0xffadd4) (BuildId: b0389c65bba4749e)
    #3 InitSSL third-party/wolfssl/src/internal.c:7455:11 (camera_helper_test+0xffb89d) (BuildId: b0389c65bba4749e)
    #4 wolfSSL_new third-party/wolfssl/src/ssl.c:1491:15 (camera_helper_test+0xf1c4d6) (BuildId: b0389c65bba4749e)
    #5 wolfssl_connect_step1 arvr/third-party/curl/curl-8.9.1/lib/vtls/wolfssl.c:864:21 (camera_helper_test+0x10b2afd) (BuildId: b0389c65bba4749e)
    #6 wolfssl_connect_common arvr/third-party/curl/curl-8.9.1/lib/vtls/wolfssl.c:1622:14 (camera_helper_test+0x10b2afd)
    #7 wolfssl_connect_nonblocking arvr/third-party/curl/curl-8.9.1/lib/vtls/wolfssl.c:1703:10 (camera_helper_test+0x10b0c8e) (BuildId: b0389c65bba4749e)
    #8 ssl_connect_nonblocking arvr/third-party/curl/curl-8.9.1/lib/vtls/vtls.c:495:10 (camera_helper_test+0xea0bcb) (BuildId: b0389c65bba4749e)
    #9 ssl_cf_connect arvr/third-party/curl/curl-8.9.1/lib/vtls/vtls.c:1693:14 (camera_helper_test+0xea0bcb)
    #10 Curl_conn_cf_connect arvr/third-party/curl/curl-8.9.1/lib/cfilters.c:371:12 (camera_helper_test+0xea463d) (BuildId: b0389c65bba4749e)
    #11 cf_setup_connect arvr/third-party/curl/curl-8.9.1/lib/connect.c:1307:14 (camera_helper_test+0xea9e24) (BuildId: b0389c65bba4749e)
    #12 Curl_conn_cf_connect arvr/third-party/curl/curl-8.9.1/lib/cfilters.c:371:12 (camera_helper_test+0xea463d) (BuildId: b0389c65bba4749e)
    #13 cf_hc_baller_connect arvr/third-party/curl/curl-8.9.1/lib/cf-https-connect.c:136:15 (camera_helper_test+0x111e390) (BuildId: b0389c65bba4749e)
    #14 cf_hc_connect arvr/third-party/curl/curl-8.9.1/lib/cf-https-connect.c:288:16 (camera_helper_test+0x111e390)
    #15 Curl_conn_connect arvr/third-party/curl/curl-8.9.1/lib/cfilters.c:419:14 (camera_helper_test+0xea4875) (BuildId: b0389c65bba4749e)
    #16 multi_runsingle arvr/third-party/curl/curl-8.9.1/lib/multi.c:2107:16 (camera_helper_test+0xe8dd3d) (BuildId: b0389c65bba4749e)
    #17 curl_multi_perform arvr/third-party/curl/curl-8.9.1/lib/multi.c:2739:18 (camera_helper_test+0xe8ca91) (BuildId: b0389c65bba4749e)
    #18 easy_transfer arvr/third-party/curl/curl-8.9.1/lib/easy.c:676:15 (camera_helper_test+0xe8794a) (BuildId: b0389c65bba4749e)
    #19 easy_perform arvr/third-party/curl/curl-8.9.1/lib/easy.c:771:42 (camera_helper_test+0xe8794a)
    #20 curl_easy_perform arvr/third-party/curl/curl-8.9.1/lib/easy.c:790:10 (camera_helper_test+0xe8794a)

  Previous write of size 1 at 0x000003184840 by thread T3:
    #0 Sha512_SetTransform third-party/wolfssl/wolfcrypt/src/sha512.c:485:25 (camera_helper_test+0xfa06e4) (BuildId: b0389c65bba4749e)
    #1 wc_InitSha384_ex third-party/wolfssl/wolfcrypt/src/sha512.c:1465:5 (camera_helper_test+0xfa06e4)
    #2 InitHandshakeHashes third-party/wolfssl/src/internal.c:6886:11 (camera_helper_test+0xffadd4) (BuildId: b0389c65bba4749e)
    #3 InitSSL third-party/wolfssl/src/internal.c:7455:11 (camera_helper_test+0xffb89d) (BuildId: b0389c65bba4749e)
    #4 wolfSSL_new third-party/wolfssl/src/ssl.c:1491:15 (camera_helper_test+0xf1c4d6) (BuildId: b0389c65bba4749e)
    #5 wolfssl_connect_step1 arvr/third-party/curl/curl-8.9.1/lib/vtls/wolfssl.c:864:21 (camera_helper_test+0x10b2afd) (BuildId: b0389c65bba4749e)
    #6 wolfssl_connect_common arvr/third-party/curl/curl-8.9.1/lib/vtls/wolfssl.c:1622:14 (camera_helper_test+0x10b2afd)
    #7 wolfssl_connect_nonblocking arvr/third-party/curl/curl-8.9.1/lib/vtls/wolfssl.c:1703:10 (camera_helper_test+0x10b0c8e) (BuildId: b0389c65bba4749e)
    #8 ssl_connect_nonblocking arvr/third-party/curl/curl-8.9.1/lib/vtls/vtls.c:495:10 (camera_helper_test+0xea0bcb) (BuildId: b0389c65bba4749e)
    #9 ssl_cf_connect arvr/third-party/curl/curl-8.9.1/lib/vtls/vtls.c:1693:14 (camera_helper_test+0xea0bcb)
    #10 Curl_conn_cf_connect arvr/third-party/curl/curl-8.9.1/lib/cfilters.c:371:12 (camera_helper_test+0xea463d) (BuildId: b0389c65bba4749e)
    #11 cf_setup_connect arvr/third-party/curl/curl-8.9.1/lib/connect.c:1307:14 (camera_helper_test+0xea9e24) (BuildId: b0389c65bba4749e)
    #12 Curl_conn_cf_connect arvr/third-party/curl/curl-8.9.1/lib/cfilters.c:371:12 (camera_helper_test+0xea463d) (BuildId: b0389c65bba4749e)
    #13 cf_hc_baller_connect arvr/third-party/curl/curl-8.9.1/lib/cf-https-connect.c:136:15 (camera_helper_test+0x111e390) (BuildId: b0389c65bba4749e)
    #14 cf_hc_connect arvr/third-party/curl/curl-8.9.1/lib/cf-https-connect.c:288:16 (camera_helper_test+0x111e390)
    #15 Curl_conn_connect arvr/third-party/curl/curl-8.9.1/lib/cfilters.c:419:14 (camera_helper_test+0xea4875) (BuildId: b0389c65bba4749e)
    #16 multi_runsingle arvr/third-party/curl/curl-8.9.1/lib/multi.c:2107:16 (camera_helper_test+0xe8dd3d) (BuildId: b0389c65bba4749e)
    #17 curl_multi_perform arvr/third-party/curl/curl-8.9.1/lib/multi.c:2739:18 (camera_helper_test+0xe8ca91) (BuildId: b0389c65bba4749e)
    #18 easy_transfer arvr/third-party/curl/curl-8.9.1/lib/easy.c:676:15 (camera_helper_test+0xe8794a) (BuildId: b0389c65bba4749e)
    #19 easy_perform arvr/third-party/curl/curl-8.9.1/lib/easy.c:771:42 (camera_helper_test+0xe8794a)
    #20 curl_easy_perform arvr/third-party/curl/curl-8.9.1/lib/easy.c:790:10 (camera_helper_test+0xe8794a)

[anhu]

Hi @alexsn , Thanks for this bug report. I'll be looking into this for you.

[alexsn]

@anhu: possible fix for the above issue, feel free to use it as you see fit

diff --git a/third-party/wolfssl/wolfcrypt/src/aes.c b/third-party/wolfssl/wolfcrypt/src/aes.c
--- a/third-party/wolfssl/wolfcrypt/src/aes.c
+++ b/third-party/wolfssl/wolfcrypt/src/aes.c
@@ -606,18 +606,6 @@
         #define AESNI_ALIGN 16
     #endif
 
-    static int checkedAESNI = 0;
-    static int haveAESNI  = 0;
-    static word32 intel_flags = 0;
-
-    static WARN_UNUSED_RESULT int Check_CPU_support_AES(void)
-    {
-        intel_flags = cpuid_get_flags();
-
-        return IS_INTEL_AESNI(intel_flags) != 0;
-    }
-
-
     /* tell C compiler these are asm functions in case any mix up of ABI underscore
        prefix between clang/gcc/llvm etc */
     #ifdef HAVE_AES_CBC
@@ -4444,11 +4432,7 @@
 
     #ifdef WOLFSSL_AESNI
         aes->use_aesni = 0;
-        if (checkedAESNI == 0) {
-            haveAESNI  = Check_CPU_support_AES();
-            checkedAESNI = 1;
-        }
-        if (haveAESNI) {
+        if (IS_INTEL_AESNI(cpuid_get_flags())) {
             #ifdef WOLFSSL_LINUXKM
             /* runtime alignment check */
             if ((wc_ptr_t)&aes->key & (wc_ptr_t)0xf) {
@@ -8355,6 +8339,7 @@
 
 #ifdef WOLFSSL_AESNI
     if (aes->use_aesni) {
+        const word32 intel_flags = cpuid_get_flags();
 #ifdef HAVE_INTEL_AVX2
         if (IS_INTEL_AVX2(intel_flags)) {
             AES_GCM_encrypt_avx2(in, out, authIn, iv, authTag, sz, authInSz, ivSz,
@@ -8919,6 +8904,7 @@
 
 #ifdef WOLFSSL_AESNI
     if (aes->use_aesni) {
+        const word32 intel_flags = cpuid_get_flags();
 #ifdef HAVE_INTEL_AVX2
         if (IS_INTEL_AVX2(intel_flags)) {
             AES_GCM_decrypt_avx2(in, out, authIn, iv, authTag, sz, authInSz, ivSz,
diff --git a/third-party/wolfssl/wolfcrypt/src/sha256.c b/third-party/wolfssl/wolfcrypt/src/sha256.c
--- a/third-party/wolfssl/wolfcrypt/src/sha256.c
+++ b/third-party/wolfssl/wolfcrypt/src/sha256.c
@@ -383,7 +383,7 @@
     }
 #define XTRANSFORM_LEN(...) inline_XTRANSFORM_LEN(__VA_ARGS__)
 
-    static void Sha256_SetTransform(void)
+    WC_ATTRIBUTE_CONSTRUCTOR static void Sha256_SetTransform(void)
     {
 
         if (transform_check)
diff --git a/third-party/wolfssl/wolfcrypt/src/sha512.c b/third-party/wolfssl/wolfcrypt/src/sha512.c
--- a/third-party/wolfssl/wolfcrypt/src/sha512.c
+++ b/third-party/wolfssl/wolfcrypt/src/sha512.c
@@ -433,7 +433,7 @@
         return ret;
     }
 
-    static void Sha512_SetTransform(void)
+    WC_ATTRIBUTE_CONSTRUCTOR static void Sha512_SetTransform(void)
     {
         if (transform_check)
             return;
diff --git a/third-party/wolfssl/wolfssl/wolfcrypt/settings.h b/third-party/wolfssl/wolfssl/wolfcrypt/settings.h
--- a/third-party/wolfssl/wolfssl/wolfcrypt/settings.h
+++ b/third-party/wolfssl/wolfssl/wolfcrypt/settings.h
@@ -2243,6 +2243,12 @@
     #define XGEN_ALIGN
 #endif
 
+#ifndef NO_ATTRIBUTE_CONSTRUCTOR
+    #define WC_ATTRIBUTE_CONSTRUCTOR __attribute__((constructor))
+#else
+    #define WC_ATTRIBUTE_CONSTRUCTOR
+#endif
+
 #if defined(__mips) || defined(__mips64) || \
     defined(WOLFSSL_SP_MIPS64) || defined(WOLFSSL_SP_MIPS)
     #undef WOLFSSL_SP_INT_DIGIT_ALIGN

[anhu]

@alexsn , Its great that you submitted a patch!! I will have a close look at it and review with my engineering team regarding next steps.

[anhu]

Hi @alexsn , I've assigned this ticket to @douzzer as this is more within his area of expertise. Please stay tuned.
Warm regards, Anthony

[douzzer]

Hi @alexsn . The setup code you're talking about here is idempotent, making it intrinsically safe (race-free). So there's no need to add any additional overhead. In particular, it's better to avoid reevaluating IS_INTEL_AESNI(cpuid_get_flags()) for each SetKey op.

That said, I'll go ahead and add comments around these setup calls to highlight the dependence on idempotency.

In any case, if you find any non-idempotent code with static or global data in the mix, we do want to hear about it and fix it!

Thanks for the report.