gh-libtom/libtomcrypt
Watch
1
0
Fork
You've already forked libtomcrypt
 0
Code Issues 42 Pull requests 17 Releases 9 Wiki Activity

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

Merged
karel-m merged 2 commits from pr/ecc-non-asn1-part into develop 2018-05-23 02:30:39 -05:00
Conversation 112 Commits 2 Files changed 55 +6915 -1112
karel-m commented 2017-06-21 15:28:24 -05:00 (Migrated from github.com)

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
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: ```C 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
🎉 1
karel-m commented 2017-06-21 16:37:47 -05:00 (Migrated from github.com)

A note (https://github.com/libtom/libtomcrypt/pull/65#issuecomment-308957259) 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...

A note (https://github.com/libtom/libtomcrypt/pull/65#issuecomment-308957259) 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 commented 2017-06-22 04:54:03 -05:00 (Migrated from github.com)

Another note related to this PR:

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

Another note related to this PR: We will need `sqrtmod_prime()` in tfm and gmp before this can be merged.
sjaeckel (Migrated from github.com) reviewed 2017-06-22 11:01:02 -05:00
sjaeckel (Migrated from github.com) left a comment

First part of a review... To Be Continued

First part of a review... To Be Continued
src/headers/tomcrypt_pk.h
sjaeckel (Migrated from github.com) commented 2017-06-22 10:50:51 -05:00

c.f. my comment to ecc_export_raw()

c.f. my comment to `ecc_export_raw()`
src/headers/tomcrypt_pk.h
@ -290,1 +299,4 @@
/** ECC key's domain parameters */
typedef struct {
/** The size of the curve in octets */
sjaeckel (Migrated from github.com) commented 2017-06-22 10:50:48 -05:00

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
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 ```
src/headers/tomcrypt_pk.h
@ -315,0 +345,4 @@
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, const ecc_key *key);
int ecc_get_oid_str(char *out, unsigned long *outlen, const ecc_key *key);
sjaeckel (Migrated from github.com) commented 2017-06-22 10:53:58 -05:00

👍

:+1:
src/pk/ecc/ecc_dp_find_by_name.c
sjaeckel (Migrated from github.com) commented 2017-06-22 10:55:57 -05:00

why is this cast required?

why is this cast required?
src/pk/ecc/ecc_dp_find_by_oid.c
sjaeckel (Migrated from github.com) commented 2017-06-22 10:56:04 -05:00

same

same
src/pk/ecc/ecc_dp_find_by_params.c
sjaeckel (Migrated from github.com) commented 2017-06-22 10:56:17 -05:00

same

same
src/pk/ecc/ecc_make_key.c
sjaeckel (Migrated from github.com) commented 2017-06-22 10:58:19 -05:00

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

looks like we'll loop here forever as well and that the standard says to do so...
karel-m (Migrated from github.com) reviewed 2017-06-22 13:57:46 -05:00
src/pk/ecc/ecc_dp_find_by_name.c
karel-m (Migrated from github.com) commented 2017-06-22 13:57:46 -05:00

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);
If you mean `ecc_dp_find_by_name` it is for convenience like: ```C dp = ecc_dp_find_by_name("SECP160R2"); err = ecc_make_key_ex(prng, wprng, key, dp); ```
karel-m (Migrated from github.com) reviewed 2017-06-22 13:58:46 -05:00
src/pk/ecc/ecc_dp_find_by_oid.c
karel-m (Migrated from github.com) commented 2017-06-22 13:58:46 -05:00

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

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
karel-m (Migrated from github.com) reviewed 2017-06-22 13:59:49 -05:00
src/pk/ecc/ecc_dp_find_by_params.c
karel-m (Migrated from github.com) commented 2017-06-22 13:59:49 -05:00

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

I'll will also move this to ECC part 2 (this one might end up removed completely)
karel-m (Migrated from github.com) reviewed 2017-06-22 14:00:30 -05:00
src/pk/ecc/ecc_make_key.c
karel-m (Migrated from github.com) commented 2017-06-22 14:00:30 -05:00

OK, I'll apply PK_MAX_RETRIES here

