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 <[email protected]>

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			\

bitcoin/privkey.c

@@ -0,0 +1,24 @@
+#include "privkey.h"
+#include <assert.h>
+#include <ccan/mem/mem.h>
+#include <ccan/str/hex/hex.h>
+#include <common/type_to_string.h>
+
+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;
+}

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 {

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];

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)

bitcoin/test/run-secret_eq_consttime.c

@@ -0,0 +1,136 @@
+#include <assert.h>
+#include <bitcoin/privkey.c>
+#include <ccan/err/err.h>
+#include <ccan/time/time.h>
+#include <stdio.h>
+
+/* 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;
+}

bitcoin/test/run-tx-encode.c

@@ -4,7 +4,7 @@
 #include <bitcoin/tx.c>
 #include <bitcoin/varint.c>
 #include <ccan/str/hex/hex.h>
-#include <common/utils.c>
+#include <common/utils.h>
 
 const char extended_tx[] = "02000000000101b5bef485c41d0d1f58d1e8a561924ece5c476d86cff063ea10c8df06136eb31d00000000171600144aa38e396e1394fb45cbf83f48d1464fbc9f498fffffffff0140330f000000000017a9140580ba016669d3efaf09a0b2ec3954469ea2bf038702483045022100f2abf9e9cf238c66533af93f23937eae8ac01fb6f105a00ab71dbefb9637dc9502205c1ac745829b3f6889607961f5d817dfa0c8f52bdda12e837c4f7b162f6db8a701210204096eb817f7efb414ef4d3d8be39dd04374256d3b054a322d4a6ee22736d03b00000000";
 

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,17 +157,17 @@ 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);
 	assert(derive_htlc_basepoint(&info->seed, &info->basepoints.htlc,
 					&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);

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;

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;