Fix logic of memory_cleanse() on MSVC and clean up docs #16158

real-or-random · 2019-06-06T10:13:08Z

When working on bitcoin-core/secp256k1#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.

Commit fbf327b ("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.

laanwj · 2019-06-06T10:20:03Z

This is one of the things that has been 'fixed' so many times, can we please be sure it's okay this time?
(change looks OK to me, though I don't think clearing twice is that bad, it can't hurt at least)

real-or-random · 2019-06-06T10:23:05Z

Yeah, so there is no 100% optimal solution for clearing memory, but please note that the goal of this PR is not to start this discussion about what the right method is. This merely fixes a weirdness on in the logic introduced by #11558.

practicalswift · 2019-06-06T14:43:51Z

utACK 751aff1 (read code - looks correct)

maflcko · 2019-06-06T14:55:49Z

Fine on changing the docs, but is it really necessary to remove the redundant std::memset(ptr, 0, len).

real-or-random · 2019-06-06T15:21:48Z

Well, as you say, the call is redundant, so it's okay not to remove it but I'd argue it's desirable to remove it

for readability: It just looks wrong to do the same operation twice. I think people would complain about code that does x = 0; x = 0;.
for efficiency: While MSVC inlines the call to SecureZeroMemory, it is not clever enough to optimize the call to memset out, see https://godbolt.org/z/Y2TIWC.

src/support/cleanse.cpp

practicalswift · 2019-06-07T13:56:37Z

ACK 4e1941e (read code - looks correct)

jonasnick

ACK read the code and the comments. Comments are way better than before. The cited paper ("Dead Store Elimination (Still) Considered Harmful") is an excellent resource.

src/support/cleanse.cpp

practicalswift · 2019-06-27T22:30:43Z

@real-or-random Would you mind fixing the typo? After that I think this should be ready for merge :-)

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.

real-or-random · 2019-07-01T11:01:30Z

@practicalswift @jonasnick
I fixed the typo, can you reACK?

practicalswift · 2019-07-01T11:27:07Z

utACK f53a70c :-)

laanwj · 2019-07-03T11:02:32Z

code review ACK f53a70c

f53a70c Improve documentation of memory_cleanse() (Tim Ruffing) cac30a4 Clean up logic in memory_cleanse() for MSVC (Tim Ruffing) Pull request description: When working on bitcoin-core/secp256k1#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 f53a70c :-) laanwj: code review ACK f53a70c Tree-SHA512: 1c2fd98ae62b34b3e6e59d1178b293af969a9e06cbb7df02a699ce8802f145a336f72edb178c520e3ecec81f7e8083828f90a5ba6367d966a2c7d7c0dd6c0475

laanwj · 2019-07-03T11:10:14Z

Seeing the merge message this PR title is badly worded: yes, you can consider clearing the memory twice a logic error, but most people would consider it a logic error if it would result in not clearing the memory.

So to be 100% clear for anyone getting here: this is not a security fix.

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: bitcoin/bitcoin@cac30a4 > Commit fbf327b ("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. bitcoin/bitcoin@f53a70c > 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. This is a backport of Core [[bitcoin/bitcoin#16158 | PR16158]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8375

…up docs f53a70c Improve documentation of memory_cleanse() (Tim Ruffing) cac30a4 Clean up logic in memory_cleanse() for MSVC (Tim Ruffing) Pull request description: When working on bitcoin-core/secp256k1#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 bitcoin#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 f53a70c :-) laanwj: code review ACK f53a70c Tree-SHA512: 1c2fd98ae62b34b3e6e59d1178b293af969a9e06cbb7df02a699ce8802f145a336f72edb178c520e3ecec81f7e8083828f90a5ba6367d966a2c7d7c0dd6c0475

fanquake added the Utils/log/libs label Jun 6, 2019

fanquake assigned fanquake and unassigned fanquake Jun 6, 2019

fanquake added the Windows label Jun 6, 2019

practicalswift reviewed Jun 6, 2019

View reviewed changes

src/support/cleanse.cpp Outdated Show resolved Hide resolved

real-or-random force-pushed the cleanup-cleanse branch from 751aff1 to 4e1941e Compare June 7, 2019 13:35

jonasnick approved these changes Jun 25, 2019

View reviewed changes

src/support/cleanse.cpp Outdated Show resolved Hide resolved

real-or-random force-pushed the cleanup-cleanse branch from 4e1941e to f53a70c Compare July 1, 2019 11:00

laanwj merged commit f53a70c into bitcoin:master Jul 3, 2019

real-or-random mentioned this pull request Jul 5, 2019

Increase robustness against UB in secp256k1_scalar_cadd_bit bitcoin-core/secp256k1#647

Merged

real-or-random mentioned this pull request Nov 25, 2019

Is the compiler optimizing out some of the benchmarks? bitcoin-core/secp256k1#667

Closed

str4d mentioned this pull request Apr 30, 2020

memory_cleanse backports zcash/zcash#4482

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix logic of memory_cleanse() on MSVC and clean up docs #16158

Fix logic of memory_cleanse() on MSVC and clean up docs #16158

Uh oh!

real-or-random commented Jun 6, 2019

Uh oh!

laanwj commented Jun 6, 2019 •

edited

Loading

Uh oh!

real-or-random commented Jun 6, 2019

Uh oh!

practicalswift commented Jun 6, 2019

Uh oh!

maflcko commented Jun 6, 2019

Uh oh!

real-or-random commented Jun 6, 2019 •

edited

Loading

Uh oh!

Uh oh!

practicalswift commented Jun 7, 2019

Uh oh!

jonasnick left a comment

Uh oh!

Uh oh!

practicalswift commented Jun 27, 2019

Uh oh!

real-or-random commented Jul 1, 2019

Uh oh!

practicalswift commented Jul 1, 2019

Uh oh!

laanwj commented Jul 3, 2019

Uh oh!

laanwj commented Jul 3, 2019

Uh oh!

Uh oh!

Fix logic of memory_cleanse() on MSVC and clean up docs #16158

Fix logic of memory_cleanse() on MSVC and clean up docs #16158

Uh oh!

Conversation

real-or-random commented Jun 6, 2019

Uh oh!

laanwj commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

real-or-random commented Jun 6, 2019

Uh oh!

practicalswift commented Jun 6, 2019

Uh oh!

maflcko commented Jun 6, 2019

Uh oh!

real-or-random commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jun 7, 2019

Uh oh!

jonasnick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Jun 27, 2019

Uh oh!

real-or-random commented Jul 1, 2019

Uh oh!

practicalswift commented Jul 1, 2019

Uh oh!

laanwj commented Jul 3, 2019

Uh oh!

laanwj commented Jul 3, 2019

Uh oh!

Uh oh!

laanwj commented Jun 6, 2019 •

edited

Loading

real-or-random commented Jun 6, 2019 •

edited

Loading