OpenPGP 3.4+: Get list of supported algorithms from Algorithm Information #2287

ya-isakov · 2021-04-04T08:59:58Z

OpenPGP Card 3.4+ supports getting list of algos from the new Algoritm Information DO
https://gnupg.org/ftp/specs/OpenPGP-smart-card-application-3.4.pdf, 4.4.3.11
If at least one algo is in this list, use it, and ignore hardcoded.

Tested on Yubikey 5 NFC with FW 5.2.4. Handled few problems with Yubikey not
following the spec.

Also, fixed problem with algorithm find, which will return EC algorithm with the same
key size, even if curve is not the same (e.g., curves 1.3.132.0.34 and 1.3.36.3.3.2.8.1.1.11 has
the same keysize of 384 bits, so it was possible that one returned in place of other)

This is needed for Yubikey to support more algorithms, than are used in Algorithm Attributes DO,
in case it was initialized with gpg, and not pkcs15-tool.

Checklist

PKCS#11 module is tested
Windows minidriver is tested
macOS tokend is tested

Jakuje

Looks good. Just couple of comments around the code style. Tested with Nitrokey Start and I did not notice any issues.

src/libopensc/card-openpgp.c

ya-isakov · 2021-04-06T16:44:25Z

@Jakuje thank you for review. IIRC, Nitrokeys are based on GNUK, which OpenPGP Card 2.0 with some extensions, so, my code should not work for it. But it's good to know that I haven't broke anything for other tokens :) I've changed logic a little bit (now, DO FA should be checked independent from DO 6E, but, still, if any algo is found by it, code in DO 6E will ignore info from DOs 7E -> C1-C3. Could you, please, test it again?

Jakuje · 2021-04-07T08:48:09Z

@Jakuje thank you for review. IIRC, Nitrokeys are based on GNUK, which OpenPGP Card 2.0 with some extensions, so, my code should not work for it.

Right. I was writing the initial support for ed25519 in opensc around the gnuk.

But it's good to know that I haven't broke anything for other tokens :) I've changed logic a little bit (now, DO FA should be checked independent from DO 6E, but, still, if any algo is found by it, code in DO 6E will ignore info from DOs 7E -> C1-C3. Could you, please, test it again?

Retested current version and looks working fine.

src/libopensc/card-openpgp.c

ya-isakov · 2021-04-07T18:46:25Z

After some research, I found that OpenSC is parsing Import-Format byte, but then it's not storing it anywhere. So, it's set in pgp_parse_algo_attr_blob, but the struct, where it is set, is created in pgp_get_card_features, but nowhere In this function I can see usage of keyformat field (e.g, it is not added as algo field) Then, keyformat is checked only once - in pgp_build_extended_header_list, and this function is called in openpgp_store_key. In the same function, key_info is created, but no keyformat field is set.

So, I think I can assume, that as this setting is ignored, my current approach is fine - for tokens < 3.4, the setting will be read from DO 6E, and ignored. For 3.4+ tokens, if DO FA has this byte set for any algo, it will be ignored, again...

dengert · 2021-04-07T19:44:18Z

Looks like the keyformat deals with the format of the private key on the card.

#define SC_OPENPGP_KEYFORMAT_RSA_STD	0    /* See 4.3.3.6 Algorithm Attributes */
#define SC_OPENPGP_KEYFORMAT_RSA_STDN	1    /* OpenPGP card spec v2 */
#define SC_OPENPGP_KEYFORMAT_RSA_CRT	2
#define SC_OPENPGP_KEYFORMAT_RSA_CRTN	3
#define SC_OPENPGP_KEYFORMAT_EC_STD	0
#define SC_OPENPGP_KEYFORMAT_EC_STDPUB	0xFF

And in OpenPGP-3.4.1 it shows how the STD and CRT are used.

So unless you are loading or reading a private key, you should not care, because you will never see it. src/tools/openpgp-tool.c uses it in pgp_store_key and only supports some of them.

ya-isakov · 2021-04-10T07:27:59Z

I've fixed problem with multiple ECDH-DERIVE mechanisms, which surfaced after DO FA parsing. Now, derive works for X25519 (with MRs #2286 and #2293)

src/libopensc/card-openpgp.c

src/libopensc/card.c

frankmorgner

The code looks good, just a formatting issue

Jakuje · 2021-09-30T09:22:22Z

@ya-isakov can you rebase the patch set on the current master and resolve the conflicts?

ya-isakov · 2021-10-01T21:51:56Z

@Jakuje, Sure, I'll do it on this weekend.

…tion OpenPGP Card 3.4+ supports getting list of algos from the new Algoritm Information DO https://gnupg.org/ftp/specs/OpenPGP-smart-card-application-3.4.pdf, 4.4.3.11 If at least one algo is in this list, use it, and ignore hardcoded. Tested on Yubikey 5 NFC with FW 5.2.4. Handled few problems with Yubikey not following the spec. Also, fixed problem with algorithm find, which will return EC algorithm with the same key size, even if curve is not the same.

If we're finding EC algo, and curve is not matched, another algo with the same key length shoud not be returned.

- Now it's checked independent from DO 6E - If no algos can be parsed from DO FA, code will fallback to other ways - Indentation fixes

After parsing DO FA, algorithm info could have both X25519 and older EC curves. This leads to register of two mechanisms with same type, CKM_ECDH1_DERIVE, but different key types. This could lead to a situation, that C_DeriveKey will throw an error, while user is trying to derive using X25519 key. This commit fixes this, to have a list of key types, and registering the second mechanism will just add key type to list in the existing mechanism, and updates it's mech_info.

