-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Clear sensitive memory without getting optimized out (revival of #636) #1579
base: master
Are you sure you want to change the base?
Conversation
deebc69
to
abb3be2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_CHECKMEM_UNDEFINE(p, len): |
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 <isle2983@yahoo.com>
Co-Authored-By: isle2983 <isle2983@yahoo.com> Co-Authored-By: Pieter Wuille <pieter.wuille@gmail.com>
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 <isle2983@yahoo.com>
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.
abb3be2
to
ac0e41b
Compare
Thanks, added that, and rebased on master. |
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_memczero
got it as well (see PR Don't use reserved identifiers memczero and benchmark_verify_t #835 / commit e89278f)secp256k1_memclear
is 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.