ID of cryptographic objects

[Viktor TARASOV-2]

Hi,

I try import a private key on the card that
already contains one private key without corresponding public key.

The result is that the IDs of the newly imported private key and
corresponding public part
are not the same.

Sure, application that imported the second key
could supply some convenient ID for the both parts, private and public.

Nevertheless, IMHO, it would be nice, for the cryptographic objects (and
maybe for the others)
to have the possibility of some unique ID calculated from the object
itself, as it was discussed in thread:
'CKA_ID and pkcs15 ID' 05.09.2005 13:34 .

The idea is to have a choice of method to calculate the ID:
- SHA1 of the modulus (Mozilla style),
- SHA1 of public key (recommended by RFC2459)
- or the actual one byte ID (default).
Then use some additional profile option to indicate the method to be used.


Any objection if I implement it?

Kind wishes,
Viktor Tarasov.

--
Viktor Tarasov <viktor.tarasov@...>

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Aktiv Co. Aleksey Samsonov-2]

Viktor TARASOV:
> Hi,

Hi

> Nevertheless, IMHO, it would be nice, for the cryptographic objects (and
> maybe for the others)
> to have the possibility of some unique ID calculated from the object
> itself, as it was discussed in thread:
> 'CKA_ID and pkcs15 ID' 05.09.2005 13:34 .
>
> The idea is to have a choice of method to calculate the ID:
> - SHA1 of the modulus (Mozilla style),
> - SHA1 of public key (recommended by RFC2459)
> - or the actual one byte ID (default).
> Then use some additional profile option to indicate the method to be used.
>
>
> Any objection if I implement it?

I think, this is a true idea.

Furthermore
http://www.opensc-project.org/opensc/browser/trunk/src/pkcs15init/profile.c#L564:
         idx = id->value[id->len-1]; /* choice file ID */
it may be a surprise to use.

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Viktor TARASOV-2]

Aktiv Co. Aleksey Samsonov wrote:

> Viktor TARASOV:
>> Hi,
>
> Hi
>
>> Nevertheless, IMHO, it would be nice, for the cryptographic objects (and
>> maybe for the others)
>> to have the possibility of some unique ID calculated from the object
>> itself, as it was discussed in thread:
>> 'CKA_ID and pkcs15 ID' 05.09.2005 13:34 .
>>
>> The idea is to have a choice of method to calculate the ID:
>> - SHA1 of the modulus (Mozilla style),
>> - SHA1 of public key (recommended by RFC2459)
>> - or the actual one byte ID (default).
>> Then use some additional profile option to indicate the method to be
>> used.
>>
>>
>> Any objection if I implement it?
>
> I think, this is a true idea.

It's commited ...

> Furthermore
> http://www.opensc-project.org/opensc/browser/trunk/src/pkcs15init/profile.c#L564: 
>
>         idx = id->value[id->len-1]; /* choice file ID */
> it may be a surprise to use.
... probably, a little bit too fast.

Thank you. I will explicitly look at the impact on the file index .
For me (Oberthur) the files indexes were corrects.

By the way, IMHO, file index and ID have to be completely dissociated.

--
Viktor Tarasov <viktor.tarasov@...>

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Aktiv Co. Aleksey Samsonov-2]

Viktor TARASOV:
> Aktiv Co. Aleksey Samsonov wrote:
>> Viktor TARASOV:
>>> Hi,
>> Hi

Hi,

>>> Nevertheless, IMHO, it would be nice, for the cryptographic objects (and
>>> maybe for the others)
>>> to have the possibility of some unique ID calculated from the object
>>> itself, as it was discussed in thread:
>>> 'CKA_ID and pkcs15 ID' 05.09.2005 13:34 .
>>>
>>> The idea is to have a choice of method to calculate the ID:
>>> - SHA1 of the modulus (Mozilla style),
>>> - SHA1 of public key (recommended by RFC2459)
>>> - or the actual one byte ID (default).
>>> Then use some additional profile option to indicate the method to be
>>> used.
>>>
>>>
>>> Any objection if I implement it?
>> I think, this is a true idea.
>
> It's commited ...

Thanks, but some remarks:

Potencial memory leaks (see /* */):

1) sc_pkcs15_pubkey_from_prvkey:

579:        pubkey = (struct sc_pkcs15_pubkey *) calloc(1, sizeof(struct
sc_pkcs15_pubkey));
...
584:        switch (prvkey->algorithm) {
...
595: and 616:        arr[ii].dst->data = malloc(arr[ii].src->len);
                      if (!arr[ii].dst->data)
                              return SC_ERROR_OUT_OF_MEMORY; /*
free(arr[XX].dst->data); free(pubkey) */
...
627:        default:
                     sc_error(ctx, "Unsupported private key algorithm");
                     return SC_ERROR_NOT_SUPPORTED; /* free(pubkey) */
...

2) sc_pkcs15_pubkey_from_cert:

615:        pubkey = (struct sc_pkcs15_pubkey *) calloc(1, sizeof(struct
sc_pkcs15_pubkey));

...
658:        SC_TEST_RET(ctx, SC_ERROR_INVALID_DATA, "BIO new memory
buffer error"); /* free(pubkey) */
...
662:        SC_TEST_RET(ctx, SC_ERROR_INVALID_DATA, "X509 parse error");
/* BIO_free(mem); free(pubkey) */
...
666:        SC_TEST_RET(ctx, SC_ERROR_INVALID_DATA, "Get public key
error"); /* (if (pkey) free(EVP_PKEY_free(pkey);); X509_free(x);
BIO_free(mem); free(pubkey) */

...
669:        pubkey->u.rsa.modulus.data = malloc(pubkey->u.rsa.modulus.len);

             pubkey->u.rsa.exponent.len = BN_num_bytes(pkey->pkey.rsa->e);
             pubkey->u.rsa.exponent.data =
malloc(pubkey->u.rsa.exponent.len);

             if (!pubkey->u.rsa.modulus.data ||
!pubkey->u.rsa.exponent.data)
                     SC_TEST_RET(ctx, SC_ERROR_OUT_OF_MEMORY, "Cannot
allocate key components"); /* free(pubkey->u.rsa.modulus.data);
free(pubkey->u.rsa.exponent.data);  ;EVP_PKEY_free(pkey); X509_free(x);
BIO_free(mem); free(pubkey) */

             if (BN_bn2bin(pkey->pkey.rsa->n,
pubkey->u.rsa.modulus.data) != pubkey->u.rsa.modulus.len)
                     SC_TEST_RET(ctx, SC_ERROR_INVALID_DATA, "BN to BIN
conversion error"); /* !!! */
             if (BN_bn2bin(pkey->pkey.rsa->e,
pubkey->u.rsa.exponent.data) != pubkey->u.rsa.exponent.len)
                     SC_TEST_RET(ctx, SC_ERROR_INVALID_DATA, "BN to BIN
conversion error"); /* !!! */


Also (style, mix tab/space character):
src/pkcs15init/pkcs15-lib.c:1397
src/pkcs15init/pkcs15-lib.c:1477
src/pkcs15init/pkcs15-lib.c:1393
src/libopensc/pkcs15.h:491:         struct sc_pkcs15_pubkey **__out__);
src/libopensc/pkcs15-pubkey.c:655

and:
pkcs15-pubkey.c: In function 'sc_pkcs15_pubkey_from_cert':
pkcs15-pubkey.c:677: warning: comparison between signed and unsigned
pkcs15-pubkey.c:679: warning: comparison between signed and unsigned

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Viktor TARASOV-2]

Aktiv Co. Aleksey Samsonov wrote:
> Viktor TARASOV:
<skipped>
>> It's commited ...
>
> Thanks, but some remarks:
>
> Potencial memory leaks (see /* */):

Thanks for your code revision. I'll be more attentive.

Considering the 'SC_ERROR_OUT_OF_MEMORY' error,
IMHO, it's quiet dangerous to regroup the allocations,
then, if at least one pointer is not valid,
try to liberate the allocated memory without verifying of every
particular pointer.
src/pkcs15init/pkcs15-rtecp.c +529
src/pkcs15init/pkcs15-rtecp.c +542
src/libopensc/card-rutoken.c +1143
...


Kind wishes,

--
Viktor Tarasov <viktor.tarasov@...>

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Aktiv Co. Aleksey Samsonov-2]

Viktor TARASOV:

> Aktiv Co. Aleksey Samsonov wrote:
>> Viktor TARASOV:
> <skipped>
>>> It's commited ...
>> Thanks, but some remarks:
>>
>> Potencial memory leaks (see /* */):
>
> Thanks for your code revision. I'll be more attentive.
>
> Considering the 'SC_ERROR_OUT_OF_MEMORY' error,
> IMHO, it's quiet dangerous to regroup the allocations,
> then, if at least one pointer is not valid,
> try to liberate the allocated memory without verifying of every
> particular pointer.
> src/pkcs15init/pkcs15-rtecp.c +529
> src/pkcs15init/pkcs15-rtecp.c +542
> src/libopensc/card-rutoken.c +1143
> ...

What do you mean by "quiet dangerous to regroup the allocations"?

src/pkcs15init/pkcs15-rtecp.c:
496:     sc_rtecp_genkey_data_t data;
...
523:            data.u.rsa.modulus = calloc(1, data.u.rsa.modulus_len);
524:            data.u.rsa.exponent_len = key_info->modulus_length / 8 / 2;
525:            data.u.rsa.exponent = calloc(1, data.u.rsa.exponent_len);
526:            if (!data.u.rsa.modulus || !data.u.rsa.exponent)
527:            {
/* 1. data.u.rsa.modulus == VALID_PTR;  data.u.rsa.exponent == NULL */
/* 2. data.u.rsa.modulus == NULL;  data.u.rsa.exponent == NULL */
/* 3. data.u.rsa.modulus == NULL;  data.u.rsa.exponent == VALID_PTR */
528:                    free(data.u.rsa.modulus); /* if
data.u.rsa.modulus == NULL then no effect, else free mem */
529:                    free(data.u.rsa.exponent); if
data.u.rsa.exponent == NULL then no effect, else free mem */
                         /* data is local variable */
                         SC_FUNC_RETURN(card->ctx, 0,
SC_ERROR_OUT_OF_MEMORY);
                 }

http://www.opengroup.org/onlinepubs/7990989775/xsh/calloc.html:
RETURN VALUE
Upon successful completion with both nelem and elsize non-zero, calloc()
returns a pointer to the allocated space. If either nelem or elsize is
0, then either a null pointer or a unique pointer value that can be
successfully passed to free() is returned. Otherwise, it returns a null
pointer and sets errno to indicate the error.

http://www.opengroup.org/onlinepubs/7990989775/xsh/free.html:
DESCRIPTION
If ptr is a null pointer, no action occurs.
_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Viktor TARASOV-2]

Aktiv Co. Aleksey Samsonov wrote:

> Viktor TARASOV:
>> Aktiv Co. Aleksey Samsonov wrote:
>>> Viktor TARASOV:
>> <skipped>
>>>> It's commited ...
>>> Thanks, but some remarks:
>>>
>>> Potencial memory leaks (see /* */):
>>
>> Thanks for your code revision. I'll be more attentive.
>>
>> Considering the 'SC_ERROR_OUT_OF_MEMORY' error,
>> IMHO, it's quiet dangerous to regroup the allocations,
>> then, if at least one pointer is not valid,
>> try to liberate the allocated memory without verifying of every
>> particular pointer.
>> src/pkcs15init/pkcs15-rtecp.c +529
>> src/pkcs15init/pkcs15-rtecp.c +542
>> src/libopensc/card-rutoken.c +1143
>> ...
>
> What do you mean by "quiet dangerous to regroup the allocations"?
>
> src/pkcs15init/pkcs15-rtecp.c:
> 496:     sc_rtecp_genkey_data_t data;
> ...
> 523:            data.u.rsa.modulus = calloc(1, data.u.rsa.modulus_len);
> 524:            data.u.rsa.exponent_len = key_info->modulus_length / 8
> / 2;
> 525:            data.u.rsa.exponent = calloc(1, data.u.rsa.exponent_len);
> 526:            if (!data.u.rsa.modulus || !data.u.rsa.exponent)
> 527:            {
> /* 1. data.u.rsa.modulus == VALID_PTR;  data.u.rsa.exponent == NULL */
> /* 2. data.u.rsa.modulus == NULL;  data.u.rsa.exponent == NULL */
> /* 3. data.u.rsa.modulus == NULL;  data.u.rsa.exponent == VALID_PTR */
> 528:                    free(data.u.rsa.modulus); /* if
> data.u.rsa.modulus == NULL then no effect, else free mem */
> 529:                    free(data.u.rsa.exponent); if
> data.u.rsa.exponent == NULL then no effect, else free mem */
>                         /* data is local variable */
>                         SC_FUNC_RETURN(card->ctx, 0,
> SC_ERROR_OUT_OF_MEMORY);
>                 }
>
> http://www.opengroup.org/onlinepubs/7990989775/xsh/calloc.html:
> RETURN VALUE
> Upon successful completion with both nelem and elsize non-zero,
> calloc() returns a pointer to the allocated space. If either nelem or
> elsize is 0, then either a null pointer or a unique pointer value that
> can be successfully passed to free() is returned. Otherwise, it
> returns a null pointer and sets errno to indicate the error.
>
> http://www.opengroup.org/onlinepubs/7990989775/xsh/free.html:
> DESCRIPTION
> If ptr is a null pointer, no action occurs.

Thanks. I've been wrong.

Kind wishes.


--
Viktor Tarasov <viktor.tarasov@...>

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Aleksey Samsonov]

Viktor TARASOV:
> Aktiv Co. Aleksey Samsonov wrote:
>> Viktor TARASOV:
>>> Aktiv Co. Aleksey Samsonov wrote:
>>>> Viktor TARASOV:
>>> <skipped>
>>>>> It's commited ...

Thanks, good work.


--- /trunk/src/libopensc/pkcs15-pubkey.c (revision 3818)
+++ /trunk/src/libopensc/pkcs15-pubkey.c (revision 3820)
@@ -70,5 +70,5 @@

  static const struct sc_asn1_entry c_asn1_gostr3410key_attr[] = {
-       { "value",         SC_ASN1_PATH, SC_ASN1_TAG_SEQUENCE |
SC_ASN1_CONS, 0, NULL, NULL },
+       { "value",      SC_ASN1_PATH, SC_ASN1_TAG_SEQUENCE |
SC_ASN1_CONS, 0, NULL, NULL },
         { "params_r3410",  SC_ASN1_INTEGER, SC_ASN1_TAG_INTEGER, 0,
NULL, NULL },

;)) Those space characters were deliberately solution, but let it be.

Also I'm going to do some changes for GOST key, if you don't mind.

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Viktor TARASOV-2]

Aleksey Samsonov wrote:

<skipped>
> Also I'm going to do some changes for GOST key, if you don't mind.
Heartly wellcome.
GOST is not more in my mind, but still in my heart .

--
Viktor Tarasov <viktor.tarasov@...>

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Viktor TARASOV-2]

Aktiv Co. Aleksey Samsonov wrote:
> Viktor TARASOV:
>
<skipped>
>> Nevertheless, IMHO, it would be nice, for the cryptographic objects (and
>> maybe for the others)
>> to have the possibility of some unique ID calculated from the object
>> itself.
<skipped>
> Furthermore
> http://www.opensc-project.org/opensc/browser/trunk/src/pkcs15init/profile.c#L564: 
>
>         idx = id->value[id->len-1]; /* choice file ID */
> it may be a surprise to use.
>
In fact,
surprises are possibles with the cards that implements a 'New API' of
pkcs15init.
The 'almost random' last byte of intrinsic ID can be the same for two
objects of the same type .

IMHO, the index to instantiate a file from the profile should be selected
with the regards to the existing files, and not with the regards to the
object's ID.

So, for a while, cards with a 'New API' for pkcs15init are recommended
to use the default ID style.

Kind wishes,

--
Viktor Tarasov <viktor.tarasov@...>

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Aleksey Samsonov]

Hello,

Viktor TARASOV wrote:
> Aktiv Co. Aleksey Samsonov wrote:
>> Viktor TARASOV:
> <skipped>
>>> It's commited ...
>> Thanks, but some remarks:
>>
>> Potencial memory leaks (see /* */):
>
> Thanks for your code revision.

