ECC-step1: ASN.1 changes required for future ECC enhancements #309

2017-10-11T14:16:34-05:00

karel-m commented

2017-10-11 14:16:34 -05:00

(Migrated from github.com)

[edited by @sjaeckel]

documentation added/updated
tests added/updated
add tests for choice with custom-type
add tests for custom-types with other types
new/changed file headers fixed/updated
~~re-work OID handling~~ will be done later(c.f. https://github.com/libtom/libtomcrypt/pull/309#issuecomment-350570048)

This breaks backwards compatibility as it removes LTC_ASN1_CONSTRUCTED and LTC_ASN1_CONTEXT_SPECIFIC which were somehow nonsense anyways...

Please review as there's some new code, biggest parts of the der_*_custom_type.c files are c&p from sequence.

If you don't like some of the naming feel free to propose something better!

All comments very welcome!

[edited by @sjaeckel] - [x] documentation added/updated - [x] tests added/updated - [x] add tests for choice with custom-type - [x] add tests for custom-types with other types - [x] new/changed file headers fixed/updated - [x] ~~re-work OID handling~~ _will be done later_(c.f. https://github.com/libtom/libtomcrypt/pull/309#issuecomment-350570048) This breaks backwards compatibility as it removes `LTC_ASN1_CONSTRUCTED` and `LTC_ASN1_CONTEXT_SPECIFIC` which were somehow nonsense anyways... Please review as there's some new code, biggest parts of the `der_*_custom_type.c` files are c&p from `sequence`. If you don't like some of the naming feel free to propose something better! All comments very welcome!

sjaeckel (Migrated from github.com) reviewed

2017-10-12 04:02:24 -05:00

src/headers/tomcrypt_pk.h

					
				@ -522,0 +526,4 @@

				typedef enum {

				   LTC_ASN1_PC_PRIMITIVE = 0x0,

				   LTC_ASN1_PC_CONSTRUCTED = 0x1,

I'd say unsigned char is inappropriate here as

0 is a valid context-specific tag
tags can be longer than 1 byte (yes this is currently nowhere taken into account so I think it's the time now to do it better)

I think we have two possibilities now:

introduce a flags field that would then for now include Class and P/C of the ASN.1 identifier and optional of our implementation. Then have an unsigned long tag; which is taken into account when Class indicates a context-specific tag.
we can use one of the signed types where -1 indicates invalid/unused

I'd say `unsigned char` is inappropriate here as 1. `0` is a valid context-specific tag 2. tags can be longer than 1 byte (yes this is currently nowhere taken into account so I think it's the time now to do it better) I think we have two possibilities now: 1. introduce a flags field that would then for now include _Class_ and _P/C_ of the ASN.1 identifier and _optional_ of our implementation. Then have an `unsigned long tag;` which is taken into account when _Class_ indicates a context-specific tag. 2. we can use one of the signed types where `-1` indicates `invalid/unused`

if we go for possibility 2 regarding the tag: How about starting to use <stdbool.h>? as this is clearly a boolean :-)

if we go for possibility 2 regarding the `tag`: How about starting to use `<stdbool.h>`? as this is clearly a boolean :-)

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

this is a "private" function, just change the default signature and adapt all its invocations

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

most likely a dumb comment, but ... we're taking the optional flag here into account and this function is called as well from der_encode_sequence_ex() where it is used to determine if we would BOF but der_encode_sequence_ex() itself ignores the optional flag...

most likely a dumb comment, but ... we're taking the `optional` flag here into account and this function is called as well from `der_encode_sequence_ex()` where it is used to determine if we would BOF but `der_encode_sequence_ex()` itself ignores the `optional` flag...

karel-m commented

2017-11-29 06:41:03 -06:00

(Migrated from github.com)

I would like to have also the following commit as a part of this PR https://github.com/libtom/libtomcrypt/pull/187/commits/6bb9661bfa1c11f9671328c508983bfbe9122cf3

I am not sure if it is worth to rename couple of things here to be more correct e.g.

enum public_key_algorithms > enum built_in_oids
constants PKA_DSA > LTC_OID_DSA
maybe pk_get_oid >der_get_oid
but I am not sure

I would like to have also the following commit as a part of this PR https://github.com/libtom/libtomcrypt/pull/187/commits/6bb9661bfa1c11f9671328c508983bfbe9122cf3 I am not sure if it is worth to rename couple of things here to be more correct e.g. * ` enum public_key_algorithms` > ` enum built_in_oids` * constants `PKA_DSA` > `LTC_OID_DSA` * maybe `pk_get_oid` >`der_get_oid` * but I am not sure

sjaeckel commented

2017-12-03 16:59:47 -06:00

(Migrated from github.com)

I would like to have also the following commit as a part of this PR 6bb9661

I first thought why not, now I think this change belongs to #187 as this would add code here which isn't used anywhere yet.

I am not sure if it is worth to rename couple of things here to be more correct e.g.

enum public_key_algorithms > enum built_in_oids

I think we should keep it like that as long there's not really a need and it still kinda completely represents the functionality :)

constants PKA_DSA > LTC_OID_DSA

Do you see the need to make this public?

maybe pk_get_oid >der_get_oid

OID doesn't belong to DER, it's a general ASN.1 feature and it's spec'ed in X.660. As nobody knows X.660 (compared to X.509) I would propose to move src/misc/pk_get_oid.c to src/pk/asn1/oid/oid_XXX.c (oid_by_pka() for now?... and still keep it private...)

but I am not sure

me neither

> I would like to have also the following commit as a part of this PR 6bb9661 I first thought why not, now I think this change belongs to #187 as this would add code here which isn't used anywhere yet. > I am not sure if it is worth to rename couple of things here to be more correct e.g. > > enum public_key_algorithms > enum built_in_oids I think we should keep it like that as long there's not really a need and it still kinda completely represents the functionality :) > constants PKA_DSA > LTC_OID_DSA Do you see the need to make this public? > maybe pk_get_oid >der_get_oid OID doesn't belong to DER, it's a general ASN.1 feature and it's spec'ed in X.660. As nobody knows X.660 (compared to X.509) I would propose to move `src/misc/pk_get_oid.c` to `src/pk/asn1/oid/oid_XXX.c` (`oid_by_pka()` for now?... and still keep it private...) > but I am not sure me neither

karel-m commented

2017-12-10 12:53:16 -06:00

(Migrated from github.com)

OK, let's solve OID stuff later. To move forward we should polish/finetune and merge this PR to develop.

sjaeckel commented

2017-12-11 11:02:08 -06:00

(Migrated from github.com)

OK, let's solve OID stuff later. To move forward we should polish/finetune and merge this PR to develop.

yep, so let's get #335 ready and merged so we can finish this one

> OK, let's solve OID stuff later. To move forward we should polish/finetune and merge this PR to develop. yep, so let's get #335 ready and merged so we can finish this one

karel-m commented

2017-12-18 10:04:48 -06:00

(Migrated from github.com)

@sjaeckel could you please rebase pr/ecc-asn1-part-minimal on the latest develop?

@sjaeckel could you please rebase `pr/ecc-asn1-part-minimal` on the latest `develop`?

sjaeckel commented

2017-12-18 14:35:30 -06:00

(Migrated from github.com)

@sjaeckel could you please rebase pr/ecc-asn1-part-minimal on the latest develop?

done, doc&some tests still missing but hopefully I'll finish that in the next days

> @sjaeckel could you please rebase pr/ecc-asn1-part-minimal on the latest develop? done, doc&some tests still missing but hopefully I'll finish that in the next days

karel-m commented

2017-12-18 15:01:28 -06:00

(Migrated from github.com)

Thank you, I have also rebased other ECC branches so that now we have the following chain:

develop > pr/ecc-asn1-part-minimal (AKA ECC-step1 #309) > pr/ecc-non-asn1-part (AKA ECC-step2 #236) > pr/ecc-asn1-part (AKA ECC-step-3 #187)

I still need to adjust the new import/export functions in "ECC-step-3 #187" to your new API (I mean ecc_import_openssl, ecc_export_openssl and ecc_import_pkcs8).

Thank you, I have also rebased other ECC branches so that now we have the following chain: `develop` > `pr/ecc-asn1-part-minimal` (AKA ECC-step1 #309) > `pr/ecc-non-asn1-part` (AKA ECC-step2 #236) > `pr/ecc-asn1-part` (AKA ECC-step-3 #187) I still need to adjust the new import/export functions in "ECC-step-3 #187" to your new API (I mean ecc_import_openssl, ecc_export_openssl and ecc_import_pkcs8).

sjaeckel commented

2017-12-25 09:16:09 -06:00

(Migrated from github.com)

That should be it, please feel free to review.

karel-m commented

2017-12-27 14:40:10 -06:00

(Migrated from github.com)

@sjaeckel the following simple ASN1 sequence somehow confuses der_decode_sequence (crash).

void *x, *y;
ltc_asn1_list seq[2];
unsigned char der[] = {
  0x30,0x41,0x02,0x84,0x7f,0xff,0xff,0xff,0x1e,0x41,0xb4,0x79,0xad,0x57,0x69,
  0x05,0xb9,0x60,0xfe,0x14,0xea,0xdb,0x91,0xb0,0xcc,0xf3,0x48,0x43,0xda,0xb9,
  0x16,0x17,0x3b,0xb8,0xc9,0xcd,0x02,0x1d,0x00,0xad,0xe6,0x59,0x88,0xd2,0x37,
  0xd3,0x0f,0x9e,0xf4,0x1d,0xd4,0x24,0xa4,0xe1,0xc8,0xf1,0x69,0x67,0xcf,0x33,
  0x65,0x81,0x3f,0xe8,0x78,0x62,0x36
};
mp_init_multi(&x, &y, NULL);
LTC_SET_ASN1(seq, 0, LTC_ASN1_INTEGER, x, 1UL);
LTC_SET_ASN1(seq, 1, LTC_ASN1_INTEGER, y, 1UL);
der_decode_sequence(der, sizeof(der), seq, 2);

@sjaeckel the following simple ASN1 sequence somehow confuses `der_decode_sequence` (crash). ```C void *x, *y; ltc_asn1_list seq[2]; unsigned char der[] = { 0x30,0x41,0x02,0x84,0x7f,0xff,0xff,0xff,0x1e,0x41,0xb4,0x79,0xad,0x57,0x69, 0x05,0xb9,0x60,0xfe,0x14,0xea,0xdb,0x91,0xb0,0xcc,0xf3,0x48,0x43,0xda,0xb9, 0x16,0x17,0x3b,0xb8,0xc9,0xcd,0x02,0x1d,0x00,0xad,0xe6,0x59,0x88,0xd2,0x37, 0xd3,0x0f,0x9e,0xf4,0x1d,0xd4,0x24,0xa4,0xe1,0xc8,0xf1,0x69,0x67,0xcf,0x33, 0x65,0x81,0x3f,0xe8,0x78,0x62,0x36 }; mp_init_multi(&x, &y, NULL); LTC_SET_ASN1(seq, 0, LTC_ASN1_INTEGER, x, 1UL); LTC_SET_ASN1(seq, 1, LTC_ASN1_INTEGER, y, 1UL); der_decode_sequence(der, sizeof(der), seq, 2); ```

karel-m commented

2017-12-27 14:51:19 -06:00

(Migrated from github.com)

What about to simplify this:

LTC_SET_ASN1(&ecparam, 0, LTC_ASN1_OBJECT_IDENTIFIER, dp.oid, dp.oidlen);
LTC_SET_ASN1(&pub_xy,  0, LTC_ASN1_RAW_BIT_STRING, bin_xy,    len_xy);
LTC_SET_ASN1(seq_priv, 0, LTC_ASN1_SHORT_INTEGER,  &one,      1);
LTC_SET_ASN1(seq_priv, 1, LTC_ASN1_OCTET_STRING,   bin_k,     len_k);
LTC_SET_ASN1(seq_priv, 2, LTC_ASN1_CUSTOM_TYPE,    &ecparams, 1);
LTC_SET_ASN1(seq_priv, 3, LTC_ASN1_CUSTOM_TYPE,    &pub_xy,   1);
LTC_SET_ASN1_IDENTIFIER(seq_priv, 2, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0);
LTC_SET_ASN1_IDENTIFIER(seq_priv, 3, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 1);

to something like:

LTC_SET_ASN1       (seq_priv, 0, LTC_ASN1_SHORT_INTEGER,  &one,      1);
LTC_SET_ASN1       (seq_priv, 1, LTC_ASN1_OCTET_STRING,   bin_k,     len_k);
LTC_SET_ASN1_CUSTOM(seq_priv, 2, LTC_ASN1_OBJECT_IDENTIFIER, dp.oid, dp.oidlen, &ecparam, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0);
LTC_SET_ASN1_CUSTOM(seq_priv, 3, LTC_ASN1_RAW_BIT_STRING, bin_xy, len_xy, &pub_xy, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 1);

I would also prefer shorter names for macros/constants (e.g. LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED)

What about to simplify this: ``` LTC_SET_ASN1(&ecparam, 0, LTC_ASN1_OBJECT_IDENTIFIER, dp.oid, dp.oidlen); LTC_SET_ASN1(&pub_xy, 0, LTC_ASN1_RAW_BIT_STRING, bin_xy, len_xy); LTC_SET_ASN1(seq_priv, 0, LTC_ASN1_SHORT_INTEGER, &one, 1); LTC_SET_ASN1(seq_priv, 1, LTC_ASN1_OCTET_STRING, bin_k, len_k); LTC_SET_ASN1(seq_priv, 2, LTC_ASN1_CUSTOM_TYPE, &ecparams, 1); LTC_SET_ASN1(seq_priv, 3, LTC_ASN1_CUSTOM_TYPE, &pub_xy, 1); LTC_SET_ASN1_IDENTIFIER(seq_priv, 2, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0); LTC_SET_ASN1_IDENTIFIER(seq_priv, 3, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 1); ``` to something like: ``` LTC_SET_ASN1 (seq_priv, 0, LTC_ASN1_SHORT_INTEGER, &one, 1); LTC_SET_ASN1 (seq_priv, 1, LTC_ASN1_OCTET_STRING, bin_k, len_k); LTC_SET_ASN1_CUSTOM(seq_priv, 2, LTC_ASN1_OBJECT_IDENTIFIER, dp.oid, dp.oidlen, &ecparam, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0); LTC_SET_ASN1_CUSTOM(seq_priv, 3, LTC_ASN1_RAW_BIT_STRING, bin_xy, len_xy, &pub_xy, LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 1); ``` I would also prefer shorter names for macros/constants (e.g. LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED)

sjaeckel commented

2017-12-28 05:56:30 -06:00

(Migrated from github.com)

@sjaeckel the following simple ASN1 sequence somehow confuses der_decode_sequence (crash).

I'll have a look into the crash, that shouldn't happen, but even dumpasn1 hangs in an endless loop when trying to decode this and openssl asn1parse errors-out...

I would also prefer shorter names for macros/constants (e.g. LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED)

proposals? TBH I prefer speaking macros/enum names instead of cryptic abbreviations, but probably someone else can come up with something better... and regarding the length... well auto-completion does the typing for me in these cases...

regarding LTC_SET_ASN1_CUSTOM - I'll add that

> @sjaeckel the following simple ASN1 sequence somehow confuses der_decode_sequence (crash). I'll have a look into the crash, that shouldn't happen, but even `dumpasn1` hangs in an endless loop when trying to decode this and `openssl asn1parse` errors-out... > I would also prefer shorter names for macros/constants (e.g. LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED) proposals? TBH I prefer speaking macros/enum names instead of cryptic abbreviations, but probably someone else can come up with something better... and regarding the length... well auto-completion does the typing for me in these cases... regarding `LTC_SET_ASN1_CUSTOM` - I'll add that

sjaeckel commented

2017-12-28 06:12:09 -06:00

(Migrated from github.com)

regarding LTC_SET_ASN1_CUSTOM - I'll add that

I'll see what's possible, but I guess it will still be something like

...
LTC_SET_ASN1       (custom, 0, LTC_ASN1_OBJECT_IDENTIFIER, dp.oid, dp.oidlen);
LTC_SET_ASN1_CUSTOM(seq_priv, 2,LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0, &custom[0]);
...

> regarding LTC_SET_ASN1_CUSTOM - I'll add that I'll see what's possible, but I guess it will still be something like ``` ... LTC_SET_ASN1 (custom, 0, LTC_ASN1_OBJECT_IDENTIFIER, dp.oid, dp.oidlen); LTC_SET_ASN1_CUSTOM(seq_priv, 2,LTC_ASN1_CL_CONTEXT_SPECIFIC, LTC_ASN1_PC_CONSTRUCTED, 0, &custom[0]); ... ```

karel-m commented

2017-12-28 11:13:44 -06:00

(Migrated from github.com)

Ad the ugly ASN1 sequence - it is an invalid DSA signature from wycheproof test suite, ASN.1 parser should simply reject it (BTW the current develop rejects it as expected).

Ad LTC_SET_ASN1_CUSTOM - it make sense to add only if it makes things easier/shorter, I'll try to think it over in more details. Anyway I have switched my EC improvements to the API that is currently available in this PR and it works.

Ad the ugly ASN1 sequence - it is an invalid DSA signature from wycheproof test suite, ASN.1 parser should simply reject it (BTW the current develop rejects it as expected). Ad LTC_SET_ASN1_CUSTOM - it make sense to add only if it makes things easier/shorter, I'll try to think it over in more details. Anyway I have switched my EC improvements to the API that is currently available in this PR and it works.

karel-m commented

2017-12-29 16:13:01 -06:00

(Migrated from github.com)

My quick&dirty hack:

diff --git a/src/ltc/pk/asn1/der/general/der_decode_asn1_length.c b/src/ltc/pk/asn1/der/general/der_decode_asn1_length.c
index 2d40ef0..dc7a5be 100644
--- a/src/ltc/pk/asn1/der/general/der_decode_asn1_length.c
+++ b/src/ltc/pk/asn1/der/general/der_decode_asn1_length.c
@@ -57,6 +57,7 @@ int der_decode_asn1_length(const unsigned char *in, unsigned long *inlen, unsign
          offset += real_len;
       }
    }
+   if (offset + *outlen > *inlen) return CRYPT_BUFFER_OVERFLOW;
    *inlen = offset;

    return CRYPT_OK;
diff --git a/src/ltc/pk/asn1/der/object_identifier/der_decode_object_identifier.c b/src/ltc/pk/asn1/der/object_identifier/der_decode_object_identifier.c
index 530aa91..9f48dbc 100644
--- a/src/ltc/pk/asn1/der/object_identifier/der_decode_object_identifier.c
+++ b/src/ltc/pk/asn1/der/object_identifier/der_decode_object_identifier.c
@@ -56,7 +56,7 @@ int der_decode_object_identifier(const unsigned char *in,    unsigned long  inle
    }
    x += y;

-   if (len < 1 || (len + x) > inlen) {
+   if (len < 1 || len > inlen - x) {
       return CRYPT_INVALID_PACKET;
    }

My quick&dirty hack: ```diff diff --git a/src/ltc/pk/asn1/der/general/der_decode_asn1_length.c b/src/ltc/pk/asn1/der/general/der_decode_asn1_length.c index 2d40ef0..dc7a5be 100644 --- a/src/ltc/pk/asn1/der/general/der_decode_asn1_length.c +++ b/src/ltc/pk/asn1/der/general/der_decode_asn1_length.c @@ -57,6 +57,7 @@ int der_decode_asn1_length(const unsigned char *in, unsigned long *inlen, unsign offset += real_len; } } + if (offset + *outlen > *inlen) return CRYPT_BUFFER_OVERFLOW; *inlen = offset; return CRYPT_OK; diff --git a/src/ltc/pk/asn1/der/object_identifier/der_decode_object_identifier.c b/src/ltc/pk/asn1/der/object_identifier/der_decode_object_identifier.c index 530aa91..9f48dbc 100644 --- a/src/ltc/pk/asn1/der/object_identifier/der_decode_object_identifier.c +++ b/src/ltc/pk/asn1/der/object_identifier/der_decode_object_identifier.c @@ -56,7 +56,7 @@ int der_decode_object_identifier(const unsigned char *in, unsigned long inle } x += y; - if (len < 1 || (len + x) > inlen) { + if (len < 1 || len > inlen - x) { return CRYPT_INVALID_PACKET; } ```

karel-m commented

2017-12-29 17:33:42 -06:00

(Migrated from github.com)

Another ugly sequence (again from wycheproof) of 2 integers + 2 extra zero bytes that should be rejected as CRYPT_INPUT_TOO_LONG; however it is accepted:

0x30,0x45,0x02,0x21,0x00,0xb7,0xba,0xba,0xe9,0x33,0x2b,0x54,0xb8,0xa3,0xa0,0x5b,0x70,0x04,0x57,
0x98,0x21,0xa8,0x87,0xa1,0xb2,0x14,0x65,0xf7,0xdb,0x8a,0x3d,0x49,0x1b,0x39,0xfd,0x2c,0x3f,0x02,
0x20,0x74,0x72,0x91,0xdd,0x2f,0x3f,0x44,0xaf,0x7a,0xce,0x68,0xea,0x33,0x43,0x1d,0x6f,0x94,0xe4,
0x18,0xc1,0x06,0xa6,0xe7,0x62,0x85,0xcd,0x59,0xf4,0x32,0x60,0xec,0xce,0x00,0x00

Another ugly sequence (again from wycheproof) of 2 integers + 2 extra zero bytes that should be rejected as `CRYPT_INPUT_TOO_LONG`; however it is accepted: ``` 0x30,0x45,0x02,0x21,0x00,0xb7,0xba,0xba,0xe9,0x33,0x2b,0x54,0xb8,0xa3,0xa0,0x5b,0x70,0x04,0x57, 0x98,0x21,0xa8,0x87,0xa1,0xb2,0x14,0x65,0xf7,0xdb,0x8a,0x3d,0x49,0x1b,0x39,0xfd,0x2c,0x3f,0x02, 0x20,0x74,0x72,0x91,0xdd,0x2f,0x3f,0x44,0xaf,0x7a,0xce,0x68,0xea,0x33,0x43,0x1d,0x6f,0x94,0xe4, 0x18,0xc1,0x06,0xa6,0xe7,0x62,0x85,0xcd,0x59,0xf4,0x32,0x60,0xec,0xce,0x00,0x00 ```

karel-m commented

2018-01-02 04:15:35 -06:00

(Migrated from github.com)

@sjaeckel please have a look at the der_toolong_test tests in der_test.c

The new tests are related to CRYPT_INPUT_TOO_LONG scenario which is IMO still not handled correctly.

@sjaeckel please have a look at the `der_toolong_test` tests in `der_test.c` The new tests are related to `CRYPT_INPUT_TOO_LONG` scenario which is IMO still not handled correctly.

Sign in to join this conversation.

No reviewers