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

real-or-random commented at 10:13 AM on June 6, 2019: member

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.

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.

cac30a436c

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

fanquake assigned fanquake on Jun 6, 2019

fanquake unassigned fanquake on Jun 6, 2019

fanquake added the label Windows on Jun 6, 2019

laanwj commented at 10:20 AM on June 6, 2019: member

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 commented at 10:23 AM on June 6, 2019: member

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 commented at 2:43 PM on June 6, 2019: contributor

utACK 751aff1e50ec75f1f57dd72640eaca9a22ee7406 (read code - looks correct)

MarcoFalke commented at 2:55 PM on June 6, 2019: member

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

real-or-random commented at 3:21 PM on June 6, 2019: member

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.

in src/support/cleanse.cpp:29 in 751aff1e50 outdated

  46 | +     * Quoting Adam Langley <agl@google.com> in commit ad1907fe73334d6c696c8539646c21b11178f20f
  47 | +     * in BoringSSL (ISC License):
  48 | +     *    As best as we can tell, this is sufficient to break any optimisations that
  49 | +     *    might try to eliminate "superfluous" memsets.
  50 | +     * This method used in memzero_explicit() the Linux kernel, too. Its advantage is that it is
  51 | +     * pretty efficient, because the compiler can still implement the memset() efficently,

practicalswift commented at 10:41 PM on June 6, 2019:

"Efficently" should be efficiently :-)

real-or-random commented at 1:36 PM on June 7, 2019:

fixed with force-push

real-or-random force-pushed on Jun 7, 2019

practicalswift commented at 1:56 PM on June 7, 2019: contributor

ACK 4e1941e8bd3a72adb2291af8148bee8a508bcd95 (read code - looks correct)

in src/support/cleanse.cpp:28 in 4e1941e8bd outdated

  43 | +     *
  44 | +     * Quoting Adam Langley <agl@google.com> in commit ad1907fe73334d6c696c8539646c21b11178f20f
  45 | +     * in BoringSSL (ISC License):
  46 | +     *    As best as we can tell, this is sufficient to break any optimisations that
  47 | +     *    might try to eliminate "superfluous" memsets.
  48 | +     * This method used in memzero_explicit() the Linux kernel, too. Its advantage is that it is

jonasnick commented at 12:10 PM on June 25, 2019:

nit: I can't parse the first sentence - perhaps missing a verb and "in"?

jonasnick approved

jonasnick commented at 12:13 PM on June 25, 2019: contributor

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.

practicalswift commented at 10:30 PM on June 27, 2019: contributor

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

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.

f53a70ce95

real-or-random force-pushed on Jul 1, 2019

real-or-random commented at 11:01 AM on July 1, 2019: member

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

practicalswift commented at 11:27 AM on July 1, 2019: contributor

utACK f53a70ce95231d34bb14cd6c58503927e8d7ff59 :-)

laanwj commented at 11:02 AM on July 3, 2019: member

code review ACK f53a70ce95231d34bb14cd6c58503927e8d7ff59

laanwj merged this on Jul 3, 2019

laanwj closed this on Jul 3, 2019

laanwj referenced this in commit 7f985d6c81 on Jul 3, 2019

laanwj commented at 11:10 AM on July 3, 2019: member

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.

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

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

jasonbcox referenced this in commit a0384bda1e on Nov 13, 2020

PastaPastaPasta referenced this in commit 7efdda4676 on Jun 27, 2021

PastaPastaPasta referenced this in commit 5c0bf5a9fb on Jun 28, 2021

PastaPastaPasta referenced this in commit f95eee6bf5 on Jun 29, 2021

PastaPastaPasta referenced this in commit 38567ca4d7 on Jul 1, 2021

PastaPastaPasta referenced this in commit c2143f62fc on Jul 1, 2021

PastaPastaPasta referenced this in commit 99ec02f43c on Jul 12, 2021

PastaPastaPasta referenced this in commit 6fd162e296 on Jul 13, 2021

DrahtBot locked this on Dec 16, 2021