Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add constant-time point multplication for ECDH #252

Merged
merged 7 commits into from
Aug 2, 2015

Conversation

apoelstra
Copy link
Contributor

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

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

sipa commented May 17, 2015 via email

@gmaxwell
Copy link
Contributor

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

spelling.

@apoelstra
Copy link
Contributor Author

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 apoelstra force-pushed the ecdh-mult branch 4 times, most recently from 6519ac3 to 2a268de Compare May 23, 2015 16:44
@apoelstra
Copy link
Contributor Author

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
Copy link
Contributor

sipa commented Jun 13, 2015

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 :))

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! Thanks for catching this.

@peterdettman
Copy link
Contributor

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.

* -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple -> multiply

@apoelstra
Copy link
Contributor Author

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

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

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
Copy link
Contributor Author

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).

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to have been missed in the API change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@peterdettman
Copy link
Contributor

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

@apoelstra
Copy link
Contributor Author

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
Copy link
Contributor Author

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 apoelstra force-pushed the ecdh-mult branch 2 times, most recently from 0820126 to e0e7987 Compare July 29, 2015 22:22
@apoelstra
Copy link
Contributor Author

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.

int main(void) {
bench_multiply_t data;

run_benchmark("ecdh_mult", bench_multiply, bench_multiply_setup, NULL, &data, 10, 20000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, ECDH is no longer just a multiplication.

@sipa
Copy link
Contributor

sipa commented Jul 31, 2015

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
Copy link
Contributor Author

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

@sipa
Copy link
Contributor

sipa commented Aug 1, 2015

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

apoelstra and others added 5 commits August 1, 2015 10:57
…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.
@sipa sipa merged commit 72ae443 into bitcoin-core:master Aug 2, 2015
sipa added a commit that referenced this pull request Aug 2, 2015
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)
@sipa sipa mentioned this pull request Aug 28, 2015
@Kagami
Copy link

Kagami commented Nov 5, 2015

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
Copy link
Contributor Author

@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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants