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

2017-04-04T03:59:41-05:00

karel-m commented

2017-04-04 03:59:41 -05:00

(Migrated from github.com)

This PR was extracted from #65

I have written this code in 2014 (just an excuse in case it starts to seem that I am not completely sure what these changes are good for).

The main reason for this patch was ASN.1 support for importing/exporting EC keys which are stored in ASN.1 structures using esoteric stuff like context specific tags and optional items in ASN.1 sequences.

You will notice on the first sight the ugly duplication in API that I have introduced (to keep backwards compatibility of "non *_ex" functions):

der_decode_subject_public_key_info
der_decode_subject_public_key_info_ex

and

der_length_sequence
der_length_sequence_ex

This PR was extracted from #65 I have written this code in 2014 (just an excuse in case it starts to seem that I am not completely sure what these changes are good for). The main reason for this patch was ASN.1 support for importing/exporting EC keys which are stored in ASN.1 structures using esoteric stuff like context specific tags and optional items in ASN.1 sequences. You will notice on the first sight the ugly duplication in API that I have introduced (to keep backwards compatibility of "non *_ex" functions): ``` der_decode_subject_public_key_info der_decode_subject_public_key_info_ex ``` and ``` der_length_sequence der_length_sequence_ex ```

sjaeckel (Migrated from github.com) reviewed

2017-04-04 05:19:59 -05:00

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

was this a bug before or this one now?

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

👍 I like this change!

:+1: I like this change!

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

https://github.com/libtom/libtomcrypt/pull/65#discussion_r108974027

karel-m (Migrated from github.com) reviewed

2017-04-04 07:27:29 -05:00

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

fixed

karel-m (Migrated from github.com) reviewed

2017-04-04 07:28:08 -05:00

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

Well, I am not sure.

coveralls commented

2017-04-04 07:54:09 -05:00

(Migrated from github.com)

Coverage decreased (-0.2%) to 87.605% when pulling c666bc4acd45c57cf9d30ddb75f0e5e323f648f6 on pr/ecc-asn1-part into fbe7d222c3 on develop.

[![Coverage Status](https://coveralls.io/builds/10920885/badge)](https://coveralls.io/builds/10920885) Coverage decreased (-0.2%) to 87.605% when pulling **c666bc4acd45c57cf9d30ddb75f0e5e323f648f6 on pr/ecc-asn1-part** into **fbe7d222c3fafb3df7f0048cdbeb532a06ea198d on develop**.

sjaeckel commented

2017-04-04 08:55:04 -05:00

(Migrated from github.com)

IIUC this implements context-specific tag encoding differently than in the flexi decoder (it's stored in tag and we currently use the used field to store). Should we then also adjust the flexi decoder or is this not the same?

IIUC this implements context-specific tag encoding differently than in the flexi decoder (it's stored in `tag` and we currently use the `used` field to store). Should we then also adjust the flexi decoder or is this not the same?

karel-m commented

2017-04-04 13:24:37 -05:00

(Migrated from github.com)

Ad context-specific tag encoding - when I added tag I probably did not notice that used might already exist for the same reason.

I am in no way an expert at ASN.1, when I created the patch back in 2014 the only goal was to be able to load/save EC keys which I somehow managed. But some parts are rather hacks/workarounds.

If you want some testing keys, go to https://github.com/DCIT/perl-CryptX/tree/master/t/data and take any of ECC related *.der files.

Ad context-specific tag encoding - when I added `tag` I probably did not notice that `used` might already exist for the same reason. I am in no way an expert at ASN.1, when I created the patch back in 2014 the only goal was to be able to load/save EC keys which I somehow managed. But some parts are rather hacks/workarounds. If you want some testing keys, go to https://github.com/DCIT/perl-CryptX/tree/master/t/data and take any of ECC related `*.der` files.

sjaeckel (Migrated from github.com) reviewed

2017-04-05 17:45:36 -05:00

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

XMEMMOVE()?

`XMEMMOVE()`?

sjaeckel commented

2017-04-05 17:53:54 -05:00

(Migrated from github.com)

Ad context-specific tag encoding - when I added tag I probably did not notice that used might already exist for the same reason.

As of the comment on the used field we could just re-use it instead of adding tag. Most likely it'd make sense to rename it to something else. Initially it was just kind of a hack so the struct definition won't change. As I don't see a way to somehow mangle the optional part without changing the struct I think a rename would be fine.

I am in no way an expert at ASN.1, when I created the patch back in 2014 the only goal was to be able to load/save EC keys which I somehow managed. But some parts are rather hacks/workarounds.

If you want some testing keys, go to https://github.com/DCIT/perl-CryptX/tree/master/t/data and take any of ECC related *.der files.

I'm also no expert at ASN.1 but I'll have a look when I have the time.

Would it probably make sense to split this PR up into two parts? One with the changes for tag and one for optional and 'the rest'?

> Ad context-specific tag encoding - when I added `tag` I probably did not notice that `used` might already exist for the same reason. As of the comment on the `used` field we could just re-use it instead of adding `tag`. Most likely it'd make sense to rename it to something else. Initially it was just kind of a hack so the struct definition won't change. As I don't see a way to somehow mangle the `optional` part without changing the struct I think a rename would be fine. > I am in no way an expert at ASN.1, when I created the patch back in 2014 the only goal was to be able to load/save EC keys which I somehow managed. But some parts are rather hacks/workarounds. > > If you want some testing keys, go to https://github.com/DCIT/perl-CryptX/tree/master/t/data and take any of ECC related `*.der` files. I'm also no expert at ASN.1 but I'll have a look when I have the time. Would it probably make sense to split this PR up into two parts? One with the changes for `tag` and one for `optional` and 'the rest'?

karel-m (Migrated from github.com) reviewed

2017-04-06 02:35:46 -05:00

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

Looking at the code I believe that missing if (!ordered) { continue; } was a bug

Looking at the code I believe that missing `if (!ordered) { continue; }` was a bug

sjaeckel (Migrated from github.com) reviewed

2017-04-06 16:11:01 -05:00

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

is this one correct? this looks fishy to me as the used field is only used when decoding but der_length_sequence_ex() is used when encoding

is this one correct? this looks fishy to me as the `used` field is only used when decoding but `der_length_sequence_ex()` is used when encoding

karel-m (Migrated from github.com) reviewed

2017-04-06 17:02:20 -05:00

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

The der_length_sequence (which calls der_length_sequence_ex) is called during decoding - see der_decode_sequence_ex.c.

Possible theory: it seems that during decode operation I use optional to indicate that it is ok if the item is missing and used to indicate whether it was really decoded.

The `der_length_sequence` (which calls `der_length_sequence_ex`) is called during decoding - see `der_decode_sequence_ex.c`. Possible theory: it seems that during decode operation I use `optional` to indicate that it is ok if the item is missing and `used` to indicate whether it was really decoded.

sjaeckel (Migrated from github.com) reviewed

2017-04-07 04:11:11 -05:00

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

Yeah sure, I missed that. Sorry.

karel-m commented

2017-06-21 16:01:02 -05:00

(Migrated from github.com)

Now rebased on top of ECC enhancements part 1 (curves y^2 = x^3 + ax + b) #236

Now rebased on top of `ECC enhancements part 1 (curves y^2 = x^3 + ax + b)` #236

karel-m (Migrated from github.com) reviewed

2017-06-22 03:48:52 -05:00

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

Can I extract this single change as a separate "small fix" PR?

sjaeckel (Migrated from github.com) reviewed

2017-06-22 04:15:30 -05:00

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

sure!

karel-m commented

2017-09-22 16:03:06 -05:00

(Migrated from github.com)

UPDATE: this PR adds the following functions to the public API

int  ecc_export_openssl(unsigned char *out, unsigned long *outlen, int type, ecc_key *key);
int  ecc_import_openssl(const unsigned char *in, unsigned long inlen, ecc_key *key);
int  ecc_import_pkcs8(const unsigned char *in,  unsigned long inlen, const void *pwd, unsigned long pwdlen, ecc_key *key);

UPDATE: this PR adds the following functions to the public API ```C int ecc_export_openssl(unsigned char *out, unsigned long *outlen, int type, ecc_key *key); int ecc_import_openssl(const unsigned char *in, unsigned long inlen, ecc_key *key); int ecc_import_pkcs8(const unsigned char *in, unsigned long inlen, const void *pwd, unsigned long pwdlen, ecc_key *key); ```

karel-m commented

2017-12-19 07:05:19 -06:00

(Migrated from github.com)

@sjaeckel I want to load the following ASN.1 structure using your new API:

  private key format: http://tools.ietf.org/html/rfc5915
  
  ECPrivateKey ::= SEQUENCE {                                    # SEQUENCE
   version        INTEGER { ecPrivkeyVer1(1) } (ecPrivkeyVer1),  # INTEGER       :01
   privateKey     OCTET STRING,                                  # OCTET STRING
   [0] ECParameters                                              # see above
   [1] publicKey                                                 # BIT STRING
  }

This seems to fail:

   LTC_SET_ASN1(seq_priv, 0, LTC_ASN1_SHORT_INTEGER,     &pkver,   1UL);
   LTC_SET_ASN1(seq_priv, 1, LTC_ASN1_OCTET_STRING,      bin_k,    (unsigned long)ECC_MAXSIZE);
   LTC_SET_ASN1(seq_priv, 2, LTC_ASN1_OBJECT_IDENTIFIER, curveoid, 16UL);
   LTC_SET_ASN1(seq_priv, 3, LTC_ASN1_RAW_BIT_STRING,    bin_xy,   (unsigned long)8*(2*ECC_MAXSIZE+2));
   
   LTC_SET_ASN1_IDENTIFIER(seq_priv, 2, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0x0000); /* context specific 0 */
   LTC_SET_ASN1_IDENTIFIER(seq_priv, 3, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0x0001); /* context specific 1 */

   err = der_decode_sequence(in, inlen, seq_priv, 4);

What is the right way?

@sjaeckel I want to load the following ASN.1 structure using your new API: ``` private key format: http://tools.ietf.org/html/rfc5915 ECPrivateKey ::= SEQUENCE { # SEQUENCE version INTEGER { ecPrivkeyVer1(1) } (ecPrivkeyVer1), # INTEGER :01 privateKey OCTET STRING, # OCTET STRING [0] ECParameters # see above [1] publicKey # BIT STRING } ``` This seems to fail: ```C LTC_SET_ASN1(seq_priv, 0, LTC_ASN1_SHORT_INTEGER, &pkver, 1UL); LTC_SET_ASN1(seq_priv, 1, LTC_ASN1_OCTET_STRING, bin_k, (unsigned long)ECC_MAXSIZE); LTC_SET_ASN1(seq_priv, 2, LTC_ASN1_OBJECT_IDENTIFIER, curveoid, 16UL); LTC_SET_ASN1(seq_priv, 3, LTC_ASN1_RAW_BIT_STRING, bin_xy, (unsigned long)8*(2*ECC_MAXSIZE+2)); LTC_SET_ASN1_IDENTIFIER(seq_priv, 2, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0x0000); /* context specific 0 */ LTC_SET_ASN1_IDENTIFIER(seq_priv, 3, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0x0001); /* context specific 1 */ err = der_decode_sequence(in, inlen, seq_priv, 4); ``` What is the right way?

karel-m commented

2017-12-19 08:19:12 -06:00

(Migrated from github.com)

@sjaeckel here is the ASN1 which I am trying to load:

   static const unsigned char short_pri[] = { /* private + long public, curve by OID */
      0x30, 0x74, 0x02, 0x01, 0x01, 0x04, 0x20, 0x0c, 0xf1, 0xad, 0x2f, 0x03, 0xf7, 0x91, 0x1b, 0xba,
      0x03, 0xcf, 0x23, 0x37, 0xc8, 0xf2, 0xf7, 0x36, 0xce, 0x65, 0xf1, 0x84, 0x2d, 0x7d, 0x9f, 0x5f,
      0x9e, 0x21, 0xd9, 0x5e, 0x49, 0xbd, 0x23, 0xa0, 0x07, 0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x0a,
      0xa1, 0x44, 0x03, 0x42, 0x00, 0x04, 0x2a, 0xf9, 0x0b, 0xda, 0xbe, 0x71, 0x66, 0x9e, 0xd1, 0xcf,
      0x12, 0xd0, 0x24, 0xaf, 0xba, 0xb6, 0x7f, 0xfb, 0x96, 0x27, 0x3e, 0x2f, 0xbd, 0x1e, 0xd5, 0xf9,
      0x8d, 0x6c, 0x73, 0x9d, 0xc5, 0x16, 0x91, 0xbd, 0xb2, 0xb9, 0x1b, 0x40, 0x10, 0x5a, 0xb7, 0x6c,
      0x6e, 0x32, 0x5b, 0xf7, 0x63, 0x62, 0x94, 0x24, 0x24, 0xdb, 0xec, 0x3f, 0x8b, 0xe5, 0x6e, 0x4b,
      0x64, 0x37, 0x31, 0x24, 0x79, 0x4d
   };

$ openssl asn1parse -inform der -in short_pri.der
    0:d=0  hl=2 l= 116 cons: SEQUENCE
    2:d=1  hl=2 l=   1 prim: INTEGER           :01
    5:d=1  hl=2 l=  32 prim: OCTET STRING      [HEX DUMP]:0CF1AD2F03F7911BBA03CF2337C8F2F736CE65F1842D7D9F5F9E21D95E49BD23
   39:d=1  hl=2 l=   7 cons: cont [ 0 ]
   41:d=2  hl=2 l=   5 prim: OBJECT            :secp256k1
   48:d=1  hl=2 l=  68 cons: cont [ 1 ]
   50:d=2  hl=2 l=  66 prim: BIT STRING

@sjaeckel here is the ASN1 which I am trying to load: ```C static const unsigned char short_pri[] = { /* private + long public, curve by OID */ 0x30, 0x74, 0x02, 0x01, 0x01, 0x04, 0x20, 0x0c, 0xf1, 0xad, 0x2f, 0x03, 0xf7, 0x91, 0x1b, 0xba, 0x03, 0xcf, 0x23, 0x37, 0xc8, 0xf2, 0xf7, 0x36, 0xce, 0x65, 0xf1, 0x84, 0x2d, 0x7d, 0x9f, 0x5f, 0x9e, 0x21, 0xd9, 0x5e, 0x49, 0xbd, 0x23, 0xa0, 0x07, 0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x0a, 0xa1, 0x44, 0x03, 0x42, 0x00, 0x04, 0x2a, 0xf9, 0x0b, 0xda, 0xbe, 0x71, 0x66, 0x9e, 0xd1, 0xcf, 0x12, 0xd0, 0x24, 0xaf, 0xba, 0xb6, 0x7f, 0xfb, 0x96, 0x27, 0x3e, 0x2f, 0xbd, 0x1e, 0xd5, 0xf9, 0x8d, 0x6c, 0x73, 0x9d, 0xc5, 0x16, 0x91, 0xbd, 0xb2, 0xb9, 0x1b, 0x40, 0x10, 0x5a, 0xb7, 0x6c, 0x6e, 0x32, 0x5b, 0xf7, 0x63, 0x62, 0x94, 0x24, 0x24, 0xdb, 0xec, 0x3f, 0x8b, 0xe5, 0x6e, 0x4b, 0x64, 0x37, 0x31, 0x24, 0x79, 0x4d }; ``` ``` $ openssl asn1parse -inform der -in short_pri.der 0:d=0 hl=2 l= 116 cons: SEQUENCE 2:d=1 hl=2 l= 1 prim: INTEGER :01 5:d=1 hl=2 l= 32 prim: OCTET STRING [HEX DUMP]:0CF1AD2F03F7911BBA03CF2337C8F2F736CE65F1842D7D9F5F9E21D95E49BD23 39:d=1 hl=2 l= 7 cons: cont [ 0 ] 41:d=2 hl=2 l= 5 prim: OBJECT :secp256k1 48:d=1 hl=2 l= 68 cons: cont [ 1 ] 50:d=2 hl=2 l= 66 prim: BIT STRING ```

sjaeckel commented

2017-12-24 11:48:00 -06:00

(Migrated from github.com)

Can you please read the doc and give feedback if it is sufficient to fix the issue?
My patch to ecc_import_openssl() has this summary:

$ git diff --stat src/pk/ecc/ecc_import_openssl.c
 src/pk/ecc/ecc_import_openssl.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

I can also patch it but I'd like to know if the doc is good enough to understand what to do.

Can you please read the doc and give feedback if it is sufficient to fix the issue? My patch to `ecc_import_openssl()` has this summary: ``` $ git diff --stat src/pk/ecc/ecc_import_openssl.c src/pk/ecc/ecc_import_openssl.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) ``` I can also patch it but I'd like to know if the doc is good enough to understand what to do.

karel-m commented

2018-06-03 13:11:39 -05:00

(Migrated from github.com)

@sjaeckel we should rebase this one onto a develop

I have created a branch pr/ecc-asn1-part-BACKUP because AFAIK you have some uncommitted stuff related to this PR.

@sjaeckel we should rebase this one onto a develop I have created a branch `pr/ecc-asn1-part-BACKUP` because AFAIK you have some uncommitted stuff related to this PR.

sjaeckel commented

2018-06-03 13:16:48 -05:00

(Migrated from github.com)

IIUC that's only 1 commit which is left

and I've already rebased locally, so go ahead

IIUC that's only 1 commit which is left and I've already rebased locally, so go ahead

karel-m commented

2018-06-03 13:24:00 -05:00

(Migrated from github.com)

Don't you have some pkcs8 changes/improvements locally?

I have not done any coding in this PR/branche since you have asked me whether you can work on pkcs8. Therefore I think it would be better if you push --force your local changes.

Don't you have some pkcs8 changes/improvements locally? I have not done any coding in this PR/branche since you have asked me whether you can work on pkcs8. Therefore I think it would be better if you `push --force` your local changes.

sjaeckel commented

2018-06-03 13:53:40 -05:00

(Migrated from github.com)

Don't you have some pkcs8 changes/improvements locally?

yeah, but nothing ready for publishing yet as I didn't really have the time ...

> Don't you have some pkcs8 changes/improvements locally? yeah, but nothing ready for publishing yet as I didn't really have the time ...

karel-m commented

2018-06-03 15:13:48 -05:00

(Migrated from github.com)

OK, now rebased.

Is it worth splitting this PR into:

ecc_export_openssl + ecc_import_openssl + ecc_import_x509 (which is IMO ready for being merged into develop, except maybe a discussion whether the name _openssl is proper here or we can find something better)
ecc_import_pkcs8 (here are definitely some opportunities for improvement + I think the rsa_import_pkcs8 deserves a similar facelift as well)

?

OK, now rebased. Is it worth splitting this PR into: * `ecc_export_openssl` + `ecc_import_openssl` + `ecc_import_x509` (which is IMO ready for being merged into develop, except maybe a discussion whether the name `_openssl ` is proper here or we can find something better) * `ecc_import_pkcs8` (here are definitely some opportunities for improvement + I think the `rsa_import_pkcs8` deserves a similar facelift as well) ?

karel-m commented

2018-06-04 13:17:07 -05:00

(Migrated from github.com)

@sjaeckel could you please review: ecc_export_openssl + ecc_import_openssl + ecc_import_x509?

karel-m commented

2018-06-07 15:02:53 -05:00

(Migrated from github.com)

@sjaeckel I have moved pkcs8 stuff to #403 - could you please review/approve what's left in this PR? (ecc_export_openssl + ecc_import_openssl + ecc_import_x509)

sjaeckel (Migrated from github.com) reviewed

2018-06-10 06:52:11 -05:00

src/pk/ecc/ecc_import_openssl.c

					
				@ -0,0 +1,130 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

What do you think about factoring-out those tries in their own functions?

_ecc_import_x509_with_oid()
_ecc_import_x509_with_curve()
ecc_import_subject_public_key_info() /* private API which calls the two above */
ecc_import_private_with_oid() /* private API */
ecc_import_private_with_curve() /* private API */

This would also allow to call ecc_import_subject_public_key_info() from ecc_import_x509() instead of this multi-purpose import function.

What do you think about factoring-out those tries in their own functions? ``` _ecc_import_x509_with_oid() _ecc_import_x509_with_curve() ecc_import_subject_public_key_info() /* private API which calls the two above */ ecc_import_private_with_oid() /* private API */ ecc_import_private_with_curve() /* private API */ ``` This would also allow to call `ecc_import_subject_public_key_info()` from `ecc_import_x509()` instead of this multi-purpose import function.

karel-m (Migrated from github.com) reviewed

2018-06-10 13:37:22 -05:00

src/pk/ecc/ecc_import_openssl.c

					
				@ -0,0 +1,130 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Frankly, it looks like too much work for more of less no gain. As it will not be part of public API we can change it any-time later.

My doubts here are more about whether ecc_export_openssl or ecc_export_<something_else> (and the same for ecc_import_openssl) would be better. Or generally whether the new API is fine as changing/renaming it later will be a pain.

As you can see from the code the available options for saving/loading an ecc key are pretty wide. For ecc_import_openssl I have chosen the approach "load whatever DER the openssl is able to save" therefore the name and therefore a single, maybe slightly overloaded, import function.

Frankly, it looks like too much work for more of less no gain. As it will not be part of public API we can change it any-time later. My doubts here are more about whether `ecc_export_openssl` or `ecc_export_<something_else>` (and the same for `ecc_import_openssl`) would be better. Or generally whether the new API is fine as changing/renaming it later will be a pain. As you can see from the code the available options for saving/loading an ecc key are pretty wide. For `ecc_import_openssl` I have chosen the approach "load whatever DER the openssl is able to save" therefore the name and therefore a single, maybe slightly overloaded, import function.

sjaeckel (Migrated from github.com) reviewed

2018-06-10 16:49:33 -05:00

src/pk/ecc/ecc_import_openssl.c

					
				@ -0,0 +1,130 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Frankly, it looks like too much work for more of less no gain

I think 30 minutes for a bit more readability is well invested.

Why I really did it was that I didn't like the fact that it would've been possible to import a private-key from a SubjectPublicKeyInfo.

> Frankly, it looks like too much work for more of less no gain I think 30 minutes for a bit more readability is well invested. Why I really did it was that I didn't like the fact that it would've been possible to import a private-key from a `SubjectPublicKeyInfo`.

karel-m (Migrated from github.com) reviewed

2018-06-11 02:55:02 -05:00

src/pk/ecc/ecc_import_openssl.c

					
				@ -0,0 +1,130 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

👍 Looks good. Thank you.

sjaeckel (Migrated from github.com) approved these changes

2018-06-11 12:13:00 -05:00

Sign in to join this conversation.

No reviewers

sjaeckel

No labels

No milestone

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!187

				`@ -0,0 +1,130 @@`
				`/* LibTomCrypt, modular cryptographic library -- Tom St Denis`

Rows
Columns