Add constant-time point multplication for ECDH #252

apoelstra · 2015-05-15T19:48:41Z

Add constant-time secp256k1_point_multiply for ECDH

Designed with clear separation of the wNAF conversion, precomputation
and exponentiation (since the precomp at least we will probably want
to separate in the API for users who reuse points a lot).

Future work:

actually separate precomp in the API
do multiexp rather than single exponentiation

sipa · 2015-05-17T05:48:01Z

I'm on vacation until june 1st. I'll review afterwards.

gmaxwell · 2015-05-20T05:30:41Z

"/** Do an ellitic curve scalar multiplication in constant time."

spelling.

apoelstra · 2015-05-22T17:10:44Z

Added endomorphism optimization.

On my laptop (Lenovo T440p i7-4700MQ 2.4Ghz) I see 85.6us without endomorphism, 107us with. I will need to investigate this..

Edit: Oh, I needed to shrink the wNAF input size from 256 bits to 128 when the endomorphism is enabled. When this is done I see 64.4us, a 25% improvement :)

apoelstra · 2015-05-23T16:52:19Z

Pushed; had to do some algebraic changes to get the endomorphism stuff to work. The pre-endomorphism commits have been changed in some minor ways (the unit tests are a bit cleaner, secp256k1_scalar_wnaf_force_odd is replaced by secp256k1_scalar_wnaf_cond_negate and secp256k1_scalar_is_even, and the return type of secp256k1_ecdh_wnaf is now void since it used to return a fixed value, which was confusing. This value is now computed by a macro.)

As for the endomorphism stuff, the wNAF conversion is actually a bit different. We require all wNAF digits to be odd, so to represent even 256-bit numbers, we (a) negate the number, making it odd since our modulus is odd; (b) convert this to wNAF; (c) negate the individual digits, un-negating the number while keeping all digits odd. In this way all 256-bit numbers can be represented. However, with the endomorphism optimization, we find ourselves converting 128-bit numbers to wNAF, which cannot be negated this way. (They can, but their negations are 256 bits, so we lose the optimization.) We therefore use a different strategy for handling even numbers when endomorphism optimization is enabled. Specifically, we add 1 to even numbers, add 2 to odd ones, and compensate after the multiplication by subtracting either P or 2P from the result. This is suggested in the Okeya/Tagaki paper.

With this strategy, the perf numbers are less impressive: with endomorphism optimization on my laptop can do a multiplication in 68.9us.

sipa · 2015-06-13T12:34:30Z

I have a few nits regarding naming and a question (see above), but the code looks well-written, well-documented and well-tested. Nice!

apoelstra · 2015-06-13T15:29:02Z

When I add a test against secp256k1_ec_pubkey_create that runs 100 times with a random_scalar_order scalar it fails usually when the endomorphism optimization is enabled. Investigating.

Edit: So this occurs specifically with scalars whose lambda-decomposition consists of a positive and a negative number (where "negative" means high bit set). Two positives or two negatives are OK, which seems inexplicable ... those numbers are treated totally independently by the point-multiply code, so it is weird that there'd be a problem with their relative sign and not their absolute ones.

I doubt this has anything to do with the basepoint being the generator or not; it happens that the existing test code always uses the same hardcoded scalar whose lambda-decomposition consists of two positive values, hence our not seeing this bug with the existing tests.

apoelstra · 2015-06-24T13:29:03Z

Rebased on #261, which fixes the problems I was having earlier with the endomorphism. Also add the additional test @sipa suggested (which didn't pass until #261 :))

peterdettman · 2015-06-27T12:01:36Z

src/secp256k1.c

