pkcs11-tool: add GOSTR3410 keypair generation #997

konstantinpersidskiy · 2017-03-16T16:00:19Z

Hello

Recently I found out that pkcs11-tool cannot generate keypairs of mechanism GOSTR3410 (although this mechanism is shown as supported in pkcs11-tool -M). This mechanism is a standard mechanism specified by laws in some countries, so I think it is important to make it work properly in OpenSC.

This commit finishes implementing GOSTR3410 keypair generation feature; now pairs can be generated just like they are with other mechanisms.

Example of keypair generation using native OpenSC pkcs15 framework:

$ pkcs11-tool -m GOSTR3410-KEY-PAIR-GEN -k --key-type GOSTR3410:C -d 1234 -l -p '12345678'
Using slot 0 with a present token (0x0)
Key pair generated:
Private Key Object; GOSTR3410 
  PARAMS OID: 06072a850302022303
  label:      Private Key
  ID:         1234
  Usage:      sign
Public Key Object; GOSTR3410 
  PARAMS OID: 06072a850302022303
  VALUE:      044084a5de6a1bfd68ff6b106f6921d4da163244b15ea27dd3a87968d40959a6
              e4170df7674bc7ba098080d7ac2cfe80b29b4ed4369dfedf6e13e42ae32eb7b3
              3233
  label:      Private Key
  ID:         1234
  Usage:      verify

Example of keypair generation using another pkcs module:

$ pkcs11-tool --module "/home/user/Downloads/librtpkcs11ecp.so" -m GOSTR3410-KEY-PAIR-GEN -k --key-type GOSTR3410:B -d 4321 -l -p '12345678'
Using slot 0 with a present token (0x0)
Key pair generated:
Private Key Object; GOSTR3410 
  PARAMS OID: 06072a850302022302
  label:      
  ID:         4321
  Usage:      sign
Public Key Object; GOSTR3410 
  PARAMS OID: 06072a850302022302
  VALUE:      4c0a3a792a9c03cf227be5b4f94d668a11ce34071b29d9da2585fa1d7cbd1c74
              eb874e468245882c31a66a590cec62699b73b225b69e69ed298ff104be7a8c4f
  label:      
  ID:         4321
  Usage:      verify

I believe this commit will be useful. Looking forward for reviews and comments.
Regards, Konstantin Persidskiy

frankmorgner · 2017-03-20T11:04:54Z

src/libopensc/pkcs15-pubkey.c

