New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
X25519 improvements 2 in PKCS11-tool, PKCS15 routines and tools and openpgp #3090
base: master
Are you sure you want to change the base?
Conversation
src/libopensc/pkcs15-pubkey.c
Outdated
volatile int gdb_test = 0; /* so can reset via gdb for testing new way */ | ||
|
||
LOG_FUNC_CALLED(ctx); | ||
sc_copy_asn1_entry(c_asn1_ec_pointQ, asn1_ec_pointQ); | ||
sc_format_asn1_entry(asn1_ec_pointQ + 0, key->ecpointQ.value, &key->ecpointQ.len, 1); | ||
|
||
if (gdb_test == 1) { | ||
key_len = key->ecpointQ.len * 8; /* encode in bit string */ | ||
sc_format_asn1_entry(asn1_ec_pointQ + 0, key->ecpointQ.value, &key_len, 1); | ||
} else { | ||
key_len = key->ecpointQ.len; | ||
sc_format_asn1_entry(asn1_ec_pointQ + 1, key->ecpointQ.value, &key_len, 1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before merging we will certainly need to remove the debugging variables like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the outstanding question of #3000 Did OpenSC return the CKA_EC_POINT in the wrong ASN.1?
It also looks like other packages followed our lead. This would not be easy to fix. The remedy is to use the CKA_PUBKEY_KEY_INFO as SPKI introduced in KCS11 2.40.
Other commits in this PR adds CKA_PUBKEY_KEY_INFO to OpenSC and will accept CKA_EC_POINT in either BIT STRING or OCTET STRING.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that given that we believe the right format should be the bit string, we should output bit string and properly test it with other applications that might expect the other way round and fix them, ideally early in the release cycle. Having this in the code comment should be quite enough. Other option might be to provide some environment variable or configuration option for backward compatibility to satisfy unfixed applications.
Reading through this, I think having a configuration option to revert would be likely the best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid that OpenSC was one of the first smartcard pkcs11 project to include ECDSA and got it wrong and other projects picked up on it.
You at RedHat have contacts with p11-kit and PKCS11 OASIS people. Can you ask them if we got to wrong, and what it would to convert? Or is the solution for every one to use CKA_PUBLIC_KEY_INFO that returns a SPKI format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some discussion on the OASIS PKCS#11 mailing list to get the format clarified, but as far as I remember, they were not able to provide much more clarity (I do not follow the discussions there much now)
There was recently exactly this topic discussed here:
https://lists.oasis-open.org/archives/pkcs11/202310/msg00008.html
There seems to be some changes in the PKCS#11 3.1.
Let me know if there is something else to bring up there.
src/libopensc/pkcs15-pubkey.c
Outdated
if (key->u.ec.params.der.value) | ||
free(key->u.ec.params.der.value); | ||
if (key->u.ec.params.named_curve) | ||
free(key->u.ec.params.named_curve); | ||
if (key->u.ec.ecpointQ.value) | ||
free(key->u.ec.ecpointQ.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needless if(x) free(x). Please remove the needless if
here, similarly as it was done in the eddsa case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following the coding style in the routine which does the same for RSA, GOST and EC.
a7a524be1c (Doug Engert 2023-12-08 20:18:51 -0600 1169) case SC_ALGORITHM_EDDSA:
a7a524be1c (Doug Engert 2023-12-08 20:18:51 -0600 1170) case SC_ALGORITHM_XEDDSA:
I can remove all the needless if
if you want..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please. This was common antipatern in the past and in a lot of places, probably making people feel safer, but there is not a single case when this would be useful for anything
src/tools/pkcs11-tool.c
Outdated
{CKA_WRAP, &_true, sizeof(_true)}, | ||
{CKA_UNWRAP, &_true, sizeof(_true)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ecdsa does can not do wrapping either so these should go away too.
src/tools/pkcs11-tool.c
Outdated
#if OPENSSL_VERSION_NUMBER >= 0x30000000L | ||
key_id = EVP_PKEY_get_id(pkey); | ||
#else | ||
key_id = EVP_PKEY_id(pkey); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have this abstraction in compat-ossl header file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be eliminated. OpenSC 1.1.1 and others define function EVP_PKEY_id
OpenSC 3.0, 3.1, 3.2 and 3.3 and above define function EVP_PKEY_get_id
but have this define: #define EVP_PKEY_id EVP_PKEY_get_id
I will go ahead and use the EVP_PKEY_id
e8f1dd1
to
4bcfbe7
Compare
@Jakuje thanks for the review. I rebased with all the typos and minor changes as "fixup" and resolved those conversations. There are still a few that I left for further comments. |
Thank you for the prompt reply and addressing the most important issues. I think this is a good start. But we need to catch the corner cases that now make the fuzzer fail, fix to todos ... but good to see all the other tests work for now. |
@Jakuje @frankmorgner "The Check Code Style / build (pull_request)" Many of the changes are to pre existing code, especially to "ec_curve" and other tables that have been formatted for easy reading and to code I did not change. I would like to propose, that for these tables, we use Any ideas. (Also have fix for the bug found by fuzzing.) |
As suggested in comments in OpenSC#3090 simplify code when clearing a pubkey. Not shown in diff next line is: sc_mem_clear(key, sizeof(*key)); Date: Thu Mar 28 15:37:32 2024 -0500 On branch X25519-improvements-2 Changes to be committed: modified: pkcs15-pubkey.c
496b5ab
to
934084a
Compare
With large PRs there will always be some reformatting and better to do it per small parts than all code base as it was in the clang format, which had much more files and much more LoC changes.
Yes, the formatter takes some contexts in to be able to format the new code.
I am ok with keeping the tables in the |
The part I did not like was it took multi-line if statements and made one long line. |
If you will split them manually (with correct breaking after operator and with correct indentation), it will not reformat them to long line anymore. |
As suggested in comments in OpenSC#3090 simplify code when clearing a pubkey. Not shown in diff next line is: sc_mem_clear(key, sizeof(*key)); Date: Thu Mar 28 15:37:32 2024 -0500 On branch X25519-improvements-2 Changes to be committed: modified: pkcs15-pubkey.c
56e89c3
to
ae71812
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code wise it looks good (few nits inline). But the openpgp test now fails, which does not look like a false positive. Can you check it?
Given this is a change in behavior, I would like to get this merged rather sooner than later so we have time to test it against other applications and tools before next release.
src/libopensc/pkcs15-pubkey.c
Outdated
} | ||
for (ii = 0; ec_curve_infos[ii].name; ii++) { | ||
size_t len = strlen(ec_curve_infos[ii].name); | ||
if (ecparams->der.len - 2 != len || memcmp(ec_curve_infos[ii].name, ecparams->der.value + 2, len) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is printable string. Should we be strict about the case or use the strncasecmp
to ignore the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But "printable string" was added for "PKCS11 V 3.0 for experimental curves" possibly without registered OIDs to avoid some made up unregistered OID slipping in to general use which is worse then passing in a printable string for testing experimental curves without an OID. In that case it is up to the developer to define the string and we should not second guest if it is case sensitive or not.
The whole problem with 25519 curves was that to use PKCS11 required the use of an OID, and the experimenters used OIDs registered but in private OID space, (better then made up OIDs) but not good to have standards based on private OIDs.
So I would suggest leaving memcmp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation-wise I only have some minor comments.
Regarding the backward compatibility, I don't have a good overview if we really need that. My feeling is that there may only be some experiments where this has been tried out. So maybe we don't need to support the old (and bad) format anymore.
src/libopensc/card-openpgp.c
Outdated
@@ -2516,19 +2520,46 @@ pgp_update_new_algo_attr(sc_card_t *card, sc_cardctl_openpgp_keygen_info_t *key_ | |||
|
|||
if (priv->ext_caps & EXT_CAP_ALG_ATTR_CHANGEABLE) { | |||
/* ECDSA and ECDH */ | |||
/* TODO -DEE could map new OIDs to old OID needed for card here */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please integrate this comment to the one a couple of lines below to give it more context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the comments as ec_curves_alt
below is used to convert RFC 8410 OID to olde OIDs that are written to the card.
src/libopensc/pkcs15-algo.c
Outdated
/* RFC 8410, needed to parse/create X509 certs/pubkeys */ | ||
{ SC_ALGORITHM_EDDSA, {{1, 3, 101, 112, -1}}, NULL, NULL, NULL }, | ||
/* RFC 8410, needed to parse/create X509 certs/pubkeys */ | ||
/* TODO DEE add asn1_decode_ec_params, asn1_encode_ec_params,asn1_free_ec_params */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the DEE identifier (git history has information about the author)
Also, please also add this very comment to the SC_ALGORITHM_XEDDSA block below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed comment and added the asn1_decode_ec_params, asn1_encode_ec_params,asn1_free_ec_params as needed.
src/libopensc/pkcs15-prkey.c
Outdated
@@ -569,20 +569,12 @@ sc_pkcs15_erase_prkey(struct sc_pkcs15_prkey *key) | |||
case SC_ALGORITHM_GOSTR3410: | |||
free(key->u.gostr3410.d.data); | |||
break; | |||
case SC_ALGORITHM_EC: | |||
case SC_ALGORITHM_EC: /* EC, Edwards and Montgomery use common ec params */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put this comment into a new line
sc_encode_oid (ctx, &id, &buf, &len); | ||
if (ecparams->der.value && ecparams->der.len && ecparams->der.len > 2) { | ||
/* caller provided a der version of OID */ | ||
switch (ecparams->der.value[0]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of handling the ASN.1 buffer by hand in case 0x13
, please use sc_asn1_read_tag directly.
src/pkcs15init/pkcs15-openpgp.c
Outdated
/* extract oid the way we need to import it to OpenPGP Card */ | ||
/* DEE NO extract oid the way we need to import it to OpenPGP Card */ | ||
/* TODO DEE pass the oid. will convert to asn1 before writing */ | ||
/* TODO DEE not sure id test for der.len >2 is needed */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the DEE identifier. Also, I do not understand what the comments try to tell me. Please clarify the comment or remove it.
@@ -2426,6 +2441,7 @@ static void verify_signature(CK_SLOT_ID slot, CK_SESSION_HANDLE session, | |||
unsigned char rs_buffer[512]; | |||
bytes = getEC_POINT(session, key, &len); | |||
free(bytes); | |||
/* TODO DEE EDDSA and EC_POINT returned in BIT STRING needs some work */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the DEE identifier
static const struct alg_spec alg_types_asym[] = { | ||
{ "rsa", SC_ALGORITHM_RSA, 1024 }, | ||
{ "rsa", SC_ALGORITHM_RSA, 2048 }, /* new default */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this is a big jump, 3000 bits should be the recommended minimum key size nowadays
@@ -749,9 +770,18 @@ parse_alg_spec(const struct alg_spec *types, const char *spec, unsigned int *key | |||
if (*spec == '/' || *spec == '-' || *spec == ':') | |||
spec++; | |||
|
|||
/* if we have everything for EDDSA or XEDDSA */ | |||
if (*spec == 0x00 && *keybits && (algorithm == SC_ALGORITHM_EDDSA || algorithm == SC_ALGORITHM_XEDDSA) && prkey) { | |||
prkey->u.ec.params.named_curve = strdup(types[types_idx].spec); /* correct case */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use some error checking on strdup
As suggested in comments in OpenSC#3090 simplify code when clearing a pubkey. Not shown in diff next line is: sc_mem_clear(key, sizeof(*key)); Date: Thu Mar 28 15:37:32 2024 -0500 On branch X25519-improvements-2 Changes to be committed: modified: pkcs15-pubkey.c
6bc0d76
to
6d3d21c
Compare
As suggested in comments in OpenSC#3090 simplify code when clearing a pubkey. Not shown in diff next line is: sc_mem_clear(key, sizeof(*key)); Date: Thu Mar 28 15:37:32 2024 -0500 On branch X25519-improvements-2 Changes to be committed: modified: pkcs15-pubkey.c
6d3d21c
to
53b43b6
Compare
Thanks. A lot to keep me busy
…On Mon, Apr 29, 2024, 2:58 AM Jakub Jelen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/libopensc/pkcs15-pubkey.c
<#3090 (comment)>:
> + volatile int gdb_test = 0; /* so can reset via gdb for testing new way */
LOG_FUNC_CALLED(ctx);
sc_copy_asn1_entry(c_asn1_ec_pointQ, asn1_ec_pointQ);
- sc_format_asn1_entry(asn1_ec_pointQ + 0, key->ecpointQ.value, &key->ecpointQ.len, 1);
+
+ if (gdb_test == 1) {
+ key_len = key->ecpointQ.len * 8; /* encode in bit string */
+ sc_format_asn1_entry(asn1_ec_pointQ + 0, key->ecpointQ.value, &key_len, 1);
+ } else {
+ key_len = key->ecpointQ.len;
+ sc_format_asn1_entry(asn1_ec_pointQ + 1, key->ecpointQ.value, &key_len, 1);
+ }
I had some discussion on the OASIS PKCS#11 mailing list to get the format
clarified, but as far as I remember, they were not able to provide much
more clarity (I do not follow the discussions there much now)
There was recently exactly this topic discussed here:
https://lists.oasis-open.org/archives/pkcs11/202310/msg00008.html
There seems to be some changes in the PKCS#11 3.1.
Let me know if there is something else to bring up there.
—
Reply to this email directly, view it on GitHub
<#3090 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGTIMKGEAR7RIWIPSIE7S3Y7X4SFAVCNFSM6AAAAABFK7PMO6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMRXG44DQNJZGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
On branch X25519-improvements-2 Changes to be committed: modified: tools/pkcs11-tool.c
1.3.6.1.4.1159.15.1 should be 1.3.6.1.4.11591.15.1 openpgp writes 2B 06 01 04 01 DA 47 0F 01 to card which comes from OID 06 09 2B 06 01 04 01 DA 47 0F 01 https://lapo.it/asn1js/ (asn.1 parser) reports this as: OBJECT IDENTIFIER 1.3.6.1.4.1.11591.15.1 curve25519 (GNU encryption algorithm) https://www.gnupg.org/oids.html lists this as Ed25519 On branch X25519-improvements-2 Changes to be committed: modified: tools/pkcs11-tool.c
…RYPT On branch X25519-improvements-2 Changes to be committed: modified: tools/pkcs11-tool.c
OpenSSL treats EVP_PKEY_EC, EVP_PKEY_X25519 and EVP_PKEY_X448 as different key types. Refer to the other key as a peer key. Use mech_mech as it is passed into derive_ec_key. On branch X25519-improvements-2 Changes to be committed: modified: src/tools/pkcs11-tool.c
On branch X25519-improvements-2 Changes to be committed: modified: tools/pkcs11-tool.c
EVP_KEY_X25519 is defined but not EVP_KEY_X448. Test if defined. Changes to be committed: modified: src/tools/pkcs11-tool.c
CKA_EC_POINT for eddsa and xeddsa are bit strings. Changes to be committed: modified: src/tools/pkcs11-tool.c
As suggeseted by .github/workflows/doc.yml On branch X25519-improvements-2 Changes to be committed: modified: files/files.html modified: tools/tools.html
On branch X25519-improvements-2 Changes to be committed: modified: pkcs15-pubkey.c
On branch X25519-improvements-2 Changes to be committed: modified: libopensc/card-openpgp.c
On branch X25519-improvements-2 Changes to be committed: modified: libopensc/pkcs15-algo.c
On branch X25519-improvements-2 Changes to be committed: modified: libopensc/pkcs15-prkey.c
On branch X25519-improvements-2 Changes to be committed: modified: tools/pkcs11-tool.c
On branch X25519-improvements-2 Changes to be committed: modified: libopensc/pkcs15-pubkey.c
Make it easier to tell difference between EC, EDDSA and XEDDSA On branch X25519-improvements-2 Changes to be committed: modified: src/libopensc/opensc.h modified: src/libopensc/pkcs15-pubkey.c
On branch X25519-improvements-2 Changes to be committed: modified: src/libopensc/card-openpgp.c modified: src/libopensc/card-openpgp.h
Changes to be committed: modified: libopensc/card.c
On branch X25519-improvements-2 Changes to be committed: modified: libopensc/pkcs15-pubkey.c
Two fields were being copied from the dst the src sc_copy_ec_params is only used in pkcs15init/pkcs15-lib.c On branch X25519-improvements-2 Changes to be committed: modified: libopensc/card.c
On branch X25519-improvements-2 Changes to be committed: modified: ../pkcs15init/pkcs15-lib.c
On branch X25519-improvements-2 Changes to be committed: modified: card.c
…DSA work On branch X25519-improvements-2 Changes to be committed: modified: src/pkcs15init/pkcs15-openpgp.c
On branch X25519-improvements-2 Changes to be committed: modified: libopensc/card-openpgp.c
Changes to be committed: modified: pkcs15init/pkcs15-lib.c
…en_info keytype is used to map SC_ALGORITHM_* to/from SC_OPENPGP_KEYALGO_* On branch X25519-improvements-2 Changes to be committed: modified: libopensc/cardctl.h modified: libopensc/pkcs15-prkey.c modified: pkcs15init/pkcs15-lib.c modified: pkcs15init/pkcs15-openpgp.c
sc_clear_ec_params used free allocated menory and clear other data in struct sc_ec_parameters On branch X25519-improvements-2 Changes to be committed: modified: libopensc/opensc.h
Add support write_object support for ED448 and X448 objects, but no cards current suported by OpenSC implement these. Fix bug with n_attrs in derive-ec-key. Allow read_object of an EC_POINT to be in either OCTET_STRING or BIT_STRING On branch X25519-improvements-2 Changes to be committed: modified: tools/pkcs11-tool.c
sc_clear_ec_params clears an struct sc_ec_parameters by freeing allocated memory. card_find_alg will first check if info->algroithm is one that uses sc_ec_parameters and then checks that the OIDs match. then check if keylength match. On branch X25519-improvements-2 Changes to be committed: modified: libopensc/card.c
Fix several problems with use of sc_ec_parameters On branch X25519-improvements-2 Changes to be committed: modified: pkcs15init/pkcs15-lib.c
Improvments and fixes for mem leaks and GUNK and mapping RFC8410 OIDs. When writing or generating a key add all known algs to card->algrorithms. Fix some BYTES4BITS bugs and formating. Add note about borblems trying to store RFC8410 type key. On branch X25519-improvements-2 Changes to be committed: modified: libopensc/card-openpgp.c modified: pkcs15init/pkcs15-openpgp.c
On branch X25519-improvements-2 Changes to be committed: modified: libopensc/libopensc.exports
Base OIDs for EDWARDS and MONTGOMERY keys on the size of ecpointQ bewween 32 for 25519 and 56 for 448 keys. On branch X25519-improvements-2 Changes to be committed: modified: pkcs11/framework-pkcs15.c
On branch X25519-improvements-2 Changes to be committed: modified: pkcs15init/pkcs15-isoApplet.c
Various changes for RFC8410 curves On branch X25519-improvements-2 Changes to be committed: modified: libopensc/pkcs15-algo.c modified: libopensc/pkcs15-prkey.c modified: libopensc/pkcs15-pubkey.c
0c542d9
to
3fa1d48
Compare
The latest push for this PR are based on 865cb43 pkcs11-curr-v3.0-os section "2.3 Elliptic Curve" includes in CKK_EC_EDWARDS and CKK_EC_MONTGOMERY keys which have a CKA_EC_PARMS, which has an RFC8410 refer to as Ed25519, Ed448, X25519 and X448 as different key types, and in ASN.1 is at the same level as the CKK_EC. The RFC8410 ASN.1 does not have a EC_PARAMS. Within OpenSC the terms EDDSA, refers to CKK_EC_EDWARDS i.e. Ed25519 or Ed448 which can be distinguished by The OpenSSL only uses the RFC 8410, But tests with Yubikey and NitroStart GUNK cards shows the RFC 8032 and RFC 7748 curves used by OpenPGP can verify signatures and produce the same derived keys. What is not in this PR (yet) is support in OpenSC is The If I was going to do this again, I would change
to be something like:
and simplify many of the if statements. I would also replace |
Now that 0.25.0 has been released, this PR is being resubmitted and rebased on current master.
Fixes: #2952 which explains most of what was done.
It also addresses #3000 in that it can accept a CKA_EC_POINT as either in a ASN.1 BIT STRING or OCTET STRING but currently only return CKA_EC_POINT in ASN.1 OCTET STRING. It also add support for CKA_PUBLIC_KEY_INFO which is SPKI format.
It also add support for all four curves defined in RFC 8410 and and maps the uses of the experimental OIDs to these official OIDs in OpenSC, and in card-opensc.c that will still write the old OIDs to the cards as needed.
Individual commits have additional information.
Tested using using Yubikey NFC and Nitro Start tokens which only use the old OIDs and do not yet support the X448 keys.
-->
Checklist