ECC-step2: curves y^2 = x^3 + ax + b #236

karel-m · 2017-06-21T20:28:24Z

While hacking on ecc_sign+verify_hash_rfc7518 earlier today I realized that it would be possible to extract from my branch miko-ecc-enhancements the crucial part implementing generalized elliptic curves y^2 = x^3 + ax + b (current develop branch supports only y^2 = x^3 - 3x + b).

The API changes comprise of the following new functions:

int  ecc_export_raw(unsigned char *out, unsigned long *outlen, int type, ecc_key *key);
int  ecc_import_raw(const unsigned char *in, unsigned long inlen, ecc_key *key, ltc_ecc_set_type *dp);

ltc_ecc_set_type* ecc_dp_find_by_oid(unsigned long *oid, unsigned long oidsize);
ltc_ecc_set_type* ecc_dp_find_by_name(char *curve_name);
ltc_ecc_set_type* ecc_dp_find_by_params(char *hex_prime, char *hex_A, char *hex_B, char *hex_order, char *hex_Gx, char *hex_Gy, unsigned long cofactor);

These are missing due to #187

ecc_export_full - export EC key in OpenSSL compatible format
ecc_import_full - import EC key in OpenSSL compatible format
ecc_import_pkcs8 - import EC key in PKCS#8 format

karel-m · 2017-06-21T21:37:47Z

