Prevent optimization of buffer clearing.

Some buffers that contain sensitive information are cleared after use. If the compiler sees that memset is a "pure" function, and that the buffer is not used after the memset, it may optimize away the memset. To prevent this from happening, read from the buffer after the memset.
huangyue009 · Oct 18, 2016 · 0a26e03 · 0a26e03
1 parent dc47f14
commit 0a26e03
Show file tree

Hide file tree

Showing 8 changed files with 67 additions and 27 deletions.
diff --git a/apps/debug/client/profthrd.c b/apps/debug/client/profthrd.c
@@ -1519,7 +1519,6 @@ Return Value:
         return NULL;
     }
 
-    memset(IntegerArray, 0, ArgumentCount * sizeof(ULONG));
     for (Index = 0; Index < ArgumentCount; Index += 1) {
         Integer = strtoul(Arguments[Index], &AfterScan, 0);
         if (AfterScan == Arguments[Index]) {

diff --git a/apps/libc/crypt/crypt.c b/apps/libc/crypt/crypt.c
@@ -435,7 +435,7 @@ Return Value:
     // No security droppings.
     //
 
-    memset(Hash, 0, MD5_HASH_SIZE);
+    SECURITY_ZERO(Hash, MD5_HASH_SIZE);
     return ClCryptBuffer;
 }
 
@@ -830,12 +830,12 @@ Return Value:
     // Clear things out to avoid leaving security context around.
     //
 
-    memset(Hash, 0, SHA256_HASH_SIZE);
-    memset(Hash2, 0, SHA256_HASH_SIZE);
-    memset(PBytes, 0, KeyLength);
-    memset(SBytes, 0, SaltLength);
-    memset(&Context, 0, sizeof(Context));
-    memset(&Context2, 0, sizeof(Context2));
+    SECURITY_ZERO(Hash, SHA256_HASH_SIZE);
+    SECURITY_ZERO(Hash2, SHA256_HASH_SIZE);
+    SECURITY_ZERO(PBytes, KeyLength);
+    SECURITY_ZERO(SBytes, SaltLength);
+    SECURITY_ZERO(&Context, sizeof(Context));
+    SECURITY_ZERO(&Context2, sizeof(Context2));
     return Buffer;
 }
 
@@ -1307,12 +1307,12 @@ Return Value:
     // Clear things out to avoid leaving security context around.
     //
 
-    memset(Hash, 0, SHA512_HASH_SIZE);
-    memset(Hash2, 0, SHA512_HASH_SIZE);
-    memset(PBytes, 0, KeyLength);
-    memset(SBytes, 0, SaltLength);
-    memset(&Context, 0, sizeof(Context));
-    memset(&Context2, 0, sizeof(Context2));
+    SECURITY_ZERO(Hash, SHA512_HASH_SIZE);
+    SECURITY_ZERO(Hash2, SHA512_HASH_SIZE);
+    SECURITY_ZERO(PBytes, KeyLength);
+    SECURITY_ZERO(SBytes, SaltLength);
+    SECURITY_ZERO(&Context, sizeof(Context));
+    SECURITY_ZERO(&Context2, sizeof(Context2));
     return Buffer;
 }
 

diff --git a/apps/libc/crypt/cryptp.h b/apps/libc/crypt/cryptp.h
@@ -22,6 +22,21 @@ Module Name:
 
 #define LIBCRYPT_API __DLLEXPORT
 
+//
+// --------------------------------------------------------------------- Macros
+//
+
+//
+// This macro zeros memory and ensures that the compiler doesn't optimize away
+// the memset.
+//
+
+#define SECURITY_ZERO(_Buffer, _Size)                                       \
+    {                                                                       \
+        memset((_Buffer), 0, (_Size));                                      \
+        *(volatile char *)(_Buffer) = *((volatile char *)(_Buffer) + 1);    \
+    }
+
 //
 // ---------------------------------------------------------------- Definitions
 //

diff --git a/apps/libc/dynamic/libcp.h b/apps/libc/dynamic/libcp.h
@@ -43,6 +43,17 @@ Module Name:
         (_Stream)->Flags |= (_Orientation);                        \
     }
 
+//
+// This macro zeros memory and ensures that the compiler doesn't optimize away
+// the memset.
+//
+
+#define SECURITY_ZERO(_Buffer, _Size)                                       \
+    {                                                                       \
+        memset((_Buffer), 0, (_Size));                                      \
+        *(volatile char *)(_Buffer) = *((volatile char *)(_Buffer) + 1);    \
+    }
+
 //
 // This macro asserts that the file permission bits are equivalent.
 //

diff --git a/apps/libc/dynamic/line.c b/apps/libc/dynamic/line.c
@@ -205,7 +205,7 @@ Return Value:
                     memcpy(NewBuffer, ClGetpassBuffer, ClGetpassBufferSize);
                 }
 
