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

maaku · 2017-08-30T05:49:23Z

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

sipa

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

sipa · 2017-08-30T05:54:58Z

src/support/cleanse.cpp

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

sipa · 2017-08-30T05:57:41Z

src/support/cleanse.cpp

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).

maaku · 2017-08-30T07:28:29Z

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

practicalswift · 2017-08-30T08:20:34Z

Concept ACK!

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

kallewoof · 2017-08-30T08:31:59Z

utACK 9c4f8b5188a53f39c0849f860c8227fbb5a77bc6

Leviathn · 2017-08-30T16:18:23Z

Concept ACK - minimizing OpenSSL exposure is good.

ISC seems less strict than even MIT, appears compatible.

jtimon · 2017-08-31T19:20:07Z

Concept ACK

theuni · 2017-08-31T20:15:25Z

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 · 2017-09-01T16:02:45Z

@cfields you can review the ASM at least.

jonasschnelli · 2017-09-03T17:22:46Z

utACK 9c4f8b5188a53f39c0849f860c8227fbb5a77bc6

laanwj · 2017-09-05T21:51:44Z

src/support/cleanse.cpp

What does a bare __asm do?

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.

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 · 2017-09-05T21:52:41Z

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

kallewoof

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

  {
    char x[32];
    sprintf(x, "1234567890123456789012345678901");
    memory_cleanse(x, 32);
  }

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

kallewoof · 2017-09-06T03:33:44Z

src/support/cleanse.cpp

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

practicalswift · 2017-09-06T11:39:47Z

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 · 2017-09-06T13:58:33Z

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

…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

TheBlueMatt · 2017-09-07T20:35:29Z

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 · 2017-09-07T20:44:21Z

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 · 2017-09-15T23:27:21Z

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 · 2017-09-15T23:35:21Z

I think this is fine as-is.

utACK 1444c2e

laanwj · 2017-09-16T05:16:11Z

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 1444c2e

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

…ensure memory clearing even with -lto 1444c2e Switch memory_cleanse implementation to BoringSSL's to ensure memory clearing even with link-time optimization. (Adam Langley) Pull request description: 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 Tree-SHA512: 8134998663c1501e3ce48fbbd6ab41de981f0855e3f4d25d2e86ff8056c917d82c751c88e9c39660319ebfbc8283dce594c3e4fc7f87080a212a2cdba57ea511

…L's to ensure memory clearing even with -lto 1444c2e Switch memory_cleanse implementation to BoringSSL's to ensure memory clearing even with link-time optimization. (Adam Langley) Pull request description: 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 Tree-SHA512: 8134998663c1501e3ce48fbbd6ab41de981f0855e3f4d25d2e86ff8056c917d82c751c88e9c39660319ebfbc8283dce594c3e4fc7f87080a212a2cdba57ea511

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.

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.

Summary: ...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 ``` Backport of core [[bitcoin/bitcoin#11196 | PR11196]]. Test Plan: ninja all check Run the windows Gitian build. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6135

sipa reviewed Aug 30, 2017

View reviewed changes

maaku force-pushed the fix-memory-clense branch from 084674d to 9c4f8b5 Compare August 30, 2017 07:27

fanquake added the Refactoring label Aug 30, 2017

dcousens approved these changes Sep 5, 2017

View reviewed changes

laanwj reviewed Sep 5, 2017

View reviewed changes

kallewoof reviewed Sep 6, 2017

View reviewed changes

maaku force-pushed the fix-memory-clense branch from 16770aa to 1444c2e Compare September 6, 2017 20:46

laanwj merged commit 1444c2e into bitcoin:master Sep 16, 2017

theuni mentioned this pull request Oct 25, 2017

Minimal code changes to allow msvc compilation #11558

Merged

IgorDurovic mentioned this pull request Apr 2, 2018

Utils and libraries: back porting bitcoin RNG improvement PIVX-Project/PIVX#576

Closed

SkinnyPeteTheGiraffe mentioned this pull request May 18, 2018

Add Cleanse Object to Reduce Calls To OPENSSL_cleanse To A Single Place SlateTeam/SlatechainCore#1

Open

Warrows mentioned this pull request Oct 28, 2018

[Refactoring] Abstract out and switch openssl cleanse PIVX-Project/PIVX#761

Merged

codablock mentioned this pull request Dec 6, 2018

Serialize the block header in CBlockHeader::GetHash() dashpay/dash#2531

Merged

UdjinM6 mentioned this pull request Jul 20, 2019

Backport chacha-poly1305, prepare for V2 P2P Encrypted Messaging dashpay/dash#3032

Merged

str4d mentioned this pull request Apr 30, 2020

memory_cleanse backports zcash/zcash#4482

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

maaku commented Aug 30, 2017

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

sipa Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

sipa Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

maaku commented Aug 30, 2017

Uh oh!

practicalswift commented Aug 30, 2017

Uh oh!

kallewoof commented Aug 30, 2017

Uh oh!

Leviathn commented Aug 30, 2017

Uh oh!

jtimon commented Aug 31, 2017

Uh oh!

theuni commented Aug 31, 2017

Uh oh!

gmaxwell commented Sep 1, 2017

Uh oh!

jonasschnelli commented Sep 3, 2017

Uh oh!

laanwj Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

sipa Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 5, 2017

Uh oh!

kallewoof left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Sep 7, 2017

Uh oh!

laanwj commented Sep 7, 2017

Uh oh!

maaku commented Sep 15, 2017

Uh oh!

sipa commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

kallewoof left a comment •

edited

Loading

practicalswift commented Sep 6, 2017 •

edited

Loading

kallewoof commented Sep 6, 2017 •

edited

Loading

sipa commented Sep 15, 2017 •

edited

Loading

laanwj commented Sep 16, 2017 •

edited

Loading