Thanks, but some potencial memory leaks. See patch in attachment.


Index: src/pkcs15init/pkcs15-lib.c
===================================================================
--- src/pkcs15init/pkcs15-lib.c (revision 3858)
+++ src/pkcs15init/pkcs15-lib.c (working copy)
@@ -2324,8 +2324,10 @@
  SC_FUNC_RETURN(ctx, 1, 0);
  }
 
+ rv = SC_ERROR_INTERNAL;
+
  /* Skip silently if key is not inintialized. */
- if (pubkey->algorithm == SC_ALGORITHM_RSA && !pubkey->u.rsa.modulus.len)
+ if (pubkey->algorithm == SC_ALGORITHM_RSA && !pubkey->u.rsa.modulus.data)
  goto done;
  else if (pubkey->algorithm == SC_ALGORITHM_DSA && !pubkey->u.dsa.pub.data)
  goto done;
@@ -2353,11 +2355,9 @@
  size_t id_data_len = 0;
 
  rv = sc_pkcs15_encode_pubkey(ctx, pubkey, &id_data, &id_data_len);
- SC_TEST_RET(ctx, rv, "Encoding public key error");
+ if (rv != SC_SUCCESS || !id_data || !id_data_len)
+ goto done;
 
- if (!id_data || !id_data_len)
- SC_TEST_RET(ctx, SC_ERROR_INTERNAL, "Encoding public key error");
-
  SHA1(id_data, id_data_len, id->value);
  id->len = SHA_DIGEST_LENGTH;
 
@@ -2365,14 +2365,17 @@
  }
  else   {
  sc_debug(ctx, "Unsupported ID style: %i", profile->id_style);
- SC_TEST_RET(ctx, SC_ERROR_NOT_SUPPORTED, "Non supported ID style");
+ rv = SC_ERROR_NOT_SUPPORTED;
+ goto done;
  }
 
+ rv = (int)id->len;
+
 done:
  if (allocated)
  sc_pkcs15_free_pubkey(pubkey);
 
- SC_FUNC_RETURN(ctx, 1, id->len);
+ SC_FUNC_RETURN(ctx, 1, rv);
 #endif
 }
 
Index: src/libopensc/pkcs15-pubkey.c
===================================================================
--- src/libopensc/pkcs15-pubkey.c (revision 3858)
+++ src/libopensc/pkcs15-pubkey.c (working copy)
@@ -612,7 +612,7 @@
  break;
  default:
  sc_debug(ctx, "Unsupported private key algorithm");
- return SC_ERROR_NOT_SUPPORTED;
+ rv = SC_ERROR_NOT_SUPPORTED;
  }
 
  if (rv)

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Viktor TARASOV-2]

Aleksey Samsonov wrote:

> Hello,
>
> Viktor TARASOV wrote:
>> Aktiv Co. Aleksey Samsonov wrote:
>>> Viktor TARASOV:
>> <skipped>
>>>> It's commited ...
>>> Thanks, but some remarks:
>>>
>>> Potencial memory leaks (see /* */):
>>
>> Thanks for your code revision.
>
> Thanks, but some potencial memory leaks. See patch in attachment.

You can apply this patch, if you think it should be.


As for me, there is no potential leaks -- I trust entirely the
sc_asn1_encode() .

Agree, there is an excessive 'if' .

Personally, I prefer to know with more precision where an error took place,
but I agree, it's a question of taste.

Kind wishes,

--
Viktor Tarasov <viktor.tarasov@...>

_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel

[Aleksey Samsonov]

Hello,

Viktor TARASOV wrote:
> Aleksey Samsonov wrote:
>> Thanks, but some potencial memory leaks. See patch in attachment.
>
> You can apply this patch, if you think it should be.

ok

> As for me, there is no potential leaks -- I trust entirely the
> sc_asn1_encode() .
>
> Agree, there is an excessive 'if' .
>
> Personally, I prefer to know with more precision where an error took place,
> but I agree, it's a question of taste.

Does anyone think that there is potencial memory leaks and correction is
necessary?
_______________________________________________
opensc-devel mailing list
opensc-devel@...
http://www.opensc-project.org/mailman/listinfo/opensc-devel