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

karel-m · 2017-10-11T19:16:34Z

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. ECC-step1: ASN.1 changes required for future ECC enhancements #309 (comment))

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 · 2017-10-12T08:46:06Z

src/headers/tomcrypt_pk.h

+   /** Flag used to indicate optional items in ASN.1 sequences */
+   int           optional;
+   /** Flag used to indicate context specific tags on ASN.1 sequence items */
+   unsigned char tag;


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

sjaeckel · 2017-10-12T08:47:59Z

src/headers/tomcrypt_pk.h

   /** The used flag, this is used by the CHOICE ASN.1 type to indicate which choice was made */
   int           used;
+   /** Flag used to indicate optional items in ASN.1 sequences */
+   int           optional;


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

sjaeckel · 2017-10-12T08:57:25Z

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

       }

+       /* some items may be optional during import */
+       if (!list[i].used && list[i].optional) continue;


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

sjaeckel · 2017-10-12T08:58:48Z

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

+   return der_decode_subject_public_key_info_ex(in, inlen, algorithm, public_key, public_key_len,
+                                                parameters_type, parameters, parameters_len, NULL);
+}
+


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

karel-m · 2017-11-29T12:41:03Z

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

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 · 2017-12-03T22:59:47Z

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 · 2017-12-10T18:53:16Z

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

sjaeckel · 2017-12-11T17:02:08Z

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 · 2017-12-18T16:04:48Z

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

sjaeckel · 2017-12-18T20:35:30Z

@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 · 2017-12-18T21:01:28Z

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 · 2017-12-25T15:16:09Z

That should be it, please feel free to review.

karel-m · 2017-12-27T20:40:10Z

@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);

karel-m · 2017-12-27T20:51:19Z

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 · 2017-12-28T11:56:30Z

@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

[1] https://misc.daniel-marschall.de/asn.1/oid_facts.html

As `der_decode_asn1_length()` will now also decode a uint64 with all 0xff the old style would overflow in that check which "wouldn't be good"^TM. The old way the length-checks were written were kind of fine when building on 64bit architectures, but have the same problem on 32bit.

This merges der_decode_sequence_ex() into a new der_decode_custom_type_ex() which can decode SEQUENCEs, SETs and custom types.

sjaeckel reviewed Oct 12, 2017

View reviewed changes

sjaeckel added this to the next milestone Oct 12, 2017

sjaeckel force-pushed the pr/ecc-asn1-part-minimal branch from 2e58def to c6a0cee Compare November 22, 2017 21:56

sjaeckel self-assigned this Nov 22, 2017

sjaeckel force-pushed the pr/ecc-asn1-part-minimal branch 5 times, most recently from 34cb489 to 22fc31b Compare November 27, 2017 13:46

karel-m force-pushed the pr/ecc-asn1-part-minimal branch from 22fc31b to 62291e0 Compare November 29, 2017 12:25

karel-m changed the title ~~ASN.1 changes required for future ECC enhancements~~ ECC-step1: ASN.1 changes required for future ECC enhancements Nov 29, 2017

sjaeckel force-pushed the pr/ecc-asn1-part-minimal branch from 62291e0 to 418e8b5 Compare December 3, 2017 20:06

sjaeckel mentioned this pull request Dec 3, 2017

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

Merged

sjaeckel force-pushed the pr/ecc-asn1-part-minimal branch from 418e8b5 to 3f7e2a9 Compare December 17, 2017 00:03

sjaeckel force-pushed the pr/ecc-asn1-part-minimal branch from 3f7e2a9 to 87899ec Compare December 18, 2017 20:34

sjaeckel force-pushed the pr/ecc-asn1-part-minimal branch 4 times, most recently from 7217f44 to 19cabc1 Compare December 24, 2017 17:43

sjaeckel force-pushed the pr/ecc-asn1-part-minimal branch 6 times, most recently from 251b7b6 to 498291a Compare January 22, 2018 09:04

sjaeckel added enhancement feature labels Feb 18, 2018

karel-m and others added 18 commits February 25, 2018 20:42

ASN.1 changes required fo future ECC enhancements

f155d3c

no need for der_decode_subject_public_key_info_ex

fd7c2b8

fix naming of SubjectPublicKeyInfo de-&encoder

54dd6ce

add CRYPT_PK_ASN1_ERROR

2a78ed3

add ASN.1-length functions

64875d3

add maps for ASN.1 en-/decoding

509ad52

add ASN.1-identifier functions

1b3a757

add required ASN.1 custom-types functions

0d02137

implement the Custom-type de-/encoders

5230977

Update makefiles

799e147

remove LTC_ASN1_CONSTRUCTED and LTC_ASN1_CONTEXT_SPECIFIC

b75c3ce

update/add more DER tests

3431763

use new ASN.1 functionality

756bc7f

add testvectors from [1]

7e2d163

[1] https://misc.daniel-marschall.de/asn.1/oid_facts.html

add flags to der_decode_sequence()

9d03c38

add der_decode_custom_type_ex()

e4efd70

This merges der_decode_sequence_ex() into a new der_decode_custom_type_ex() which can decode SEQUENCEs, SETs and custom types.

update doc

d89326b

sjaeckel force-pushed the pr/ecc-asn1-part-minimal branch from 498291a to d89326b Compare February 25, 2018 19:42

sjaeckel merged commit 64298c1 into develop Feb 25, 2018

karel-m deleted the pr/ecc-asn1-part-minimal branch February 26, 2018 11:56

sjaeckel mentioned this pull request Mar 23, 2018

Re-work OID handling #362

Closed

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

sjaeckel Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

sjaeckel Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

sjaeckel Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

sjaeckel Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

karel-m commented Nov 29, 2017

Uh oh!

sjaeckel commented Dec 3, 2017

Uh oh!

karel-m commented Dec 10, 2017

Uh oh!

sjaeckel commented Dec 11, 2017

Uh oh!

karel-m commented Dec 18, 2017

Uh oh!

sjaeckel commented Dec 18, 2017

Uh oh!

karel-m commented Dec 18, 2017

Uh oh!

sjaeckel commented Dec 25, 2017

Uh oh!

karel-m commented Dec 27, 2017

Uh oh!

karel-m commented Dec 27, 2017

Uh oh!

sjaeckel commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

karel-m commented Oct 11, 2017 •

edited by sjaeckel

Loading

sjaeckel commented Dec 28, 2017 •

edited

Loading