@@ -1081,6 +1081,8 @@ sc_pkcs15_dup_pubkey(struct sc_context *ctx, struct sc_pkcs15_pubkey *key, struc
 pubkey->algorithm = key->algorithm;

 if (key->alg_id) {
+ if (key->algorithm == SC_ALGORITHM_GOSTR3410)
+ key->alg_id->params = &(key->u.gostr3410.params);


Could you please check whether we could improve initializing of the data structures instead?

frankmorgner · 2017-03-20T11:07:16Z

src/pkcs11/framework-pkcs15.c

 keygen_args.prkey_args.key.algorithm = SC_ALGORITHM_GOSTR3410;
 pub_args.key.algorithm = SC_ALGORITHM_GOSTR3410;
 set_gost_params(&keygen_args.prkey_args.params.gost, &pub_args.params.gost,
 pPubTpl, ulPubCnt, pPrivTpl, ulPrivCnt);
 keybits = SC_PKCS15_GOSTR3410_KEYSIZE;
+ memcpy(&keygen_args.prkey_args.key.u.gostr3410.params.key, kp, 8*sizeof(kp[0]));
+ memcpy(&keygen_args.prkey_args.key.u.gostr3410.params.hash, hp, 8*sizeof(kp[0]));


an int is not guaranteed to have exactly 8 bytes.

frankmorgner · 2017-03-20T11:22:36Z

src/tools/pkcs11-tool.c

+ else if (!strcmp("B", p_param_set)) 
+ memcpy(gost_params, (CK_BYTE[]) {0x06, 0x07, 0x2a, 0x85, 0x03, 0x02, 0x02, 0x23, 0x02}, PARAMS_SIZE);
+ else if (!strcmp("C", p_param_set)) 
+ memcpy(gost_params, (CK_BYTE[]) {0x06, 0x07, 0x2a, 0x85, 0x03, 0x02, 0x02, 0x23, 0x03}, PARAMS_SIZE);


Please don't use magic values here; at least use a good name for them.

frankmorgner · 2017-03-20T11:23:12Z

src/pkcs11/framework-pkcs15.c

@@ -2741,11 +2741,17 @@ pkcs15_gen_keypair(struct sc_pkcs11_slot *slot, CK_MECHANISM_PTR pMechanism,
 goto kpgen_done;

 if (keytype == CKK_GOSTR3410) {
+ unsigned int kp[] = {1, 2, 643, 2, 2, 35, 1, (unsigned int)-1};
+ unsigned int hp[] = {1, 2, 643, 2, 2, 30, 1, (unsigned int)-1};


please use a better name for your OIDs.

Why are they needed on the PKCS#11 level?

frankmorgner · 2017-03-20T11:50:11Z

Please also check the CI errors

konstantinpersidskiy · 2017-04-20T12:06:54Z

Thanks for the review!
I improved the things you pointed to (and some others). Now the code is more readable and changes are placed in right components.

frankmorgner · 2017-04-22T15:21:11Z

Thanks for the update, please have a look at the CI error.

konstantinpersidskiy · 2017-04-27T09:47:48Z

I already checked new CI errors and they seem to be internal:

travis reported build to exit with status 0 but didn't finish build job somehow which led to an error
https://travis-ci.org/OpenSC/OpenSC/jobs/226311036
some of appveyor build jobs failed on downloading files from download.microsoft.com
https://ci.appveyor.com/project/LudovicRousseau/opensc/build/0.16.0.1240/job/c8j48qfu41ugxvgj
https://ci.appveyor.com/project/LudovicRousseau/opensc/build/0.16.0.1240/job/e256s3rixg9w0evr
some others appveyor build jobs couldn't recognize 'nmake' command and failed
https://ci.appveyor.com/project/LudovicRousseau/opensc/build/0.16.0.1240/job/33sh04db7m5aa593
https://ci.appveyor.com/project/LudovicRousseau/opensc/build/0.16.0.1240/job/3x2a13931m7e3qyv
https://ci.appveyor.com/project/LudovicRousseau/opensc/build/0.16.0.1240/job/yikidsla8fah0lgh
https://ci.appveyor.com/project/LudovicRousseau/opensc/build/0.16.0.1240/job/j6cr0x6aumx83it5

frankmorgner · 2017-05-02T21:18:51Z

src/libopensc/pkcs15.c

@@ -856,6 +856,19 @@ sc_pkcs15_card_clear(struct sc_pkcs15_card *p15card)
 }


+int sc_copy_gost_params(struct sc_pkcs15_gost_parameters *dst, struct sc_pkcs15_gost_parameters *src)


Why don't you move this implementation to src/pkcs15init/pkcs15-lib.c, which is the only place where this is used?

Yes, you are right. Moved it.

konstantinpersidskiy · 2017-05-30T07:48:57Z

@frankmorgner , I've made changes to the code you mentioned, can you please check it out?

frankmorgner · 2017-05-30T10:30:57Z

can you be more specific? CI errors were fixed with 49d876b, but I think you didn't look at the length check.

konstantinpersidskiy · 2017-05-30T16:16:51Z

@frankmorgner , I think i don't understand what you mean by "length check". I thought I've fixed all the code you asked in the series of commits (and the last commit also passes CI checks).
If I collapse these commits in one resulting commit, will that be OK? Is this what you mean? Or otherwise could you please point out what exactly is wrong with my code so I can fix that?

frankmorgner · 2017-05-19T13:20:00Z

src/pkcs11/framework-pkcs15.c

- if (second_params)
- second_params->gostr3410 = gostr3410_param_oid[i].param;
+ for (param_index = 0; param_index < nn; ++param_index) {
+ if (!memcmp(gost_params_oid_from_template, gostr3410_param_oid[param_index].encoded_oid, len))


I think you need to check len != (sizeof gostr3410_param_oid[param_index].encoded_oid).

frankmorgner · 2017-05-19T13:22:36Z

src/pkcs11/framework-pkcs15.c

+ return CKR_ATTRIBUTE_VALUE_INVALID;
+
+ for (hash_index = 0; hash_index < nn; ++hash_index) {
+ if (!memcmp(gost_hash_params_oid_from_template, gostr3410_hash_param_oid[hash_index].encoded_oid, len))


I think you need to check len != (sizeof gostr3410_param_oid[param_index].encoded_oid).

frankmorgner · 2017-05-30T20:27:38Z

sorry, I just realized that the comments were pending and I never finished the review...

pkcs11-tool: add GOSTR3410 keypair generation

9c096cc

frankmorgner requested changes Mar 20, 2017

View reviewed changes

Konstantin Persidskiy added 3 commits April 12, 2017 08:26

Refactor gost3410 generation

449d41e

Fix copy gost3410 params and indents

2dc8d39

Rename hash oid, improve gost params copy

da2b28f

frankmorgner requested changes May 2, 2017

View reviewed changes

Make changes c89 compatible

49d876b

konstantinpersidskiy force-pushed the gostr3410_genkeypair branch from 2c2ecf3 to 49d876b Compare May 16, 2017 10:02

frankmorgner requested changes May 30, 2017

View reviewed changes

frankmorgner added this to In Progress in Release 0.17.0 May 31, 2017

Allow gostr3410 params oid be of any size

9eaad42

konstantinpersidskiy force-pushed the gostr3410_genkeypair branch from 1c3fa52 to 9eaad42 Compare June 5, 2017 14:55

frankmorgner approved these changes Jun 9, 2017

View reviewed changes

frankmorgner merged commit 083cec8 into OpenSC:master Jun 9, 2017

frankmorgner moved this from In Progress to Done in Release 0.17.0 Jun 9, 2017

Jakuje mentioned this pull request Aug 8, 2022

master - build on Ubuntu 22.04 and Openssl 3.0 #2586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkcs11-tool: add GOSTR3410 keypair generation #997

pkcs11-tool: add GOSTR3410 keypair generation #997

konstantinpersidskiy commented Mar 16, 2017

frankmorgner Mar 20, 2017

frankmorgner Mar 20, 2017

frankmorgner Mar 20, 2017

frankmorgner Mar 20, 2017

frankmorgner commented Mar 20, 2017

konstantinpersidskiy commented Apr 20, 2017

frankmorgner commented Apr 22, 2017

konstantinpersidskiy commented Apr 27, 2017

frankmorgner May 2, 2017

konstantinpersidskiy May 16, 2017

konstantinpersidskiy commented May 30, 2017

frankmorgner commented May 30, 2017

konstantinpersidskiy commented May 30, 2017

frankmorgner May 19, 2017

frankmorgner May 19, 2017

frankmorgner commented May 30, 2017

		@@ -856,6 +856,19 @@ sc_pkcs15_card_clear(struct sc_pkcs15_card *p15card)
		}


		int sc_copy_gost_params(struct sc_pkcs15_gost_parameters dst, struct sc_pkcs15_gost_parameters src)

pkcs11-tool: add GOSTR3410 keypair generation #997

pkcs11-tool: add GOSTR3410 keypair generation #997

Conversation

konstantinpersidskiy commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankmorgner commented Mar 20, 2017

konstantinpersidskiy commented Apr 20, 2017

frankmorgner commented Apr 22, 2017

konstantinpersidskiy commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konstantinpersidskiy commented May 30, 2017

frankmorgner commented May 30, 2017

konstantinpersidskiy commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankmorgner commented May 30, 2017