Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory zeroization improvements #185

Open
gmaxwell opened this issue Jan 11, 2015 · 7 comments · May be fixed by #636
Open

Memory zeroization improvements #185

gmaxwell opened this issue Jan 11, 2015 · 7 comments · May be fixed by #636

Comments

@gmaxwell
Copy link
Contributor

Existing 'best effort' zeriozation for private data is hardly even best effort. At a minimum we should consider doing this via an extern-ed function and memset_s if available. No guarantees can still be provided, of course.

We might also consider wrapping the API entrance of private data handling functions like:

handle_data(){
ret=handle_data_impl();
handle_data_zero_stack();
return ret;
}

Where _zero_stack uses slightly more stack than the whole callgraph for _impl and zeros it, in order to catch private data spilled onto the stack during execution before returning outside of our control.

I'm not sure where exactly where the line between best effort and security theatre is... there is only so much that can really be done (esp in portable code) here.

@briansmith
Copy link
Contributor

I'm not sure what you mean by "no guarantees can be provided." C11 memset_s and SecureZeroMemory both guarantee zeroization when they are used, when they are available.

@sipa
Copy link
Contributor

sipa commented Apr 28, 2015 via email

@dcousens
Copy link
Contributor

guarantee that the compiler didn't copy parts of that data to other places
(like the stack).

@sipa can you ever make that guarantee? At least, not without checking the generated assembly every time?

@sipa
Copy link
Contributor

sipa commented Apr 28, 2015 via email

@gmaxwell
Copy link
Contributor Author

@briansmith As Pieter noted, the compiler can and will store arbitrary secret data in random places on the stack (in fact, it can even stash them in random globals and such). This isn't just hypothetical either.

It can also leave them sitting around in registers, and from there other code can save them onto the stack or other locations where they might escape.

I'm dubious about the value of having memset_s() in the standard: I'll gladly use it for better best effort; but I think the standards committee made an error in judgement-- since it leads to incorrect reasoning about what can actually be accomplished.

So the best we can do is best-effort; maybe best effort with some non-runtime tests (e.g. load secrets with known patterns and search the stack for them after execution). I'm all for best effort where better can't be done. Approaches like I suggested of flushing out the stack have a fighting chance at being reliable at least for now... though even that isn't guaranteed to work; e.g. you can't guarantee the compiler will give the zero_stack function accesses to the redzone on architectures that have them, if zero_stack is just plain old C.

@gmaxwell gmaxwell added this to the initial release milestone Aug 27, 2015
briansmith added a commit to briansmith/ring that referenced this issue Feb 5, 2016
Ultimately, it's better to invest effort in alternative forms of
protection of key material.

Calling `OPENSSL_cleanse` with a NULL pointer is not safe, but
`OPENSSL_cleanse` is often called in cleanup code, especially error-
handling code, where it is difficult to keep track of the NULLness of
things. The likelihood of getting this wrong is compounded by the fact
that, in OpenSSL upstream, calling `OPENSSL_cleanse(NULL, x)` for any
`x` is safe (a no-op). BoringSSL upstream doesn't want to change its
`OPENSSL_cleanse` to work like OpenSSL's. We don't want to worry about
the issue.

Apart from that, by inspection, it is clear that there are many places
in the code that don't call `OPENSSL_clease` where they "should". It
would be difficult to find all the places where a call to
`OPENSSL_clease` "should" be inserted. It is unlikely we'll ever get it
right. Actually, it's basically impossible to get it right using this
coding pattern. See
http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html
and bitcoin-core/secp256k1#185.

Besides all that, the zeroization isn't free. Especially in the case of
non-MSVC platforms, it either interferes with the optimizer or it
doesn't work. More importantly, thinking about how to make this
approach work wastes a lot of time that could be spent actually
improving the fundementals of the security of the code.
@real-or-random
Copy link
Contributor

Useful resources:
https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/yang
https://media.ccc.de/v/35c3-9788-memsad

So we all know that there no single right way of doing this. For building on #448 I think I'll switch to memset + memory barrier where this is available (GCC, clang) because it does not require external libraries incl. glibc, and the compiler can still optimize the memset (just not optimize it out). The USENIX paper confirms that this is good performance-wise.

As a fallback, we can try to use platform-specific functions or a volatile store, I need to see how ugly it gets.

By the way, Bitcoin Core currently uses memset + memory barrier or Windows's SecureZeroMemory:
https://github.com/bitcoin/bitcoin/blob/master/src/support/cleanse.cpp#L31
(However, note the weird logic: On Windows, the memory is cleared twice.)
This restricts Core to GCG, clang or MSVC. I think we want to be more portable in secp256k1.

@gmaxwell
Copy link
Contributor Author

gmaxwell commented Jun 5, 2019

ACK

laanwj added a commit to bitcoin/bitcoin that referenced this issue Jul 3, 2019
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
monstrobishi pushed a commit to DeFiCh/ain that referenced this issue Sep 6, 2020
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jun 27, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jun 28, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jun 29, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jul 1, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jul 1, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jul 12, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jul 13, 2021
…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
gades pushed a commit to cosanta/cosanta-core that referenced this issue Apr 29, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants