From 0a2d4756e1f54d774642ab0b8ecd69cc8a2f55f0 Mon Sep 17 00:00:00 2001 From: Thomas Snider Date: Thu, 23 Mar 2017 14:07:51 -0700 Subject: [PATCH 1/7] [wallet] Securely erase potentially sensitive keys/values --- src/support/cleanse.h | 1 + src/wallet/db.h | 41 +++++++++++++++++++++-------------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/support/cleanse.h b/src/support/cleanse.h index c6fe12912..e58485a51 100644 --- a/src/support/cleanse.h +++ b/src/support/cleanse.h @@ -8,6 +8,7 @@ #include +// Attempt to overwrite data in the specified memory span. void memory_cleanse(void *ptr, size_t len); #endif // BITCOIN_SUPPORT_CLEANSE_H diff --git a/src/wallet/db.h b/src/wallet/db.h index 608723593..a1185da06 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -128,22 +128,23 @@ 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()); + bool success = false; + if (datValue.get_data() != NULL) { + // Unserialize value + try { + CDataStream ssValue((char*)datValue.get_data(), (char*)datValue.get_data() + datValue.get_size(), SER_DISK, CLIENT_VERSION); + ssValue >> value; + success = true; + } catch (const std::exception&) { + // In this case success remains '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 + memory_cleanse(datValue.get_data(), datValue.get_size()); + free(datValue.get_data()); } - - // Clear and free memory - memset(datValue.get_data(), 0, datValue.get_size()); - free(datValue.get_data()); - return (ret == 0); + return ret == 0 && success; } template @@ -170,8 +171,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 +194,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 +214,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 +259,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; From c502acc7c4512faf17d97b3808391395a6f1d4e4 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Tue, 29 Aug 2017 22:26:12 -0700 Subject: [PATCH 2/7] Switch memory_cleanse implementation to BoringSSL's to ensure memory clearing even with link-time optimization. The implementation we currently use from OpenSSL prevents the compiler from optimizing away clensing operations on blocks of memory that are about to be released, but this protection is not extended to link-time optimization. This commit copies the solution cooked up by Google compiler engineers which uses inline assembly directives to instruct the compiler not to optimize out the call under any circumstances. As the code is in-lined, this has the added advantage of removing one more OpenSSL dependency. Regarding license compatibility, Google's contributions to BoringSSL library, including this code, is made available under the ISC license, which is MIT compatible. BoringSSL git commit: ad1907fe73334d6c696c8539646c21b11178f20f --- src/support/cleanse.cpp | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/support/cleanse.cpp b/src/support/cleanse.cpp index 7a4da12b4..619cb5f5c 100644 --- a/src/support/cleanse.cpp +++ b/src/support/cleanse.cpp @@ -5,9 +5,35 @@ #include "cleanse.h" -#include +#include +/* Compilers have a bad habit of removing "superfluous" memset calls that + * are trying to zero memory. For example, when memset()ing a buffer and + * then free()ing it, the compiler might decide that the memset is + * unobservable and thus can be removed. + * + * Previously we used OpenSSL which tried to stop this by a) implementing + * memset in assembly on x86 and b) putting the function in its own file + * for other platforms. + * + * This change removes those tricks in favour of using asm directives to + * scare the compiler away. As best as our compiler folks can tell, this is + * sufficient and will continue to be so. + * + * Adam Langley + * Commit: ad1907fe73334d6c696c8539646c21b11178f20f + * BoringSSL (LICENSE: ISC) + */ void memory_cleanse(void *ptr, size_t len) { - OPENSSL_cleanse(ptr, len); + std::memset(ptr, 0, len); + + /* As best as we can tell, this is sufficient to break any optimisations that + might try to eliminate "superfluous" memsets. If there's an easy way to + detect memset_s, it would be better to use that. */ +#if defined(_MSC_VER) + __asm; +#else + __asm__ __volatile__("" : : "r"(ptr) : "memory"); +#endif } From 2fcc466273b6d0435f17ff1d2ecddbe0360d4070 Mon Sep 17 00:00:00 2001 From: Aaron Clauson Date: Fri, 10 Nov 2017 07:06:49 +1100 Subject: [PATCH 3/7] Minimal code changes to allow msvc compilation. Zcash: Only changes that did not conflict. --- src/bench/base58.cpp | 2 +- src/bench/checkqueue.cpp | 2 +- src/compat.h | 10 ++++++++++ src/support/cleanse.cpp | 6 +++++- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/bench/base58.cpp b/src/bench/base58.cpp index 489fc9825..28acd47a4 100644 --- a/src/bench/base58.cpp +++ b/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()); } } diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index c4aacb8a7..596d66870 100644 --- a/src/bench/checkqueue.cpp +++ b/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 { diff --git a/src/compat.h b/src/compat.h index b8c238f22..4d6027c67 100644 --- a/src/compat.h +++ b/src/compat.h @@ -31,6 +31,7 @@ #include #include #include +#include #else #include #include @@ -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) diff --git a/src/support/cleanse.cpp b/src/support/cleanse.cpp index 619cb5f5c..3de472fa4 100644 --- a/src/support/cleanse.cpp +++ b/src/support/cleanse.cpp @@ -7,6 +7,10 @@ #include +#if defined(_MSC_VER) +#include // For SecureZeroMemory. +#endif + /* Compilers have a bad habit of removing "superfluous" memset calls that * are trying to zero memory. For example, when memset()ing a buffer and * then free()ing it, the compiler might decide that the memset is @@ -32,7 +36,7 @@ void memory_cleanse(void *ptr, size_t len) might try to eliminate "superfluous" memsets. If there's an easy way to detect memset_s, it would be better to use that. */ #if defined(_MSC_VER) - __asm; + SecureZeroMemory(ptr, len); #else __asm__ __volatile__("" : : "r"(ptr) : "memory"); #endif From f27ff314e6f4cd138d98241eb9691a8fd53c5384 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 5 Jun 2019 22:43:28 +0200 Subject: [PATCH 4/7] Clean up logic in memory_cleanse() for MSVC Commit fbf327b13868861c2877c5754caf5a9816f2603c ("Minimal code changes to allow msvc compilation.") was indeed minimal in terms of lines touched. But as a result of that minimalism it changed the logic in memory_cleanse() to first call std::memset() and then additionally the MSVC-specific SecureZeroMemory() function, and it also moved a comment to the wrong location. This commit removes the superfluous call to std::memset() on MSVC and ensures that the comment is in the right position again. --- src/support/cleanse.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/support/cleanse.cpp b/src/support/cleanse.cpp index 3de472fa4..3b5912ac5 100644 --- a/src/support/cleanse.cpp +++ b/src/support/cleanse.cpp @@ -30,14 +30,14 @@ */ void memory_cleanse(void *ptr, size_t len) { +#if defined(_MSC_VER) + SecureZeroMemory(ptr, len); +#else std::memset(ptr, 0, len); /* As best as we can tell, this is sufficient to break any optimisations that might try to eliminate "superfluous" memsets. If there's an easy way to detect memset_s, it would be better to use that. */ -#if defined(_MSC_VER) - SecureZeroMemory(ptr, len); -#else __asm__ __volatile__("" : : "r"(ptr) : "memory"); #endif } From 1ccdb5734d0a8cd9d593d839854a5fb2aa549f4f Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 5 Jun 2019 22:44:04 +0200 Subject: [PATCH 5/7] Improve documentation of memory_cleanse() So far, the documentation of memory_cleanse() is a verbatim copy of the commit message in BoringSSL, where this code was originally written. However, our code evolved since then, and the commit message is not particularly helpful in the code but is rather of historical interested in BoringSSL only. This commit improves improves the comments around memory_cleanse() and gives a better rationale for the method that we use. This commit touches only comments. --- src/support/cleanse.cpp | 32 ++++++++++++-------------------- src/support/cleanse.h | 3 ++- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/support/cleanse.cpp b/src/support/cleanse.cpp index 3b5912ac5..85e158bfa 100644 --- a/src/support/cleanse.cpp +++ b/src/support/cleanse.cpp @@ -11,33 +11,25 @@ #include // For SecureZeroMemory. #endif -/* Compilers have a bad habit of removing "superfluous" memset calls that - * are trying to zero memory. For example, when memset()ing a buffer and - * then free()ing it, the compiler might decide that the memset is - * unobservable and thus can be removed. - * - * Previously we used OpenSSL which tried to stop this by a) implementing - * memset in assembly on x86 and b) putting the function in its own file - * for other platforms. - * - * This change removes those tricks in favour of using asm directives to - * scare the compiler away. As best as our compiler folks can tell, this is - * sufficient and will continue to be so. - * - * Adam Langley - * Commit: ad1907fe73334d6c696c8539646c21b11178f20f - * BoringSSL (LICENSE: ISC) - */ void memory_cleanse(void *ptr, size_t len) { #if defined(_MSC_VER) + /* SecureZeroMemory is guaranteed not to be optimized out by MSVC. */ SecureZeroMemory(ptr, len); #else std::memset(ptr, 0, len); - /* As best as we can tell, this is sufficient to break any optimisations that - might try to eliminate "superfluous" memsets. If there's an easy way to - detect memset_s, it would be better to use that. */ + /* Memory barrier that scares the compiler away from optimizing out the memset. + * + * Quoting Adam Langley 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 in memzero_explicit() 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 } diff --git a/src/support/cleanse.h b/src/support/cleanse.h index e58485a51..1b2bdc13e 100644 --- a/src/support/cleanse.h +++ b/src/support/cleanse.h @@ -8,7 +8,8 @@ #include -// Attempt to overwrite data in the specified memory span. +/** 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 From 6a4ff45e8aef34c2a54d246d14c3d6abd5467cc6 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 1 May 2020 09:41:27 +1200 Subject: [PATCH 6/7] Use BOOST_SCOPE_EXIT_TPL to clean and free datValue in CDB::Read --- src/wallet/db.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/wallet/db.h b/src/wallet/db.h index a1185da06..5fd512024 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -17,6 +17,7 @@ #include #include +#include #include @@ -129,22 +130,22 @@ protected: datValue.set_flags(DB_DBT_MALLOC); int ret = pdb->get(activeTxn, &datKey, &datValue, 0); memory_cleanse(datKey.get_data(), datKey.get_size()); - bool success = false; 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; - success = true; } catch (const std::exception&) { - // In this case success remains 'false' + return false; } - - // Clear and free memory - memory_cleanse(datValue.get_data(), datValue.get_size()); - free(datValue.get_data()); } - return ret == 0 && success; + return (ret == 0); } template From 8ea2f467cdd289373a8848a70648d99abf103a20 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 1 May 2020 11:00:49 +1200 Subject: [PATCH 7/7] Improve memory_cleanse documentation Co-authored-by: Daira Hopwood --- src/support/cleanse.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/support/cleanse.cpp b/src/support/cleanse.cpp index 85e158bfa..9fa2fc182 100644 --- a/src/support/cleanse.cpp +++ b/src/support/cleanse.cpp @@ -19,13 +19,16 @@ void memory_cleanse(void *ptr, size_t len) #else std::memset(ptr, 0, len); - /* Memory barrier that scares the compiler away from optimizing out the memset. + /* 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 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 in memzero_explicit() the Linux kernel, too. Its advantage is that it + * 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.