A note (#65 (comment)) related to this PR:

We need a ltm-1.0 package backported to trusty to fix these build failures.
I had to remove the ltm from sid in #217 as I couldn't update clang otherwise...

sjaeckel · 2017-06-22T09:54:03Z

Another note related to this PR:

We will need sqrtmod_prime() in tfm and gmp before this can be merged.

sjaeckel

First part of a review... To Be Continued

sjaeckel · 2017-06-22T15:50:48Z

src/headers/tomcrypt_pk.h

 int  ecc_export(unsigned char *out, unsigned long *outlen, int type, ecc_key *key);
 int  ecc_import(const unsigned char *in, unsigned long inlen, ecc_key *key);
 int  ecc_import_ex(const unsigned char *in, unsigned long inlen, ecc_key *key, const ltc_ecc_set_type *dp);
+int  ecc_export_raw(unsigned char *out, unsigned long *outlen, int type, ecc_key *key);


especially since this has the same signature as ecc_export(), how about re-using the same pattern that introduced PK_STD?

#define PK_RAW 0x2000 #define PK_COMPRESSED 0x4000

With EC things are a bit tricky as we have couple more scenarios:

there is just one way of exporting RAW private key

RAW public key can be exported in full/long form

of compressed/short form
we have our old DER format (basically compatible just with LTC) where the curve is identified just by its size

later when dealing with OpenSSL compatible DER format we can export the key + curve OID

or the key + all curve params

I am not yet sure what would be the best approach here. But yes we can handle all of them in one single ecc_(import|export).

On the other hand in ECC part 2 we call ecc_(import|export)_raw during importing/exporting OpenSSL keys as ecc_(import|export)_raw is kind of a lower level action.

sjaeckel · 2017-06-22T15:50:51Z

src/headers/tomcrypt_pk.h

   PK_PUBLIC=0,
-   PK_PRIVATE=1
+   PK_PRIVATE=1,
+   PK_PUBLIC_COMPRESSED=2 /* used only when exporting public ECC key */


c.f. my comment to ecc_export_raw()

sjaeckel · 2017-06-22T15:53:58Z

src/headers/tomcrypt_pk.h


+int  ecc_verify_key(ecc_key *key);
+
+#ifdef LTC_SOURCE


sjaeckel · 2017-06-22T15:55:57Z

src/pk/ecc/ecc_dp_find_by_name.c

+
+   for (i = 0; ltc_ecc_sets[i].size != 0; i++) {
+      if (ltc_ecc_sets[i].name != NULL && XSTRCMP(ltc_ecc_sets[i].name, curve_name) == 0) {
+         return (ltc_ecc_set_type*)&ltc_ecc_sets[i];


why is this cast required?

If you mean ecc_dp_find_by_name it is for convenience like:

dp = ecc_dp_find_by_name("SECP160R2"); err = ecc_make_key_ex(prng, wprng, key, dp);

what I meant was "why is this return (ltc_ecc_set_type*)&ltc_ecc_sets[i]; cast required?"

and I just got the answer... because the API signature is lying and you're returning a const object as non-const

src/pk/ecc/ecc_dp_find_by_name.c: In function ‘ecc_dp_find_by_name’: src/pk/ecc/ecc_dp_find_by_name.c:20:17: warning: return discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] return &ltc_ecc_sets[i];

three possibilities to solve this in order of preference:

make the return value a const ltc_ecc_set_type*

make a copy of the object you want to return before returning it as non-const (which the caller then has to free)

make ltc_ecc_sets not const

This stuff gets even more complicated in ECC part 2 where we are importing keys in OpenSSL compatible DER format:

one scenario is that the imported key has curve specified by OID - in this case we use ecc_dp_find_by_oid(..) which finds corresponding ltc_ecc_sets[i] item (const)

the other scenario is that the imported key has explicitly defined curve parametes - here we can try whether these curve params do exist in our ltc_ecc_sets - this is the idea behind ecc_dp_find_by_params(..) - otherwise we have to allocate and properly fill ltc_ecc_set_type structure and free it during the key destruction in ecc_free (here it is tricky as ecc_free has no idea whether the ltc_ecc_set_type structure was dynamically allocated or not)

and suddenly possibility 2 seems to become the preferred one, simply always allocate a copy so we can always free

Yes, and it should be perhaps better named ecc_dp_new_by_name

I have created some draft related to "possibility 2"

https://github.com/libtom/libtomcrypt/blob/pr/ecc-non-asn1-part/src/pk/ecc/ecc_dp_utils1.c

But it turned into a question: Why do we make copies of hexadecimal strings? It would make IMO more sense to convert those hex string into bignums during making a "copy":

https://github.com/libtom/libtomcrypt/blob/pr/ecc-non-asn1-part/src/pk/ecc/ecc_dp_utils2.c

sjaeckel · 2017-06-22T15:56:04Z

src/pk/ecc/ecc_dp_find_by_oid.c

+   for (i = 0; ltc_ecc_sets[i].size != 0; i++) {
+      if ((oidsize == ltc_ecc_sets[i].oid.OIDlen) &&
+          (XMEM_NEQ(oid, ltc_ecc_sets[i].oid.OID, sizeof(unsigned long) * ltc_ecc_sets[i].oid.OIDlen) == 0)) {
+         return (ltc_ecc_set_type*)&ltc_ecc_sets[i];


this can be moved to ECC part2, where it is required as some DER formats do not contain all curve parameters but only OID of the curve

sjaeckel · 2017-06-22T15:56:17Z

src/pk/ecc/ecc_dp_find_by_params.c

+          _hexstrcmp(ltc_ecc_sets[i].Gx,    hex_Gx)    == 0 &&
+          _hexstrcmp(ltc_ecc_sets[i].Gy,    hex_Gy)    == 0 &&
+          ltc_ecc_sets[i].cofactor == cofactor) {
+         return (ltc_ecc_set_type*)&ltc_ecc_sets[i];


I'll will also move this to ECC part 2 (this one might end up removed completely)

sjaeckel · 2017-06-22T15:58:19Z

src/pk/ecc/ecc_make_key.c

+   orderbits = mp_count_bits(order);
+   do {
+     if ((err = rand_bn_bits(key->k, orderbits, prng, wprng)) != CRYPT_OK)                      { goto errkey; }
+   } while (mp_iszero(key->k) || mp_cmp(key->k, order) != LTC_MP_LT);


looks like we'll loop here forever as well and that the standard says to do so...

OK, I'll apply PK_MAX_RETRIES here

sjaeckel · 2017-07-06T11:23:03Z

A note (#65 (comment)) related to this PR:

We need a ltm-1.0 package backported to trusty to fix these build failures.
I had to remove the ltm from sid in #217 as I couldn't update clang otherwise...

done

karel-m · 2017-07-10T21:13:57Z

Some thoughts about applying similar API ideas as we did in RSA, DSA, DH.

This is what we have in 1.18-rc1 (and probably want to keep it for backwards compatibility):

void ecc_sizes(int *low, int *high);
int ecc_get_size(ecc_key *key);
int ecc_make_key(prng_state *prng, int wprng, int keysize, ecc_key *key);
int ecc_make_key_ex(prng_state *prng, int wprng, ecc_key *key, const ltc_ecc_set_type *dp);
void ecc_free(ecc_key *key);
int ecc_export(unsigned char *out, unsigned long *outlen, int type, ecc_key *key);
int ecc_import(const unsigned char *in, unsigned long inlen, ecc_key *key);
int ecc_import_ex(const unsigned char *in, unsigned long inlen, ecc_key *key, const ltc_ecc_set_type *dp);
int ecc_ansi_x963_export(ecc_key *key, unsigned char *out, unsigned long *outlen);
int ecc_ansi_x963_import(const unsigned char *in, unsigned long inlen, ecc_key *key);
int ecc_ansi_x963_import_ex(const unsigned char *in, unsigned long inlen, ecc_key *key, ltc_ecc_set_type *dp);
int ecc_shared_secret(ecc_key *private_key, ecc_key *public_key, unsigned char *out, unsigned long *outlen);
int ecc_encrypt_key(const unsigned char *in, unsigned long inlen, unsigned char *out, unsigned long *outlen, prng_state *prng, int wprng, int hash, ecc_key *key);
int ecc_decrypt_key(const unsigned char *in, unsigned long inlen, unsigned char *out, unsigned long *outlen, ecc_key *key);
int ecc_sign_hash_rfc7518(const unsigned char *in, unsigned long inlen, unsigned char *out, unsigned long *outlen, prng_state *prng, int wprng, ecc_key *key);
int ecc_sign_hash(const unsigned char *in, unsigned long inlen, unsigned char *out, unsigned long *outlen, prng_state *prng, int wprng, ecc_key *key);
int ecc_verify_hash_rfc7518(const unsigned char *sig, unsigned long siglen, const unsigned char *hash, unsigned long hashlen, int *stat, ecc_key *key);
int ecc_verify_hash(const unsigned char *sig, unsigned long siglen, const unsigned char *hash, unsigned long hashlen, int *stat, ecc_key *key);

I propose the following new additions to public API in the "next" (UPDATED):

int ecc_set_dp(const ltc_ecc_set_type *set, ecc_key *key);
int ecc_set_dp_name(const char *curve_name, ecc_key *key);
int ecc_set_key(const unsigned char *in, unsigned long inlen, int type, ecc_key *key);
int ecc_generate_key(prng_state *prng, int wprng, ecc_key *key);
int ecc_export_key(unsigned char *out, unsigned long *outlen, int type, ecc_key *key);

karel-m · 2017-07-17T18:56:09Z

This PR was re-factored as proposed here: #236 (comment)

Currently it is ready only for API review (I did my best to keep the API as much backwards compatible as possible).

Definitely needs at least one more pass to check/add more LTC_ARGCHK's + check whether we correctly free what has to be freed etc.

karel-m · 2017-09-21T16:08:35Z

Just for record - the idea:

int ecc_get_set(const char *curve_name, const ltc_ecc_set_type** dp);
/* or */
const ltc_ecc_set_type* ecc_get_set(const char *curve_name);

karel-m · 2017-09-22T15:18:34Z

UPDATE: the latest version of this PR adds the following to the public API

int  ecc_get_set_by_name(const char* name, const ltc_ecc_set_type** dp);
int  ecc_set_dp(const ltc_ecc_set_type *set, ecc_key *key);
int  ecc_generate_key(prng_state *prng, int wprng, ecc_key *key);
int  ecc_set_key(const unsigned char *in, unsigned long inlen, int type, ecc_key *key);
int  ecc_get_key(unsigned char *out, unsigned long *outlen, int type, ecc_key *key);

karel-m · 2017-10-10T16:22:09Z

closing this one? AFAICT it is still unmerged.

karel-m · 2017-10-10T16:22:50Z

sorry, already reopened

buggywhip · 2018-04-07T20:56:51Z

In the end I have decided for: int ecc_get_curve(const char* name_or_oid, const ltc_ecc_curve** cu);

I just saw that. Bingo!

karel-m · 2018-04-09T21:13:04Z

@fperrad could you please check (const params + usual stuff) with your linter pr/ecc-non-asn1-part branch?

fperrad · 2018-04-10T16:46:50Z

@karel-m here, the harvest:

--- Module:   src/pk/ecc/ecc_verify_hash.c (C)
                                                  _
   if ((err = mp_add_d(a, 3, a_plus3)) != CRYPT_OK) {
src/pk/ecc/ecc_verify_hash.c  46  Info 838: Previously assigned value to variable 'err' has not been used

--- Module:   src/pk/ecc/ltc_ecc_verify_key.c (C)
                        _
   prime = key->dp.prime;
src/pk/ecc/ltc_ecc_verify_key.c  30  Info 838: Previously assigned value to variable 'prime' has not been used
                        _
   order = key->dp.order;
src/pk/ecc/ltc_ecc_verify_key.c  31  Info 838: Previously assigned value to variable 'order' has not been used
                    _
   a     = key->dp.A;
src/pk/ecc/ltc_ecc_verify_key.c  32  Info 838: Previously assigned value to variable 'a' has not been used

src/pk/ecc/ecc_sign_hash.c  120  Info 818: Pointer parameter 'key' (line 21) could be declared as pointing to const
src/pk/ecc/ltc_ecc_points.c  56  Info 818: Pointer parameter 'p' (line 49) could be declared as pointing to const
src/pk/ecc/ltc_ecc_points.c  65  Info 818: Pointer parameter 'dst' (line 58) could be declared as pointing to const
src/pk/ecc/ecc_get_key.c  54  Info 818: Pointer parameter 'key' (line 22) could be declared as pointing to const
src/pk/ecc/ecc_ansi_x963_export.c  62  Info 818: Pointer parameter 'key' (line 25) could be declared as pointing to const
src/pk/ecc/ecc_export.c  65  Info 818: Pointer parameter 'key' (line 27) could be declared as pointing to const
src/pk/ecc/ecc_verify_hash.c  160  Info 818: Pointer parameter 'key' (line 21) could be declared as pointing to const
src/pk/ecc/ecc_get_size.c  30  Info 818: Pointer parameter 'key' (line 24) could be declared as pointing to const
src/pk/ecc/ecc_shared_secret.c  70  Info 818: Pointer parameter 'private_key' (line 27) could be declared as pointing to const
src/pk/ecc/ecc_shared_secret.c  70  Info 818: Pointer parameter 'public_key' (line 27) could be declared as pointing to const
src/pk/ecc/ltc_ecc_verify_key.c  67  Info 818: Pointer parameter 'key' (line 22) could be declared as pointing to const

karel-m · 2018-04-10T18:06:13Z

@fperrad I have fixed all of them except:

src/pk/ecc/ltc_ecc_points.c  56  Info 818: Pointer parameter 'p' (line 49) could be declared as pointing to const
src/pk/ecc/ltc_ecc_points.c  65  Info 818: Pointer parameter 'dst' (line 58) could be declared as pointing to const

These look like false positives. Please re-run the check again.

fperrad · 2018-04-10T18:40:32Z

I agree look like false positive.

There are 2 new ones:

src/pk/ecc/ecc_decrypt_key.c  136  Info 818: Pointer parameter 'key' (line 30) could be declared as pointing to const
src/pk/ecc/ecc_encrypt_key.c  129  Info 818: Pointer parameter 'key' (line 34) could be declared as pointing to const

fperrad · 2018-04-11T09:46:54Z

I missed this one:

Info 765: external '_curve_names' (line 17, file src/pk/ecc/ecc_get_curve.c) could be made static

sjaeckel · 2018-04-13T12:26:37Z

src/headers/tomcrypt_custom.h

+   #define LTC_ECC_SECP256R1
+   #define LTC_ECC_SECP384R1
+   #define LTC_ECC_SECP521R1
+   /* OLD deprecated (but still working) defines */


0001-don-t-use-deprecated-LTC_ECCxxx-defines-anymore.patch.txt

what do you think about applying this patch?

The patch is good. Please commit&push it to this branch.

applied & I was a bit surprised when the ./test ecc-run returned a nop instead of a passed :)

karel-m · 2018-04-27T17:18:07Z

Should I start working on doc update or do we plan more changes to API / struct & co. ?

sjaeckel · 2018-04-27T20:14:44Z

Go for the docs! 👍

sjaeckel · 2018-05-02T21:52:21Z

And feel free to squash again :)

karel-m · 2018-05-06T15:18:24Z

FYI: I have tried to build dropbear with the API changes included in this PR. Unfortunately we are going to break dropbear build quite severely. I'll try to investigate whether we can somehow lower the impact.

Or maybe we should simply submit a patch for dropbear alongside the release of this new API.

sjaeckel · 2018-05-06T20:44:05Z

FYI: I have tried to build dropbear with the API changes included in this PR. Unfortunately we are going to break dropbear build quite severely. I'll try to investigate whether we can somehow lower the impact.

Or maybe we should simply submit a patch for dropbear alongside the release of this new API.

I'll have a look and probably @mkj can add his two cents as well :)

mkj · 2018-05-07T14:54:45Z

I'm fine with updating Dropbear to work with the new API - I've only had a quick look over it so far though. ecc_set_key() can replace Dropbear's buf_get_ecc_raw_pubkey(), and places Dropbear uses key->dp will use ltc_ecc_curve instead.

My main concern is how the ABI breakage happens if people have built Dropbear using system libtomcrypt on Debian - I guess the shared library version number will bump?

karel-m · 2018-05-13T19:59:29Z

@mkj my first draft (not in a pull-request quality yet) is here karel-m/dropbear@4530ff6

Ad ABI breakage we definitely will bump ABI & SO versions (as it turned out it is necessary even with smaller API changes than this one)

sjaeckel

After the Xth try to review I arrived at the end of the PR :)

karel-m · 2018-05-22T20:50:46Z

Great I'll squash it to a single commit and merge to develop.

I'll open a separate PR for doc changes

This was referenced Jun 21, 2017

ECC-step3: extra import/export (ASN.1 related) #187

Merged

ECC enhancements #65

Closed

ECC improvements #34

Closed

sjaeckel modified the milestone: next Jun 22, 2017

karel-m force-pushed the pr/ecc-non-asn1-part branch from 856a405 to 0499f30 Compare June 22, 2017 14:39

sjaeckel reviewed Jun 22, 2017

View reviewed changes

sjaeckel force-pushed the pr/ecc-non-asn1-part branch from 81a5ec5 to 85bda1e Compare July 6, 2017 11:17

karel-m force-pushed the pr/ecc-non-asn1-part branch from 34783be to 1e2f6f9 Compare July 17, 2017 18:46

karel-m force-pushed the pr/ecc-non-asn1-part branch from 8f49290 to 5eb5544 Compare September 16, 2017 17:50

karel-m changed the base branch from develop to release/1.18.0 September 16, 2017 17:51

karel-m force-pushed the pr/ecc-non-asn1-part branch from 5be9fa6 to 6277924 Compare September 20, 2017 13:31

karel-m mentioned this pull request Sep 20, 2017

ecc_test function is not working #295

Closed

karel-m force-pushed the pr/ecc-non-asn1-part branch from 12b889e to 9823d3f Compare September 21, 2017 19:28

karel-m force-pushed the pr/ecc-non-asn1-part branch from 99cd128 to 8874f75 Compare September 22, 2017 20:18

sjaeckel closed this Oct 10, 2017

sjaeckel changed the base branch from release/1.18.0 to develop October 10, 2017 16:04

sjaeckel reopened this Oct 10, 2017

karel-m force-pushed the pr/ecc-non-asn1-part branch from 8874f75 to f54b702 Compare October 10, 2017 16:35

karel-m force-pushed the pr/ecc-non-asn1-part branch 2 times, most recently from b0dfaaa to 991d9e3 Compare October 26, 2017 06:58

karel-m force-pushed the pr/ecc-non-asn1-part branch 2 times, most recently from b41ed27 to bbf2487 Compare April 9, 2018 11:58

karel-m force-pushed the pr/ecc-non-asn1-part branch from 84d8906 to 19c009b Compare April 10, 2018 15:02

karel-m force-pushed the pr/ecc-non-asn1-part branch from 2db998a to d112b0d Compare April 11, 2018 11:19

sjaeckel reviewed Apr 13, 2018

View reviewed changes

karel-m force-pushed the pr/ecc-non-asn1-part branch from 1a6cf0e to 54ba133 Compare April 27, 2018 17:05

karel-m force-pushed the pr/ecc-non-asn1-part branch 2 times, most recently from e0c7170 to 885f81d Compare May 5, 2018 16:03

karel-m force-pushed the pr/ecc-non-asn1-part branch 2 times, most recently from 0cd0205 to 362275e Compare May 13, 2018 09:33

sjaeckel approved these changes May 20, 2018

View reviewed changes

karel-m added 2 commits May 22, 2018 23:02

Update makefiles

ea32b2b

ECC curves y^2 = x^3 + ax + b

24c0eb8

karel-m force-pushed the pr/ecc-non-asn1-part branch from 362275e to 24c0eb8 Compare May 22, 2018 21:03

karel-m merged commit fe665c5 into develop May 23, 2018

karel-m deleted the pr/ecc-non-asn1-part branch June 3, 2018 18:15

ECC-step2: curves y^2 = x^3 + ax + b #236

ECC-step2: curves y^2 = x^3 + ax + b #236

Uh oh!

Conversation

karel-m commented Jun 21, 2017

Uh oh!

karel-m commented Jun 21, 2017 • edited by sjaeckel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented Jun 22, 2017

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karel-m Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karel-m Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjaeckel commented Jul 6, 2017

Uh oh!

karel-m commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karel-m commented Jul 17, 2017

Uh oh!

karel-m commented Sep 21, 2017

Uh oh!

karel-m commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karel-m commented Oct 10, 2017

Uh oh!

karel-m commented Oct 10, 2017

Uh oh!

buggywhip commented Apr 7, 2018 via email

Uh oh!

karel-m commented Apr 9, 2018

Uh oh!

fperrad commented Apr 10, 2018

Uh oh!

karel-m commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fperrad commented Apr 10, 2018

karel-m commented Jun 21, 2017 •

edited by sjaeckel

Loading

karel-m Jun 22, 2017 •

edited

Loading

karel-m Jun 22, 2017 •

edited

Loading

karel-m commented Jul 10, 2017 •

edited

Loading

karel-m commented Sep 22, 2017 •

edited

Loading

karel-m commented Apr 10, 2018 •

edited

Loading

karel-m commented May 13, 2018 •

edited

Loading