From f95eee6bf574ba588433bfad8407f95ca4ae6545 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 3 Jul 2019 13:01:45 +0200 Subject: [PATCH] Merge #16158: Fix logic of memory_cleanse() on MSVC and clean up docs f53a70ce95231d34bb14cd6c58503927e8d7ff59 Improve documentation of memory_cleanse() (Tim Ruffing) cac30a436cab3641bba3b774d3d3ddbc426e7908 Clean up logic in memory_cleanse() for MSVC (Tim Ruffing) Pull request description: When working on https://github.com/bitcoin-core/secp256k1/issues/185, I noticed that the logic in memory_cleanse(), which is supposed to clear memory securely, is weird on MSVC. While it's correct, it's at least a code smell because the code clears the memory twice on MSVC. This weirdness was introduced by #11558. This PR fixes the logic on MSVC and also improves the docs around this function. Best reviewed in individual commits, see the commit messages for more rationale. The second commit touches only comments. ACKs for top commit: practicalswift: utACK f53a70ce95231d34bb14cd6c58503927e8d7ff59 :-) laanwj: code review ACK f53a70ce95231d34bb14cd6c58503927e8d7ff59 Tree-SHA512: 1c2fd98ae62b34b3e6e59d1178b293af969a9e06cbb7df02a699ce8802f145a336f72edb178c520e3ecec81f7e8083828f90a5ba6367d966a2c7d7c0dd6c0475 --- src/support/cleanse.cpp | 36 ++++++++++++++---------------------- src/support/cleanse.h | 3 ++- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/support/cleanse.cpp b/src/support/cleanse.cpp index 8d3c7369b49504..b3aed13b081621 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) { - 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 is guaranteed not to be optimized out by MSVC. */ SecureZeroMemory(ptr, len); #else + std::memset(ptr, 0, len); + + /* 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 f020216c7300a6..524433108dbf70 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