Clear sensitive memory without getting optimized out (revival of #636) #1579
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
theStack
force-pushed
the
revival_of_pr636_cleanse
branch
from
deebc69
to
abb3be2
Compare
There was a problem hiding this comment.
Concept ACK (obviously)
Thanks for reviving this, I never had the time/motivation to come back to this PR, but it's important.
We should call SECP256K1_CHECKMEM_UNDEFINE (
Line 27 in b307614
secp256k1_memclear after clearing the memory. Reading cleared memory would clearly be a bug.
This code is not supposed to handle secret data.
We rely on memset() and an __asm__ memory barrier where it's available or on SecureZeroMemory() on Windows. The fallback implementation uses a volatile function pointer to memset which the compiler is not clever enough to optimize.
There are two uses of the secp256k1_fe_clear() function that are now separated
into these two functions in order to reflect the intent:
1) initializing the memory prior to being used -> converted to fe_set_int( . , 0 )
2) zeroing the memory after being used such that no sensitive data remains. ->
remains as fe_clear()
In the latter case, 'magnitude' and 'normalized' need to be overwritten when
VERIFY is enabled.
Co-Authored-By: isle2983 <[email protected]>
Co-Authored-By: isle2983 <[email protected]> Co-Authored-By: Pieter Wuille <[email protected]>
All of the invocations of secp256k1_memclear() operate on stack memory and happen after the function is done with the memory object. This commit replaces existing memset() invocations and also adds secp256k1_memclear() to code locations where clearing was missing; there is no guarantee that this commit covers all code locations where clearing is necessary. Co-Authored-By: isle2983 <[email protected]>
This gives the caller more control about whether the state should be cleaned (= should be considered secret), which will be useful for example for Schnorr signature verification in the future. Moreover, it gives the caller the possibility to clean a hash struct without finalizing it.
theStack
force-pushed
the
revival_of_pr636_cleanse
branch
from
abb3be2
to
ac0e41b
Compare
Thanks, added that, and rebased on master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR picks up #636 (which in turn picked up #448, so this is take number three) and is essentially a rebase on master.
Some changes to the original PR:
secp256k1_prefix again, since the related helper_memczerogot it as well (see PR Don't use reserved identifiers memczero and benchmark_verify_t #835 / commit e89278f)secp256k1_memclearis now also done on modules that have been newly introduced since then, i.e. schnorr and ellswift (of course, there is still no guarantee that all places where clearing is necessary are covered)So far I haven't looked at any disassembly and possible performance implications yet (there were some concerns expressed in #636 (comment)), happy to go deeper there if this gets Concept ACKed.
The proposed method of using a memory barrier to prevent optimizating away the memset is still used in BoringSSL (where it was originally picked up from) and in the Linux Kernel, see e.g. https://github.com/google/boringssl/blob/5af122c3dfc163b5d1859f1f450756e8e320a142/crypto/mem.c#L335 and https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/string.h#L348 / https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/compiler.h#L102
Fixes #185.