Switch memory_cleanse implementation to BoringSSL's to ensure memory clearing even with -lto

maaku commented at 5:49 am on August 30, 2017: contributor

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

in src/support/cleanse.cpp:10 in 084674dc94 outdated

 4@@ -5,9 +5,36 @@
 5 
 6 #include "cleanse.h"
 7 
 8-#include <openssl/crypto.h>
 9+#include <string.h>
10 
11+/* Use asm directives to protect OPENSSL_cleanse.

sipa commented at 5:54 am on August 30, 2017:

This comment doesn’t seem to apply in the current form of the code.

in src/support/cleanse.cpp:35 in 084674dc94 outdated

32+    memset(ptr, 0, len);
33+
34+    /* As best as we can tell, this is sufficient to break any optimisations that
35+       might try to eliminate "superfluous" memsets. If there's an easy way to
36+       detect memset_s, it would be better to use that. */
37+#if defined(OPENSSL_WINDOWS)

sipa commented at 5:57 am on August 30, 2017:

I’m not sure OpenSSL’s headers expose such a macro, and even if they do, this risks breaking things if OpenSSL gets removed entirely at some point. I believe you just want _MSC_VER here to detect MSVC (as the else branch should work fine in MinGW).

sipa commented at 6:01 am on August 30, 2017: member

Big concept ACK; there’s no need to use OpenSSL for something as simple as this

maaku force-pushed on Aug 30, 2017

maaku commented at 7:28 am on August 30, 2017: contributor

Nits addressed (don’t push code late at night!)

fanquake added the label Refactoring on Aug 30, 2017

practicalswift commented at 8:20 am on August 30, 2017: contributor

Concept ACK!

Too bad there is no bulletproof way to detect memset_s(…) :-(

kallewoof commented at 8:31 am on August 30, 2017: member

utACK 9c4f8b5188a53f39c0849f860c8227fbb5a77bc6

Leviathn commented at 4:18 pm on August 30, 2017: none

Concept ACK - minimizing OpenSSL exposure is good.

ISC seems less strict than even MIT, appears compatible.

jtimon commented at 7:20 pm on August 31, 2017: contributor

Concept ACK

theuni commented at 8:15 pm on August 31, 2017: member

concept ACK. and utACK 9c4f8b5188a53f39c0849f860c8227fbb5a77bc6. It’ll be great to be rid of the openssl dependency at such a low level.

The logic seems sane. It’d be nice if we could add a test, but I suppose we can’t test reliably without affecting the outcome.

gmaxwell commented at 4:02 pm on September 1, 2017: contributor

@cfields you can review the ASM at least.

jonasschnelli commented at 5:22 pm on September 3, 2017: contributor

utACK 9c4f8b5188a53f39c0849f860c8227fbb5a77bc6

dcousens approved

in src/support/cleanse.cpp:35 in 9c4f8b5188 outdated

32+
33+    /* As best as we can tell, this is sufficient to break any optimisations that
34+       might try to eliminate "superfluous" memsets. If there's an easy way to
35+       detect memset_s, it would be better to use that. */
36+#if defined(_MSC_VER)
37+    __asm;

laanwj commented at 9:51 pm on September 5, 2017:

What does a bare __asm do?

sipa commented at 9:59 pm on September 5, 2017:

Presumbly the same as the GCC extended asm block above, and hopefully prevents the code from being optimized out.

However, I’m not sure to what extent we want to support MSVC, especially as __asm is only supported on x86, not x86_64.

laanwj commented at 5:25 pm on September 6, 2017:

However, I’m not sure to what extent we want to support MSVC, especially as __asm is only supported on x86, not x86_64.

We don’t support MSVC (I don’t know of any dev who cares to maintain support for it) however It shouldn’t be unnecessarily difficult to build with it, so if possible we should try to be compatible with it.

laanwj commented at 9:52 pm on September 5, 2017: member

Concept ACK, though I don’t understand why this works (certainly in the MSCV case), I would prefer a more explanatory comment.

in src/support/cleanse.cpp:8 in 9c4f8b5188 outdated

4@@ -5,9 +5,35 @@
5 
6 #include "cleanse.h"
7 
8-#include <openssl/crypto.h>
9+#include <string.h>

kallewoof commented at 3:33 am on September 6, 2017:

μNit: Maybe <cstring> instead, since this is C++.

kallewoof commented at 3:36 am on September 6, 2017: member

ACK

I tried this. This code: https://gist.github.com/kallewoof/200748b8093194135c91074ef8929a5b compiled with g++ -O3 -S results in this assembly output: https://gist.github.com/kallewoof/8b7501ea561c8363757abd52dd547f84 (macOS) https://gist.github.com/kallewoof/1edc1d2dfffd6b9cbd4b894b17da86fc (ubu17.04) whereas if the asm stuff is commented out, you get this: https://gist.github.com/kallewoof/531384f4e577a2b10b4f3f88e638547c (macOS) https://gist.github.com/kallewoof/75846ad463cb5ab1954d90705a41452f (ubu17.04)

(The movqs at lines 45-48 (macOS first assembly) are zeroing the data in the first assembly output.)

I also tried the

0  {
1    char x[32];
2    sprintf(x, "1234567890123456789012345678901");
3    memory_cleanse(x, 32);
4  }

variant, and it gives the same results. The assembly output is slightly different though: https://gist.github.com/kallewoof/b733a0a1f8d430d935e268347de17633

practicalswift commented at 11:39 am on September 6, 2017: contributor

Since we - in contrast to BoringSSL - have the luxury of always compiling with assertions enabled, what about also adding an assertion assert(len == 0 || static_cast<uint8_t*>(ptr)[0] == 0); as a postcondition? Better safe than sorry :-)

kallewoof commented at 1:58 pm on September 6, 2017: member

@practicalswift: I think that affects the resulting code. It basically skips cleans for untouched memory. By touching memory it will forego that optimization.

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

1444c2e7d0

maaku force-pushed on Sep 6, 2017

TheBlueMatt commented at 8:35 pm on September 7, 2017: member

In the original boringssl review at https://boringssl-review.googlesource.com/c/boringssl/+/1339 it was noted that on WIndows its likely better to just use SecureZeroMemory…maybe do that in place of the string __asm for MSVC?

laanwj commented at 8:44 pm on September 7, 2017: member

I vaguely remember there was something wrong/insecure about SecureZeroMemory but I can’t remember what and a quick search doesn’t turn up anything. I think it was something with being a Win32 API function (syscall) so it makes it easier for malware to hook the function and scoop up all the sensitive data. Anyhow, I don’t know if it matters.

maaku commented at 11:27 pm on September 15, 2017: contributor

Should I switch to SecureZeroMemory on windows? I don’t know enough about windows to evaluate that risk. Or is this ready to merge as-is?

sipa commented at 11:35 pm on September 15, 2017: member

I think this is fine as-is.

utACK 1444c2e7d0a243690b960c1fefe5f36bf5ca7e54

laanwj commented at 5:16 am on September 16, 2017: member

Should I switch to SecureZeroMemory on windows?

I also think it’s fine as it is. Note that @TheBlueMatt doesn’t say to use that call generally on Windows, but specifically on MSVC. Someone that maintains MSVC support should do the evaluation of the subtleties for what function/method to use there. Just using BoringSSL’s code is “best effort” to support it as anything. utACK https://github.com/bitcoin/bitcoin/pull/11196/commits/1444c2e7d0a243690b960c1fefe5f36bf5ca7e54

(also, if they make another decision re: MSVC we can always take it over)

laanwj merged this on Sep 16, 2017

laanwj closed this on Sep 16, 2017

laanwj referenced this in commit e278f86c53 on Sep 16, 2017

UdjinM6 referenced this in commit 5794242a73 on Jul 20, 2019

PastaPastaPasta referenced this in commit 60ba106ccf on Jul 21, 2019

PastaPastaPasta referenced this in commit 84f0de2e13 on Jul 22, 2019

PastaPastaPasta referenced this in commit 72fbee959b on Jul 23, 2019

PastaPastaPasta referenced this in commit db9bd96338 on Aug 6, 2019

barrystyle referenced this in commit 21f6df88c3 on Jan 22, 2020

zkbot referenced this in commit 17324f085f on Apr 30, 2020

zkbot referenced this in commit 7a0fe273d8 on Apr 30, 2020

deadalnix referenced this in commit 6d6a8b346c on May 19, 2020

ftrader referenced this in commit 5bb31e9981 on Aug 17, 2020

DrahtBot locked this on Sep 8, 2021

Switch memory_cleanse implementation to BoringSSL’s to ensure memory clearing even with -lto #11196