OK, I'll apply PK_MAX_RETRIES here
karel-m (Migrated from github.com) reviewed 2017-06-22 14:08:48 -05:00
src/headers/tomcrypt_pk.h
@ -290,1 +299,4 @@
/** ECC key's domain parameters */
typedef struct {
/** The size of the curve in octets */
karel-m (Migrated from github.com) commented 2017-06-22 14:08:48 -05:00

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

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)`.
karel-m (Migrated from github.com) reviewed 2017-06-22 14:13:12 -05:00
src/headers/tomcrypt_pk.h
@ -290,1 +299,4 @@
/** ECC key's domain parameters */
typedef struct {
/** The size of the curve in octets */
karel-m (Migrated from github.com) commented 2017-06-22 14:13:12 -05:00

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.

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 (Migrated from github.com) reviewed 2017-06-23 02:27:48 -05:00
src/pk/ecc/ecc_dp_find_by_name.c
sjaeckel (Migrated from github.com) commented 2017-06-23 02:27:48 -05:00

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:

  1. make the return value a const ltc_ecc_set_type*
  2. make a copy of the object you want to return before returning it as non-const (which the caller then has to free)
  3. make ltc_ecc_sets not const
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: 1. make the return value a `const ltc_ecc_set_type*` 1. make a copy of the object you want to return before returning it as non-const (which the caller then has to free) 1. make `ltc_ecc_sets` not const
karel-m (Migrated from github.com) reviewed 2017-06-23 02:46:59 -05:00
src/pk/ecc/ecc_dp_find_by_name.c
karel-m (Migrated from github.com) commented 2017-06-23 02:46:59 -05:00

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)
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)
sjaeckel (Migrated from github.com) reviewed 2017-06-23 03:03:10 -05:00
src/pk/ecc/ecc_dp_find_by_name.c
sjaeckel (Migrated from github.com) commented 2017-06-23 03:03:10 -05:00

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

and suddenly possibility 2 seems to become the preferred one, simply always allocate a copy so we can always free
karel-m (Migrated from github.com) reviewed 2017-06-23 05:18:31 -05:00
src/pk/ecc/ecc_dp_find_by_name.c
karel-m (Migrated from github.com) commented 2017-06-23 05:18:31 -05:00

Yes, and it should be perhaps better named ecc_dp_new_by_name

Yes, and it should be perhaps better named `ecc_dp_new_by_name`
karel-m (Migrated from github.com) reviewed 2017-06-25 10:50:43 -05:00
src/pk/ecc/ecc_dp_find_by_name.c
karel-m (Migrated from github.com) commented 2017-06-25 10:50:43 -05:00

I have created some draft related to "possibility 2"

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

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 commented 2017-07-06 06:23:03 -05:00 (Migrated from github.com)

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

> 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 commented 2017-07-10 16:13:57 -05:00 (Migrated from github.com)

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);
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): ```C 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): ```C 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 commented 2017-07-17 13:56:09 -05:00 (Migrated from github.com)

This PR was re-factored as proposed here: https://github.com/libtom/libtomcrypt/pull/236#issuecomment-314246411

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.

This PR was re-factored as proposed here: https://github.com/libtom/libtomcrypt/pull/236#issuecomment-314246411 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 commented 2017-09-21 11:08:35 -05:00 (Migrated from github.com)

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);
Just for record - the idea: ```C 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 commented 2017-09-22 10:18:34 -05:00 (Migrated from github.com)

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);
UPDATE: the latest version of this PR adds the following to the public API ```C 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 commented 2017-10-10 11:22:09 -05:00 (Migrated from github.com)

closing this one? AFAICT it is still unmerged.

closing this one? AFAICT it is still unmerged.
karel-m commented 2017-10-10 11:22:50 -05:00 (Migrated from github.com)

sorry, already reopened

sorry, already reopened
karel-m commented 2017-11-09 14:51:56 -06:00 (Migrated from github.com)

@sjaeckel could you please have a look at API additions proposed in https://github.com/libtom/libtomcrypt/pull/236#issuecomment-331476974 ? Just the API, no need to go through the implementation at the moment.

@sjaeckel could you please have a look at API additions proposed in https://github.com/libtom/libtomcrypt/pull/236#issuecomment-331476974 ? Just the API, no need to go through the implementation at the moment.
sjaeckel (Migrated from github.com) reviewed 2017-11-09 17:04:09 -06:00
sjaeckel (Migrated from github.com) left a comment

first a question, after an answer more comments :)

first a question, after an answer more comments :)
src/headers/tomcrypt_pk.h
@ -274,7 +277,13 @@ typedef struct {
sjaeckel (Migrated from github.com) commented 2017-11-09 17:01:55 -06:00

is ltc_ecc_set_type the right name for this struct? shouldn't it better be called ltc_ecc_curve

is `ltc_ecc_set_type` the right name for this struct? shouldn't it better be called `ltc_ecc_curve`
src/headers/tomcrypt_pk.h
@ -288,18 +297,36 @@ typedef struct {
void *z;
} ecc_point;
sjaeckel (Migrated from github.com) commented 2017-11-09 17:00:27 -06:00

this will prevent compilation of a user-space app without defining LTC_SOURCE

this will prevent compilation of a user-space app without defining `LTC_SOURCE`
karel-m (Migrated from github.com) reviewed 2017-11-09 17:21:12 -06:00
src/headers/tomcrypt_pk.h
@ -274,7 +277,13 @@ typedef struct {
karel-m (Migrated from github.com) commented 2017-11-09 17:21:12 -06:00

Yes, it would. However, I have not introduced this struct (only added A, cofactor, OID) it already exists and is part of the API. So for backwards compatibility we have to stay with ltc_ecc_set_type.

Yes, it would. However, I have not introduced this struct (only added A, cofactor, OID) it already exists and is part of the API. So for backwards compatibility we have to stay with `ltc_ecc_set_type`.
sjaeckel (Migrated from github.com) reviewed 2017-11-09 17:23:04 -06:00
src/headers/tomcrypt_pk.h
@ -274,7 +277,13 @@ typedef struct {
sjaeckel (Migrated from github.com) commented 2017-11-09 17:23:04 -06:00

we're going to change the API's so why shouldn't we fix the names in the same step?

we're going to change the API's so why shouldn't we fix the names in the same step?
karel-m (Migrated from github.com) reviewed 2017-11-09 17:27:28 -06:00
src/headers/tomcrypt_pk.h
@ -288,18 +297,36 @@ typedef struct {
void *z;
} ecc_point;
karel-m (Migrated from github.com) commented 2017-11-09 17:27:28 -06:00

The newly introduced ltc_ecc_dp was intended to be "invisible" from public API. User-space apps should neither see nor manipulate this struct directly but only via ecc_set_dp(..).

But I have to check whether this intention really happened.

The newly introduced `ltc_ecc_dp` was intended to be "invisible" from public API. User-space apps should neither see nor manipulate this struct directly but only via `ecc_set_dp(..)`. But I have to check whether this intention really happened.
sjaeckel (Migrated from github.com) reviewed 2017-11-09 17:29:20 -06:00
src/headers/tomcrypt_pk.h
@ -288,18 +297,36 @@ typedef struct {
void *z;
} ecc_point;
sjaeckel (Migrated from github.com) commented 2017-11-09 17:29:20 -06:00

this won't be possible as ecc_key contains an instance of ltc_ecc_dp

this won't be possible as `ecc_key` contains an instance of `ltc_ecc_dp`
karel-m (Migrated from github.com) reviewed 2017-11-09 17:30:57 -06:00
src/headers/tomcrypt_pk.h
@ -274,7 +277,13 @@ typedef struct {
karel-m (Migrated from github.com) commented 2017-11-09 17:30:57 -06:00

I wanted to keep the changes as small as possible. In fact we are "only" adding to the public API:

  • 3 new items into ltc_ecc_set_type struct
  • 5 completely new functions
I wanted to keep the changes as small as possible. In fact we are "only" adding to the public API: * 3 new items into `ltc_ecc_set_type` struct * 5 completely new functions
karel-m (Migrated from github.com) reviewed 2017-11-09 17:36:12 -06:00
src/headers/tomcrypt_pk.h
@ -288,18 +297,36 @@ typedef struct {
void *z;
} ecc_point;
karel-m (Migrated from github.com) commented 2017-11-09 17:36:12 -06:00

You are right. So either moving oid_st to public API or switching to unsigned long oid[16]; unsigned long oidlen;.

You are right. So either moving `oid_st` to public API or switching to `unsigned long oid[16]; unsigned long oidlen;`.
sjaeckel (Migrated from github.com) reviewed 2017-11-09 17:38:52 -06:00
src/headers/tomcrypt_pk.h
@ -274,7 +277,13 @@ typedef struct {
sjaeckel (Migrated from github.com) commented 2017-11-09 17:38:52 -06:00

I'd say with 1.18 out of the door "keeping changes small" is finished. We're not going to release a 1.18.1 with these ECC changes, even if the changes don't look that huge we're going to have to bump the ABI again... in the light of the late ABI bump for 1.18 we could (should!?) have included this in 1.18 :/ sorry for that

I'd say with 1.18 out of the door "keeping changes small" is finished. We're not going to release a 1.18.1 with these ECC changes, even if the changes don't look that huge we're going to have to bump the ABI again... in the light of the late ABI bump for 1.18 we could (should!?) have included this in 1.18 :/ sorry for that
karel-m (Migrated from github.com) reviewed 2017-11-09 17:49:06 -06:00
src/headers/tomcrypt_pk.h
@ -274,7 +277,13 @@ typedef struct {
karel-m (Migrated from github.com) commented 2017-11-09 17:49:06 -06:00

No need to be sorry. Ad ABI versioning I think we can keep it independent on lib versioning and simply bump it when necessary.

No need to be sorry. Ad ABI versioning I think we can keep it independent on lib versioning and simply bump it when necessary.
J08nY (Migrated from github.com) reviewed 2017-11-29 14:47:05 -06:00
src/headers/tomcrypt_pk.h
J08nY (Migrated from github.com) commented 2017-11-29 14:47:05 -06:00

@karel-m What is the reason for removing this?

@karel-m What is the reason for removing this?
sjaeckel (Migrated from github.com) reviewed 2017-11-30 02:48:43 -06:00
src/headers/tomcrypt_pk.h
sjaeckel (Migrated from github.com) commented 2017-11-30 02:48:43 -06:00

To me it looks like it's not needed as a user will normally not identify the curve by its size but by its name.
Also the size got ambiguous now that there are multiple standard-curves with the same size.

But that's just a wild guess :)
@karel-m ?

To me it looks like it's not needed as a user will normally not identify the curve by its size but by its name. Also the size got ambiguous now that there are multiple standard-curves with the same size. But that's just a wild guess :) @karel-m ?
buggywhip commented 2017-11-30 09:02:38 -06:00 (Migrated from github.com)

Sent fm iTouch

On Nov 30, 2017, at 12:48 AM, Steffen Jaeckel notifications@github.com wrote:

To me it looks like it's not needed as a user will normally not identify the curve by its size but by its name.

Definitely agree!

Sent fm iTouch > On Nov 30, 2017, at 12:48 AM, Steffen Jaeckel <notifications@github.com> wrote: > > To me it looks like it's not needed as a user will normally not identify the curve by its size but by its name. > Definitely agree!
J08nY (Migrated from github.com) reviewed 2017-11-30 11:26:53 -06:00
src/headers/tomcrypt_pk.h
J08nY (Migrated from github.com) commented 2017-11-30 11:26:53 -06:00

Hmm, I think that keeping the size or rather, changing the size to refer to the curve size in bits would be better. Many crypto libraries allow one to just specify a requested curve size in bits and requested field type and then provide a default named curve if one is present. (For example Java Cryptographic Architecture requires that)

Removing the size or storing the byte size can also pose problems for curves of bit size % 8 != 0.

Hmm, I think that keeping the size or rather, changing the size to refer to the curve size in bits would be better. Many crypto libraries allow one to just specify a requested curve size in bits and requested field type and then provide a default named curve if one is present. (For example Java Cryptographic Architecture requires that) Removing the size or storing the byte size can also pose problems for curves of bit size `% 8 != 0`.
sjaeckel (Migrated from github.com) reviewed 2017-11-30 12:00:37 -06:00
src/headers/tomcrypt_pk.h
sjaeckel (Migrated from github.com) commented 2017-11-30 12:00:37 -06:00

There's still ecc_make_key() which takes the size of the desired key and then chooses one for you :)

There's still `ecc_make_key()` which takes the size of the desired key and then chooses one for you :)
sjaeckel (Migrated from github.com) reviewed 2017-11-30 12:12:38 -06:00
src/headers/tomcrypt_pk.h
@ -309,3 +336,3 @@
/** the ECC params provided */
extern const ltc_ecc_set_type ltc_ecc_sets[];
extern const ltc_ecc_curve ltc_ecc_curves[];
sjaeckel (Migrated from github.com) commented 2017-11-30 12:12:38 -06:00

that one's redundant in the public API with ecc_make_key_ex()

that one's redundant in the public API with `ecc_make_key_ex()`
sjaeckel (Migrated from github.com) reviewed 2017-11-30 12:30:26 -06:00
src/math/tfm_desc.c
sjaeckel (Migrated from github.com) commented 2017-11-30 12:30:26 -06:00

why can't we just set this to NULL and check before the invocation?
Just an idea...
AFAIU there's no functional difference of compressed and uncompressed points, right?
so we could disable compression at runtime in case tfm is used... (in fact that would be the same behavior at runtime than what is currently done in ecc_encrypt_key() at compile-time)

why can't we just set this to NULL and check before the invocation? Just an idea... AFAIU there's no functional difference of compressed and uncompressed points, right? so we could disable compression at runtime in case tfm is used... (in fact that would be the same behavior at runtime than what is currently done in `ecc_encrypt_key()` at compile-time)
sjaeckel (Migrated from github.com) reviewed 2017-11-30 12:33:29 -06:00
src/headers/tomcrypt_pk.h
@ -309,3 +336,3 @@
/** the ECC params provided */
extern const ltc_ecc_set_type ltc_ecc_sets[];
extern const ltc_ecc_curve ltc_ecc_curves[];
sjaeckel (Migrated from github.com) commented 2017-11-30 12:33:29 -06:00

ah, or is there a use-case where you want to do the following?

ecc_get_set_by_name()
ecc_set_dp()
ecc_set_key(..., PK_PUBLIC, ...)
ah, or is there a use-case where you want to do the following? ``` ecc_get_set_by_name() ecc_set_dp() ecc_set_key(..., PK_PUBLIC, ...) ```
J08nY (Migrated from github.com) reviewed 2017-11-30 13:56:16 -06:00
src/headers/tomcrypt_pk.h
J08nY (Migrated from github.com) commented 2017-11-30 13:56:16 -06:00

ecc_make_key() currently works by looking at the size attribute of curves.

This PR removes that attribute and does the lookup at ecc_set_dp_size by hardcoding the values.

`ecc_make_key()` currently works by looking at the `size` attribute of curves. This PR removes that attribute and does the lookup at [ecc_set_dp_size](https://github.com/libtom/libtomcrypt/pull/236/files#diff-72b3a87b26a1dfde185685298ed58b33R51) by hardcoding the values.
J08nY (Migrated from github.com) reviewed 2017-11-30 14:03:57 -06:00
src/headers/tomcrypt_pk.h
@ -309,3 +336,3 @@
/** the ECC params provided */
extern const ltc_ecc_set_type ltc_ecc_sets[];
extern const ltc_ecc_curve ltc_ecc_curves[];
J08nY (Migrated from github.com) commented 2017-11-30 14:03:57 -06:00

Well, for example when manually importing the key from other/sources formats. This should provide the convenience of having function calls instead of manipulating the ecc_key struct directly.

Well, for example when manually importing the key from other/sources formats. This should provide the convenience of having function calls instead of manipulating the ecc_key struct directly.
sjaeckel (Migrated from github.com) reviewed 2017-12-01 02:33:18 -06:00
sjaeckel (Migrated from github.com) left a comment

I've stopped to type {curve,dp} and only used XX in the end... probably also not in every place I could/should have.

I don't know which one is better, curve or dp, we talked about using curve in a comment before, that's why I mention it here. I think dp (domain parameters) sounds a bit more appropriate, but I'm not sure.

In general I can say that this feels like a too big PR to review properly...

I've stopped to type `{curve,dp}` and only used XX in the end... probably also not in every place I could/should have. I don't know which one is better, curve or dp, we talked about using curve in a comment before, that's why I mention it here. I think dp (domain parameters) sounds a bit more appropriate, but I'm not sure. In general I can say that this feels like a too big PR to review properly...
src/pk/ecc/ecc_free.c
@ -28,7 +23,12 @@
void ecc_free(ecc_key *key)
sjaeckel (Migrated from github.com) commented 2017-11-30 12:38:31 -06:00

this could be one call

or can't it because this is also called in cases where either of one (dp, key) is set and the other isn't?

this could be one call or can't it because this is also called in cases where either of one (dp, key) is set and the other isn't?
src/pk/ecc/ecc_get_set.c
sjaeckel (Migrated from github.com) commented 2017-12-01 01:57:43 -06:00

ecc_get_curve_by_name?
ecc_get_dp_by_name?

`ecc_get_curve_by_name`? `ecc_get_dp_by_name`?
src/pk/ecc/ecc_make_key.c
sjaeckel (Migrated from github.com) commented 2017-12-01 01:49:09 -06:00

yes I know that this has been the way like that before, but it somehow feels wrong that someone calls a function with a partially initialized key and on error this function free's the stuff it didn't allocate...

this comment applies to several places where this pattern is used...

yes I know that this has been the way like that before, but it somehow feels wrong that someone calls a function with a partially initialized key and on error this function free's the stuff it didn't allocate... this comment applies to several places where this pattern is used...
src/pk/ecc/ecc_set_dp.c
@ -0,0 +1,90 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2017-12-01 01:54:36 -06:00

ecc_set_curve_by_size() ?
ecc_set_dp_by_size() ?

`ecc_set_curve_by_size()` ? `ecc_set_dp_by_size()` ?
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2017-12-01 02:01:47 -06:00

ecc_set_XX_by_oid

`ecc_set_XX_by_oid`
sjaeckel (Migrated from github.com) commented 2017-12-01 02:02:07 -06:00

ecc_copy_XX

`ecc_copy_XX`
sjaeckel (Migrated from github.com) commented 2017-12-01 02:07:15 -06:00

ecc_set_XX_from_mpis

`ecc_set_XX_from_mpis`
src/pk/ecc/ecc_test.c
@ -21,68 +16,12 @@
sjaeckel (Migrated from github.com) commented 2017-12-01 02:10:39 -06:00

at least return CRYPT_NOP :)

at least return `CRYPT_NOP` :)
src/pk/ecc/ltc_ecc_is_point_at_infinity.c
@ -0,0 +1,62 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2017-12-01 02:20:20 -06:00

IIUC this is a critical test, so I don't think the logic that goes "if everything went fine we should trigger the error" applies here, but we should go for "if something fails we should trigger the error".

so we should set retval = 1 before the mp_init_multi and set it to zero in inverted logic here.

most likely even better would be to return err and add another int*-argument for the current retval.

IIUC this is a critical test, so I don't think the logic that goes "if everything went fine we should trigger the error" applies here, but we should go for "if something fails we should trigger the error". so we should set `retval = 1` before the `mp_init_multi` and set it to zero in inverted logic here. most likely even better would be to return `err` and add another `int*`-argument for the current `retval`.
src/pk/ecc/ltc_ecc_projective_add_point.c
@ -45,14 +41,32 @@ int ltc_ecc_projective_add_point(ecc_point *P, ecc_point *Q, ecc_point *R, void
return err;
sjaeckel (Migrated from github.com) commented 2017-12-01 02:25:09 -06:00

I've seen this code now for at least the 3rd time, can you please add a ltc_ecc_set_to_point_at_infinity()

I've seen this code now for at least the 3rd time, can you please add a `ltc_ecc_set_to_point_at_infinity()`
sjaeckel (Migrated from github.com) reviewed 2017-12-01 02:34:14 -06:00
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2017-12-01 02:34:14 -06:00

src can also be const

`src` can also be `const`
sjaeckel (Migrated from github.com) reviewed 2017-12-01 02:36:06 -06:00
src/headers/tomcrypt_pk.h
@ -309,3 +336,3 @@
/** the ECC params provided */
extern const ltc_ecc_set_type ltc_ecc_sets[];
extern const ltc_ecc_curve ltc_ecc_curves[];
sjaeckel (Migrated from github.com) commented 2017-12-01 02:36:06 -06:00

thou shalt never manipulate the ecc_key struct directly!

thou shalt never manipulate the `ecc_key` struct directly!
sjaeckel (Migrated from github.com) reviewed 2017-12-01 02:48:29 -06:00
src/headers/tomcrypt_pk.h
sjaeckel (Migrated from github.com) commented 2017-12-01 02:48:29 -06:00

This PR removes that attribute and does the lookup at ecc_set_dp_size by hardcoding the values.

yeah, it's truly not elegant at all... :)

I think it'd make sense to add a ecc_get_dp_by_bits (or sth like that) which finally uses bits instead of octets, then iterate there over the ltc_ecc_sets array, and compare the mp_count_bits(key->dp.prime). ecc_make_key() would then call that one with keysize*8

> This PR removes that attribute and does the lookup at ecc_set_dp_size by hardcoding the values. yeah, it's truly not elegant at all... :) I think it'd make sense to add a `ecc_get_dp_by_bits` (or sth like that) which finally uses bits instead of octets, then iterate there over the `ltc_ecc_sets` array, and compare the `mp_count_bits(key->dp.prime)`. `ecc_make_key()` would then call that one with `keysize*8`
karel-m (Migrated from github.com) reviewed 2017-12-01 13:21:51 -06:00
src/headers/tomcrypt_pk.h
karel-m (Migrated from github.com) commented 2017-12-01 13:21:51 -06:00

The int size is simply redundant (it is the byte size of prime). It used to be a unique identifier of elliptic curve but it is not true any more. For some sizes we several curves so it does not make sense to ask for a curve just by giving a size (no matter whether byte size or bit size). You should use curve name or curve OID.

The ecc_set_dp_size was implemented just for compatibility reasons. It is not nice but it is a backward compatible mapping between old and new API.

The `int size` is simply redundant (it is the byte size of `prime`). It used to be a unique identifier of elliptic curve but it is not true any more. For some sizes we several curves so it does not make sense to ask for a curve just by giving a size (no matter whether byte size or bit size). You should use curve name or curve OID. The `ecc_set_dp_size` was implemented just for compatibility reasons. It is not nice but it is a backward compatible mapping between old and new API.
karel-m (Migrated from github.com) reviewed 2017-12-01 13:42:48 -06:00
src/headers/tomcrypt_pk.h
@ -309,3 +336,3 @@
/** the ECC params provided */
extern const ltc_ecc_set_type ltc_ecc_sets[];
extern const ltc_ecc_curve ltc_ecc_curves[];
karel-m (Migrated from github.com) commented 2017-12-01 13:42:48 -06:00

The sequence: ecc_get_set_by_name + ecc_set_dp + ecc_set_key is IMO pretty handy. Consider ecc_set_key as a kind of "import key" function that is capable of loading raw private/public keys (e.g. bitcoin ECDSA keys uses this raw format, the curve is fixed/well-known "secp256k1").

The sequence: `ecc_get_set_by_name` + `ecc_set_dp` + `ecc_set_key` is IMO pretty handy. Consider `ecc_set_key` as a kind of "import key" function that is capable of loading raw private/public keys (e.g. bitcoin ECDSA keys uses this raw format, the curve is fixed/well-known "secp256k1").
karel-m commented 2017-12-02 11:36:43 -06:00 (Migrated from github.com)

this feels like a too big PR to review properly

Yes it is big but the nature of this change is simply quite complex.

I suggest firstly focus on reviewing newly added functions (like proper naming, calling conventions ...) + changes to the publicly "visible" structs.

After that I can write the updated documentation (which can reveal couple of API gotchas).

And in the end we can polish implementation details.

> this feels like a too big PR to review properly Yes it is big but the nature of this change is simply quite complex. I suggest firstly focus on reviewing newly added functions (like proper naming, calling conventions ...) + changes to the publicly "visible" structs. After that I can write the updated documentation (which can reveal couple of API gotchas). And in the end we can polish implementation details.
sjaeckel commented 2017-12-03 14:11:30 -06:00 (Migrated from github.com)

Yes it is big but the nature of this change is simply quite complex.

I suggest firstly focus on reviewing newly added functions (like proper naming, calling conventions ...) + changes to the publicly "visible" structs.

After that I can write the updated documentation (which can reveal couple of API gotchas).

And in the end we can polish implementation details.

sounds like a plan, we should probably now first finish #309 , I'll add documentation soon, but I think from a coding PoV it's fine. Then we go on with this one.

> Yes it is big but the nature of this change is simply quite complex. > > I suggest firstly focus on reviewing newly added functions (like proper naming, calling conventions ...) + changes to the publicly "visible" structs. > > After that I can write the updated documentation (which can reveal couple of API gotchas). > > And in the end we can polish implementation details. sounds like a plan, we should probably now first finish #309 , I'll add documentation soon, but I think from a coding PoV it's fine. Then we go on with this one.
sjaeckel (Migrated from github.com) reviewed 2018-03-23 08:54:38 -05:00
sjaeckel (Migrated from github.com) left a comment

I've added some more comments, as all the old ones still need to be addressed.
Thanks!

I've added some more comments, as all the old ones still need to be addressed. Thanks!
src/pk/ecc/ecc_get_set.c
sjaeckel (Migrated from github.com) commented 2018-03-23 07:37:56 -05:00
  1. this should get a pre-lookup of alternative names
...
const char* searched_name = NULL;
const struct {
   const char* ltc_name;
   const char* alt_name;
} alt_names[] = {
   { "SECP256R1", "NISTP256" },
   { "SECP256R1", "PRIME256V1" },
...
};
for(i=0;i < sizeof(alt_names)/sizeof(alt_names[0]);++i) {
   if (XSTRCASECMP(...) == 0) {
      searched_name = alt_names[i].ltc_name;
      break;
   }
}
if (searched_name == NULL) searched_name = name;
...
  1. we should follow Postel's law "Lenient input, strict output"
    a. we should accept the official&historic names (e.g. "Curve P-256", "secp256r1", "NIST P-256" and ltc's old "ECC-256" for "NISTP256")
    b. we should agree on which standard to refer to for the output and then use that (I'd say we go with NIST names "Curve P-256" etc.), this one should then also be used in ltc_ecc_sets
    c. we should use something like strcasecmp() instead of strcmp()... I say "something like" because that compare function should also ignore at least spaces and dashes as "NISTP256" is called e.g. "NIST P-256" on djb's safecurves but "nistp256" in some SSH RFC's ... or we simply add both "NISTP256" and "NIST P-256" to the alt_names table and use strcasecmp() ;)
1. this should get a pre-lookup of alternative names ``` ... const char* searched_name = NULL; const struct { const char* ltc_name; const char* alt_name; } alt_names[] = { { "SECP256R1", "NISTP256" }, { "SECP256R1", "PRIME256V1" }, ... }; for(i=0;i < sizeof(alt_names)/sizeof(alt_names[0]);++i) { if (XSTRCASECMP(...) == 0) { searched_name = alt_names[i].ltc_name; break; } } if (searched_name == NULL) searched_name = name; ... ``` 2. we should follow Postel's law "Lenient input, strict output" a. we should accept the official&historic names (e.g. "Curve P-256", "secp256r1", "NIST P-256" and ltc's old "ECC-256" for "NISTP256") b. we should agree on which standard to refer to for the output and then use that (I'd say we go with NIST names "Curve P-256" etc.), this one should then also be used in `ltc_ecc_sets` c. we should use something like `strcasecmp()` instead of `strcmp()`... I say "something like" because that compare function should also ignore at least spaces and dashes as "NISTP256" is called e.g. "NIST P-256" on djb's safecurves but "nistp256" in some SSH RFC's ... or we simply add both "NISTP256" and "NIST P-256" to the `alt_names` table and use `strcasecmp()` ;)
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2018-03-23 07:10:36 -05:00

how about converting the key elements once to strings and strcmp() then the elements of the sets?
alternatively add a function like

static int _ecc_cmp_hex_bn(const char* h, void* b, void* tmp)
{
   if (mp_read_radix(tmp, h, 16) != CRYPT_OK) return CRYPT_ERROR;
   if (mp_cmp(tmp, b) != LTC_MP_EQ) return CRYPT_ERROR;
   return CRYPT_OK;
}
how about converting the `key` elements once to strings and `strcmp()` then the elements of the sets? alternatively add a function like ``` static int _ecc_cmp_hex_bn(const char* h, void* b, void* tmp) { if (mp_read_radix(tmp, h, 16) != CRYPT_OK) return CRYPT_ERROR; if (mp_cmp(tmp, b) != LTC_MP_EQ) return CRYPT_ERROR; return CRYPT_OK; } ```
karel-m (Migrated from github.com) reviewed 2018-03-25 16:18:31 -05:00
src/headers/tomcrypt_pk.h
@ -274,7 +277,13 @@ typedef struct {
karel-m (Migrated from github.com) commented 2018-03-25 16:18:31 -05:00

done (renamed to ltc_ecc_curve)

done (renamed to ltc_ecc_curve)
karel-m (Migrated from github.com) reviewed 2018-03-25 16:18:47 -05:00
src/math/tfm_desc.c
karel-m (Migrated from github.com) commented 2018-03-25 16:18:47 -05:00

done

done
karel-m (Migrated from github.com) reviewed 2018-03-25 16:43:12 -05:00
src/pk/ecc/ecc_set_dp.c
@ -0,0 +1,90 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
karel-m (Migrated from github.com) commented 2018-03-25 16:43:12 -05:00

done (ecc_set_dp_by_size)

done (ecc_set_dp_by_size)
karel-m (Migrated from github.com) reviewed 2018-03-25 16:53:00 -05:00
src/pk/ecc/ecc_get_set.c
karel-m (Migrated from github.com) commented 2018-03-25 16:53:00 -05:00

renamed to ecc_get_curve_by_name here some background when I use dp and when curve

  • a curve is always a pointer to ltc_ecc_curve struct containing curve definition as hex strings
  • domain parameters dp is part of the key, I mean ecc_key which looks like:
typedef struct {
    int type;         /* PK_PRIVATE or PK_PUBLIC */
    ltc_ecc_dp dp;    /* domain parameters */
    ecc_point pubkey; /* public key */
    void *k;          /* private key - MPI number */
}
  • so dp is a structure containing also curve parameters but not as hexadecimal strings, but loaded as mpi numbers

So ecc_get_curve_by_.. means: find ltc_ecc_curve struct which fits given criteria. On the other hand ecc_set_dp_.. means setup dp part of ecc_key

renamed to `ecc_get_curve_by_name` here some background when I use `dp` and when `curve` * a `curve` is always a pointer to `ltc_ecc_curve` struct containing curve definition as **hex strings** * domain parameters `dp` is part of the key, I mean `ecc_key` which looks like: ``` typedef struct { int type; /* PK_PRIVATE or PK_PUBLIC */ ltc_ecc_dp dp; /* domain parameters */ ecc_point pubkey; /* public key */ void *k; /* private key - MPI number */ } ``` * so `dp` is a structure containing also curve parameters but not as hexadecimal strings, but loaded as **mpi numbers** So `ecc_get_curve_by_..` means: find `ltc_ecc_curve` struct which fits given criteria. On the other hand `ecc_set_dp_..` means setup `dp` part of `ecc_key`
karel-m (Migrated from github.com) reviewed 2018-03-25 16:54:22 -05:00
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
karel-m (Migrated from github.com) commented 2018-03-25 16:54:21 -05:00

done

done
karel-m (Migrated from github.com) reviewed 2018-03-25 16:55:49 -05:00
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
karel-m (Migrated from github.com) commented 2018-03-25 16:55:49 -05:00

done

done
karel-m (Migrated from github.com) reviewed 2018-03-25 16:58:11 -05:00
src/pk/ecc/ecc_test.c
@ -21,68 +16,12 @@
karel-m (Migrated from github.com) commented 2018-03-25 16:58:11 -05:00

done

done
karel-m (Migrated from github.com) reviewed 2018-03-25 17:16:16 -05:00
src/pk/ecc/ltc_ecc_is_point_at_infinity.c
@ -0,0 +1,62 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
karel-m (Migrated from github.com) commented 2018-03-25 17:16:16 -05:00

done ( return err and add another int*-argument)

done ( return err and add another int*-argument)
sjaeckel (Migrated from github.com) reviewed 2018-03-26 11:45:04 -05:00
sjaeckel (Migrated from github.com) left a comment

Looks good!

Looks good!
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2018-03-26 11:21:27 -05:00

copied from https://github.com/libtom/libtomcrypt/pull/236#discussion_r176715691

how about converting the key elements once to strings and strcmp() then the elements of the sets?
alternatively add a function like

static int _ecc_cmp_hex_bn(const char* h, void* b, void* tmp)
{
   if (mp_read_radix(tmp, h, 16) != CRYPT_OK) return CRYPT_ERROR;
   if (mp_cmp(tmp, b) != LTC_MP_EQ) return CRYPT_ERROR;
   return CRYPT_OK;
}
_copied from https://github.com/libtom/libtomcrypt/pull/236#discussion_r176715691_ how about converting the `key` elements once to strings and `strcmp()` then the elements of the sets? alternatively add a function like ``` static int _ecc_cmp_hex_bn(const char* h, void* b, void* tmp) { if (mp_read_radix(tmp, h, 16) != CRYPT_OK) return CRYPT_ERROR; if (mp_cmp(tmp, b) != LTC_MP_EQ) return CRYPT_ERROR; return CRYPT_OK; } ```
src/pk/ecc/ltc_ecc_projective_add_point.c
@ -45,14 +41,32 @@ int ltc_ecc_projective_add_point(ecc_point *P, ecc_point *Q, ecc_point *R, void
return err;
sjaeckel (Migrated from github.com) commented 2018-03-26 11:37:32 -05:00

as there's also a case above that does similar stuff how about adding

#define _LTC_ECC_POINT_ZERO 0, 0, 1
#define _LTC_ECC_POINT_INF 1, 1, 0
int ltc_ecc_set_coordinates(ltc_mp_digit x, ltc_mp_digit y, ltc_mp_digit z, ecc_point *P);
as there's also a case above that does similar stuff how about adding ``` #define _LTC_ECC_POINT_ZERO 0, 0, 1 #define _LTC_ECC_POINT_INF 1, 1, 0 int ltc_ecc_set_coordinates(ltc_mp_digit x, ltc_mp_digit y, ltc_mp_digit z, ecc_point *P); ```
karel-m (Migrated from github.com) reviewed 2018-03-27 07:28:20 -05:00
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
karel-m (Migrated from github.com) commented 2018-03-27 07:28:20 -05:00