ya-isakov · 2021-10-04T19:03:05Z

I've rebased my branch, fixed leftover comments, but I need to check, if it is still working :) Will do it soon.

Jakuje

just minor nits. I think we are good to go with this. Tested locally with Nitrokey start and all looks fine.

src/pkcs11/mechanism.c

szszszsz · 2021-12-01T11:51:55Z

@Jakuje thank you for review. IIRC, Nitrokeys are based on GNUK, which OpenPGP Card 2.0 with some extensions, so, my code should not work for it.

A bit off topic: Nitrokey Pro and Nitrokey Storage contain OpenPGP v3.3 card. Nitrokey Start is indeed v2.0+ (with chosen parts of v3.3 spec) software OpenPGP implementation, based on GNUK.

Jakuje · 2021-12-01T12:05:57Z

@szszszsz thanks for clarification and thanks for reminder I wanted to merge this finally.

It would be great if we could get the nits I mentioned last month fixed. But anyway, if there will be no further concerns, I will merge this during the next week.

Jakuje · 2021-12-04T14:07:08Z

Thank you!

ya-isakov force-pushed the do_fa branch from 0922b1f to 1901038 Compare April 4, 2021 09:03

ya-isakov changed the title ~~OpenPGP: Get list of supported algorithms from DO FA~~ OpenPGP: Get list of supported algorithms from Algorithm Information Apr 4, 2021

ya-isakov force-pushed the do_fa branch 3 times, most recently from f923d32 to d6f519d Compare April 4, 2021 10:22

ya-isakov changed the title ~~OpenPGP: Get list of supported algorithms from Algorithm Information~~ OpenPGP 3.4+: Get list of supported algorithms from Algorithm Information Apr 4, 2021

ya-isakov mentioned this pull request Apr 4, 2021

OpenPGP 3.4+: Enable ed25519/curve25519 support #2286

Merged

3 tasks

ya-isakov force-pushed the do_fa branch 7 times, most recently from cfe0c2b to e1d5ce0 Compare April 4, 2021 18:08

Jakuje reviewed Apr 6, 2021

View reviewed changes

src/libopensc/card-openpgp.c Outdated Show resolved Hide resolved

src/libopensc/card-openpgp.c Outdated Show resolved Hide resolved

src/libopensc/card-openpgp.c Outdated Show resolved Hide resolved

Jakuje reviewed Apr 7, 2021

View reviewed changes

src/libopensc/card-openpgp.c Outdated Show resolved Hide resolved

ya-isakov marked this pull request as draft April 7, 2021 13:49

ya-isakov marked this pull request as ready for review April 7, 2021 18:57

This was referenced Apr 14, 2021

[RFC] OpenSC should allow to sign data with length more than 512 bytes. #2300

Closed

p11test is ignoring errors, and not failing tests #2304

Closed

Jakuje reviewed May 20, 2021

View reviewed changes

src/libopensc/card-openpgp.c Outdated Show resolved Hide resolved

frankmorgner approved these changes Aug 31, 2021

View reviewed changes

src/libopensc/card.c Show resolved Hide resolved

frankmorgner reviewed Aug 31, 2021

View reviewed changes

ya-isakov force-pushed the do_fa branch 2 times, most recently from 6afc361 to 0f122ee Compare October 4, 2021 18:30

ya-isakov added 5 commits October 4, 2021 20:39

Fix find for EC algos returning wrong algo

fd41e35

If we're finding EC algo, and curve is not matched, another algo with the same key length shoud not be returned.

Some changes in DO FA handling:

a703468

- Now it's checked independent from DO 6E - If no algos can be parsed from DO FA, code will fallback to other ways - Indentation fixes

Fix pass of handling_algos if DO 6E is not available

97cbd68

Fix error check in pgp_enumerate_blob

75c64b0

ya-isakov force-pushed the do_fa branch from 0f122ee to 4f9a2f8 Compare October 4, 2021 18:50

Jakuje approved these changes Oct 5, 2021

View reviewed changes

src/pkcs11/mechanism.c Outdated Show resolved Hide resolved

src/pkcs11/mechanism.c Outdated Show resolved Hide resolved

Fix comments from review

a8df293

frankmorgner approved these changes Dec 3, 2021

View reviewed changes

Jakuje merged commit 2cc7b10 into OpenSC:master Dec 4, 2021

xhanulik mentioned this pull request Jul 12, 2022

Prepare for release 0.23.0 #2426

Closed

OpenPGP 3.4+: Get list of supported algorithms from Algorithm Information #2287

OpenPGP 3.4+: Get list of supported algorithms from Algorithm Information #2287

Uh oh!

Conversation

ya-isakov commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ya-isakov commented Apr 6, 2021

Uh oh!

Jakuje commented Apr 7, 2021

Uh oh!

Uh oh!

ya-isakov commented Apr 7, 2021

Uh oh!

dengert commented Apr 7, 2021

Uh oh!

ya-isakov commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frankmorgner left a comment

Choose a reason for hiding this comment

Uh oh!

Jakuje commented Sep 30, 2021

Uh oh!

ya-isakov commented Oct 1, 2021

Uh oh!

ya-isakov commented Oct 4, 2021

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

szszszsz commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jakuje commented Dec 1, 2021

Uh oh!

Jakuje commented Dec 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ya-isakov commented Apr 4, 2021 •

edited

Loading

ya-isakov commented Apr 10, 2021 •

edited

Loading

szszszsz commented Dec 1, 2021 •

edited

Loading