From: Theo Buehler Subject: Re: PKCS#11 Ed25519 support To: Damien Miller Cc: openssh@openssh.com, tech@openbsd.org Date: Fri, 25 Jul 2025 11:18:41 +0200 On Fri, Jul 25, 2025 at 04:28:20PM +1000, Damien Miller wrote: > Hi, > > Now that the PKCS#11 refactoring has landed, it's now quite > straightforward to teach OpenSSH how to use Ed25519 keys stored in > PKCS#11 tokens. > > The only wrinkle is that different tokens seems to use different > ASN.1 encodings for the public key and its type identifier. > This implementation handles the two possibilities I've observed so > far: Yubikeys and SoftHSM2. > > Note: this requires the pkcs11.h update diff I sent out earlier. > > ok? Generally looks good. A few comments inline. > > commit 772de3d70ad014d55fae5b3e2866b48325dc473f > Author: Damien Miller > Date: Sat May 24 13:02:44 2025 +1000 > > PKCS#11 Ed25519 key support > > diff --git a/ssh-pkcs11.c b/ssh-pkcs11.c > index 7874eff..a0aca3d 100644 > --- a/ssh-pkcs11.c > +++ b/ssh-pkcs11.c > @@ -41,6 +41,7 @@ > #include "ssh-pkcs11.h" > #include "digest.h" > #include "xmalloc.h" > +#include "crypto_api.h" > > struct pkcs11_slotinfo { > CK_TOKEN_INFO token; > @@ -699,6 +700,69 @@ pkcs11_sign_ecdsa(struct sshkey *key, > return ret; > } > > +static int > +pkcs11_sign_ed25519(struct sshkey *key, > + u_char **sigp, size_t *lenp, > + const u_char *data, size_t datalen, > + const char *alg, const char *sk_provider, > + const char *sk_pin, u_int compat) > +{ > + struct pkcs11_key *k11; > + struct pkcs11_slotinfo *si; > + CK_FUNCTION_LIST *f; > + CK_ULONG slen = 0; > + CK_RV rv; > + u_char *sig = NULL; > + CK_BYTE *xdata = NULL; > + int ret = -1; > + > + if (sigp != NULL) > + *sigp = 0; > + if (lenp != NULL) > + *lenp = 0; > + > + if ((k11 = pkcs11_lookup_key(key)) == NULL) { > + error_f("no key found"); > + return SSH_ERR_KEY_NOT_FOUND; > + } > + > + if (pkcs11_get_key(k11, CKM_EDDSA) == -1) { > + error("pkcs11_get_key failed"); > + return SSH_ERR_AGENT_FAILURE; > + } > + > + debug3_f("sign using provider %s slotidx %lu", > + k11->provider->name, (u_long)k11->slotidx); > + > + f = k11->provider->function_list; > + si = &k11->provider->slotinfo[k11->slotidx]; > + > + xdata = xmalloc(datalen); > + memcpy(xdata, data, datalen); > + sig = xmalloc(crypto_sign_ed25519_BYTES); > + slen = crypto_sign_ed25519_BYTES; > + > + rv = f->C_Sign(si->session, xdata, datalen, sig, &slen); > + if (rv != CKR_OK) { > + error("C_Sign failed: %lu", rv); > + goto done; > + } > + if (slen != crypto_sign_ed25519_BYTES) { > + error_f("bad signature length: %lu", (u_long)slen); > + goto done; > + } > + if ((ret = ssh_ed25519_encode_store_sig(sig, slen, sigp, lenp)) != 0) > + fatal_fr(ret, "couldn't store signature"); > + > + /* success */ > + ret = 0; > + done: > + if (xdata != NULL) > + freezero(xdata, datalen); The xdata != NULL check isn't really needed. It is documented that freezero() with NULL performs no action (including no length check). That said, I prefer to pass NULL, 0 to freezero(), which doesn't work well with the length from an input parameter. > + free(sig); > + return ret; > +} > + > /* remove trailing spaces */ > static char * > rmspace(u_char *buf, size_t len) > @@ -1009,6 +1073,114 @@ fail: > return key; > } > > +static struct sshkey * > +pkcs11_fetch_ed25519_pubkey(struct pkcs11_provider *p, CK_ULONG slotidx, > + CK_OBJECT_HANDLE *obj) > +{ > + CK_ATTRIBUTE key_attr[3]; > + CK_SESSION_HANDLE session; > + CK_FUNCTION_LIST *f = NULL; > + CK_RV rv; > + struct sshkey *key = NULL; > + const unsigned char *d = NULL; > + size_t len; > + char *hex = NULL; > + int success = -1, i; > + const u_char oid1[14] = { I'd call this curveName[14] or perhaps 'name[14]' (it isn't an oid as you note yourself in the comment). > + 0x13, 0x0c, 0x65, 0x64, 0x77, 0x61, 0x72, 0x64, > + 0x73, 0x32, 0x35, 0x35, 0x31, 0x39, > + }; /* PrintableString { "edwards25519" } */ > + const u_char oid2[5] = { and this plain oid[]. You're dealing with the "choice of three parameter representation methods" to which they added the somewhat confusingly named fourth alternaltive curveName (since the oId is often referred to as the curve name): Parameters ::= CHOICE { ecParameters ECParameters, oId CURVES.&id({CurveNames}), implicitlyCA NULL, curveName PrintableString } ecParameters are out of scope since these would require Ed25519 to be encoded as a Weierstrass curve (it isn't - it's a Montgomery curve) and I don't think you want to support the not-really-standardized wei25519 unless unexpectedly some token using it becomes popular. I have never seen implicitlyCA used in the wild (the idea is that parameters are inherited from the CA) and they label it as MUST NOT use, so only supporting oId and curveName seems the right call. And adding further string variants will be easy enough if such tokens show up. See https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/cs01/pkcs11-curr-v3.0-cs01.html#_Toc30061180 > + 0x06, 0x03, 0x2b, 0x65, 0x70, > + }; /* OBJECT_IDENTIFIER { 1.3.101.112 } */ > + > + memset(&key_attr, 0, sizeof(key_attr)); > + key_attr[0].type = CKA_ID; > + key_attr[1].type = CKA_EC_POINT; /* XXX or CKA_VALUE ? */ > + key_attr[2].type = CKA_EC_PARAMS; > + > + session = p->slotinfo[slotidx].session; > + f = p->function_list; > + > + /* figure out size of the attributes */ > + rv = f->C_GetAttributeValue(session, *obj, key_attr, 3); > + if (rv != CKR_OK) { > + error("C_GetAttributeValue failed: %lu", rv); > + return (NULL); > + } > + > + /* > + * Allow CKA_ID (always first attribute) to be empty, but > + * ensure that none of the others are zero length. > + * XXX assumes CKA_ID is always first. > + */ > + if (key_attr[1].ulValueLen == 0 || > + key_attr[2].ulValueLen == 0) { > + error("invalid attribute length"); > + return (NULL); > + } > + > + /* allocate buffers for attributes */ > + for (i = 0; i < 3; i++) { > + if (key_attr[i].ulValueLen > 0) > + key_attr[i].pValue = xcalloc(1, key_attr[i].ulValueLen); > + } > + > + /* retrieve ID, public point and curve parameters of EC key */ > + rv = f->C_GetAttributeValue(session, *obj, key_attr, 3); > + if (rv != CKR_OK) { > + error("C_GetAttributeValue failed: %lu", rv); > + goto fail; > + } > + > + /* Expect one of the supported oids in CKA_EC_PARAMS */ As noted above, oids is the wrong word. "supported alternatives" > + d = (u_char *)key_attr[2].pValue; > + len = key_attr[2].ulValueLen; > + if ((len != sizeof(oid1) || memcmp(d, oid1, sizeof(oid1)) != 0) && > + (len != sizeof(oid2) || memcmp(d, oid2, sizeof(oid2)) != 0)) { > + hex = tohex(d, len); > + logit_f("unsupported CKA_EC_PARAMS: %s (len %zu)", hex, len); > + goto fail; > + } > + > + /* > + * Expect an either a raw 32 byte pubkey or an OCTET_STRING with s/Expect an/Expect/ OCTET STRING is usually written without _ > + * a 32 byte pubkey in CKA_VALUE > + */ > + d = (u_char *)key_attr[1].pValue; > + len = key_attr[1].ulValueLen; > + if (len == ED25519_PK_SZ + 2 && d[0] == 0x04 && d[1] == ED25519_PK_SZ) { > + d += 2; > + len -= 2; > + } > + if (len != ED25519_PK_SZ) { > + hex = tohex(key_attr[1].pValue, key_attr[1].ulValueLen); > + logit_f("CKA_EC_POINT invalid octet str: %s (len %lu)", > + hex, (u_long)key_attr[1].ulValueLen); > + goto fail; > + } > + > + if ((key = sshkey_new(KEY_UNSPEC)) == NULL) > + fatal_f("sshkey_new failed"); > + key->ed25519_pk = xmalloc(ED25519_PK_SZ); > + memcpy(key->ed25519_pk, d, ED25519_PK_SZ); > + key->type = KEY_ED25519; > + key->flags |= SSHKEY_FLAG_EXT; > + if (pkcs11_record_key(p, slotidx, &key_attr[0], key)) > + goto fail; > + /* success */ > + success = 0; > + fail: > + if (success != 0) { > + sshkey_free(key); > + key = NULL; > + } > + free(hex); > + for (i = 0; i < 3; i++) > + free(key_attr[i].pValue); > + return key; > +} > + > static int > pkcs11_fetch_x509_pubkey(struct pkcs11_provider *p, CK_ULONG slotidx, > CK_OBJECT_HANDLE *obj, struct sshkey **keyp, char **labelp) > @@ -1026,6 +1198,7 @@ pkcs11_fetch_x509_pubkey(struct pkcs11_provider *p, CK_ULONG slotidx, > int r, i, nid, success = -1; > const u_char *cp; > char *subject = NULL; > + size_t len; > > *keyp = NULL; > *labelp = NULL; > @@ -1158,6 +1331,26 @@ pkcs11_fetch_x509_pubkey(struct pkcs11_provider *p, CK_ULONG slotidx, > goto out; > /* success */ > success = 0; > + } else if (EVP_PKEY_base_id(evp) == EVP_PKEY_ED25519) { > + if ((key = sshkey_new(KEY_UNSPEC)) == NULL || > + (key->ed25519_pk = calloc(1, ED25519_PK_SZ)) == NULL) > + fatal_f("allocation failed"); > + len = ED25519_PK_SZ; > + if (!EVP_PKEY_get_raw_public_key(evp, key->ed25519_pk, &len)) { > + ossl_error("EVP_PKEY_get_raw_public_key failed"); > + goto out; > + } > + if (len != ED25519_PK_SZ) { > + error_f("incorrect returned public key " > + "length for ed25519"); > + goto out; > + } > + key->type = KEY_ED25519; > + key->flags |= SSHKEY_FLAG_EXT; > + if (pkcs11_record_key(p, slotidx, &cert_attr[0], key)) > + goto out; > + /* success */ > + success = 0; > } else { > error("unknown certificate key type"); > goto out; > @@ -1383,6 +1576,9 @@ pkcs11_fetch_keys(struct pkcs11_provider *p, CK_ULONG slotidx, > case CKK_ECDSA: > key = pkcs11_fetch_ecdsa_pubkey(p, slotidx, &obj); > break; > + case CKK_EC_EDWARDS: > + key = pkcs11_fetch_ed25519_pubkey(p, slotidx, &obj); > + break; > default: > /* XXX print key type? */ > key = NULL; > @@ -1852,6 +2048,10 @@ pkcs11_sign(struct sshkey *key, > case KEY_ECDSA_CERT: > return pkcs11_sign_ecdsa(key, sigp, lenp, data, datalen, > alg, sk_provider, sk_pin, compat); > + case KEY_ED25519: > + case KEY_ED25519_CERT: > + return pkcs11_sign_ed25519(key, sigp, lenp, data, datalen, > + alg, sk_provider, sk_pin, compat); > default: > return SSH_ERR_KEY_TYPE_UNKNOWN; > } > @@ -2006,16 +2206,27 @@ pkcs11_destroy_keypair(char *provider_id, char *pin, unsigned long slotidx, > *err = rv; > key_type = -1; > } > - if (key_type == CKK_RSA) > + switch (key_type) { > + case CKK_RSA: > k = pkcs11_fetch_rsa_pubkey(p, slotidx, &obj); > - else if (key_type == CKK_ECDSA) > + break; > + case CKK_ECDSA: > k = pkcs11_fetch_ecdsa_pubkey(p, slotidx, &obj); > + break; > + case CKK_EC_EDWARDS: > + k = pkcs11_fetch_ed25519_pubkey(p, slotidx, &obj); > + break; > + default: > + debug_f("unsupported key type %lu", (u_long)key_type); > + continue; > + } > > if ((rv = f->C_DestroyObject(session, obj)) != CKR_OK) { > debug_f("could not destroy public key 0x%hhx", keyid); > *err = rv; > goto out; > } > + sshkey_free(k); It probably doesn't matter since the function is unused (unless I'm missing something), but this doesn't look right. This function returns k afterward (why?), so a caller would UAF. > } > > out: >