+        } else {
+            secp256k1_ecdh_point_multiply(&res, &pt, &s);
+            secp256k1_ge_set_gej(&pt, &res);
+            ret = secp256k1_eckey_pubkey_serialize(&pt, point, pointlen, *pointlen <= 33);


The output point is supposed to be secret (ECDH agreement), but _pubkey_serialize uses _normalize_var internally because it's assuming a public key point.

Yikes! Thanks for catching this.

peterdettman · 2015-06-27T12:32:09Z

A couple of issues with the documentation for secp256k1_point_multiply:

The return code -1 is listed for "scalar was zero", but it looks to me like it would actually return 0 in that case (via secp256k1_eckey_pubkey_serialize).
Is the scalar actually restricted to not overflow the curve order? In which case, "a 32-byte scalar" is a little misleading.

peterdettman · 2015-06-27T12:32:46Z

include/secp256k1.h

+ *          -1: scalar was zero (cannot serialize output point)
+ *          -2: scalar overflow
+ *          -3: invalid input point
+ *  In:      scalar:   a 32-byte scalar with which to multiple the point


multiple -> multiply

apoelstra · 2015-06-29T16:59:17Z

Rebase and fix minor comments from Peter. TODO fix serialization timing leak.

apoelstra · 2015-06-29T20:19:29Z

I've pushed a new version which fixes the timing leak. I've changed the API to output the hash of the computed point rather than the point itself. @gmaxwell had suggested early on that giving the user a curvepoint was "too low level" and not consistent with our policy of not exposing raw cryptographic objects.

The concern was that a user who has access to a "ECDH secret" which is a curvepoint might try to build his or her own cryptosystem in a fragile or broken way. I think this timing leak is an example of me getting burned by this -- I didn't think to look for this timing leak because I was thinking in terms of "points" rather than "secrets".

apoelstra · 2015-06-29T20:30:21Z

This means that #262 is going to need to be changed to use secp256k1_point_multiply rather than secp256k1_ecdh_point_multiply since the latter now outputs a hash (which is algebraically useless).

peterdettman · 2015-06-30T04:07:00Z

src/bench_ecdh.c

+    bench_multiply_t *data = (bench_multiply_t*)arg;
+
+    for (i = 0; i < 20000; i++) {
+        CHECK(secp256k1_ecdh_point_multiply(data->point, &data->pointlen, data->scalar) == 1);


Seems to have been missed in the API change?

Yeah, oops. Should we put -Werror in Travis to prevent this oversight in future?

peterdettman · 2015-07-15T13:37:34Z

@apoelstra See peterdettman@de5436e for some ideas on how to recover another 1% or so from the cmov table-lookup.

apoelstra · 2015-07-15T18:19:12Z

Thanks Peter. I've cherry-picked that commit. The total perf hit from doing constant-time lookups now looks like 1.7% to me.

apoelstra · 2015-07-29T15:42:38Z

Rebase; change API to take a pubkey_t rather than point/pointlen, and to take a context_t.

- Add zero/one sanity check tests for ecmult - Add unit test for secp256k1_scalar_split_lambda_var - Typo fix in `ge_equals_ge`; was comparing b->y to itself, should have been comparing a->y to b->y - Normalize y-coordinate in `random_group_element_test`; this is needed to pass random group elements as the first argument to `ge_equals_ge`, which I will do in a future commit.

apoelstra · 2015-07-29T23:14:55Z

I've added a commit which moves the ECDH code into its own module but leaves the constant-time multiply code in the main library. It also renames the const-multiply functions, so the result is kinda a mess of renaming and moving.

I'll go back and rebase the original changeset as though this had been the layout from the start, but first I'd like an ACK that this is how we want to organize things.

sipa · 2015-07-31T14:09:19Z

src/bench_ecdh.c

+int main(void) {
+    bench_multiply_t data;
+
+    run_benchmark("ecdh_mult", bench_multiply, bench_multiply_setup, NULL, &data, 10, 20000);


Technically, ECDH is no longer just a multiplication.

sipa · 2015-07-31T14:31:28Z

ACK design, with a few more nits.

Please squash and I'll merge.

EDIT: no need to squash everything, but some logical steps are nice... ecmult_const then ecmult_const + endo then optimizations then ECDH module... or whatever you like.

Designed with clear separation of the wNAF conversion, precomputation and exponentiation (since the precomp at least we will probably want to separate in the API for users who reuse points a lot. Future work: - actually separate precomp in the API - do multiexp rather than single exponentiation

apoelstra · 2015-07-31T19:53:32Z

Thanks for the comments @sipa. All fixed, and rebased for a cleaner history.

sipa · 2015-08-01T14:16:33Z

Thanks! Can you also make some changes to .travis so this gets tested?

…plit_lambda_var` constant time This has the effect of making `secp256k1_scalar_mul_shift_var` constant time in both input scalars. Keep the _var name because it is NOT constant time in the shift amount. As used in `secp256k1_scalar_split_lambda_var`, the shift is always the constant 272, so this function becomes constant time, and it loses the `_var` suffix.

72ae443 Improve perf. of cmov-based table lookup (Peter Dettman) 92e53fc Implement endomorphism optimization for secp256k1_ecmult_const (Andrew Poelstra) ed35d43 Make `secp256k1_scalar_add_bit` conditional; make `secp256k1_scalar_split_lambda_var` constant time (Andrew Poelstra) 91c0ce9 Add benchmarks for ECDH and const-time multiplication (Andrew Poelstra) 0739bbb Add ECDH module which works by hashing the output of ecmult_const (Andrew Poelstra) 4401500 Add constant-time multiply `secp256k1_ecmult_const` for ECDH (Andrew Poelstra) baa75da tests: add a couple tests (Andrew Poelstra)

Kagami · 2015-11-05T11:51:26Z

It's a bit unfortunate that secp256k1 always hash the derived secret, because it means I can't use it for existing protocols like Bitmessage.

Any ideas how can I get Px with secp256k1? Only by implementing my own version of secp256k1_ecdh? (Note that Bitmessage uses SHA512(Px), so it's not unsafe.)

apoelstra · 2015-11-05T12:59:55Z

@Kagami As you say, ECIES actually only needs a hash of Px, though it's not our hash. Can you open an issue requesting we extend the ECDH API to support a custom hash function (like the signing functions support custom nonce functions)?

This is something we've considered but haven't had any concrete usecases and I guess it forgot to get written down..

apoelstra force-pushed the ecdh-mult branch 4 times, most recently from 9483793 to a6d9c2d Compare May 15, 2015 21:36

apoelstra force-pushed the ecdh-mult branch from cb901b1 to a0992eb Compare May 22, 2015 17:10

apoelstra force-pushed the ecdh-mult branch 4 times, most recently from 6519ac3 to 2a268de Compare May 23, 2015 16:44

apoelstra mentioned this pull request Jun 16, 2015

secp256k1_gej_add_ge does the wrong thing with points P, Q with P_y = -Q_y but P≠-Q #257

Closed

apoelstra force-pushed the ecdh-mult branch from 2a268de to 2896833 Compare June 23, 2015 20:19

peterdettman mentioned this pull request Jun 26, 2015

x-only ECDH without sqrt #262

Open

peterdettman reviewed Jun 27, 2015
View reviewed changes

apoelstra force-pushed the ecdh-mult branch from 2896833 to d18f467 Compare June 29, 2015 16:59

apoelstra force-pushed the ecdh-mult branch 2 times, most recently from 417cc48 to 370c270 Compare June 29, 2015 20:15

peterdettman reviewed Jun 30, 2015
View reviewed changes

apoelstra force-pushed the ecdh-mult branch from 370c270 to 48dc8e3 Compare June 30, 2015 20:04

apoelstra force-pushed the ecdh-mult branch from 5069c77 to dc7558a Compare July 29, 2015 15:42

apoelstra force-pushed the ecdh-mult branch 2 times, most recently from 0820126 to e0e7987 Compare July 29, 2015 22:22

sipa reviewed Jul 31, 2015
View reviewed changes

apoelstra force-pushed the ecdh-mult branch from e0e7987 to 337a988 Compare July 31, 2015 19:53

apoelstra and others added 5 commits August 1, 2015 10:57

Add ECDH module which works by hashing the output of ecmult_const

0739bbb

Add benchmarks for ECDH and const-time multiplication

91c0ce9

Implement endomorphism optimization for secp256k1_ecmult_const

92e53fc

Improve perf. of cmov-based table lookup

72ae443

apoelstra force-pushed the ecdh-mult branch from 337a988 to 72ae443 Compare August 1, 2015 15:57

sipa merged commit 72ae443 into bitcoin-core:master Aug 2, 2015

sipa mentioned this pull request Aug 28, 2015

Support for optional parts #277

Closed

Kagami mentioned this pull request Nov 5, 2015

Support custom hash functions in ECDH API #352

Closed

sipa mentioned this pull request Nov 28, 2016

add generic constant time multi-exp for ECDH #100

Closed

apoelstra deleted the ecdh-mult branch June 19, 2017 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add constant-time point multplication for ECDH #252

Add constant-time point multplication for ECDH #252

apoelstra commented May 15, 2015

sipa commented May 17, 2015 via email

gmaxwell commented May 20, 2015

apoelstra commented May 22, 2015

apoelstra commented May 23, 2015

sipa commented Jun 13, 2015

apoelstra commented Jun 13, 2015

apoelstra commented Jun 24, 2015

peterdettman Jun 27, 2015

apoelstra Jun 29, 2015

peterdettman commented Jun 27, 2015

peterdettman Jun 27, 2015

apoelstra commented Jun 29, 2015

apoelstra commented Jun 29, 2015

apoelstra commented Jun 29, 2015

peterdettman Jun 30, 2015

apoelstra Jun 30, 2015

peterdettman commented Jul 15, 2015

apoelstra commented Jul 15, 2015

apoelstra commented Jul 29, 2015

apoelstra commented Jul 29, 2015

sipa Jul 31, 2015

sipa commented Jul 31, 2015

apoelstra commented Jul 31, 2015

sipa commented Aug 1, 2015

Kagami commented Nov 5, 2015

apoelstra commented Nov 5, 2015

Add constant-time point multplication for ECDH #252

Add constant-time point multplication for ECDH #252

Conversation

apoelstra commented May 15, 2015

sipa commented May 17, 2015 via email

gmaxwell commented May 20, 2015

apoelstra commented May 22, 2015

apoelstra commented May 23, 2015

sipa commented Jun 13, 2015

apoelstra commented Jun 13, 2015

apoelstra commented Jun 24, 2015

peterdettman Jun 27, 2015

Choose a reason for hiding this comment

apoelstra Jun 29, 2015

Choose a reason for hiding this comment

peterdettman commented Jun 27, 2015

peterdettman Jun 27, 2015

Choose a reason for hiding this comment

apoelstra commented Jun 29, 2015

apoelstra commented Jun 29, 2015

apoelstra commented Jun 29, 2015

peterdettman Jun 30, 2015

Choose a reason for hiding this comment

apoelstra Jun 30, 2015

Choose a reason for hiding this comment

peterdettman commented Jul 15, 2015

apoelstra commented Jul 15, 2015

apoelstra commented Jul 29, 2015

apoelstra commented Jul 29, 2015

sipa Jul 31, 2015

Choose a reason for hiding this comment

sipa commented Jul 31, 2015

apoelstra commented Jul 31, 2015

sipa commented Aug 1, 2015

Kagami commented Nov 5, 2015

apoelstra commented Nov 5, 2015