kokke / tiny-ECDH-c

Small portable Elliptic-Curve Diffie-Hellman in C
The Unlicense
254 stars 64 forks source link

Handle coeff_a = 0 #2

Closed caspehre closed 6 years ago

caspehre commented 6 years ago

This PR addresses Issue #1 coeff_a can only be 1 or 0 in the NIST recommended curves. So I test for 1 or 0 and modify calculations accordingly. I'm aware that there are room for improvements, it is a bit waste of ROM to allocate full vector of coeff_a when we only look at one bit.

I also include some general bugfixes regarding shift operations, calculation of bitlength, and zeroing out unwanted bits. (maybe this should be in separate PR?)

Thank you again for this project, it is a very good starting point in understanding the gf-arithmetic!

kokke commented 6 years ago

Thanks for the kind words @caspehre :)

I am sure this is also the reason I had trouble getting ECDSA working last week. I spend two nights getting neither signature signing nor verification to work. I will walk down that path again soon with your fresh input.

Thank you very much for the feedback :) BTW according to github statistics, only about 45 different IP addresses have visited this project, cheers.

kokke commented 6 years ago

The check to see if coeff-a == 1 should be able to be done at compile time as well. I'll look into that as well.

caspehre commented 6 years ago

You're welcome. It is all a matter of timing ;) I have this working (at least can get expected outcome from posted examples) for K163 + K283. One change I have made that is not published to github, is that I use u32* in main api functions, and separate variables for x and y components. I did this partly as I suspected alignment issues, and partly to better fit my other code. I don't think there are any issues with your implementation, but it may be something to look into.

kokke commented 6 years ago

I had considered splitting the public key into separate x- and y-coordinates, other projects does that too,

I just thought it easier to imagine a binary blob being passed around - but that's possible with unions too, and will not create alignment problems or trouble with pointer-aliasing either.

Good feedback to be sure, thanks a bunch :)

May I ask what you're using the code for?

kokke commented 6 years ago

Also, am I misunderstanding you or are you testing against some other implementation?

I did some testing, and it seems K409 + K571 fails with the current changes. The other eight curves seem to work fine.

caspehre commented 6 years ago

Hi,

I’m testing against this document: http://read.pudn.com/downloads168/doc/772358/TestVectorsforSEC%201-gec2.pdf

I have not tried K409 + K571 (only K163 + K283). I might have a look tonight.

Kind regards

From: kokke [mailto:notifications@github.com] Sent: Monday, November 20, 2017 11:30 PM To: kokke/tiny-ECDH-c tiny-ECDH-c@noreply.github.com Cc: Casper Ehrenborg casper@ehtu.se; Mention mention@noreply.github.com Subject: Re: [kokke/tiny-ECDH-c] Handle coeff_a = 0 (#2)

Also, am I misunderstanding you or are you testing against some other implementation?

I did some testing, and it seems K409 + K571 fails with the current changes. The other eight curves seem to work fine.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/kokke/tiny-ECDH-c/pull/2#issuecomment-345853296, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AgMR4s-EZ6SfMa0T365GRV-R8JiNG5Bwks5s4f1ugaJpZM4Qk72Y.