Resolves #184
I didn’t add a sign/verify/recover example yet because I’d prefer that the example included hashing a message, but we don’t provide a hash function so I decided to start only with key generation and ECDH, and get feedback.
I did not check the example on windows, but did try to implement it correctly, if anyone has access to a windows machine(and knows how to build on windows hehe) please test this :)
P.S. Should the usage examples be MIT or CC0(Public Domain)?
28+ secp256k1_pubkey pubkey2;
29+ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
30+ while (1) {
31+ if (!fill_random(seckey1, sizeof(seckey1)) || !fill_random(seckey2, sizeof(seckey2))) {
32+ printf("Failed to generate randomness\n");
33+ return 1;
25+ close(fd);
26+ if (res != (ssize_t)size) {
27+ return 0;
28+ }
29+#endif
30+ return 1;
#error at the top of the file if none were taken.
24+ unsigned char shared_secret1[32];
25+ unsigned char shared_secret2[32];
26+ size_t len;
27+ secp256k1_pubkey pubkey1;
28+ secp256k1_pubkey pubkey2;
29+ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
CONTEXT_SIGN flag is chosen?
15+ if (res != STATUS_SUCCESS) {
16+ return 0;
17+ }
18+#elif defined(__linux__)
19+ ssize_t res;
20+ int fd = open("/dev/urandom", O_RDONLY);
I really wanted to use getrandom but it’s not available in the libc in our travis.
We could add dist: bionic and that will probably solve this, but my thought was
“if even our travis doesn’t support it then we should probably not use this in the examples”
See: https://travis-ci.org/github/elichai/secp256k1/jobs/681424984#L403
Oh weird, it’s really not in Ubuntu LTS 16.04. You need glibc 2.25 at least. -.- I wanted to point out getrandom too…
I think we should at least add an explanatory comment or a link to an explanation. And to be honest, I’d prefer to use getrandom here and have an example that breaks compilation if it’s not available and refer people to /dev/urandom then.
At least LTS 16.04 will have “end of standard support” in 2021.
I thought about that, but how do you propose to do this? like should I involve autotools in this now?(to detect getrandom) I really preferred to keep the examples minimal and self standing.
another advantage of /dev/urandom is that I can just stick MacOS and BSD variants on it and it should work
like should I involve autotools in this now?(to detect getrandom) I really preferred to keep the examples minimal and self standing.
Hm ok, what I had in mind was rather not compiling the examples at all in our build system. But I see that this is not great either….
edit: see discussion below
0@@ -0,0 +1,31 @@
1+#if defined(_WIN32)
2+#include <bcrypt.h>
3+#elif defined(__linux__)
4+#include <fcntl.h>
5+#include <unistd.h>
6+#else
7+#error "Couldn't identify the OS"
I skipped BSD because I didn’t think anyone is really using it, but forgot about OSX. I’ll look into adding them both, see how compilcated it gets (I want to avoid something like https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp and only support mostly recent OS versions)
Now that I re-read my last sentence :) you might be right about not compiling them by default, I just find that whatever’s not compiled by default I always break and find out in the CI lol
I’m trying to redo the travis and at least add MacOS to the CI hmm stand alone make can be interesting, should that compile libsecp by it self? I can write something simple.
Otherwise we can still compile it in our CI, turn it off by default and bump our travis to bionic
getrandom if available, not compile by default, but add a travis test. Easy to revisit this later if there is demand for other systems but most important is the code, and not that it compiles on every user’s system.
Oh I didn’t see your comment here. I suggested Linux/MacOS/Windows here: #749 (comment) But I’d also be okay with just Linux. I think the main point is that we good documentation.
And I have a somewhat strong opinion on /dev/urandom vs getrandom. The issue with /dev/urandom is that it never blocks, i.e., it doesn’t block at early boot when there has never been enough entropy. That’s rare for Linux on the desktop but may be the case for a lightweight Linux on some device. So I’d really prefer getrandom and a comment to use /dev/urandom if getrandom is not available. If we don’t compile the examples, then it does not matter if it’s not available.
As I said, I’ll try to find some refs. If you want, go ahead with this PR and I’ll open a PR on top of it.
Very nice!
Alternatively the module-specific examples could be in their respective module directory. But I think either way is fine.
Alternatively the module-specific examples could be in their respective module directory. But I think either way is fine.
I thought about it, but I think that for newcomers it’s easier to see an examples directory at the top level and start from there.
70+ printf("Compressed Pubkey2: ");
71+ print_hex(compressed_pubkey2, sizeof(compressed_pubkey2));
72+ printf("\nShared Secret: ");
73+ print_hex(shared_secret1, sizeof(shared_secret1));
74+
75+
My feeling is this entire issue of secret key handling deserves a broader discussion.
We don’t have a key generation function and Core won’t need one. If we’re now targeting different users more, maybe it’s time to have one. But it’s hard, see all the discussion points above. And it really depends on the user’s platform. So actually I’d be happy with good examples and some references to correct methods to get randomness on different OSes.
On the other hand, nothing should stop us from exporting a cleaning function in #636, even if we don’t have one for key gen.
What are other libraries doing for key generation?
19+ unsigned char compressed_pubkey[33];
20+ unsigned char uncompressed_pubkey[65];
21+ size_t len;
22+ secp256k1_pubkey pubkey;
23+ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
24+ while (1) {
I’d prefer not to loop here and instead bail out if we hit an invalid key.
The probability of hitting an invalid key is negligible if your randomness is good. So this loop is only relevant if your randomness is broken. And then it hides the fact that your randomness is broken.
While the probability of an invalid key is negligible (something like 1/2^128?), I feel that if the verify function is provided in the library then it could still be informative to include in the examples.
0if (!secp256k1_ec_seckey_verify(ctx, seckey)) {
1 printf("Invalid secret key\n");
2 return 1;
3}
35+ /* The docs in secp256k1.h above the `secp256k1_ec_pubkey_create` function say:
36+ * "pointer to a context object, initialized for signing"
37+ * Which is why we create a context for signing(SECP256K1_CONTEXT_SIGN).
38+ * (The docs for `secp256k1_ecdh` don't require any special context, just some context) */
39+
40+ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
keygen example with a schnorr and ecdsa examples.
Please review and comment if you think something I’ve done isn’t best practice or some comment’s wording isn’t good enough
0@@ -0,0 +1,124 @@
1+/**********************************************************************
2+ * Copyright (c) 2020 Elichai Turkel *
67@@ -68,6 +68,13 @@ libsecp256k1 is built using autotools:
68 $ make check
69 $ sudo make install # optional
70
71+Usage Examples
72+-----------
73+ Usage Examples can be found in the [examples](examples) directory:
--enable-examples configuration flag.
9@@ -10,10 +10,14 @@ tests
10 exhaustive_tests
11 gen_context
12 valgrind_ctime_test
13+keygen_example
ecdsa_example and schnorr_example are missing.
129+ecdsa_example_SOURCES = examples/ecdsa.c
130+ecdsa_example_CPPFLAGS = -I$(top_srcdir)/include $(SECP_INCLUDES)
131+ecdsa_example_LDADD = libsecp256k1.la $(SECP_LIBS)
132+ecdsa_example_LDFLAGS = -static
133+TESTS += ecdsa_example
134+endif
USE_EXAMPLES applies to all examples.
7+ * In this file only the best practice randomness sources are used
8+ * Platform randomness sources:
9+ * 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
10+ * 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
11+ * FreeBSD -> `getrandom(2)`(`sys/random.h`), if not available `kern.arandom` should be used. https://www.freebsd.org/cgi/man.cgi?query=getrandom, https://www.freebsd.org/cgi/man.cgi?query=random&sektion=4
12+ * OpenBSD -> `getentropy(2)`(`unistd.h`), if not available `/dev/urandom` should be used. https://man.openbsd.org/getentropy, https://man.openbsd.org/urandom
arc4random as recommended in the getentropy man page? Same for macOS.
I think the reason was that some platforms still use RC4 for that syscall: https://security.stackexchange.com/a/172905/188909
Looking over implementations, it looks like OpenSSL[0], libsodium internal[1], Rust’s getrandom[2], Go’s[3], CPython[4], GnuTLS[5], Bitcoin[6] all use getentropy in OpenBSD, but libsodium’s external API uses arc4random[7].
Arguably, OpenSSL, libsodium internal, GnuTLS and bitcoin all use it to seed a CPRNG, but Rust, Go, and Python’s implementations are generally meant for end users AFAIU(and are used like that).
The only implementation I’ve found that uses arc4random is libsodium’s external randomness (you could argue that it’s the only one that’s explicitly meant for cryptography end users)
So yeah, maybe for user facing API’s arc4random is better, I’m not sure though (note that these days getentropy->arc4random: https://github.com/openbsd/src/blob/master/sys/dev/rnd.c#L814)
[0] https://github.com/openssl/openssl/blob/c8830891e6cb8d0782986662ca50b8fa7c97f49f/providers/implementations/rands/seeding/rand_unix.c#L362 [1] https://github.com/jedisct1/libsodium/blob/ae4add868124a32d4e54da10f9cd99240aecc0aa/src/libsodium/randombytes/internal/randombytes_internal_random.c#L189 [2] https://github.com/rust-random/getrandom/blob/master/src/openbsd.rs#L14 [3] https://golang.org/src/crypto/rand/rand_openbsd.go#L23 [4] https://github.com/python/cpython/blob/41761933c1c30bb6003b65eef1ba23a83db4eae4/Python/bootstrap_hash.c#L463 [5] https://gitlab.com/gnutls/gnutls/-/blob/master/lib/nettle/sysrng-getentropy.c#L57 [6] https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L314 [7] https://github.com/jedisct1/libsodium/blob/ae4add868124a32d4e54da10f9cd99240aecc0aa/src/libsodium/randombytes/sysrandom/randombytes_sysrandom.c#L101
I think the reason was that some platforms still use RC4 for that syscall:
Urgh. But we’re only targeting APPLE and OpenBSD here, whose manpages both recommend arc4random. But agree that getentropy should be ok too.
How about we replace the sentence “In this file only the best practice randomness sources are used” at the top of the file with: “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.”
nit: better formatting
0 * This file is an attempt at collecting best practice methods for obtaining
1 * randomness with different operating systems. It may be out-of-date. Consult
2 * the documentation of the operating system before considering to use the
3 * methods below.
4 *
5 * Platform randomness sources:
25+
26+
27+/* Returns 1 on success, and 0 on failure. */
28+static int fill_random(unsigned char* data, unsigned long size) {
29+#if defined(_WIN32)
30+ NTSTATUS res = BCryptGenRandom(NULL, data, size, BCRYPT_USE_SYSTEM_PREFERRED_RNG);
BCRYPT_USE_SYSTEM_PREFERRED_RNG flag do?
hAlgorithm and we need to instead need to pass a specific algorithm
48+ * Should never fail */
49+ assert(secp256k1_context_randomize(ctx, randomize));
50+
51+ /*** Key Generation ***/
52+
53+ /* If the secret key is zero or out of range (bigger than secp256k1 order), the probablity of this happening is negligble
49+ assert(secp256k1_context_randomize(ctx, randomize));
50+
51+ /*** Key Generation ***/
52+
53+ /* If the secret key is zero or out of range (bigger than secp256k1 order), the probablity of this happening is negligble
54+ * non the less we try generating a new key if failed */
61+ if (secp256k1_keypair_create(ctx, &keypair, seckey)) {
62+ break;
63+ }
64+ }
65+
66+ /* Extract the XOnly Public Key from the Keypair,
55+ while (1) {
56+ if (!fill_random(seckey, sizeof(seckey))) {
57+ printf("Failed to generate randomness\n");
58+ return 1;
59+ }
60+ /* Try to create a keypair wit a valid context, it should only fail if the seckey is zero or out of range. */
25+
26+int main(void) {
27+ unsigned char msg_hash[32] = {0}; /* This should be a hash of the message. */
28+ unsigned char seckey[32];
29+ unsigned char randomize[32];
30+ unsigned char auxilary_rand[32];
46+ /* Randomizing the context is recommended to protect against side-channel leakage
47+ * See `secp256k1_context_randomize` in secp256k1.h for more information about it
48+ * Should never fail */
49+ assert(secp256k1_context_randomize(ctx, randomize));
50+
51+ while (1) {
10+
11+#include "random.h"
12+#include "secp256k1.h"
13+
14+
15+static void print_hex(unsigned char* data, size_t size) {
random.h? should I rename random.h to util.h?
123@@ -123,6 +124,31 @@ exhaustive_tests_LDFLAGS = -static
124 TESTS += exhaustive_tests
125 endif
126
127+if USE_EXAMPLES
128+noinst_PROGRAMS += ecdsa_example
129+ecdsa_example_SOURCES = examples/ecdsa.c
130+ecdsa_example_CPPFLAGS = -I$(top_srcdir)/include $(SECP_INCLUDES)
131+ecdsa_example_LDADD = libsecp256k1.la $(SECP_LIBS)
$(SECP_LIBS)$ nor $CPPFLAGS$ for any of the examples, except that schnorrsig needs CPPFLAGS(but no$(SECP_INCLUDES)` necessary).
$(SECP_INCLUDES) and $(SECP_LIBS), anything else I should change in the automake?
ecdsa_example_CPPFLAGS and ecdh_example_CPPFLAGS.
#include "secp256k1.h", I could remove it if I change it to #include "include/secp256k1.h" But I think actual users will pass -Isecp256k1/include
34+ return 0;
35+ }
36+#elif defined(__linux__) || defined(__FreeBSD__)
37+ /* If `getrandom(2)` is not available you should fallback to /dev/urandom */
38+ ssize_t res = getrandom(data, size, 0);
39+ if (res == (ssize_t)size) {
unsigned long may not fit in an ssize_t. On 32 bit systems it can therefore happen that getrandom returns -1 but fill_random returns 1. Perhaps change the size argument of fill_random to size_t, add res != -1 to this if condition and, for WIN32, check whether size exceeds unsigned long?
Argh,how do I check if size exceeds ULONG_MAX if the standard doesn’t even tell me what’s bigger size_t or unsigned long?.
What’s the “right” way to do this? :(
if (size > ULONG_MAX) return 0;? Also, instead of checking res != 1, I think it would be better to check that size <= SSIZE_MAX.
size_t is smaller than unsigned long, the constant will overflow and the condition won’t be the one it seems to be.
But I re-read the integer promotion rules and I think it checks out, if I don’t explicitly convert them to a specific type then they’ll be converted to the integer with the higher rank, preserving the right values.
P.S. Should the usage examples be MIT or CC0(Public Domain)?
I think we should make them CC0 to avoid any doubt.
40+ return 1;
41+ }
42+#elif defined(__linux__) || defined(__FreeBSD__)
43+ /* If `getrandom(2)` is not available you should fallback to /dev/urandom */
44+ ssize_t res = getrandom(data, size, 0);
45+ if (res != (ssize_t)size || size > SSIZE_MAX) {
size is bigger than SSIZE_MAX or size isn’t the same as res this will return 0.
SSIZE_MAX I’ll need to define some weird POSIX macros, I’ll instead use if (res < 0 || (size_t)res != size ) which should work assuming ssize_t = signed size_t which honestly I’m not 100% sure I can assume. sigh.
I could also cast to unsigned long as both man size_t and man ssize_t say that they’re no greater than the width of long
Added a commit fixing some mingw linking bugs:
windows.h and ntstatus.h even though they’re used in bcrypt.h (https://sourceforge.net/p/mingw-w64/bugs/903/)libbcrypt36+ return 1;
37+ }
38+ /* Randomizing the context is recommended to protect against side-channel
39+ * leakage See `secp256k1_context_randomize` in secp256k1.h for more
40+ * information about it. This should never fail. */
41+ assert(secp256k1_context_randomize(ctx, randomize));
assert(), as those get removed when compiled with -DNDEBUG.
I think for secp256k1_context_randomize it’s reasonable to change the API documentation that it always returns 1. I don’t see under what conditions you’d ever want to return failure here. In that case, it seems unnecessary to check.
There are lots of operations that only returns 0 if you pass in invalid arguments (e.g. serializing a public key to an area that’s too small). For those I think it can be recommended practice to check, but I wouldn’t consider it incorrect usage not to check if you know your code will always provide enough space.
7+#include <stdio.h>
8+#include <assert.h>
9+#include <string.h>
10+
11+#include "random.h"
12+#include "secp256k1.h"
#include <...>? As people looking at examples would likely be linking from outside the project.
If you set -I arguments to the compiler correctly, there is no problem with using <…>.
The only difference is that “…” also looks in the current path, and as that is not where one would expect the included headers, it’s pointless and I would say confusing.
FWIW Bitcoin Core does everything using <…> includes, also internal headers.
22+ unsigned char serialized_pubkey[32];
23+ unsigned char signature[64];
24+ int is_signature_valid;
25+ secp256k1_xonly_pubkey pubkey;
26+ secp256k1_keypair keypair;
27+ /* The docs in secp256k1_extrakeys.h above the `secp256k1_keypair_create`
That’s remarkably specific, and may go outdated if the header files are restructured a bit.
What about “The specification in secp256k1_extrakeys.h states that secp256k1_schnorrsig_verify needs …”?
59+
60+ /* Extract the X-only public key from the keypair. We pass NULL for
61+ * `pk_parity` as we don't care about the parity of the key, only advanced
62+ * users might care about the parity. This should never fail with a valid
63+ * context and public key. */
64+ assert(secp256k1_keypair_xonly_pub(ctx, &pubkey, NULL, &keypair));
62+ * users might care about the parity. This should never fail with a valid
63+ * context and public key. */
64+ assert(secp256k1_keypair_xonly_pub(ctx, &pubkey, NULL, &keypair));
65+
66+ /* Serialize the public key. Should always return 1 for a valid public key. */
67+ assert(secp256k1_xonly_pubkey_serialize(ctx, serialized_pubkey, &pubkey));
78+ * custom nonce function, passing `NULL` will use the BIP-340 safe default.
79+ * BIP-340 recommends passing 32 bytes of randomness to the nonce function to
80+ * improve security against side-channel attacks. Signing with a valid
81+ * context, verified keypair and the default nonce function should never
82+ * fail. */
83+ assert(secp256k1_schnorrsig_sign(ctx, signature, msg_hash, &keypair, auxiliary_rand));
105+
106+ /* This will clear everything from the context and free the memory */
107+ secp256k1_context_destroy(ctx);
108+
109+ /* It's best practice to try to remove secrets from memory after using them.
110+ * This is done because some bugs can allow an attacker leak memory, for
Grammar seems off here.
Also: an attacker accessing memory through an exploit isn’t the only reason; another one is the risk that the OS swaps out the application to disk, where it may end up on permanent storage (which might be less well-protected from attackers). Secrets in general should not be kept in memory longer than necessary.
Here and elsewhere.
69@@ -68,6 +70,10 @@ case $host_os in
70 fi
71 fi
72 ;;
73+ cygwin*|mingw*)
74+
112+ * This is done because some bugs can allow an attacker to leak memory, for
113+ * example through "out of bounds" array access (see Heartbleed), Or the OS
114+ * swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
115+ *
116+ * TODO: Prevent these writes from being optimized out, as any good compiler
117+ * will remove any writes that aren't used. */
#if (__STDC_VERSION__ >= 201112L)) could be added and then memset_s() used instead.
memset_s() is optional in C11, it’s in Annex K. You need more than _STDC_VERSION__ >= 201112L to check for its presence, see for example https://en.cppreference.com/w/c/string/byte/memset. Also, almost no compiler supports Annex K, so I doubt it’s useful in practice, see for example https://stackoverflow.com/a/50724865.
27+ * say: "pointer to a context object, initialized for signing" And the docs
28+ * above the `secp256k1_ecdsa_verify` function say: "a secp256k1 context
29+ * object, initialized for verification" which is why we create a context
30+ * for both signing and verification with the SECP256K1_CONTEXT_SIGN and
31+ * SECP256K1_CONTEXT_VERIFY flags. */
32+ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);
assert(ctx != NULL);
IIUC returning NULL can only happen in the unlikely case that allocating dynamic memory fails, but it can’t hurt. We also do that check in Bitcoin Core: https://github.com/bitcoin/bitcoin/blob/8f137e69caeb2a2ffe1aa930bd6fbc49cee4087c/src/key.cpp#L394-L395
returning NULL can only happen in the unlikely case that allocating dynamic memory fails
If that was the case then I would agree that the examples should do the check. However, context_create calls checked_malloc which will call the “error callback” if the result of malloc is NULL. The default behavior of the error callback is crashing. It could theoretically be overriden by the user to not crash (context_set_error_callback) but that would be inadvisable given that the API docs state that after the callback the originally called API function can do anything, in particular its return values are undefined. That’s why the API doc for secp256k1_context_create says that it returns a “context object” and not a “context object or NULL”.
checked_malloc and error callbacks being used. Considering that, I agree that the assert is not needed.
Concept ACK
Looking forward to the review club tomorrow! 🚀
12+#include "secp256k1.h"
13+
14+
15+
16+int main(void) {
17+ unsigned char msg_hash[32] = {0}; /* This must be a hash of the message. otherwise ECDSA is easily broken. */
Rather than initializing the hash to all zero’s and repeatedly warning the user not to do that, how about something like this?
0unsigned char msg_hash[32] = "This must be a hash of the msg.";
Good point! Maybe something like this instead?
0 /* Instead of signing the message directly, we must sign a 32-byte hash.
1 * Here the message is "Hello, world!" and the hash function was SHA-256.
2 * An actual implementation should just call SHA-256, but this example
3 * hardcodes the output to avoid depending on an additional library. */
4 unsigned char msg_hash[32] = {
5 0x31, 0x5F, 0x5B, 0xDB, 0x76, 0xD0, 0x78, 0xC4,
6 0x3B, 0x8A, 0xC0, 0x06, 0x4E, 0x4A, 0x01, 0x64,
7 0x61, 0x2B, 0x1F, 0xCE, 0x77, 0xC8, 0x69, 0x34,
8 0x5B, 0xFC, 0x94, 0xC7, 0x58, 0x94, 0xED, 0xD3,
9 };
58+ assert(secp256k1_ec_pubkey_create(ctx, &pubkey, seckey));
59+
60+ /* Serialize the pubkey in a compressed form(33 bytes). Should always return 1. */
61+ len = sizeof(compressed_pubkey);
62+ assert(secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey, &len, &pubkey, SECP256K1_EC_COMPRESSED));
63+ /* Should be the same size as the size of the output, because we passed a 33 bytes array. */
0 /* Should be the same size as the size of the output, because we passed a 33 byte array. */
83+ if (!secp256k1_ecdsa_signature_parse_compact(ctx, &sig, serialized_signature)) {
84+ printf("Failed parsing the signature\n");
85+ return 1;
86+ }
87+
88+ /*** Verification ***/
/*** Verification ***/ label be removed?
58+ }
59+#endif
60+ return 0;
61+}
62+
63+static void print_hex(unsigned char* data, size_t size) {
util.h?
56+ break;
57+ }
58+ }
59+
60+ /* Extract the X-only public key from the keypair. We pass NULL for
61+ * `pk_parity` as we don't care about the parity of the key, only advanced
Instead of “only advanced users might care about the parity”, how about something like this?
“We pass NULL for pk_parity as the parity isn’t needed for signing or verification. secp256k1_keypair_xonly_pub supports returning the parity for other use cases such as tests or verifying Taproot tweaks.”
68@@ -69,6 +69,13 @@ libsecp256k1 is built using autotools:
69 $ make check # run the test suite
70 $ sudo make install # optional
71
72+Usage Examples
73+-----------
74+ Usage Examples can be found in the [examples](examples) directory, to compile them you need configure with `--enable-examples`.
0 Usage Examples can be found in the [examples](examples) directory. To compile them you need to configure with `--enable-examples`.
1 For experimental modules, you will also need `--enable-experimental` as well as a flag for each individual module, e.g. `--enable-module-ecdh`.
Concept ACK, thanks for adding these helpful examples!
My comments are mostly cosmetic, and I think the PR should be ready to merge after review club.
38+ /* Randomizing the context is recommended to protect against side-channel
39+ * leakage See `secp256k1_context_randomize` in secp256k1.h for more
40+ * information about it. This should never fail. */
41+ return_val = secp256k1_context_randomize(ctx, randomize);
42+ assert(return_val);
43+ /*** Key Generation ***/
/*** Signing ***/ and /*** Verification ***/.
13+#include "random.h"
14+
15+
16+
17+int main(void) {
18+ unsigned char msg_hash[32] = {0}; /* This must be a hash of the message. otherwise ECDSA is easily broken. */
42+ } else {
43+ return 1;
44+ }
45+#elif defined(__linux__) || defined(__FreeBSD__)
46+ /* If `getrandom(2)` is not available you should fallback to /dev/urandom */
47+ ssize_t res = getrandom(data, size, 0);
Is there any specific reason why getrandom() is used for Linux whereas getentropy() is used for macOS? availability issues?
The GNU C library manual says this:
Most applications should use getentropy. The getrandom function is intended for low-level applications which need additional control over blocking behavior.
https://www.gnu.org/software/libc/manual/html_node/Unpredictable-Bytes.html
Hmm, mostly because I read the linux docs etc, and getrandom is a kernel thing, while getentropy is a glibc thing to emulate what OpenBSD does.
Also, most other libs also call getrandom on linux even though they call getentropy on OpenBSD/MacOS:
#748 (review)
Not sure if this is convincing enough or not
Tested ACK
Linux workstation 5.13.0-28-generic [#31](/bitcoin-core-secp256k1/31/)~20.04.1-Ubuntu SMP Wed Jan 19 14:08:10 UTC 2022 x86_64 x86_64 x86_64 GNU/LinuxMINGW64_NT-10.0-19044 DESKTOP-UV75MOO 3.3.3-341.x86_64 2022-01-18 13:00 UTC x86_64 Msys
0@@ -0,0 +1,124 @@
1+/*************************************************************************
2+ * Copyright (c) 2020-2021 Elichai Turkel *
3+ * Distributed under the CC0 software license, see the accompanying file *
4+ * EXAMPLES_COPYING or https://creativecommons.org/publicdomain/zero/1.0 *
5+ *************************************************************************/
Sorry interrupting with legal stuff and not noticing earlier… I think it’s weird to say “Copyright” and CC0 at the same time. And CC0 is strictly speaking not a software license. https://wiki.creativecommons.org/wiki/CC0_FAQ#May_I_apply_CC0_to_computer_software.3F_If_so.2C_is_there_a_recommended_implementation.3F provides some boilerplate.
Here’s a suggestion, slightly adopted from the above to make sure it applies only to a single file.
0/*************************************************************************
1 * Written in 2020-2022 by Elichai Turkel *
2 * To the extent possible under law, the author(s) have dedicated all *
3 * copyright and related and neighboring rights to the software in this *
4 * file to the public domain worldwide. This software is distributed *
5 * without any warranty. For the CC0 Public Domain Dedication, see *
6 * EXAMPLES_COPYING or https://creativecommons.org/publicdomain/zero/1.0 *
7 *************************************************************************/
78+ assert(len == sizeof(compressed_pubkey));
79+
80+ /*** Signing ***/
81+
82+ /* Generate an ECDSA signature, note that even though here `msg_hash` is set
83+ * to zeros, it MUST contain a hash, otherwise ECDSA is easily broken.
msg_hash is not set to zeros.
127+ * example through "out of bounds" array access (see Heartbleed), Or the OS
128+ * swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
129+ *
130+ * TODO: Prevent these writes from being optimized out, as any good compiler
131+ * will remove any writes that aren't used. */
132+ memset(seckey, 0, sizeof(seckey));
I don’t like the idea of having a TODO here. I mean we spent days getting the randomness right, then this feels wrong.
We could copy https://github.com/bitcoin/bitcoin/blob/7fcf53f7b4524572d1d0c9a5fdc388e87eb02416/src/support/cleanse.cpp#L14 and then really call the file util.h or refer to it…
In the interest of moving this forward, we suggest to leave this as is and I’m happy to open a further PR that fixes it.
41+ * leakage See `secp256k1_context_randomize` in secp256k1.h for more
42+ * information about it. This should never fail. */
43+ return_val = secp256k1_context_randomize(ctx, randomize);
44+ assert(return_val);
45+
46+ /*** Key Generation ***/
It may be good to add a header to every code section in this part, e.g., “Party 1: " and “Party 2:” or similar, to make sure.
This would require splitting the key generation loop but I think that’s really better than. Real code wouldn’t work like this, so I think we should avoid it in example code.
This is also something I can do in a follow-up PR.
@elichai Can you quickly fix the copyright and the msg_hash comment?
I think then this is good to go. We had many eyes on it and I can address the other two comments in another PR.
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
ACK 7c9502cece9c9e8d811333f7ab5bb22f4eb01c04
As I said, I can create a follow-up PR.
The one CI failure for the sage prover is harmless and only because just sage on CI was introduced after the last rebase of this PR. It will work on master.