From 4c433823a85cac975b0746203d94ce041c10299d Mon Sep 17 00:00:00 2001 From: Elichai Turkel Date: Mon, 17 Jan 2022 17:35:57 +0200 Subject: [PATCH] Fix review comments by Sipa --- examples/ecdh.c | 45 ++++++++++++++++++++++++++++----------------- examples/ecdsa.c | 34 ++++++++++++++++++++-------------- examples/random.h | 1 + examples/schnorr.c | 41 ++++++++++++++++++++++------------------- 4 files changed, 71 insertions(+), 50 deletions(-) diff --git a/examples/ecdh.c b/examples/ecdh.c index 44fc3ffac1..74fd5a0409 100644 --- a/examples/ecdh.c +++ b/examples/ecdh.c @@ -8,9 +8,10 @@ #include #include +#include +#include + #include "random.h" -#include "secp256k1.h" -#include "secp256k1_ecdh.h" int main(void) { @@ -21,13 +22,14 @@ int main(void) { unsigned char shared_secret1[32]; unsigned char shared_secret2[32]; unsigned char randomize[32]; + int return_val; size_t len; secp256k1_pubkey pubkey1; secp256k1_pubkey pubkey2; - /* The docs in secp256k1.h above the `secp256k1_ec_pubkey_create` function - * say: "pointer to a context object, initialized for signing" which is why - * we create a context for signing with the SECP256K1_CONTEXT_SIGN flag. + /* The specification in secp256k1.h states that `secp256k1_ec_pubkey_create` + * needs a context object initialized for signing, which is why we create + * a context with the SECP256K1_CONTEXT_SIGN flag. * (The docs for `secp256k1_ecdh` don't require any special context, just * some initialized context) */ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN); @@ -38,7 +40,8 @@ int main(void) { /* Randomizing the context is recommended to protect against side-channel * leakage See `secp256k1_context_randomize` in secp256k1.h for more * information about it. This should never fail. */ - assert(secp256k1_context_randomize(ctx, randomize)); + return_val = secp256k1_context_randomize(ctx, randomize); + assert(return_val); /*** Key Generation ***/ @@ -56,32 +59,40 @@ int main(void) { } /* Public key creation using a valid context with a verified secret key should never fail */ - assert(secp256k1_ec_pubkey_create(ctx, &pubkey1, seckey1)); - assert(secp256k1_ec_pubkey_create(ctx, &pubkey2, seckey2)); + return_val = secp256k1_ec_pubkey_create(ctx, &pubkey1, seckey1); + assert(return_val); + return_val = secp256k1_ec_pubkey_create(ctx, &pubkey2, seckey2); + assert(return_val); /* Serialize pubkey1 in a compressed form (33 bytes), should always return 1 */ len = sizeof(compressed_pubkey1); - assert(secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey1, &len, &pubkey1, SECP256K1_EC_COMPRESSED)); + return_val = secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey1, &len, &pubkey1, SECP256K1_EC_COMPRESSED); + assert(return_val); /* Should be the same size as the size of the output, because we passed a 33 bytes array. */ assert(len == sizeof(compressed_pubkey1)); /* Serialize pubkey2 in a compressed form (33 bytes) */ len = sizeof(compressed_pubkey2); - secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey2, &len, &pubkey2, SECP256K1_EC_COMPRESSED); + return_val = secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey2, &len, &pubkey2, SECP256K1_EC_COMPRESSED); + assert(return_val); + /* Should be the same size as the size of the output, because we passed a 33 bytes array. */ assert(len == sizeof(compressed_pubkey2)); /*** Creating the shared secret ***/ /* Perform ECDH with seckey1 and pubkey2. Should never fail with a verified * seckey and valid pubkey */ - assert(secp256k1_ecdh(ctx, shared_secret1, &pubkey2, seckey1, NULL, NULL)); + return_val = secp256k1_ecdh(ctx, shared_secret1, &pubkey2, seckey1, NULL, NULL); + assert(return_val); /* Perform ECDH with seckey2 and pubkey1. Should never fail with a verified * seckey and valid pubkey */ - assert(secp256k1_ecdh(ctx, shared_secret2, &pubkey1, seckey2, NULL, NULL)); + return_val = secp256k1_ecdh(ctx, shared_secret2, &pubkey1, seckey2, NULL, NULL); + assert(return_val); /* Both parties should end up with the same shared secret */ - assert(memcmp(shared_secret1, shared_secret2, sizeof(shared_secret1)) == 0); + return_val = memcmp(shared_secret1, shared_secret2, sizeof(shared_secret1)); + assert(return_val == 0); printf("Secret Key1: "); print_hex(seckey1, sizeof(seckey1)); @@ -97,10 +108,10 @@ int main(void) { /* This will clear everything from the context and free the memory */ secp256k1_context_destroy(ctx); - /* It's best practice to try to remove secrets from memory after using them. - * This is done because some bugs can allow an attacker leak memory, for - * example through out of bounds array access (see Heartbleed for example). - * Hence, we overwrite the secret key buffer with zeros. + /* It's best practice to try to clear secrets from memory after using them. + * This is done because some bugs can allow an attacker to leak memory, for + * 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 * will remove any writes that aren't used. */ diff --git a/examples/ecdsa.c b/examples/ecdsa.c index c65e3b0184..924d021dd7 100644 --- a/examples/ecdsa.c +++ b/examples/ecdsa.c @@ -8,8 +8,9 @@ #include #include +#include + #include "random.h" -#include "secp256k1.h" @@ -21,12 +22,12 @@ int main(void) { unsigned char serialized_signature[64]; size_t len; int is_signature_valid; + int return_val; secp256k1_pubkey pubkey; secp256k1_ecdsa_signature sig; - /* The docs in secp256k1.h above the `secp256k1_ec_pubkey_create` function - * say: "pointer to a context object, initialized for signing" And the docs - * above the `secp256k1_ecdsa_verify` function say: "a secp256k1 context - * object, initialized for verification" which is why we create a context + /* The specification in secp256k1.h states that `secp256k1_ec_pubkey_create` needs + * a context object initialized for signing and `secp256k1_ecdsa_verify` needs + * a context initialized for verification, which is why we create a context * for both signing and verification with the SECP256K1_CONTEXT_SIGN and * SECP256K1_CONTEXT_VERIFY flags. */ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY); @@ -37,7 +38,8 @@ int main(void) { /* Randomizing the context is recommended to protect against side-channel * leakage See `secp256k1_context_randomize` in secp256k1.h for more * information about it. This should never fail. */ - assert(secp256k1_context_randomize(ctx, randomize)); + return_val = secp256k1_context_randomize(ctx, randomize); + assert(return_val); /*** Key Generation ***/ @@ -55,11 +57,13 @@ int main(void) { } /* Public key creation using a valid context with a verified secret key should never fail */ - assert(secp256k1_ec_pubkey_create(ctx, &pubkey, seckey)); + return_val = secp256k1_ec_pubkey_create(ctx, &pubkey, seckey); + assert(return_val); /* Serialize the pubkey in a compressed form(33 bytes). Should always return 1. */ len = sizeof(compressed_pubkey); - assert(secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey, &len, &pubkey, SECP256K1_EC_COMPRESSED)); + return_val = secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey, &len, &pubkey, SECP256K1_EC_COMPRESSED); + assert(return_val); /* Should be the same size as the size of the output, because we passed a 33 bytes array. */ assert(len == sizeof(compressed_pubkey)); @@ -70,11 +74,13 @@ int main(void) { * `noncefp` and `ndata` allows you to pass a custom nonce function, passing * `NULL` will use the RFC-6979 safe default. Signing with a valid context, * verified secret key and the default nonce function should never fail. */ - assert(secp256k1_ecdsa_sign(ctx, &sig, msg_hash, seckey, NULL, NULL)); + return_val = secp256k1_ecdsa_sign(ctx, &sig, msg_hash, seckey, NULL, NULL); + assert(return_val); /* Serialize the signature in a compact form. Should always return 1 * according to the documentation in secp256k1.h. */ - assert(secp256k1_ecdsa_signature_serialize_compact(ctx, serialized_signature, &sig)); + return_val = secp256k1_ecdsa_signature_serialize_compact(ctx, serialized_signature, &sig); + assert(return_val); /*** Verification ***/ @@ -108,10 +114,10 @@ int main(void) { /* This will clear everything from the context and free the memory */ secp256k1_context_destroy(ctx); - /* It's best practice to try to remove secrets from memory after using them. - * This is done because some bugs can allow an attacker leak memory, for - * example through out of bounds array access (see Heartbleed for example). - * Hence, we overwrite the secret key buffer with zeros. + /* It's best practice to try to clear secrets from memory after using them. + * This is done because some bugs can allow an attacker to leak memory, for + * 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 * will remove any writes that aren't used. */ diff --git a/examples/random.h b/examples/random.h index 64ed9ac3c0..439226f09f 100644 --- a/examples/random.h +++ b/examples/random.h @@ -7,6 +7,7 @@ /* * This file is an attempt at collecting best practice methods for obtaining randomness with different operating systems. * It may be out-of-date. Consult the documentation of the operating system before considering to use the methods below. + * * Platform randomness sources: * Linux -> `getrandom(2)`(`sys/random.h`), if not available `/dev/urandom` should be used. http://man7.org/linux/man-pages/man2/getrandom.2.html, https://linux.die.net/man/4/urandom * macOS -> `getentropy(2)`(`sys/random.h`), if not available `/dev/urandom` should be used. https://www.unix.com/man-page/mojave/2/getentropy, https://opensource.apple.com/source/xnu/xnu-517.12.7/bsd/man/man4/random.4.auto.html diff --git a/examples/schnorr.c b/examples/schnorr.c index aa96ddb37b..6526242cc2 100644 --- a/examples/schnorr.c +++ b/examples/schnorr.c @@ -8,11 +8,11 @@ #include #include -#include "random.h" -#include "secp256k1.h" -#include "secp256k1_extrakeys.h" -#include "secp256k1_schnorrsig.h" +#include +#include +#include +#include "random.h" int main(void) { unsigned char msg_hash[32] = {0}; /* This should be a hash of the message. */ @@ -22,14 +22,14 @@ int main(void) { unsigned char serialized_pubkey[32]; unsigned char signature[64]; int is_signature_valid; + int return_val; secp256k1_xonly_pubkey pubkey; secp256k1_keypair keypair; - /* The docs in secp256k1_extrakeys.h above the `secp256k1_keypair_create` - * function say: "pointer to a context object, initialized for signing" And - * the docs above the `secp256k1_schnorrsig_verify` function say: "a - * secp256k1 context object, initialized for verification" which is why we - * create a context for both signing and verification with the - * SECP256K1_CONTEXT_SIGN and SECP256K1_CONTEXT_VERIFY flags. */ + /* The specification in secp256k1_extrakeys.h states that `secp256k1_keypair_create` + * needs a context object initialized for signing. And in secp256k1_schnorrsig.h + * they state that `secp256k1_schnorrsig_verify` needs a context initialized for + * verification, which is why we create a context for both signing and verification + * with the SECP256K1_CONTEXT_SIGN and SECP256K1_CONTEXT_VERIFY flags. */ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY); if (!fill_random(randomize, sizeof(randomize))) { printf("Failed to generate randomness\n"); @@ -38,8 +38,8 @@ int main(void) { /* Randomizing the context is recommended to protect against side-channel * leakage See `secp256k1_context_randomize` in secp256k1.h for more * information about it. This should never fail. */ - assert(secp256k1_context_randomize(ctx, randomize)); - + return_val = secp256k1_context_randomize(ctx, randomize); + assert(return_val); /*** Key Generation ***/ /* If the secret key is zero or out of range (bigger than secp256k1's @@ -61,10 +61,12 @@ int main(void) { * `pk_parity` as we don't care about the parity of the key, only advanced * users might care about the parity. This should never fail with a valid * context and public key. */ - assert(secp256k1_keypair_xonly_pub(ctx, &pubkey, NULL, &keypair)); + return_val = secp256k1_keypair_xonly_pub(ctx, &pubkey, NULL, &keypair); + assert(return_val); /* Serialize the public key. Should always return 1 for a valid public key. */ - assert(secp256k1_xonly_pubkey_serialize(ctx, serialized_pubkey, &pubkey)); + return_val = secp256k1_xonly_pubkey_serialize(ctx, serialized_pubkey, &pubkey); + assert(return_val); /*** Signing ***/ @@ -80,7 +82,8 @@ int main(void) { * improve security against side-channel attacks. Signing with a valid * context, verified keypair and the default nonce function should never * fail. */ - assert(secp256k1_schnorrsig_sign(ctx, signature, msg_hash, &keypair, auxiliary_rand)); + return_val = secp256k1_schnorrsig_sign(ctx, signature, msg_hash, &keypair, auxiliary_rand); + assert(return_val); /*** Verification ***/ @@ -106,10 +109,10 @@ int main(void) { /* This will clear everything from the context and free the memory */ secp256k1_context_destroy(ctx); - /* It's best practice to try to remove secrets from memory after using them. - * This is done because some bugs can allow an attacker leak memory, for - * example through out of bounds array access (see Heartbleed for example). - * Hence, we overwrite the secret key buffer with zeros. + /* It's best practice to try to clear secrets from memory after using them. + * This is done because some bugs can allow an attacker to leak memory, for + * 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 * will remove any writes that aren't used. */