Why does OPENSSL_cleanse look so complex and thread-unsafe?

Question

This is the implementation of OPENSSL_cleanse in OpenSSL 1.0.1i

unsigned char cleanse_ctr = 0;

void OPENSSL_cleanse(void *ptr, size_t len)
{
    unsigned char *p = ptr;
    size_t loop = len, ctr = cleanse_ctr;
    while(loop--)
    {
        *(p++) = (unsigned char)ctr;
        ctr += (17 + ((size_t)p & 0xF));
    }
    p=memchr(ptr, (unsigned char)ctr, len);
    if(p)
        ctr += (63 + (size_t)p);
    cleanse_ctr = (unsigned char)ctr;
}

It looks complex and thread-unsafe (by reading and writing global variable cleanse_ctr). Can somebody please explain a bit about this implementation? Does a user need to concern about the possible data race in it?

Answer 1

Why does OPENSSL_cleanse look so complex and thread-unsafe?

The function is complex in an attempt to keep the optimizer from removing it as dead code.

The C standard does not provide a keyword like pin to ensure a statement is not removed. If the zeroizer was removed, then the compiler folks would tell you "... but you asked for optimizations".

C11 offers memset_s in Annex K, which is guaranteed not to be removed. But Drepper and friends object to the "safer" functions, so they are not available on GNU Linux. See, for example, glibc library is missing memset_s.

OpenSSL also avoids volatile because the GCC folks interpret the standard to mean memory backed by hardware. That is, volatile memory can be changed by hardware, but not another thread. This is in contrast to Microsoft's interpretation of the qualifier.

Also note that on the Windows platform (OpenSSL is cross platform), OpenSSL could use SecureZeroMemory. Microsoft addressed the problem of the optimizer removing the code early.

---

EDIT (FEB 2016): It looks like OpenSSL 1.1.0 simplified the cleanse function: RT4116: Change cleanse to just memset. Here's the diff on mem_clr.c:

diff --git a/crypto/mem_clr.c b/crypto/mem_clr.c
index e6450a1..3389919 100644 (file)
--- a/crypto/mem_clr.c
+++ b/crypto/mem_clr.c
@@ -59,23 +59,16 @@
 #include <string.h>
 #include <openssl/crypto.h>

-extern unsigned char cleanse_ctr;
-unsigned char cleanse_ctr = 0;
+/*
+ * Pointer to memset is volatile so that compiler must de-reference
+ * the pointer and can't assume that it points to any function in
+ * particular (such as memset, which it then might further "optimize")
+ */
+typedef void *(*memset_t)(void *,int,size_t);
+
+static volatile memset_t memset_func = memset;

 void OPENSSL_cleanse(void *ptr, size_t len)
 {
-    unsigned char *p = ptr;
-    size_t loop = len, ctr = cleanse_ctr;
-
-    if (ptr == NULL)
-        return;
-
-    while (loop--) {
-        *(p++) = (unsigned char)ctr;
-        ctr += (17 + ((size_t)p & 0xF));
-    }
-    p = memchr(ptr, (unsigned char)ctr, len);
-    if (p)
-        ctr += (63 + (size_t)p);
-    cleanse_ctr = (unsigned char)ctr;
+    memset_func(ptr, 0, len);
 }

Also see Issue 455: Reimplement non-asm OPENSSL_cleanse() on OpenSSL's GitHub.

Answer 2

There is a data race in the code, but it doesn't matter because the point of the variable is just to provide varying garbage data with which to fill a piece of memory. In other words, it doesn't ever really matter what value any given thread reads from that variable. Users do not need to be concerned about it. In fact, the data race may even make the function more effective.