Support EcDSA Keys with Named Curves #47

rkeene · 2022-10-17T19:34:39Z

This changeset adds support for EcDSA Keys with Named Curves. Additionally, it may make it easier to add other kinds of keys in the future.

Not done as part of this change:
Support GnuTLS

alonbl · 2022-10-17T20:21:16Z

Hi,

Great work!

First, please do not mark as fix #17 as I already explained to you that the problem there was the existence of EC certificate which caused the RSA certificates not to be accepted.

Please refactor/rename the entire keyutil module to keyinfo as it does nothing but that. As far as I understand the interface would be something like:

struct keyinfo_s;
typedef struct keyinfo_s *keyinfo;

struct key_data_list_s {
    struct key_data_list_s *next;
    char *type;
    char *value;
}

keyinfo keyinfo_new();
void keyinfo_free(keyinfo keyinfo);
int keyinfo_get_type(keyinfo keyinfo);
int keyinfo_from_der(keyinfo keyinfo, unsigned char *der, size_t len);
gcry_sexp_t *keyinfo_to_sexp(keyinfo keyinfo);
key_data_list *keyinfo_get_key_data(keyinfo keyinfo);
void key_data_free(key_data_list list);

What do you think? This way we focus in the key logic within the keyinfo module.
Notice that the type and I believe the curve should be acquired from the certificate.

Thank you for your help!
Alon

rkeene · 2022-10-17T20:50:25Z

I'm not sure I understand the key_data_list_s linked-list. A given der (certificate)'s resultant keyinfo would only ever hold 1 key, is the list used for some other purpose ? What would value and type be ?

alonbl · 2022-10-17T21:00:21Z

I'm not sure I understand the key_data_list_s linked-list. A given der (certificate)'s resultant keyinfo would only ever hold 1 key, is the list used for some other purpose ? What would value and type be ?

all that needed in order to provide the key to assuan socket without the code that within command.c knows the internals of the key.

For RSA (and current implementation) it will be:

- type: KEY-DATA
  value: hex(n)
- type: KEY-DATA
  value: hex(e)

Not sure how we transmit the key type, but what I would like to achieve is a command.c which gets this list, iterate it and send it over the assuan socket without actually knows what the key type is.

rkeene · 2022-10-18T00:59:42Z

I refactored the code.

rkeene · 2022-10-18T18:16:05Z

@alonbl I'm trying to add GENKEY support now. Do you know where it's documented for RSA and ECC ? e.g., sending KEY-DATA with n and e for RSA -- where is that specified ?

I'm guessing that something similar needs to be done for EcDSA for q and curve, but do we also need to specify that ECC is being used and not RSA ?

rkeene · 2022-10-18T22:09:55Z

Based on some postings I found online, it looks like the "KEY-DATA" for "q" (as hex-encoded integer) and "curve" as DER-encoded OID (minus tag) is what's needed.

rkeene · 2022-10-19T00:16:11Z

This functionality is almost entirely working:

gpg: key 10ECAD2B69B1E2E3 marked as ultimately trusted
gpg: directory '/home/rkeene/.gnupg/openpgp-revocs.d' created
gpg: error reading rest of packet: Invalid argument
gpg: can't encode a 512 bit MD into a 72 bits frame, algo=10
gpg: revocation certificate stored as '/home/rkeene/.gnupg/openpgp-revocs.d/3AAF050CCCE2EE667BDAB3D010ECAD2B69B1E2E3.rev'
public and secret key created and signed.

gpg: can't encode a 512 bit MD into a 72 bits frame, algo=10
pub   nistp256 2022-10-19 [INVALID_ALGO]
      3AAF050CCCE2EE667BDAB3D010ECAD2B69B1E2E3
uid                      xxxxx <[email protected]>

rkeene · 2022-10-19T15:10:33Z

I think the remaining issues are with cmd_getattr(), but it's not clear why some of the things being done are being done there, especially KEY-ATTR and EXTCAP which mention 2048 (which I assume is a reference to the RSA key size).

