Clear sensitive memory without getting optimized out (revival of #636) #1579

1. 
   theStack commented at 9:10 pm on August 6, 2024: contributor

   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:

   • the clearing function now has the secp256k1_ prefix again, since the related helper _memczero got it as well (see PR #835 / commit e89278f211a526062745c391d48a7baf782b4b2b)
   • the original commit b17a7df8145a6a86d49c354c7e7b59a432ea5346 (“Make _set_fe_int( . , 0 ) set magnitude to 0”) is not needed anymore, since it was already applied in PR #943 (commit d49011f54c2b31807158bdf06364f331558cccc7)
   • clearing of stack memory with 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.

2. theStack force-pushed on Aug 6, 2024
3. real-or-random added the label side-channel on Aug 17, 2024
4. 
   real-or-random commented at 2:09 pm on August 17, 2024: contributor

   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 (https://github.com/bitcoin-core/secp256k1/blob/b307614401790850b48fb3ba878247290857a975/src/checkmem.h#L27) in secp256k1_memclear after clearing the memory. Reading cleared memory would clearly be a bug.

5. Don't clear secrets in pippenger implementation

   This code is not supposed to handle secret data.

   412a82f080
6. Add secp256k1_memclear() for clearing 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.

   3818a68926
7. Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear()

   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>

   7a59878769
8. Separate between clearing memory and setting to zero in tests

   Co-Authored-By: isle2983 <isle2983@yahoo.com>
   Co-Authored-By: Pieter Wuille <pieter.wuille@gmail.com>

   15e8cdda3e
9. Use secp256k1_memclear() to clear stack memory instead of memset()

   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>

   6fcbae954d
10. Implement various _clear() functions with secp256k1_memclear() c65befcca3
11. Don't rely on memset to set signed integers to 0 9afa068810
12. Introduce separate _clear functions for hash module

    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.

    ac0e41b5b7
13. theStack force-pushed on Aug 20, 2024
14. 
    theStack commented at 10:23 am on August 20, 2024: contributor

    We should call SECP256K1_CHECKMEM_UNDEFINE (

    https://github.com/bitcoin-core/secp256k1/blob/b307614401790850b48fb3ba878247290857a975/src/checkmem.h#L27

    ) in secp256k1_memclear after clearing the memory. Reading cleared memory would clearly be a bug.

    Thanks, added that, and rebased on master.

Contributors
theStack real-or-random

Labels
side-channel