ECC enhancements #65

2015-04-17T14:57:28-05:00

karel-m commented

2015-04-17 14:57:28 -05:00

(Migrated from github.com)

This PR is related to https://github.com/libtom/libtomcrypt/issues/34

sjaeckel commented

2015-04-18 03:50:19 -05:00

(Migrated from github.com)

I think the build error can be fixed in several ways:

by making point compression optional and disabling it by default
creating a new release of libtommath with the required additional algorithm, waiting until it's published
building libtommath from the git repositories
combining 1&3 and creating a special recipe with point compression enabled and build ltm from git

I would prefer going for 2 or 4

I think the build error can be fixed in several ways: 1. by making point compression optional and disabling it by default 2. creating a new release of libtommath with the required additional algorithm, waiting until it's published 3. building libtommath from the git repositories 4. combining 1&3 and creating a special recipe with point compression enabled and build ltm from git I would prefer going for 2 or 4

karel-m commented

2015-04-18 12:53:27 -05:00

(Migrated from github.com)

Or we can introduce in libtommath something like LTM_VERSION (e.g. 0x0042 for version 0.42) and then in libtomcrypt's ltm_desc.c:

/* sqrtmod_prime */
static int sqrtmod_prime(void *a, void *b, void *c)
{
   LTC_ARGCHK(a != NULL);
   LTC_ARGCHK(b != NULL);
   LTC_ARGCHK(c != NULL);
#if !defined(LTM_VERSION) || LTM_VERSION < 0x0042
   return CRYPT_LTM_TOO_OLD; 
#else
   return mpi_to_ltc_error(mp_sqrtmod_prime(a, b, c));
#endif
}

Or we can introduce in libtommath something like LTM_VERSION (e.g. 0x0042 for version 0.42) and then in libtomcrypt's `ltm_desc.c`: ``` c /* sqrtmod_prime */ static int sqrtmod_prime(void *a, void *b, void *c) { LTC_ARGCHK(a != NULL); LTC_ARGCHK(b != NULL); LTC_ARGCHK(c != NULL); #if !defined(LTM_VERSION) || LTM_VERSION < 0x0042 return CRYPT_LTM_TOO_OLD; #else return mpi_to_ltc_error(mp_sqrtmod_prime(a, b, c)); #endif } ```

karel-m commented

2017-03-14 13:52:20 -05:00

(Migrated from github.com)

@fperrad may I ask you for running your linting machine on miko-ecc-enhancements branch? Thanks.

@fperrad may I ask you for running your linting machine on `miko-ecc-enhancements` branch? Thanks.

fperrad commented

2017-03-15 00:31:55 -05:00

(Migrated from github.com)

@karel-m linting issues:

missing prototypes in tomcrypt.h : ecc_sign_hash_ex & ecc_verify_hash_ex

Info 765: external 'ecc_sign_hash_ex(const unsigned char *, unsigned long, unsigned char *, unsigned long *, union Prng_state *, int, struct ecc_key *, int)' (line 24, file src/pk/ecc/ecc_sign_hash.c) could be made static
Info 765: external 'ecc_verify_hash_ex(const unsigned char *, unsigned long, const unsigned char *, unsigned long, int *, struct ecc_key *, int)' (line 34, file src/pk/ecc/ecc_verify_hash.c) could be made static

bad indentation line 30 in src/pk/ecc/ecc_export_raw.c

bad indentation line 62 in src/pk/ecc/ecc_sign_hash.c

bad indentation line 72, 74 & 75 in src/pk/ecc/ecc_import_raw.c

@karel-m linting issues: missing prototypes in tomcrypt.h : ecc_sign_hash_ex & ecc_verify_hash_ex ``` Info 765: external 'ecc_sign_hash_ex(const unsigned char *, unsigned long, unsigned char *, unsigned long *, union Prng_state *, int, struct ecc_key *, int)' (line 24, file src/pk/ecc/ecc_sign_hash.c) could be made static Info 765: external 'ecc_verify_hash_ex(const unsigned char *, unsigned long, const unsigned char *, unsigned long, int *, struct ecc_key *, int)' (line 34, file src/pk/ecc/ecc_verify_hash.c) could be made static ``` bad indentation line 30 in src/pk/ecc/ecc_export_raw.c bad indentation line 62 in src/pk/ecc/ecc_sign_hash.c bad indentation line 72, 74 & 75 in src/pk/ecc/ecc_import_raw.c

sjaeckel commented

2017-03-15 09:41:01 -05:00

(Migrated from github.com)

can we somehow bring the coverage up again?

karel-m commented

2017-03-15 11:15:46 -05:00

(Migrated from github.com)

can we somehow bring the coverage up again?

What has to be done to bring it up?

> can we somehow bring the coverage up again? What has to be done to bring it up?

sjaeckel commented

2017-03-15 12:11:18 -05:00

(Migrated from github.com)

These aren't tested at all

 ecc_sign_hash_rfc7518()
 ecc_verify_hash_rfc7518()

There are no tests for all the added functionality
```
 pk_get_oid()
```
The new tag field in DER/ltc_asn1_list
The case a==NULL in some of the code paths that are handled specially

1. These aren't tested at all ecc_sign_hash_rfc7518() ecc_verify_hash_rfc7518() 1. There are no tests for all the added functionality pk_get_oid() 1. The new `tag` field in DER/`ltc_asn1_list` 1. The case `a==NULL` in some of the code paths that are handled specially

karel-m commented

2017-03-15 15:43:23 -05:00

(Migrated from github.com)

Oh yes, tests, I know.

coveralls commented

2017-03-29 03:41:02 -05:00

(Migrated from github.com)

Coverage decreased (-3.8%) to 83.635% when pulling 1caf67f911cac889a29f61ae234b2e2b373ed5a1 on miko-ecc-enhancements into 1b81848576 on develop.

[![Coverage Status](https://coveralls.io/builds/10823982/badge)](https://coveralls.io/builds/10823982) Coverage decreased (-3.8%) to 83.635% when pulling **1caf67f911cac889a29f61ae234b2e2b373ed5a1 on miko-ecc-enhancements** into **1b81848576c1064191930a07f67b4c092fe3069f on develop**.

sjaeckel (Migrated from github.com) reviewed

2017-03-30 11:30:49 -05:00

src/pk/asn1/der/sequence/der_encode_sequence_ex.c

					
				@ -212,8 +86,6 @@ int der_encode_sequence_ex(ltc_asn1_list *list, unsigned long inlen,

				               if ((err = der_encode_boolean(*((int *)data), out + x, &z)) != CRYPT_OK) {

				                  goto LBL_ERR;

				               }

I prefer not to use a default case when I switch on an enum as the compiler then warns me if I add elements to the enum and forget to modify the switch-case

karel-m commented

2017-04-04 02:49:44 -05:00

(Migrated from github.com)

@sjaeckel this PR is too complex (rebasing is quite often a pain), I am considering the idea of splitting this PR into two parts: 1/ ASN.1 changes + 2/ EC-only stuf.

The part 1/ would be a new PR (which I want to merge to develop before continuing on the remaining EC-only part) comprising of approx. ede958b + subset of 01c319d

What do you think?

@sjaeckel this PR is too complex (rebasing is quite often a pain), I am considering the idea of splitting this PR into two parts: 1/ ASN.1 changes + 2/ EC-only stuf. The part 1/ would be a new PR (which I want to merge to develop before continuing on the remaining EC-only part) comprising of approx. ede958b + subset of 01c319d What do you think?

coveralls commented

2017-04-04 03:07:19 -05:00

(Migrated from github.com)

Coverage decreased (-3.5%) to 84.284% when pulling c7c170dc5022564aac1597c782d3576c763e2b09 on miko-ecc-enhancements into fbe7d222c3 on develop.

[![Coverage Status](https://coveralls.io/builds/10917136/badge)](https://coveralls.io/builds/10917136) Coverage decreased (-3.5%) to 84.284% when pulling **c7c170dc5022564aac1597c782d3576c763e2b09 on miko-ecc-enhancements** into **fbe7d222c3fafb3df7f0048cdbeb532a06ea198d on develop**.

sjaeckel commented

2017-04-04 03:43:49 -05:00

(Migrated from github.com)

...splitting this PR into two parts: 1/ ASN.1 changes + 2/ EC-only stuf.

Very good idea, that also makes review easier. Can the EC part probably be split even more?

> ...splitting this PR into two parts: 1/ ASN.1 changes + 2/ EC-only stuf. Very good idea, that also makes review easier. Can the EC part probably be split even more?

karel-m commented

2017-04-04 03:47:39 -05:00

(Migrated from github.com)

Can the EC part probably be split even more?

We'll see, let's start with ASN.1 part

> Can the EC part probably be split even more? We'll see, let's start with ASN.1 part

sjaeckel commented

2017-06-16 02:42:55 -05:00

(Migrated from github.com)

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

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

karel-m commented

2017-06-21 16:34:07 -05:00

(Migrated from github.com)

I have split this PR and my branch miko-ecc-enhancements to:

#236 ECC enhancements part 1 (curves y^2 = x^3 + ax + b) - branch pr/ecc-non-asn1-part
#187 ECC enhancements part 2 (ASN.1 changes) - branch pr/ecc-asn1-part (based on pr/ecc-non-asn1-part)

So basically current miko-ecc-enhancement is the same as pr/ecc-asn1-part

There fore I am closing this PR, please move futher discussion to #236 + #187

I have split this PR and my branch `miko-ecc-enhancements` to: * #236 ECC enhancements part 1 (curves y^2 = x^3 + ax + b) - branch `pr/ecc-non-asn1-part` * #187 ECC enhancements part 2 (ASN.1 changes) - branch `pr/ecc-asn1-part` (based on `pr/ecc-non-asn1-part`) So basically current `miko-ecc-enhancement` is the same as `pr/ecc-asn1-part` There fore I am closing this PR, please move futher discussion to #236 + #187