In KEY-ATTR there's a loop of size 3 but why isn't clear either.

alonbl · 2022-10-29T21:00:03Z

I'm back to this -- do you have have any ideas @alonbl ? Otherwise I'm going to have to dig through the GPG code.

You will need to reverse engineer gnupg own scd and see what you get for each case. As there is no formal spec for the IPC this is the only way to make it work.

alonbl

Thanks!
In order to push this forward, please prepare a set of pull requests of cleanups, each per something we can quickly merge, so that it will be simpler to digest the remaining EC only related additions.

alonbl · 2022-10-29T21:01:22Z

gnupg-pkcs11-scd/command.c

@@ -30,6 +30,7 @@

 #include "common.h"
 #include "strgetopt.h"
+#include <pkcs11-helper-1.0/pkcs11.h>


why is this required?

I added this for things like CKR_OK because it wasn't clear it would otherwise be included.

gnupg-pkcs11-scd/command.c

alonbl · 2022-10-29T21:09:35Z

gnupg-pkcs11-scd/command.c

+	}
+
+	if (_pkcs11_keyinfo_mechanism(keyinfo, &pkcs11_mechanism) != CKR_OK) {
+		goto cleanup;


please focus in EC in this thread, any other change such as rename of the variables or modifying any behavior should go to other pull request.

In this case you assume that the problem is something within the token to select the hash, while as far as I remember the gnupg is the one that decides in some cases to pass the entire blob forcing us to guess what hash did he wish use to use.

anyway, in order to discuss it better please move this into other pull request.

I'm not sure what you mean.

The highlighted change is regarding selecting the mechanism for asking the PKCS#11 module to sign using the correct mechanism, as well as ensuring PKCS padding is only used when using (CKM_RSA_PKCS)

Maybe I do not understand the code, this is why it is better to do this in steps each change own pull request...

gnupg provides data which may or may not include the hash packaging, the fixup should be done in any case.

you assume that this fixup should be done only if key type is rsa, correct? have you looked at the scd sources to see if this is correct and hash never provided in this case?

The encoding specified by PKCS#1 would only be there for RSA -- EcDSA keys simply aren't large enough.

doesn't ec also signs hashes? as far as I know it does. how will the peer know what hash was used in the signature? the hash name/oid must be signed as well for this to be secure.

EcDSA keys for NIST P-256 are 33 bytes (257 bits) long (in compressed mode) with 64 byte signatures.

The CKM_ECDSA mechanism expects the input to already be hashed (https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__12__3__6__ECDSA__WITHOUT__HASHING.html) and encoded to fit within 2*keyLength.

The padding/encoding for PKCS#1 isn't needed since the keys are so much smaller.

if the digest algorithm id is not signed then I may modify the digest id in the unauthenticated message to less secure digest that its value matches the more secure so validate process will validate less secure digest without being aware of the switch. anyway, it is not that important as long as if you sign the gnupg peer will be able to verify.

The code now removes any found prefix.

gnupg-pkcs11-scd/command.c

gnupg-pkcs11-scd/keyutil.c

invidian

Thanks for the PR @rkeene, having support for ec25519/ecdsa keys would be really awesome.

rkeene · 2022-10-31T14:37:14Z

You will need to reverse engineer gnupg own scd and see what you get for each case. As there is no formal spec for the IPC this is the only way to make it work.

I'm almost done with this.

gnupg-pkcs11-scd/command.c

alonbl · 2022-10-31T15:58:26Z

gnupg-pkcs11-scd/command.c

+			use_pkcs1 = 1;
+			break;
+		case CKM_ECDSA:
+			use_pkcs1 = 0;


why do you need this variable? can't you ask below if mechanism is RSA_PKCS?

In the future there may be other mechanisms that do or do not use PKCS#1 padding.

alonbl · 2022-10-31T16:03:37Z

gnupg-pkcs11-scd/command.c

+	}
+
+	if (_pkcs11_keyinfo_mechanism(keyinfo, &pkcs11_mechanism) != CKR_OK) {
+		goto cleanup;


Maybe I do not understand the code, this is why it is better to do this in steps each change own pull request...

gnupg provides data which may or may not include the hash packaging, the fixup should be done in any case.

you assume that this fixup should be done only if key type is rsa, correct? have you looked at the scd sources to see if this is correct and hash never provided in this case?

rkeene · 2022-11-01T19:30:29Z

I'm making progress here, creating subkeys is working but signing is not yet working.

$ gpg --list-secret-keys
gpg: NOTE: THIS IS A DEVELOPMENT VERSION!
gpg: It is only intended for test purposes and should NOT be
gpg: used in a production environment or with production keys!
/home/rkeene/.gnupg/pubring.kbx
-------------------------------
sec#  nistp256 2022-10-31 [SCA]
      F053661CDB471F87D9D7C31B158A385D4B4ECDA7
uid           [ultimate] xxxxx <[email protected]>

$ echo test | gpg --clear-sign F053661CDB471F87D9D7C31B158A385D4B4ECDA7
gpg: NOTE: THIS IS A DEVELOPMENT VERSION!
gpg: It is only intended for test purposes and should NOT be
gpg: used in a production environment or with production keys!
gpg: no default secret key: Unusable secret key
gpg: F053661CDB471F87D9D7C31B158A385D4B4ECDA7: clear-sign failed: Unusable secret key

There are bugs in GPG when dealing with EcDSA keys that reside on cards.

alonbl · 2022-11-01T19:39:54Z

There are bugs in GPG when dealing with EcDSA keys that reside on cards.

there usually are... very frustrating to chase non-standard non-cooperative undocumented unstable proprietary non-modular project to provide standard interface.

thank your help, in the meanwhile I think you have done many good cleanups, if we review them one by one we may reach to a point in which the EC part will be left more or less ready until gnupg support will be sorted out.

rkeene · 2022-11-02T02:43:17Z

This appears to be working now.

$ gpg --expert --full-generate-key
gpg (GnuPG) 2.3.2-beta105; Copyright (C) 2021 Free Software Foundation, Inc.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

gpg: NOTE: THIS IS A DEVELOPMENT VERSION!
gpg: It is only intended for test purposes and should NOT be
gpg: used in a production environment or with production keys!
gpg: keybox '/home/rkeene/.gnupg/pubring.kbx' created
Please select what kind of key you want:
   (1) RSA and RSA
   (2) DSA and Elgamal
   (3) DSA (sign only)
   (4) RSA (sign only)
   (7) DSA (set your own capabilities)
   (8) RSA (set your own capabilities)
   (9) ECC (sign and encrypt) *default*
  (10) ECC (sign only)
  (11) ECC (set your own capabilities)
  (13) Existing key
  (14) Existing key from card
Your selection? 13
Enter the keygrip: F8BC7DE50508E0C578EE5F60021EB57B46D7F22B
                                                           
Possible actions for this ECC key: Sign Certify Authenticate 
Current allowed actions: Sign Certify 

   (S) Toggle the sign capability
   (A) Toggle the authenticate capability
   (Q) Finished

> q
Please specify how long the key should be valid.
         0 = key does not expire
      <n>  = key expires in n days
      <n>w = key expires in n weeks
      <n>m = key expires in n months
      <n>y = key expires in n years

> 0
Key does not expire at all
                        
GnuPG needs to construct a user ID to identify your key.

You selected this USER-ID:
    "xxxxxx <[email protected]>"

gpg: key FDE693371D5E0761 marked as ultimately trusted
gpg: revocation certificate stored as '/home/rkeene/.gnupg/openpgp-revocs.d/A5A3AE53DE9E0D69CDC98C60FDE693371D5E0761.rev'
public and secret key created and signed.

pub   nistp256 2022-11-02 [SC]
      A5A3AE53DE9E0D69CDC98C60FDE693371D5E0761
uid                      xxxxxx <[email protected]>


$ gpg --check-signatures
/home/rkeene/.gnupg/pubring.kbx
-------------------------------
pub   nistp256 2022-11-02 [SC]
      A5A3AE53DE9E0D69CDC98C60FDE693371D5E0761
uid           [ultimate] xxxxxx <[email protected]>
sig!3        FDE693371D5E0761 2022-11-02  xxxxxx <[email protected]>

gpg: 1 good signature


$ echo 'test' | gpg --clear-sign
gpg: NOTE: THIS IS A DEVELOPMENT VERSION!
gpg: It is only intended for test purposes and should NOT be
gpg: used in a production environment or with production keys!
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

test
-----BEGIN PGP SIGNATURE-----

iHUEARMIAB0WIQSlo65T3p4Nac3JjGD95pM3HV4HYQUCY2HWxgAKCRD95pM3HV4H
YWvfAQDZ0ov5/NZxJf6eDrx76uDQ3qnXAKY6EBpbLq0k1+YKVAD/bV52WO8uFtSW
1oq+Fe4jQWCBNnrMmRFA+NRaMwRYs8Q=
=EidH
-----END PGP SIGNATURE-----


$ gpg --verify
...
gpg: Signature made Tue 01 Nov 2022 09:32:38 PM CDT
gpg:                using ECDSA key A5A3AE53DE9E0D69CDC98C60FDE693371D5E0761
gpg: Good signature from "xxxxxx <[email protected]>" [ultimate]

rkeene · 2022-11-02T20:41:14Z

GPG Patch added to this bug report: https://dev.gnupg.org/T5555

alonbl · 2022-11-02T20:48:48Z

Great. Please split review to multiple requests so we can review each, start with trivial renames, then refactor.

…

On Wed, 2 Nov 2022 at 22:41 Roy Keene ***@***.***> wrote: GPG Patch added to this bug report: https://dev.gnupg.org/T5555 — Reply to this email directly, view it on GitHub <#47 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJURLPK4GGVV3GEUZZ7QM3WGLGXJANCNFSM6AAAAAARHMGCWQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

rkeene · 2022-11-03T04:46:14Z

@alonbl There aren't any trivial renames as part of this PR aside from the refactor of "keyutil" leading to keyutil_get_cert_hexgrip being renamed to keyinfo_get_hexgrip

rkeene · 2022-11-03T12:37:16Z

I've created PRs #48, #49, and #50 with bug fixes, #51 with a refactor that enables multiple kinds of keys, and #52 which adds support for signing of keys which do not use PKCS1 encoding.

mirko · 2024-11-16T17:25:39Z

I'm eagerly awaiting support of curves being supported. However last activity in either PR was in 2022 and I'm having the feeling things moved on - without this getting merged anytime soon.

Lot's of effort was put into adding this functionality, the pre-work was split up into its own atomic PRs, reviewed, approved, and closed - yet not merged (because those PRs were just intended for review?).

Is there anything one (me?) can do to push things further?

rkeene · 2024-11-17T06:30:10Z

If it makes any difference, I've been using the patched SCD for about 2 years without any problems

mirko · 2024-11-17T10:12:09Z

If it makes any difference, I've been using the patched SCD for about 2 years without any problems

It does to me!

Is this branch of your still the current state?

Also, is https://dev.gnupg.org/T5555 still necessary? I tried to comment on the ticket, but the GnuPG "community" is an absolute nightmare. Account creation is necessary, but disabled. One has to register to their mailing list and send credentials to all members to maybe eventually get an account in order to be able to comment on tickets.
Anyway, needless to say it's not the only bug report with attached patch which is getting ignored for years.

Is there any chance to also support ed25519 curves - which the YubiKey's retired slots are now also capable of (I think since YubiKey firmware version 5.7.x or something)?

rkeene · 2024-11-19T22:35:51Z

The changes are still required to GnuPG as far as I know.

Ed25519 isn't significantly different from EcDSA so support for an SCD isn't hard (it's a bit easier, since no hashing has to be done)... however, I'm not sure about support from the GnuPG side through an SCD.

Oops, I accidentally closed it.

rkeene added 2 commits October 17, 2022 14:32

Initial work on adding EcDSA Named Curve support

2edd227

Fix memory leak in listing certificates

c321bb1

rkeene added 8 commits October 17, 2022 18:28

Phase 1 of refactor (no rename) keyutil to keyinfo

69c59e3

Merge branch 'master' into feature/ec-certs

61a3095

Fix spelling errors

24e6b51

Update mechanism for checking which kind of key is being used

9881605

Move common definitions outside OpenSSL ifdef

c652797

Add backwards compatibility for getting key information

237db4b

Lower backwards compatability requirements

b4695a1

Add LibreSSL requirements and use compatible name

e3e549f

rkeene added 6 commits October 17, 2022 20:31

Added erroneously removed cleanup

850ed50

Fix memory leak

601ac7e

Added missing initializer for cleanup

bfc2a95

Fix free of stack allocated data

03d1a33

Fix memory leak in fetching keyinfo

1642687

First attempt at signing support

32aa882

Support generating key parameters for ECC keys

00a7eb9

rkeene added 5 commits October 18, 2022 17:10

Add support for encoding named curves in "GENKEY" correctly

407cfe5

Fix issue with case consistency

58e8261

Remove curve names that GnuPG does not understand

f70c0a4

Add additional comments

ebcae68

Support handling oversized data for signing

78c1534

alonbl reviewed Oct 29, 2022

View reviewed changes

invidian reviewed Oct 31, 2022

View reviewed changes

alonbl reviewed Oct 31, 2022

View reviewed changes

rkeene added 2 commits November 1, 2022 21:08

Remove any PKCS1 prefix when performing non-PKCS1 signatures

91abe4e

Return proper KEY-ATTR information

6b8232c

rkeene added 3 commits November 1, 2022 21:57

return(x) -> return x

ca19cb3

Remove abort() for invalid key types after reading unallocated memory

ba10981

Re-add whitespace error

11b4b2f

rkeene requested a review from alonbl November 2, 2022 03:06

rkeene marked this pull request as ready for review November 2, 2022 03:07

rkeene marked this pull request as draft November 2, 2022 20:44

rkeene added 4 commits November 2, 2022 23:11

Lookup named curve

f586ed2

Fix bug looking up RSA key size

42f92ed

For sanity, add default return

98d2d3c

Fix signedness of offset lookups

d7373b3

rkeene closed this Nov 19, 2024

rkeene reopened this Nov 19, 2024

Support EcDSA Keys with Named Curves #47

Are you sure you want to change the base?

Support EcDSA Keys with Named Curves #47

Conversation

rkeene commented Oct 17, 2022 • edited Loading

alonbl commented Oct 17, 2022

rkeene commented Oct 17, 2022

alonbl commented Oct 17, 2022

rkeene commented Oct 18, 2022

rkeene commented Oct 18, 2022

rkeene commented Oct 18, 2022

rkeene commented Oct 19, 2022

rkeene commented Oct 19, 2022

alonbl commented Oct 29, 2022

alonbl left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

rkeene commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeene commented Nov 1, 2022

alonbl commented Nov 1, 2022

rkeene commented Nov 2, 2022

rkeene commented Nov 2, 2022

alonbl commented Nov 2, 2022 via email

rkeene commented Nov 3, 2022

rkeene commented Nov 3, 2022 • edited Loading

mirko commented Nov 16, 2024

rkeene commented Nov 17, 2024

mirko commented Nov 17, 2024

rkeene commented Nov 19, 2024 • edited Loading

rkeene commented Oct 17, 2022 •

edited

Loading

rkeene commented Nov 3, 2022 •

edited

Loading

rkeene commented Nov 19, 2024 •

edited

Loading