Skip to content

Commit

Permalink
Merge bitcoin-core/secp256k1#1212: Prevent dead-store elimination whe…
Browse files Browse the repository at this point in the history
…n clearing secrets in examples

5660c13 prevent optimization in algorithms (Harshil Jani)

Pull request description:

  Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>

ACKs for top commit:
  sipa:
    utACK 5660c13
  real-or-random:
    utACK 5660c13

Tree-SHA512: 90024b7445c04e18a88af4099fc1ac6d1b9b2309b88dd22ae2b1f50aed7bac28b2c180cc28e1a95d5e9ec94b4c4adc44b9ada1477e6abe8efae7884c2382645c
  • Loading branch information
real-or-random committed Mar 2, 2023
2 parents 09b1d46 + 5660c13 commit 5757318
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 17 deletions.
2 changes: 1 addition & 1 deletion Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ noinst_HEADERS += contrib/lax_der_parsing.h
noinst_HEADERS += contrib/lax_der_parsing.c
noinst_HEADERS += contrib/lax_der_privatekey_parsing.h
noinst_HEADERS += contrib/lax_der_privatekey_parsing.c
noinst_HEADERS += examples/random.h
noinst_HEADERS += examples/examples_util.h

PRECOMPUTED_LIB = libsecp256k1_precomputed.la
noinst_LTLIBRARIES = $(PRECOMPUTED_LIB)
Expand Down
13 changes: 6 additions & 7 deletions examples/ecdh.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
#include <secp256k1.h>
#include <secp256k1_ecdh.h>

#include "random.h"

#include "examples_util.h"

int main(void) {
unsigned char seckey1[32];
Expand Down Expand Up @@ -112,12 +111,12 @@ int main(void) {
* example through "out of bounds" array access (see Heartbleed), Or the OS
* swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
*
* TODO: Prevent these writes from being optimized out, as any good compiler
* Here we are preventing these writes from being optimized out, as any good compiler
* will remove any writes that aren't used. */
memset(seckey1, 0, sizeof(seckey1));
memset(seckey2, 0, sizeof(seckey2));
memset(shared_secret1, 0, sizeof(shared_secret1));
memset(shared_secret2, 0, sizeof(shared_secret2));
secure_erase(seckey1, sizeof(seckey1));
secure_erase(seckey2, sizeof(seckey2));
secure_erase(shared_secret1, sizeof(shared_secret1));
secure_erase(shared_secret2, sizeof(shared_secret2));

return 0;
}
8 changes: 3 additions & 5 deletions examples/ecdsa.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@

#include <secp256k1.h>

#include "random.h"


#include "examples_util.h"

int main(void) {
/* Instead of signing the message directly, we must sign a 32-byte hash.
Expand Down Expand Up @@ -133,9 +131,9 @@ int main(void) {
* example through "out of bounds" array access (see Heartbleed), Or the OS
* swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
*
* TODO: Prevent these writes from being optimized out, as any good compiler
* Here we are preventing these writes from being optimized out, as any good compiler
* will remove any writes that aren't used. */
memset(seckey, 0, sizeof(seckey));
secure_erase(seckey, sizeof(seckey));

return 0;
}
29 changes: 29 additions & 0 deletions examples/random.h → examples/examples_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,32 @@ static void print_hex(unsigned char* data, size_t size) {
}
printf("\n");
}

#if defined(_MSC_VER)
// For SecureZeroMemory
#include <Windows.h>
#endif
/* Cleanses memory to prevent leaking sensitive info. Won't be optimized out. */
static SECP256K1_INLINE void secure_erase(void *ptr, size_t len) {
#if defined(_MSC_VER)
/* SecureZeroMemory is guaranteed not to be optimized out by MSVC. */
SecureZeroMemory(ptr, len);
#elif defined(__GNUC__)
/* We use a memory barrier that scares the compiler away from optimizing out the memset.
*
* Quoting Adam Langley <agl@google.com> in commit ad1907fe73334d6c696c8539646c21b11178f20f
* in BoringSSL (ISC License):
* As best as we can tell, this is sufficient to break any optimisations that
* might try to eliminate "superfluous" memsets.
* This method used in memzero_explicit() the Linux kernel, too. Its advantage is that it is
* pretty efficient, because the compiler can still implement the memset() efficently,
* just not remove it entirely. See "Dead Store Elimination (Still) Considered Harmful" by
* Yang et al. (USENIX Security 2017) for more background.
*/
memset(ptr, 0, len);
__asm__ __volatile__("" : : "r"(ptr) : "memory");
#else
void *(*volatile const volatile_memset)(void *, int, size_t) = memset;
volatile_memset(ptr, 0, len);
#endif
}
7 changes: 3 additions & 4 deletions examples/schnorr.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#include <secp256k1_extrakeys.h>
#include <secp256k1_schnorrsig.h>

#include "random.h"
#include "examples_util.h"

int main(void) {
unsigned char msg[12] = "Hello World!";
Expand Down Expand Up @@ -149,9 +149,8 @@ int main(void) {
* example through "out of bounds" array access (see Heartbleed), Or the OS
* swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
*
* TODO: Prevent these writes from being optimized out, as any good compiler
* Here we are preventing these writes from being optimized out, as any good compiler
* will remove any writes that aren't used. */
memset(seckey, 0, sizeof(seckey));

secure_erase(seckey, sizeof(seckey));
return 0;
}

0 comments on commit 5757318

Please sign in to comment.