-                memset(ClGetpassBuffer, 0, ClGetpassBufferSize);
+                SECURITY_ZERO(ClGetpassBuffer, ClGetpassBufferSize);
                 free(ClGetpassBuffer);
                 ClGetpassBuffer = NULL;
                 ClGetpassBufferSize = 0;

diff --git a/apps/swiss/login/chpasswd.c b/apps/swiss/login/chpasswd.c
@@ -317,11 +317,11 @@ Return Value:
     LineNumber = 1;
     while (TRUE) {
         if (Line != NULL) {
-            memset(Line, 0, strlen(Line));
+            SECURITY_ZERO(Line, strlen(Line));
         }
 
         if ((NewPassword != Password) && (NewPassword != NULL)) {
-            memset(NewPassword, 0, strlen(NewPassword));
+            SECURITY_ZERO(NewPassword, strlen(NewPassword));
             NewPassword = NULL;
         }
 
@@ -482,12 +482,12 @@ Return Value:
     }
 
     if (Line != NULL) {
-        memset(Line, 0, strlen(Line));
+        SECURITY_ZERO(Line, strlen(Line));
         free(Line);
     }
 
     if (NewPassword != NULL) {
-        memset(NewPassword, 0, strlen(NewPassword));
+        SECURITY_ZERO(NewPassword, strlen(NewPassword));
     }
 
     closelog();

diff --git a/apps/swiss/login/lutil.h b/apps/swiss/login/lutil.h
@@ -24,6 +24,21 @@ Module Name:
 #include <pwd.h>
 #include <shadow.h>
 
+//
+// --------------------------------------------------------------------- Macros
+//
+
+//
+// This macro zeros memory and ensures that the compiler doesn't optimize away
+// the memset.
+//
+
+#define SECURITY_ZERO(_Buffer, _Size)                                       \
+    {                                                                       \
+        memset((_Buffer), 0, (_Size));                                      \
+        *(volatile char *)(_Buffer) = *((volatile char *)(_Buffer) + 1);    \
+    }
+
 //
 // ---------------------------------------------------------------- Definitions
 //

diff --git a/apps/swiss/login/passwd.c b/apps/swiss/login/passwd.c
@@ -479,7 +479,7 @@ Return Value:
 MainEnd:
     closelog();
     if (NewPassword != NULL) {
-        memset(NewPassword, 0, strlen(NewPassword));
+        SECURITY_ZERO(NewPassword, strlen(NewPassword));
         free(NewPassword);
     }
 
@@ -563,7 +563,7 @@ Return Value:
         // Zero out the password buffer.
         //
 
-        memset(CurrentPassword, 0, strlen(CurrentPassword));
+        SECURITY_ZERO(CurrentPassword, strlen(CurrentPassword));
         if (Correct == FALSE) {
             sleep(LOGIN_FAIL_DELAY);
             PasswdLogMessage(LOG_WARNING,
@@ -586,7 +586,7 @@ Return Value:
         }
 
         NewPasswordCopy = strdup(NewPassword);
-        memset(NewPassword, 0, strlen(NewPassword));
+        SECURITY_ZERO(NewPassword, strlen(NewPassword));
         if (NewPasswordCopy == NULL) {
             return NULL;
         }
@@ -597,7 +597,7 @@ Return Value:
 
         NewPassword = getpass("Retype new password: ");
         if (NewPassword == NULL) {
-            memset(NewPasswordCopy, 0, strlen(NewPasswordCopy));
+            SECURITY_ZERO(NewPasswordCopy, strlen(NewPasswordCopy));
             free(NewPasswordCopy);
             NewPasswordCopy = NULL;
             return NULL;
@@ -609,7 +609,7 @@ Return Value:
         //
 
         Match = strcmp(NewPassword, NewPasswordCopy);
-        memset(NewPassword, 0, strlen(NewPassword));
+        SECURITY_ZERO(NewPassword, strlen(NewPassword));
         Correct = SwCheckPassword(NewPasswordCopy, OldPasswordHash);
         if ((Match != 0) ||
             (Correct != FALSE) ||
@@ -630,7 +630,7 @@ Return Value:
                              "New password is the same as the old one");
             }
 
-            memset(NewPasswordCopy, 0, strlen(NewPasswordCopy));
+            SECURITY_ZERO(NewPasswordCopy, strlen(NewPasswordCopy));
             free(NewPasswordCopy);
             NewPasswordCopy = NULL;
             continue;
@@ -651,15 +651,15 @@ Return Value:
                                                    0,
                                                    NewPasswordCopy);
 
-        memset(NewPasswordCopy, 0, strlen(NewPasswordCopy));
+        SECURITY_ZERO(NewPasswordCopy, strlen(NewPasswordCopy));
         free(NewPasswordCopy);
 
         //
         // Return a copy of that hash to get it out of the static buffer.
         //
 
         NewHashedPasswordCopy = strdup(NewHashedPassword);
-        memset(NewHashedPassword, 0, strlen(NewHashedPassword));
+        SECURITY_ZERO(NewHashedPassword, strlen(NewHashedPassword));
     }
 
     return NewHashedPasswordCopy;