Are protected files thread safe?

[Villain88]

Our project has a data preparation utility for encrypted fs similar to gramine-sgx-pf-crypt, but unlike it, it uses multithreading to speed up the encryption process.
The utility also works without the gramine environment.
The simplified code of the utility can be viewed here
After migrating from Gramine 1.4 to Gramine 1.6, the utility started to crash
Using git bisect, a problematic commit was detected - f186199
This is due to the migration to a new version of mbedtls.

Crash stacktrace:

#0 mbedtls_sha512_update <null> (libmbedcrypto_gramine.so.15+0x64fe3)
#1 entropy_update <null> (libmbedcrypto_gramine.so.15+0x3c590)
#2 entropy_gather_internal.part.0 <null> (libmbedcrypto_gramine.so.15+0x3c6e2)
#3 mbedtls_entropy_func <null> (libmbedcrypto_gramine.so.15+0x3c8d4)
#4 mbedtls_ctr_drbg_reseed_internal <null> (libmbedcrypto_gramine.so.15+0x2f7a3)
#5 mbedtls_ctr_drbg_random_with_add <null> (libmbedcrypto_gramine.so.15+0x2fb6d)
#6 mbedtls_random ../tools/sgx/common/pf_util.c:189 (pfTest+0x92d8)
#7 ipf_generate_random_key ../common/src/protected_files/protected_files.c:128 (pfTest+0xbc55)
#8 ipf_update_all_data_and_mht_nodes ../common/src/protected_files/protected_files.c:221 (pfTest+0xbfc6)
#9 ipf_internal_flush ../common/src/protected_files/protected_files.c:466 (pfTest+0xc8f1)
#10 ipf_get_data_node ../common/src/protected_files/protected_files.c:600 (pfTest+0xcec5)
#11 ipf_write ../common/src/protected_files/protected_files.c:1036 (pfTest+0xe1f2)
#12 pf_write ../common/src/protected_files/protected_files.c:1341 (pfTest+0xeea7)
#13 pf_encrypt_file ../tools/sgx/common/pf_util.c:340 (pfTest+0x9a18)

I fixed the problem with a small fix, but I'm not sure if that's the best way to go about it:

diff --git a/tools/sgx/common/pf_util.c b/tools/sgx/common/pf_util.c
index 2c6a36a2..ca8345ed 100644
--- a/tools/sgx/common/pf_util.c
+++ b/tools/sgx/common/pf_util.c
@@ -26,12 +26,16 @@
 #include "path_utils.h"
 #include "perm.h"
 #include "util.h"
+#include <pthread.h>
 
 /* High-level protected files helper functions. */
 
 /* PF callbacks usable in a standard Linux environment.
    Assume that pf handle is a pointer to file's fd. */
 
+
+static pthread_mutex_t g_pf_lock;
+
 static pf_status_t linux_read(pf_handle_t handle, void* buffer, uint64_t offset, size_t size) {
     int fd = *(int*)handle;
     DBG("linux_read: fd %d, buf %p, offset %zu, size %zu\n", fd, buffer, offset, size);
@@ -186,10 +190,14 @@ static mbedtls_entropy_context g_entropy;
 static mbedtls_ctr_drbg_context g_prng;
 
 static pf_status_t mbedtls_random(uint8_t* buffer, size_t size) {
-    if (mbedtls_ctr_drbg_random(&g_prng, buffer, size) != 0) {
+    pthread_mutex_lock(&g_pf_lock);
+    int ret = mbedtls_ctr_drbg_random(&g_prng, buffer, size);
+    pthread_mutex_unlock(&g_pf_lock);
+    if (ret != 0) {
         ERROR("Failed to get random bytes\n");
         return PF_STATUS_CALLBACK_FAILED;
     }
+
     return PF_STATUS_SUCCESS;
 }

It may be necessary to build mbedtls with the MBEDTLS_THREADING_C option, but I would prefer a lock-free algorithm, such as creating a separate context for each thread. Any ideas?

[dimakuv]

This is a great finding!

#1746 should fix your issue. Could you try it out?

It may be necessary to build mbedtls with the MBEDTLS_THREADING_C option, but I would prefer a lock-free algorithm, such as creating a separate context for each thread. Any ideas?

We don't/can't use MBEDTLS_THREADING_C because inside Gramine, we don't have a normal threading API (Gramine is a stand-alone non-libc environment).

Creating a separate CRT_DRBG context for each thread would be the best option, but I don't know how to do it in a generic case, sounds too complicated.

So I opted for a trivial spinlock-based fix.

[Villain88]

Thank you, of course, I'll try a little later and answer the result

[kailun-qin]

After migrating from Gramine 1.4 to Gramine 1.6, the utility started to crash
Using git bisect, a problematic commit was detected - > f186199
This is due to the migration to a new version of mbedtls.

But why the issue was exposed only when upgrading to a newer version of mbedTLS?

[Villain88]

#1746 should fix your issue. Could you try it out?

Works well, obviously spinlock is better than mutex in this context

[mkow]

I think we always considered PF code to not be thread safe and the caller was supposed to ensure proper locking, but this issue seems valid despite this - we have a global random seed object which needs locking, because it's shared even between different files.
Also, @dimakuv, @kailun-qin: We should include the fix into the upcoming minor release, as this could be security related.

[dimakuv]

@mkow Technically this file tools/sgx/common/pf_util.c is used only in the gramine-sgx-pf-crypt stand-alone utility. The fact that it is used in a third-party application was not envisaged.

But yes, I agree, this may affect security. Please see my #1746 for a quick fix.

[mkow]

Ah, good, then it's rather making code less confusing / less foot-gun-like than a security issue. I'd write in the commit message that it's not a security issue in Gramine, but if someone reuses our code for something else then they can be vulnerable.

[dimakuv]

This was fixed via #1746, closing this discussion.