Well, I did basically the opposite thing, when loading the key I do convert all curve params (hex strings) into mpis. It saved couple of hex2mpi operations (grep for mp_read_radix in the old ecc implementation).

The function _ecc_oid_lookup is not heavily used, therefore I think an extra cost is acceptable here.

Well, I did basically the opposite thing, when loading the key I do convert all curve params (hex strings) into mpis. It saved couple of hex2mpi operations (grep for `mp_read_radix` in the old ecc implementation). The function `_ecc_oid_lookup` is not heavily used, therefore I think an extra cost is acceptable here.
karel-m (Migrated from github.com) reviewed 2018-03-27 07:57:00 -05:00
src/pk/ecc/ltc_ecc_projective_add_point.c
@ -45,14 +41,32 @@ int ltc_ecc_projective_add_point(ecc_point *P, ecc_point *Q, ecc_point *R, void
return err;
karel-m (Migrated from github.com) commented 2018-03-27 07:57:00 -05:00

The ltc_ecc_set_to_point_at_infinity looks reasonable, I do not see many uses for more general ltc_ecc_set_coordinates. But it may be worth considering ltc_ecc_copy_point.

The `ltc_ecc_set_to_point_at_infinity` looks reasonable, I do not see many uses for more general `ltc_ecc_set_coordinates`. But it may be worth considering `ltc_ecc_copy_point`.
karel-m (Migrated from github.com) reviewed 2018-03-27 07:59:10 -05:00
src/pk/ecc/ecc_get_set.c
karel-m (Migrated from github.com) commented 2018-03-27 07:59:10 -05:00

TODO: I agree, a support for alternative names would be nice

TODO: I agree, a support for alternative names would be nice
karel-m commented 2018-03-27 14:33:40 -05:00 (Migrated from github.com)

Ad alternative names - what about to change the curve struct like this:

typedef struct {
   const char *names[6]; /* HERE IS THE CHANGE!!! */
   const char *prime;
   const char *A;
   const char *B;
   const char *order;
   const char *Gx;
   const char *Gy;
   unsigned long cofactor;
   unsigned long oid[16];
   unsigned long oidlen;
} ltc_ecc_curve;

and later use this:

{
  /* curve name */ { "SECP192R1", "NISTP192", "PRIME192V1", NULL, NULL, NULL },
  /* prime      */ "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFFFFFFFFFFFF",
  /* A          */ "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFFFFFFFFFFFC",
  /* B          */ "64210519E59C80E70FA7E9AB72243049FEB8DEECC146B9B1",
  /* order      */ "FFFFFFFFFFFFFFFFFFFFFFFF99DEF836146BC9B1B4D22831",
  /* Gx         */ "188DA80EB03090F67CBF20EB43A18800F4FF0AFD82FF1012",
  /* Gy         */ "07192B95FFC8DA78631011ED6B24CDD573F977A11E794811",
  /* cofactor   */ 1,
  /* OID        */ { 1,2,840,10045,3,1,1 }, 7
}
Ad alternative names - what about to change the curve struct like this: ```C typedef struct { const char *names[6]; /* HERE IS THE CHANGE!!! */ const char *prime; const char *A; const char *B; const char *order; const char *Gx; const char *Gy; unsigned long cofactor; unsigned long oid[16]; unsigned long oidlen; } ltc_ecc_curve; ``` and later use this: ```C { /* curve name */ { "SECP192R1", "NISTP192", "PRIME192V1", NULL, NULL, NULL }, /* prime */ "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFFFFFFFFFFFF", /* A */ "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFFFFFFFFFFFC", /* B */ "64210519E59C80E70FA7E9AB72243049FEB8DEECC146B9B1", /* order */ "FFFFFFFFFFFFFFFFFFFFFFFF99DEF836146BC9B1B4D22831", /* Gx */ "188DA80EB03090F67CBF20EB43A18800F4FF0AFD82FF1012", /* Gy */ "07192B95FFC8DA78631011ED6B24CDD573F977A11E794811", /* cofactor */ 1, /* OID */ { 1,2,840,10045,3,1,1 }, 7 } ```
karel-m commented 2018-03-27 14:42:39 -05:00 (Migrated from github.com)

BTW this PR has broken LTC_MECC_FP support 😞

BTW this PR has broken LTC_MECC_FP support 😞
karel-m (Migrated from github.com) reviewed 2018-03-27 15:02:27 -05:00
src/pk/ecc/ecc_free.c
@ -28,7 +23,12 @@
void ecc_free(ecc_key *key)
karel-m (Migrated from github.com) commented 2018-03-27 15:02:27 -05:00

done (cleanup in one call)

done (cleanup in one call)
karel-m (Migrated from github.com) reviewed 2018-03-27 15:05:58 -05:00
src/pk/ecc/ecc_make_key.c
karel-m (Migrated from github.com) commented 2018-03-27 15:05:58 -05:00

You are right but this pattern already exist e.g. in dsa_set_key

You are right but this pattern already exist e.g. in `dsa_set_key`
sjaeckel commented 2018-03-27 16:43:56 -05:00 (Migrated from github.com)

BTW this PR has broken LTC_MECC_FP support

unfixable?

> BTW this PR has broken LTC_MECC_FP support unfixable?
karel-m commented 2018-03-27 16:49:10 -05:00 (Migrated from github.com)

unfixable?

we are missing order in ltc_ecc_fp_mul2add params as "standard" mul2add does not need it

> unfixable? we are missing `order` in `ltc_ecc_fp_mul2add` params as "standard" mul2add does not need it
sjaeckel (Migrated from github.com) reviewed 2018-03-27 16:50:29 -05:00
src/headers/tomcrypt_pk.h
sjaeckel (Migrated from github.com) commented 2018-03-27 16:45:46 -05:00

I'm not 100% sure if I really like this

I'm not 100% sure if I really like this
src/pk/ecc/ecc_get_set.c
sjaeckel (Migrated from github.com) commented 2018-03-27 16:48:21 -05:00

please use '\0' to check for the terminating NUL byte

please use `'\0'` to check for the terminating NUL byte
sjaeckel commented 2018-03-27 18:25:51 -05:00 (Migrated from github.com)

we are missing order in ltc_ecc_fp_mul2add params as "standard" mul2add does not need it

how about adding it as an argument to ltc_ecc_fp_add_point() and call that one (from one of our API's) before any of fp_mul2add() or fp_mulmod() are called?

BUT I've no clue how widely LTC_MECC_FP is even used and if we should bother investing a lot of time...

> we are missing order in ltc_ecc_fp_mul2add params as "standard" mul2add does not need it how about adding it as an argument to `ltc_ecc_fp_add_point()` and call that one (from one of our API's) before any of `fp_mul2add()` or `fp_mulmod()` are called? _BUT_ I've no clue how widely `LTC_MECC_FP` is even used and if we should bother investing a lot of time...
sjaeckel (Migrated from github.com) reviewed 2018-03-31 05:18:43 -05:00
src/pk/ecc/ecc_get_curve_by_name.c
sjaeckel (Migrated from github.com) commented 2018-03-31 05:11:28 -05:00

Can you please use the official spelling? (as proposed in https://github.com/libtom/libtomcrypt/pull/236#discussion_r176721275)

Can you please use the official spelling? (as proposed in https://github.com/libtom/libtomcrypt/pull/236#discussion_r176721275)
src/pk/ecc/ecc_get_curve_by_name.c
sjaeckel (Migrated from github.com) commented 2018-03-31 05:10:13 -05:00

then we also need a ecc_curve_get_name() which simply returns the first element of the names

then we also need a `ecc_curve_get_name()` which simply returns the first element of the names
src/pk/ecc/ecc_set_dp.c
@ -0,0 +34,4 @@
if ((err = mp_read_radix(key->dp.base.y, curve->Gy, 16)) != CRYPT_OK) { goto error; }
if ((err = mp_set(key->dp.base.z, 1)) != CRYPT_OK) { goto error; }
/* cofactor & size */
key->dp.cofactor = curve->cofactor;
sjaeckel (Migrated from github.com) commented 2018-03-31 05:04:26 -05:00

how about adding str_get_oid(const char*, oid_st*) similar to pk_get_oid()?

how about adding `str_get_oid(const char*, oid_st*)` similar to `pk_get_oid()`?
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2018-03-31 05:03:05 -05:00

Okay, perfectly fine. But please still add _ecc_cmp_hex_bn()

Okay, perfectly fine. But please still add `_ecc_cmp_hex_bn()`
sjaeckel (Migrated from github.com) reviewed 2018-03-31 09:02:42 -05:00
src/pk/ecc/ecc_set_dp.c
@ -0,0 +34,4 @@
if ((err = mp_read_radix(key->dp.base.y, curve->Gy, 16)) != CRYPT_OK) { goto error; }
if ((err = mp_set(key->dp.base.z, 1)) != CRYPT_OK) { goto error; }
/* cofactor & size */
key->dp.cofactor = curve->cofactor;
sjaeckel (Migrated from github.com) commented 2018-03-31 09:02:42 -05:00

...which converts the input, not a LUT as the latter... :) and str_to_oid(const char*, unsigned char*, unsigned long*) would also be perfectly fine!

...which converts the input, not a LUT as the latter... :) and `str_to_oid(const char*, unsigned char*, unsigned long*)` would also be perfectly fine!
karel-m (Migrated from github.com) reviewed 2018-03-31 13:29:07 -05:00
src/pk/ecc/ecc_set_dp.c
@ -0,0 +34,4 @@
if ((err = mp_read_radix(key->dp.base.y, curve->Gy, 16)) != CRYPT_OK) { goto error; }
if ((err = mp_set(key->dp.base.z, 1)) != CRYPT_OK) { goto error; }
/* cofactor & size */
key->dp.cofactor = curve->cofactor;
karel-m (Migrated from github.com) commented 2018-03-31 13:29:07 -05:00

done, see pk_oid_str_to_num + pk_oid_num_to_str

done, see pk_oid_str_to_num + pk_oid_num_to_str
karel-m (Migrated from github.com) reviewed 2018-03-31 13:29:59 -05:00
src/pk/ecc/ecc_get_curve_by_name.c
karel-m (Migrated from github.com) commented 2018-03-31 13:29:59 -05:00

I am not sure, I do not a use case for ecc_curve_get_name

I am not sure, I do not a use case for `ecc_curve_get_name`
karel-m (Migrated from github.com) reviewed 2018-03-31 13:36:49 -05:00
src/pk/ecc/ecc_get_curve_by_name.c
karel-m (Migrated from github.com) commented 2018-03-31 13:36:49 -05:00

Do you mean:

secp256r1
NIST P-256
prime256v1
ECC-256
P-256

?

I also want to point out that apart from official names there is something like "industry standard" how these curves are named in other libraries - e.g. openssl:

$ openssl ecparam -list_curves | grep 'prime field'
  secp112r1 : SECG/WTLS curve over a 112 bit prime field
  secp112r2 : SECG curve over a 112 bit prime field
  secp128r1 : SECG curve over a 128 bit prime field
  secp128r2 : SECG curve over a 128 bit prime field
  secp160k1 : SECG curve over a 160 bit prime field
  secp160r1 : SECG curve over a 160 bit prime field
  secp160r2 : SECG/WTLS curve over a 160 bit prime field
  secp192k1 : SECG curve over a 192 bit prime field
  secp224k1 : SECG curve over a 224 bit prime field
  secp224r1 : NIST/SECG curve over a 224 bit prime field
  secp256k1 : SECG curve over a 256 bit prime field
  secp384r1 : NIST/SECG curve over a 384 bit prime field
  secp521r1 : NIST/SECG curve over a 521 bit prime field
  prime192v1: NIST/X9.62/SECG curve over a 192 bit prime field
  prime192v2: X9.62 curve over a 192 bit prime field
  prime192v3: X9.62 curve over a 192 bit prime field
  prime239v1: X9.62 curve over a 239 bit prime field
  prime239v2: X9.62 curve over a 239 bit prime field
  prime239v3: X9.62 curve over a 239 bit prime field
  prime256v1: X9.62/SECG curve over a 256 bit prime field
  wap-wsg-idm-ecid-wtls6: SECG/WTLS curve over a 112 bit prime field
  wap-wsg-idm-ecid-wtls7: SECG/WTLS curve over a 160 bit prime field
  wap-wsg-idm-ecid-wtls8: WTLS curve over a 112 bit prime field
  wap-wsg-idm-ecid-wtls9: WTLS curve over a 160 bit prime field
  wap-wsg-idm-ecid-wtls12: WTLS curvs over a 224 bit prime field
  brainpoolP160r1: RFC 5639 curve over a 160 bit prime field
  brainpoolP160t1: RFC 5639 curve over a 160 bit prime field
  brainpoolP192r1: RFC 5639 curve over a 192 bit prime field
  brainpoolP192t1: RFC 5639 curve over a 192 bit prime field
  brainpoolP224r1: RFC 5639 curve over a 224 bit prime field
  brainpoolP224t1: RFC 5639 curve over a 224 bit prime field
  brainpoolP256r1: RFC 5639 curve over a 256 bit prime field
  brainpoolP256t1: RFC 5639 curve over a 256 bit prime field
  brainpoolP320r1: RFC 5639 curve over a 320 bit prime field
  brainpoolP320t1: RFC 5639 curve over a 320 bit prime field
  brainpoolP384r1: RFC 5639 curve over a 384 bit prime field
  brainpoolP384t1: RFC 5639 curve over a 384 bit prime field
  brainpoolP512r1: RFC 5639 curve over a 512 bit prime field
  brainpoolP512t1: RFC 5639 curve over a 512 bit prime field

We should be IMO able to understand these "common" names.

Do you mean: ``` secp256r1 NIST P-256 prime256v1 ECC-256 P-256 ``` ? I also want to point out that apart from official names there is something like "industry standard" how these curves are named in other libraries - e.g. openssl: ``` $ openssl ecparam -list_curves | grep 'prime field' secp112r1 : SECG/WTLS curve over a 112 bit prime field secp112r2 : SECG curve over a 112 bit prime field secp128r1 : SECG curve over a 128 bit prime field secp128r2 : SECG curve over a 128 bit prime field secp160k1 : SECG curve over a 160 bit prime field secp160r1 : SECG curve over a 160 bit prime field secp160r2 : SECG/WTLS curve over a 160 bit prime field secp192k1 : SECG curve over a 192 bit prime field secp224k1 : SECG curve over a 224 bit prime field secp224r1 : NIST/SECG curve over a 224 bit prime field secp256k1 : SECG curve over a 256 bit prime field secp384r1 : NIST/SECG curve over a 384 bit prime field secp521r1 : NIST/SECG curve over a 521 bit prime field prime192v1: NIST/X9.62/SECG curve over a 192 bit prime field prime192v2: X9.62 curve over a 192 bit prime field prime192v3: X9.62 curve over a 192 bit prime field prime239v1: X9.62 curve over a 239 bit prime field prime239v2: X9.62 curve over a 239 bit prime field prime239v3: X9.62 curve over a 239 bit prime field prime256v1: X9.62/SECG curve over a 256 bit prime field wap-wsg-idm-ecid-wtls6: SECG/WTLS curve over a 112 bit prime field wap-wsg-idm-ecid-wtls7: SECG/WTLS curve over a 160 bit prime field wap-wsg-idm-ecid-wtls8: WTLS curve over a 112 bit prime field wap-wsg-idm-ecid-wtls9: WTLS curve over a 160 bit prime field wap-wsg-idm-ecid-wtls12: WTLS curvs over a 224 bit prime field brainpoolP160r1: RFC 5639 curve over a 160 bit prime field brainpoolP160t1: RFC 5639 curve over a 160 bit prime field brainpoolP192r1: RFC 5639 curve over a 192 bit prime field brainpoolP192t1: RFC 5639 curve over a 192 bit prime field brainpoolP224r1: RFC 5639 curve over a 224 bit prime field brainpoolP224t1: RFC 5639 curve over a 224 bit prime field brainpoolP256r1: RFC 5639 curve over a 256 bit prime field brainpoolP256t1: RFC 5639 curve over a 256 bit prime field brainpoolP320r1: RFC 5639 curve over a 320 bit prime field brainpoolP320t1: RFC 5639 curve over a 320 bit prime field brainpoolP384r1: RFC 5639 curve over a 384 bit prime field brainpoolP384t1: RFC 5639 curve over a 384 bit prime field brainpoolP512r1: RFC 5639 curve over a 512 bit prime field brainpoolP512t1: RFC 5639 curve over a 512 bit prime field ``` We should be IMO able to understand these "common" names.
sjaeckel (Migrated from github.com) reviewed 2018-03-31 13:37:34 -05:00
src/misc/pk_oid_str.c
@ -0,0 +1,82 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2018-03-31 13:37:34 -05:00

👍

*cough* der_test.c:313 *cough*

:)

:+1: \*cough\* [der_test.c:313](https://github.com/libtom/libtomcrypt/blob/develop/tests/der_test.c#L313) \*cough\* :)
sjaeckel (Migrated from github.com) reviewed 2018-03-31 13:38:34 -05:00
src/pk/ecc/ecc_get_curve_by_name.c
sjaeckel (Migrated from github.com) commented 2018-03-31 13:38:34 -05:00

sounds good

sounds good
sjaeckel (Migrated from github.com) reviewed 2018-03-31 14:00:58 -05:00
src/pk/ecc/ecc_get_curve_by_name.c
sjaeckel (Migrated from github.com) commented 2018-03-31 14:00:58 -05:00

btw regarding the names

secp256r1 (secg name)
NIST P-256 (safecurves uses this name, but fips-186-4 calls it "Curve P-256" so should we add that as well?)
prime256v1 (is that the X9.62 name?)
ECC-256 (the old libtom name)
P-256 (where does this come from?)

... and this comment is only relevant if we add ecc_curve_get_name() ... I don't like the inconsistency in the openssl naming...

...
  secp521r1 : NIST/SECG curve over a 521 bit prime field
  prime192v1: NIST/X9.62/SECG curve over a 192 bit prime fie
...
  wap-wsg-idm-ecid-wtls6: SECG/WTLS curve over a 112 bit prime field
...

I'd propose to use the names if available in this order of the standards bodies: NIST-SECG-ANSI-BRAINPOOL-OTHERS

btw regarding the names ``` secp256r1 (secg name) NIST P-256 (safecurves uses this name, but fips-186-4 calls it "Curve P-256" so should we add that as well?) prime256v1 (is that the X9.62 name?) ECC-256 (the old libtom name) P-256 (where does this come from?) ``` ... and this comment is only relevant if we add `ecc_curve_get_name()` ... I don't like the inconsistency in the openssl naming... ``` ... secp521r1 : NIST/SECG curve over a 521 bit prime field prime192v1: NIST/X9.62/SECG curve over a 192 bit prime fie ... wap-wsg-idm-ecid-wtls6: SECG/WTLS curve over a 112 bit prime field ... ``` I'd propose to use the names if available in this order of the standards bodies: NIST-SECG-ANSI-BRAINPOOL-OTHERS
buggywhip commented 2018-04-01 00:15:21 -05:00 (Migrated from github.com)

We have ecc_get_curve_by_name() but ought we also have ecc_get_curve_by_oid() using the dotted string form of OID?

int ecc_get_curve_by_oid(const char* oid, const ltc_ecc_curve** cu);

We have ecc_get_curve_by_name() but ought we also have ecc_get_curve_by_oid() using the dotted string form of OID? int ecc_get_curve_by_oid(const char* oid, const ltc_ecc_curve** cu);
buggywhip commented 2018-04-01 00:24:13 -05:00 (Migrated from github.com)

Ought we have a mechanical process to confirm that a name only appears once in a list of all names that appear in all curves?

Ought we have a mechanical process to confirm that a name only appears once in a list of all names that appear in all curves?
buggywhip commented 2018-04-01 00:33:40 -05:00 (Migrated from github.com)

I like the idea of supporting synonymous names for the same curve but am bothered by including the potentially ambiguous names like P-256 and ECC-256, past history notwithstanding. I'd much prefer developers change over to the standard-based names like secp256r1 or prime256v1. Failing that, the doc should be crystal clear that P-XXX and ECC-XXX are only supported for reasons of backward compatibility.

I like the idea of supporting synonymous names for the same curve but am bothered by including the potentially ambiguous names like P-256 and ECC-256, past history notwithstanding. I'd much prefer developers change over to the standard-based names like secp256r1 or prime256v1. Failing that, the doc should be crystal clear that P-XXX and ECC-XXX are only supported for reasons of backward compatibility.
karel-m (Migrated from github.com) reviewed 2018-04-01 09:42:03 -05:00
src/pk/ecc/ecc_get_curve_by_name.c
karel-m (Migrated from github.com) commented 2018-04-01 09:42:03 -05:00

Ad P-256 & co. - these names are also used in JSON Web Signature (JWS) related RFCs 7515, 7518. It is perhaps based on curve names from https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf

Ad P-256 & co. - these names are also used in JSON Web Signature (JWS) related RFCs 7515, 7518. It is perhaps based on curve names from https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf
karel-m commented 2018-04-01 09:44:46 -05:00 (Migrated from github.com)

ad int ecc_get_curve_by_oid(const char* oid, const ltc_ecc_curve** cu);

with current implementation you can call:

err = ecc_get_curve_by_name("1.3.132.0.31", &curve);

I know that the name is a bit misleading but we can explain it in documentation.

ad `int ecc_get_curve_by_oid(const char* oid, const ltc_ecc_curve** cu);` with current implementation you can call: ```C err = ecc_get_curve_by_name("1.3.132.0.31", &curve); ``` I know that the name is a bit misleading but we can explain it in documentation.
sjaeckel (Migrated from github.com) reviewed 2018-04-01 09:51:36 -05:00
src/misc/pk_oid_str.c
@ -0,0 +1,82 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2018-04-01 09:51:36 -05:00

lulz, no the one before was perfectly fine, I just thought you can use it there (please keep your version!)

lulz, no the one before was perfectly fine, I just thought you can use it there (please keep your version!)
karel-m (Migrated from github.com) reviewed 2018-04-01 13:01:46 -05:00
src/misc/pk_oid_str.c
@ -0,0 +1,82 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
karel-m (Migrated from github.com) commented 2018-04-01 13:01:46 -05:00

OK, I'll put it back than :)

OK, I'll put it back than :)
sjaeckel commented 2018-04-03 09:08:31 -05:00 (Migrated from github.com)

I know that the name is a bit misleading but we can explain it in documentation.

how about renaming it to ecc_get_curve(const char *id, const ltc_ecc_curve **cu)?

> I know that the name is a bit misleading but we can explain it in documentation. how about renaming it to `ecc_get_curve(const char *id, const ltc_ecc_curve **cu)`?
sjaeckel (Migrated from github.com) reviewed 2018-04-03 09:32:05 -05:00
src/misc/pk_oid_str.c
@ -0,0 +1,82 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2018-04-03 09:32:05 -05:00

how about returning CRYPT_BUFFER_OVERFLOW etc.?
I know it's still a "private" function, but then it'd already be "public-ready" :)

how about returning `CRYPT_BUFFER_OVERFLOW` etc.? I know it's still a "private" function, but then it'd already be "public-ready" :)
sjaeckel (Migrated from github.com) reviewed 2018-04-03 09:41:29 -05:00
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
sjaeckel (Migrated from github.com) commented 2018-04-03 09:41:29 -05:00

can you please add this as parameter so we don't have to do init&clear for each compare operation?

I know that this shouldn't be called this often, but still it'd be an easy improvement

can you please add this as parameter so we don't have to do init&clear for each compare operation? I know that this shouldn't be called this often, but still it'd be an easy improvement
karel-m (Migrated from github.com) reviewed 2018-04-07 13:31:04 -05:00
src/pk/ecc/ecc_set_dp_internal.c
@ -0,0 +1,129 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
karel-m (Migrated from github.com) commented 2018-04-07 13:31:04 -05:00

done

done
karel-m (Migrated from github.com) reviewed 2018-04-07 13:31:13 -05:00
src/misc/pk_oid_str.c
@ -0,0 +1,82 @@
/* LibTomCrypt, modular cryptographic library -- Tom St Denis
karel-m (Migrated from github.com) commented 2018-04-07 13:31:13 -05:00

done

done
karel-m (Migrated from github.com) reviewed 2018-04-07 13:33:11 -05:00
src/pk/ecc/ecc_get_curve_by_name.c
karel-m (Migrated from github.com) commented 2018-04-07 13:33:10 -05:00

Do we really need something like ecc_curve_get_name()? What would be the use case?

Do we really need something like `ecc_curve_get_name()`? What would be the use case?
buggywhip commented 2018-04-07 15:51:20 -05:00 (Migrated from github.com)

err = ecc_get_curve_by_name("1.3.132.0.31", &curve);

I know that the name is a bit misleading but we can explain it in documentation.

how about renaming it to ecc_get_curve(const char *id, const ltc_ecc_curve **cu)?

For me the doc should be sufficient.   ...and I kinda like the shorter name.  But I'd like to suggest we extend the arg id to identifier which can more readily encompass and associate both name and OID.
   ecc_get_curve(const char *identifier, const ltc_ecc_curve **cu)

>>`err = ecc_get_curve_by_name("1.3.132.0.31", &curve);` >> >>I know that the name is a bit misleading but we can explain it in documentation. > >how about renaming it to `ecc_get_curve(const char *id, const ltc_ecc_curve **cu)`? For me the doc should be sufficient.   ...and I kinda like the shorter name.  But I'd like to suggest we extend the arg `id` to `identifier` which can more readily encompass and associate both name and OID.    `ecc_get_curve(const char *identifier, const ltc_ecc_curve **cu)`
karel-m commented 2018-04-07 15:55:18 -05:00 (Migrated from github.com)

In the end I have decided for:

int ecc_get_curve(const char* name_or_oid, const ltc_ecc_curve** cu);
In the end I have decided for: ```C int ecc_get_curve(const char* name_or_oid, const ltc_ecc_curve** cu); ```
👍 1
buggywhip commented 2018-04-07 15:56:51 -05:00 (Migrated from github.com)

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!

> 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!
sjaeckel (Migrated from github.com) reviewed 2018-04-09 07:55:20 -05:00
src/pk/ecc/ecc_get_curve_by_name.c
sjaeckel (Migrated from github.com) commented 2018-04-09 07:55:20 -05:00

What would be the use case?

A CLI tool similar to openssl ecparam -list_curves | grep 'prime field'?
We can also leave it as it is for now and add the API as soon as that tool is written ;)

> What would be the use case? A CLI tool similar to `openssl ecparam -list_curves | grep 'prime field'`? We can also leave it as it is for now and add the API as soon as that tool is written ;)
karel-m commented 2018-04-09 16:13:04 -05:00 (Migrated from github.com)

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

@fperrad could you please check (const params + usual stuff) with your linter `pr/ecc-non-asn1-part` branch?
fperrad commented 2018-04-10 11:46:50 -05:00 (Migrated from github.com)

@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 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 commented 2018-04-10 13:06:13 -05:00 (Migrated from github.com)

@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 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 commented 2018-04-10 13:40:32 -05:00 (Migrated from github.com)

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
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 commented 2018-04-11 04:46:54 -05:00 (Migrated from github.com)

I missed this one:

Info 765: external '_curve_names' (line 17, file src/pk/ecc/ecc_get_curve.c) could be made static
I missed this one: ``` Info 765: external '_curve_names' (line 17, file src/pk/ecc/ecc_get_curve.c) could be made static ```
sjaeckel (Migrated from github.com) reviewed 2018-04-13 07:26:38 -05:00
src/headers/tomcrypt_custom.h
sjaeckel (Migrated from github.com) commented 2018-04-13 07:26:37 -05:00

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

what do you think about applying this patch?

[0001-don-t-use-deprecated-LTC_ECCxxx-defines-anymore.patch.txt](https://github.com/libtom/libtomcrypt/files/1907292/0001-don-t-use-deprecated-LTC_ECCxxx-defines-anymore.patch.txt) what do you think about applying this patch?
karel-m (Migrated from github.com) reviewed 2018-04-13 13:23:57 -05:00
src/headers/tomcrypt_custom.h
karel-m (Migrated from github.com) commented 2018-04-13 13:23:57 -05:00

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

The patch is good. Please commit&push it to this branch.
sjaeckel (Migrated from github.com) reviewed 2018-04-13 18:13:12 -05:00
src/headers/tomcrypt_custom.h
sjaeckel (Migrated from github.com) commented 2018-04-13 18:13:12 -05:00

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

applied & I was a bit surprised when the `./test ecc`-run returned a `nop` instead of a `passed` :)
karel-m commented 2018-04-27 12:18:07 -05:00 (Migrated from github.com)

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

Should I start working on doc update or do we plan more changes to API / struct & co. ?
sjaeckel commented 2018-04-27 15:14:44 -05:00 (Migrated from github.com)

Go for the docs! 👍

Go for the docs! :+1:
sjaeckel commented 2018-05-02 16:52:21 -05:00 (Migrated from github.com)

And feel free to squash again :)

And feel free to squash again :)
karel-m commented 2018-05-06 10:18:24 -05:00 (Migrated from github.com)

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.

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 commented 2018-05-06 15:44:05 -05:00 (Migrated from github.com)

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

> 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 commented 2018-05-07 09:54:45 -05:00 (Migrated from github.com)

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?

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 commented 2018-05-13 14:59:29 -05:00 (Migrated from github.com)

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

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)

@mkj my first draft (not in a pull-request quality yet) is here https://github.com/karel-m/dropbear/commit/4530ff68975932680d674a33ea477fa7afc79ade 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 (Migrated from github.com) approved these changes 2018-05-20 06:21:41 -05:00
sjaeckel (Migrated from github.com) left a comment

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

After the Xth try to review I arrived at the end of the PR :)
karel-m commented 2018-05-22 15:50:46 -05:00 (Migrated from github.com)

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

I'll open a separate PR for doc changes

Great I'll squash it to a single commit and merge to develop. I'll open a separate PR for doc changes
🎉 1
Sign in to join this conversation.
Reviewers
No reviewers
sjaeckel
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
feature

   
help wanted

   
improve API

   
invalid

   
needs documentation

   
not-planned
This ticket is not planned to be implemented and/or fixed from the core team.

  
rtfm

   
stale

   
support

   
wishlist
No labels
bug
duplicate
enhancement
feature
help wanted
improve API
invalid
needs documentation
not-planned
rtfm
stale
support
wishlist
Milestone
Clear milestone
No items
No milestone
next
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-libtom/libtomcrypt!236
No description provided.