Auto merge of #4482 - str4d:memory_cleanse-backports, r=str4d

memory_cleanse backports

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#10308
- bitcoin/bitcoin#11196
- bitcoin/bitcoin#11558
  - Only the changes that did not conflict.
- bitcoin/bitcoin#16158

Part of #145.

src/bench/base58.cpp

@@ -22,7 +22,7 @@ static void Base58Encode(benchmark::State& state)
         }
     };
     while (state.KeepRunning()) {
-        EncodeBase58(buff.begin(), buff.end());
+        EncodeBase58(buff.data(), buff.data() + buff.size());
     }
 }
 

src/bench/checkqueue.cpp

@@ -19,7 +19,7 @@ static const int MIN_CORES = 2;
 static const size_t BATCHES = 101;
 static const size_t BATCH_SIZE = 30;
 static const int PREVECTOR_SIZE = 28;
-static const int QUEUE_BATCH_SIZE = 128;
+static const unsigned int QUEUE_BATCH_SIZE = 128;
 static void CCheckQueueSpeed(benchmark::State& state)
 {
     struct FakeJobNoWork {

src/compat.h

@@ -31,6 +31,7 @@
 #include <mswsock.h>
 #include <windows.h>
 #include <ws2tcpip.h>
+#include <stdint.h>
 #else
 #include <fcntl.h>
 #include <sys/mman.h>
@@ -72,6 +73,15 @@ typedef u_int SOCKET;
 #else
 #define MAX_PATH            1024
 #endif
+#ifdef _MSC_VER
+#if !defined(ssize_t)
+#ifdef _WIN64
+typedef int64_t ssize_t;
+#else
+typedef int32_t ssize_t;
+#endif
+#endif
+#endif
 
 // As Solaris does not have the MSG_NOSIGNAL flag for send(2) syscall, it is defined as 0
 #if !defined(HAVE_MSG_NOSIGNAL) && !defined(MSG_NOSIGNAL)

src/support/cleanse.cpp

@@ -5,9 +5,34 @@
 
 #include "cleanse.h"
 
-#include <openssl/crypto.h>
+#include <cstring>
+
+#if defined(_MSC_VER)
+#include <Windows.h> // For SecureZeroMemory.
+#endif
 
 void memory_cleanse(void *ptr, size_t len)
 {
-    OPENSSL_cleanse(ptr, len);
+#if defined(_MSC_VER)
+    /* SecureZeroMemory is guaranteed not to be optimized out by MSVC. */
+    SecureZeroMemory(ptr, len);
+#else
+    std::memset(ptr, 0, len);
+
+    /* In order to prevent the compiler from optimizing out the memset, this uses an
+     * unremovable (see https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile )
+     * asm block that the compiler must assume could access arbitary memory, including
+     * the zero bytes written by std::memset.
+     *
+     * Quoting Adam Langley <agl@google.com> in commit ad1907fe73334d6c696c8539646c21b11178f20f
+     * in BoringSSL (ISC License):
+     *    As best as we can tell, this is sufficient to break any optimisations that
+     *    might try to eliminate "superfluous" memsets.
+     * This method is used by memzero_explicit() in the Linux kernel, too. Its advantage is that it
+     * is pretty efficient because the compiler can still implement the memset() efficiently,
+     * just not remove it entirely. See "Dead Store Elimination (Still) Considered Harmful" by
+     * Yang et al. (USENIX Security 2017) for more background.
+     */
+    __asm__ __volatile__("" : : "r"(ptr) : "memory");
+#endif
 }

src/support/cleanse.h

@@ -8,6 +8,8 @@
 
 #include <stdlib.h>
 
+/** Secure overwrite a buffer (possibly containing secret data) with zero-bytes. The write
+ * operation will not be optimized out by the compiler. */
 void memory_cleanse(void *ptr, size_t len);
 
 #endif // BITCOIN_SUPPORT_CLEANSE_H

src/wallet/db.h

@@ -17,6 +17,7 @@
 #include <vector>
 
 #include <boost/filesystem/path.hpp>
+#include <boost/scope_exit.hpp>
 
 #include <db_cxx.h>
 
@@ -128,21 +129,22 @@ protected:
         Dbt datValue;
         datValue.set_flags(DB_DBT_MALLOC);
         int ret = pdb->get(activeTxn, &datKey, &datValue, 0);
-        memset(datKey.get_data(), 0, datKey.get_size());
-        if (datValue.get_data() == NULL)
-            return false;
+        memory_cleanse(datKey.get_data(), datKey.get_size());
+        if (datValue.get_data() != NULL) {
+            BOOST_SCOPE_EXIT_TPL(&datValue) {
+                // Clear and free memory
+                memory_cleanse(datValue.get_data(), datValue.get_size());
+                free(datValue.get_data());
+            } BOOST_SCOPE_EXIT_END
 
-        // Unserialize value
-        try {
-            CDataStream ssValue((char*)datValue.get_data(), (char*)datValue.get_data() + datValue.get_size(), SER_DISK, CLIENT_VERSION);
-            ssValue >> value;
-        } catch (const std::exception&) {
-            return false;
+            // Unserialize value
+            try {
+                CDataStream ssValue((char*)datValue.get_data(), (char*)datValue.get_data() + datValue.get_size(), SER_DISK, CLIENT_VERSION);
+                ssValue >> value;
+            } catch (const std::exception&) {
+                return false;
+            }
         }
-
-        // Clear and free memory
-        memset(datValue.get_data(), 0, datValue.get_size());
-        free(datValue.get_data());
         return (ret == 0);
     }
 
@@ -170,8 +172,8 @@ protected:
         int ret = pdb->put(activeTxn, &datKey, &datValue, (fOverwrite ? 0 : DB_NOOVERWRITE));
 
         // Clear memory in case it was a private key
-        memset(datKey.get_data(), 0, datKey.get_size());
-        memset(datValue.get_data(), 0, datValue.get_size());
+        memory_cleanse(datKey.get_data(), datKey.get_size());
+        memory_cleanse(datValue.get_data(), datValue.get_size());
         return (ret == 0);
     }
 
@@ -193,7 +195,7 @@ protected:
         int ret = pdb->del(activeTxn, &datKey, 0);
 
         // Clear memory
-        memset(datKey.get_data(), 0, datKey.get_size());
+        memory_cleanse(datKey.get_data(), datKey.get_size());
         return (ret == 0 || ret == DB_NOTFOUND);
     }
 
@@ -213,7 +215,7 @@ protected:
         int ret = pdb->exists(activeTxn, &datKey, 0);
 
         // Clear memory
-        memset(datKey.get_data(), 0, datKey.get_size());
+        memory_cleanse(datKey.get_data(), datKey.get_size());
         return (ret == 0);
     }
 
@@ -258,8 +260,8 @@ protected:
         ssValue.write((char*)datValue.get_data(), datValue.get_size());
 
         // Clear and free memory
-        memset(datKey.get_data(), 0, datKey.get_size());
-        memset(datValue.get_data(), 0, datValue.get_size());
+        memory_cleanse(datKey.get_data(), datKey.get_size());
+        memory_cleanse(datValue.get_data(), datValue.get_size());
         free(datKey.get_data());
         free(datValue.get_data());
         return 0;