From 8340d8c0708685bbd08216a47234fc5c6a659cda Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 17 Aug 2018 13:46:34 +0930 Subject: [PATCH] secret_eq: remove in favor of constant time variant. To be safe, we should never memcmp secrets. We don't do this currently outside tests, but we're about to. The tests to prove this as constant time are the tricky bit. Signed-off-by: Rusty Russell --- bitcoin/Makefile | 1 + bitcoin/privkey.c | 24 +++++ bitcoin/privkey.h | 5 +- bitcoin/pubkey.c | 11 -- bitcoin/test/Makefile | 2 +- bitcoin/test/run-secret_eq_consttime.c | 136 +++++++++++++++++++++++++ bitcoin/test/run-tx-encode.c | 2 +- common/test/run-derive_basepoints.c | 12 +-- connectd/test/run-initiator-success.c | 2 +- connectd/test/run-responder-success.c | 2 +- 10 files changed, 174 insertions(+), 23 deletions(-) create mode 100644 bitcoin/privkey.c create mode 100644 bitcoin/test/run-secret_eq_consttime.c diff --git a/bitcoin/Makefile b/bitcoin/Makefile index a6f876f8f..33dc5f7c2 100644 --- a/bitcoin/Makefile +++ b/bitcoin/Makefile @@ -5,6 +5,7 @@ BITCOIN_SRC := \ bitcoin/block.c \ bitcoin/chainparams.c \ bitcoin/locktime.c \ + bitcoin/privkey.c \ bitcoin/pubkey.c \ bitcoin/pullpush.c \ bitcoin/script.c \ diff --git a/bitcoin/privkey.c b/bitcoin/privkey.c new file mode 100644 index 000000000..db8e2987b --- /dev/null +++ b/bitcoin/privkey.c @@ -0,0 +1,24 @@ +#include "privkey.h" +#include +#include +#include +#include + +static char *privkey_to_hexstr(const tal_t *ctx, const struct privkey *secret) +{ + /* Bitcoin appends "01" to indicate the pubkey is compressed. */ + char *str = tal_arr(ctx, char, hex_str_size(sizeof(*secret) + 1)); + hex_encode(secret, sizeof(*secret), str, hex_str_size(sizeof(*secret))); + strcat(str, "01"); + return str; +} +REGISTER_TYPE_TO_STRING(privkey, privkey_to_hexstr); +REGISTER_TYPE_TO_HEXSTR(secret); + +bool secret_eq_consttime(const struct secret *a, const struct secret *b) +{ + u8 result = 0; + for (size_t i = 0; i < sizeof(a->data); i++) + result |= a->data[i] ^ b->data[i]; + return result == 0; +} diff --git a/bitcoin/privkey.h b/bitcoin/privkey.h index 71723920c..59272bf16 100644 --- a/bitcoin/privkey.h +++ b/bitcoin/privkey.h @@ -8,8 +8,9 @@ struct secret { u8 data[32]; }; -/* Define secret_eq */ -STRUCTEQ_DEF(secret, 0, data); + +/* You probably shouldn't compare secrets in non-const time! */ +bool secret_eq_consttime(const struct secret *a, const struct secret *b); /* This is a private key. Keep it secret. */ struct privkey { diff --git a/bitcoin/pubkey.c b/bitcoin/pubkey.c index 63d369820..f84900378 100644 --- a/bitcoin/pubkey.c +++ b/bitcoin/pubkey.c @@ -86,17 +86,6 @@ int pubkey_cmp(const struct pubkey *a, const struct pubkey *b) return memcmp(keya, keyb, sizeof(keya)); } -static char *privkey_to_hexstr(const tal_t *ctx, const struct privkey *secret) -{ - /* Bitcoin appends "01" to indicate the pubkey is compressed. */ - char *str = tal_arr(ctx, char, hex_str_size(sizeof(*secret) + 1)); - hex_encode(secret, sizeof(*secret), str, hex_str_size(sizeof(*secret))); - strcat(str, "01"); - return str; -} -REGISTER_TYPE_TO_STRING(privkey, privkey_to_hexstr); -REGISTER_TYPE_TO_HEXSTR(secret); - void pubkey_to_hash160(const struct pubkey *pk, struct ripemd160 *hash) { u8 der[PUBKEY_DER_LEN]; diff --git a/bitcoin/test/Makefile b/bitcoin/test/Makefile index 935e1da88..9d33d7c76 100644 --- a/bitcoin/test/Makefile +++ b/bitcoin/test/Makefile @@ -2,7 +2,7 @@ BITCOIN_TEST_SRC := $(wildcard bitcoin/test/run-*.c) BITCOIN_TEST_OBJS := $(BITCOIN_TEST_SRC:.c=.o) BITCOIN_TEST_PROGRAMS := $(BITCOIN_TEST_OBJS:.o=) -BITCOIN_TEST_COMMON_OBJS := +BITCOIN_TEST_COMMON_OBJS := common/utils.o $(BITCOIN_TEST_PROGRAMS): $(CCAN_OBJS) $(BITCOIN_TEST_COMMON_OBJS) $(BITCOIN_TEST_OBJS): $(CCAN_HEADERS) $(BITCOIN_HEADERS) $(BITCOIN_SRC) diff --git a/bitcoin/test/run-secret_eq_consttime.c b/bitcoin/test/run-secret_eq_consttime.c new file mode 100644 index 000000000..0c803c631 --- /dev/null +++ b/bitcoin/test/run-secret_eq_consttime.c @@ -0,0 +1,136 @@ +#include +#include +#include +#include +#include + +/* AUTOGENERATED MOCKS START */ +/* AUTOGENERATED MOCKS END */ + +#define RUNS (256 * 10000) +static struct timerel const_time_test(struct secret *s1, + struct secret *s2, + size_t off) +{ + struct timeabs start, end; + int result = 0; + + memset(s1, 0, RUNS * sizeof(*s1)); + memset(s2, 0, RUNS * sizeof(*s2)); + + for (size_t i = 0; i < RUNS; i++) + s2[i].data[off] = i; + + start = time_now(); + for (size_t i = 0; i < RUNS; i++) + result += secret_eq_consttime(&s1[i], &s2[i]); + end = time_now(); + + if (result != RUNS / 256) + errx(1, "Expected %u successes at offset %zu, not %u!", + RUNS / 256, off, result); + + return time_between(end, start); +} + +static inline bool secret_eq_nonconst(const struct secret *a, + const struct secret *b) +{ + return memcmp(a, b, sizeof(*a)) == 0; +} + +static struct timerel nonconst_time_test(struct secret *s1, + struct secret *s2, + size_t off) +{ + struct timeabs start, end; + int result = 0; + + memset(s1, 0, RUNS * sizeof(*s1)); + memset(s2, 0, RUNS * sizeof(*s2)); + + for (size_t i = 0; i < RUNS; i++) + s2[i].data[off] = i; + + start = time_now(); + for (size_t i = 0; i < RUNS; i++) + result += secret_eq_nonconst(&s1[i], &s2[i]); + end = time_now(); + + if (result != RUNS / 256) + errx(1, "Expected %u successes at offset %zu, not %u!", + RUNS / 256, off, result); + + return time_between(end, start); +} + +/* Returns true if test result is expected: we consider 5% "same". */ +static bool secret_time_test(struct timerel (*test)(struct secret *s1, + struct secret *s2, + size_t off), + bool should_be_const) +{ + struct secret *s1, *s2; + struct timerel firstbyte_time, lastbyte_time, diff; + + s1 = calloc(RUNS, sizeof(*s1)); + s2 = calloc(RUNS, sizeof(*s2)); + + firstbyte_time = test(s1, s2, 0); + lastbyte_time = test(s1, s2, sizeof(s1->data)-1); + + free(s1); + free(s2); + + printf("First byte %u psec vs last byte %u psec\n", + (int)time_to_nsec(time_divide(firstbyte_time, RUNS / 1000)), + (int)time_to_nsec(time_divide(lastbyte_time, RUNS / 1000))); + + /* If they differ by more than 5%, get upset. */ + if (time_less(firstbyte_time, lastbyte_time)) + diff = time_sub(lastbyte_time, firstbyte_time); + else { + /* If the lastbyte test was faster, that's a fail it we expected + * it to be slower... */ + if (!should_be_const) + return false; + diff = time_sub(firstbyte_time, lastbyte_time); + } + + return time_less(time_multiply(diff, 20), firstbyte_time) == should_be_const; +} + +int main(void) +{ + const char *v; + int success, i; + setup_locale(); + + /* no point running this under valgrind. */ + v = getenv("VALGRIND"); + if (v && atoi(v) == 1) + exit(0); + + /* I've never seen this fail more than 5 times */ + success = 0; + for (i = 0; i < 10; i++) + success += secret_time_test(const_time_test, true); + + printf("=> Within 5%% %u/%u times\n", success, i); + if (success < i/2) + errx(1, "Only const time %u/%u?", success, i); + + /* This, should show measurable differences at least 1/2 the time. */ + success = 0; + for (i = 0; i < 10; i++) + success += secret_time_test(nonconst_time_test, false); + + printf("=> More than 5%% slower %u/%u times\n", success, i); + /* This fails without -O2 or above, at least here (x86 Ubuntu gcc 7.3) */ +#ifdef __OPTIMIZE__ + if (success < i/2) + errx(1, "memcmp seemed const time %u/%u?", success, i); +#endif + + return 0; +} diff --git a/bitcoin/test/run-tx-encode.c b/bitcoin/test/run-tx-encode.c index bbe3dffdb..dd491bd50 100644 --- a/bitcoin/test/run-tx-encode.c +++ b/bitcoin/test/run-tx-encode.c @@ -4,7 +4,7 @@ #include #include #include -#include +#include const char extended_tx[] = "02000000000101b5bef485c41d0d1f58d1e8a561924ece5c476d86cff063ea10c8df06136eb31d00000000171600144aa38e396e1394fb45cbf83f48d1464fbc9f498fffffffff0140330f000000000017a9140580ba016669d3efaf09a0b2ec3954469ea2bf038702483045022100f2abf9e9cf238c66533af93f23937eae8ac01fb6f105a00ab71dbefb9637dc9502205c1ac745829b3f6889607961f5d817dfa0c8f52bdda12e837c4f7b162f6db8a701210204096eb817f7efb414ef4d3d8be39dd04374256d3b054a322d4a6ee22736d03b00000000"; diff --git a/common/test/run-derive_basepoints.c b/common/test/run-derive_basepoints.c index 50602e3e9..543c2cb1d 100644 --- a/common/test/run-derive_basepoints.c +++ b/common/test/run-derive_basepoints.c @@ -135,8 +135,8 @@ int main(void) &info->secrets.payment_basepoint_secret)); assert(pubkey_eq(&baseline->basepoints.payment, &info->basepoints.payment)); - assert(secret_eq(&baseline->secrets.payment_basepoint_secret, - &info->secrets.payment_basepoint_secret)); + assert(secret_eq_consttime(&baseline->secrets.payment_basepoint_secret, + &info->secrets.payment_basepoint_secret)); /* derive_funding_key should give same results. */ info = new_info(ctx); @@ -157,8 +157,8 @@ int main(void) &info->secrets.revocation_basepoint_secret)); assert(pubkey_eq(&baseline->basepoints.revocation, &info->basepoints.revocation)); - assert(secret_eq(&baseline->secrets.revocation_basepoint_secret, - &info->secrets.revocation_basepoint_secret)); + assert(secret_eq_consttime(&baseline->secrets.revocation_basepoint_secret, + &info->secrets.revocation_basepoint_secret)); /* derive_htlc_basepoint should give same results. */ info = new_info(ctx); @@ -166,8 +166,8 @@ int main(void) &info->secrets.htlc_basepoint_secret)); assert(pubkey_eq(&baseline->basepoints.htlc, &info->basepoints.htlc)); - assert(secret_eq(&baseline->secrets.htlc_basepoint_secret, - &info->secrets.htlc_basepoint_secret)); + assert(secret_eq_consttime(&baseline->secrets.htlc_basepoint_secret, + &info->secrets.htlc_basepoint_secret)); tal_free(ctx); wally_cleanup(0); diff --git a/connectd/test/run-initiator-success.c b/connectd/test/run-initiator-success.c index 4d5be9d69..917c73418 100644 --- a/connectd/test/run-initiator-success.c +++ b/connectd/test/run-initiator-success.c @@ -76,7 +76,7 @@ static bool secret_eq_str(const struct secret *s, const char *str) struct secret expect; if (!hex_decode(str, strlen(str), &expect, sizeof(expect))) abort(); - return secret_eq(s, &expect); + return secret_eq_consttime(s, &expect); } secp256k1_context *secp256k1_ctx; diff --git a/connectd/test/run-responder-success.c b/connectd/test/run-responder-success.c index a22a24dd9..843535f7d 100644 --- a/connectd/test/run-responder-success.c +++ b/connectd/test/run-responder-success.c @@ -76,7 +76,7 @@ static bool secret_eq_str(const struct secret *s, const char *str) struct secret expect; if (!hex_decode(str, strlen(str), &expect, sizeof(expect))) abort(); - return secret_eq(s, &expect); + return secret_eq_consttime(s, &expect); } secp256k1_context *secp256k1_ctx;