Skip to content
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.

Sign up for GitHub

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

Some fixes for EdDSA signatures + test coverage #497

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Some fixes for EdDSA signatures + test coverage #497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
38de0a4
tests: Check signature also using eddsa keys in tsession
Jakuje Jan 9, 2025
f7dd420
tests: Test signature on generated eddsa keys
Jakuje Jan 9, 2025
92f1cda
tests: Test different EdDSA signature types
Jakuje Jan 9, 2025
c5a90de
signature: ED448 requires parameter even if none is provided by OpenSSL
Jakuje Jan 9, 2025
b634bf0
encoder: Fix printing Ed448 keys
Jakuje Jan 10, 2025
b5e98b6
tests: Generate Ed448 keys if supported
Jakuje Jan 10, 2025
f7fbb4d
objects: Fix EdDSA key comparison
Jakuje Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/encoder.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,7 @@ static int p11prov_ec_edwards_encoder_encode_text(
P11PROV_OBJ *key = (P11PROV_OBJ *)inkey;
CK_KEY_TYPE type;
CK_ULONG keysize;
const char *type_name = "ED25519";
const char *type_name = ED25519;
char *uri = NULL;
BIO *out;
int ret;
Expand All @@ -990,8 +990,8 @@ static int p11prov_ec_edwards_encoder_encode_text(
}

keysize = p11prov_obj_get_key_bit_size(key);
if (keysize == 448) {
type_name = "ED448";
if (keysize == ED448_BIT_SIZE) {
type_name = ED448;
}
if (selection & OSSL_KEYMGMT_SELECT_PRIVATE_KEY) {
CK_OBJECT_CLASS class = p11prov_obj_get_class(key);
Expand Down
70 changes: 69 additions & 1 deletion src/objects.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2477,6 +2477,31 @@ static int match_public_keys(P11PROV_OBJ *key1, P11PROV_OBJ *key2)
return ret;
}

static int p11prov_obj_get_ed_nid(CK_ATTRIBUTE *ecp)
{
const unsigned char *val = ecp->pValue;
ASN1_OBJECT *obj = d2i_ASN1_OBJECT(NULL, &val, ecp->ulValueLen);
if (obj) {
int nid = OBJ_obj2nid(obj);
ASN1_OBJECT_free(obj);
if (nid != NID_undef) {
return nid;
}
}

/* it might be the parameters are encoded printable string
* for EdDSA which OpenSSL does not understand */
if (ecp->ulValueLen == ED25519_EC_PARAMS_LEN
&& memcmp(ecp->pValue, ed25519_ec_params, ED25519_EC_PARAMS_LEN) == 0) {
return NID_ED25519;
} else if (ecp->ulValueLen == ED448_EC_PARAMS_LEN
&& memcmp(ecp->pValue, ed448_ec_params, ED448_EC_PARAMS_LEN)
== 0) {
return NID_ED448;
}
return NID_undef;
}

int p11prov_obj_key_cmp(P11PROV_OBJ *key1, P11PROV_OBJ *key2, CK_KEY_TYPE type,
int cmp_type)
{
Expand Down Expand Up @@ -2535,7 +2560,6 @@ int p11prov_obj_key_cmp(P11PROV_OBJ *key1, P11PROV_OBJ *key2, CK_KEY_TYPE type,
break;

case CKK_EC:
case CKK_EC_EDWARDS:
ret = cmp_attr(key1, key2, CKA_EC_PARAMS);
if (ret != RET_OSSL_OK) {
/* If EC_PARAMS do not match it may be due to encoding.
Expand Down Expand Up @@ -2604,6 +2628,50 @@ int p11prov_obj_key_cmp(P11PROV_OBJ *key1, P11PROV_OBJ *key2, CK_KEY_TYPE type,
cmp_type = OBJ_CMP_KEY_PUBLIC;
}
break;
case CKK_EC_EDWARDS:
/* The EdDSA params can be encoded as printable string, which is
* not recognized by OpenSSL and does not have respective EC_GROUP */
ret = cmp_attr(key1, key2, CKA_EC_PARAMS);
if (ret != RET_OSSL_OK) {
/* If EC_PARAMS do not match it may be due to encoding. */
CK_ATTRIBUTE *ec_p;
int nid1;
int nid2;

ec_p = p11prov_obj_get_attr(key1, CKA_EC_PARAMS);
if (!ec_p) {
return RET_OSSL_ERR;
}
nid1 = p11prov_obj_get_ed_nid(ec_p);
if (nid1 == NID_undef) {
return RET_OSSL_ERR;
}

ec_p = p11prov_obj_get_attr(key2, CKA_EC_PARAMS);
if (!ec_p) {
return RET_OSSL_ERR;
}
nid2 = p11prov_obj_get_ed_nid(ec_p);
if (nid2 == NID_undef) {
return RET_OSSL_ERR;
}
if (nid1 != nid2) {
return RET_OSSL_ERR;
}
}
if (cmp_type & OBJ_CMP_KEY_PRIVATE) {
/* unfortunately we can't really read private attributes
* and there is no comparison function int he PKCS11 API.
* Generally you do not have 2 identical keys stored in to two
* separate objects so the initial shortcircuit that matches if
* slotid/handle are identical will often cover this. When that
* fails we have no option but to fail for now. */
P11PROV_debug("We can't really match private keys");
/* OTOH if group and pub point match either this is a broken key
* or the private key must also match */
cmp_type = OBJ_CMP_KEY_PUBLIC;
}
break;

default:
return RET_OSSL_ERR;
Expand Down
22 changes: 15 additions & 7 deletions src/signature.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2290,10 +2290,24 @@ static int p11prov_eddsa_set_ctx_params(void *ctx, const OSSL_PARAM params[])
{
P11PROV_SIG_CTX *sigctx = (P11PROV_SIG_CTX *)ctx;
const OSSL_PARAM *p;
CK_ULONG size;
int ret;

P11PROV_debug("eddsa set ctx params (ctx=%p, params=%p)", sigctx, params);

size = p11prov_obj_get_key_bit_size(sigctx->key);
if (size != ED25519_BIT_SIZE && size != ED448_BIT_SIZE) {
P11PROV_raise(sigctx->provctx, CKR_KEY_TYPE_INCONSISTENT,
"Invalid EdDSA key size %lu", size);
return RET_OSSL_ERR;
}

/* PKCS #11 parameters are mandatory for Ed448 key type anyway */
if (size == ED448_BIT_SIZE) {
sigctx->use_eddsa_params = CK_TRUE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is correct, rather we should select an instance, presumably based on the default and bit lengths and then call through the same code at lines 2322 onward.

What are you doing here is effectively setting

    sigctx->use_eddsa_params = CK_TRUE;
    sigctx->eddsa_params.phFlag = CK_FALSE;

by omitting to set the phFlag.

I think the correct fix here involves spinning off the instance selection to a helper function, and if no params are set, calling it with "Ed25519" is size == ED25519_BIT_SIZE and with "Ed448" if ED448_BIT_SIZE

An option is to also pre-set instance constants, then if there are params they will overwrite the instance variable with whatever came from the OSSL_SIGNATURE_PARAM_INSTANCE

Once all params are handled, unconditionally call the cose that sets the various flags based on instance name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, check the table in the pkcs#11 specs:
https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.html#_Toc30061191

What are you doing here is effectively setting

this is intentional. The phFlag is false in this default default. I can set it explicitly if it will make it more clear

Missing instance name on the openssl level results in Ed448/Ed25519. I find it overcomplicated going through the openssl instance if we do not have any from the caller. The code from lines 2322 just really sets use_eddsa_params and phFlag. Moreover we return on the line 2311 when there are no parameters provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a side effect, I anted it explicit, and that can be done easily, I can do it as a followup PR if you like

sigctx->eddsa_params.phFlag = CK_FALSE;
}

if (params == NULL) {
return RET_OSSL_OK;
}
Expand All @@ -2302,20 +2316,13 @@ static int p11prov_eddsa_set_ctx_params(void *ctx, const OSSL_PARAM params[])
if (p) {
const char *instance = NULL;
bool matched = false;
CK_ULONG size;

ret = OSSL_PARAM_get_utf8_string_ptr(p, &instance);
if (ret != RET_OSSL_OK) {
return ret;
}
P11PROV_debug("Set OSSL_SIGNATURE_PARAM_INSTANCE to %s", instance);

size = p11prov_obj_get_key_bit_size(sigctx->key);
if (size != ED25519_BIT_SIZE && size != ED448_BIT_SIZE) {
P11PROV_raise(sigctx->provctx, CKR_KEY_TYPE_INCONSISTENT,
"Invalid EdDSA key size %lu", size);
return RET_OSSL_ERR;
}
if (size == ED25519_BIT_SIZE) {
if (OPENSSL_strcasecmp(instance, "Ed25519") == 0) {
matched = true;
Expand Down Expand Up @@ -2359,6 +2366,7 @@ static int p11prov_eddsa_set_ctx_params(void *ctx, const OSSL_PARAM params[])
return ret;
}
sigctx->eddsa_params.ulContextDataLen = datalen;
sigctx->use_eddsa_params = CK_TRUE;
}

return RET_OSSL_OK;
Expand Down
70 changes: 43 additions & 27 deletions tests/setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,34 +232,38 @@ if [ "${TOKENTYPE}" != "softokn" ]; then
echo "${EDPUBURI}"
echo "${EDPRIURI}"
echo "${EDCRTURI}"
fi

# FIXME The pkcs11-tool before OpenSC 0.26 does not support Ed448 so they can
# not be generated here
#
# generate ED448
#KEYID='0009'
#URIKEYID="%00%09"
#ED2CRTN="ed2Cert"
#
# pkcs11-tool "${P11DEFARGS[@]}" --keypairgen --key-type="EC:edwards448" \
# --label="${ED2CRTN}" --id="$KEYID"
# ca_sign $ED2CRTN "My ED448 Cert" $KEYID
#
# ED2BASEURIWITHPINVALUE="pkcs11:id=${URIKEYID};pin-value=${PINVALUE}"
# ED2BASEURIWITHPINSOURCE="pkcs11:id=${URIKEYID};pin-source=file:${PINFILE}"
# ED2BASEURI="pkcs11:id=${URIKEYID}"
# ED2PUBURI="pkcs11:type=public;id=${URIKEYID}"
# ED2PRIURI="pkcs11:type=private;id=${URIKEYID}"
# ED2CRTURI="pkcs11:type=cert;object=${ED2CRTN}"
#
# title LINE "ED448 PKCS11 URIS"
# echo "${EDBASEURIWITHPINVALUE}"
# echo "${EDBASEURIWITHPINSOURCE}"
# echo "${EDBASEURI}"
# echo "${EDPUBURI}"
# echo "${EDPRIURI}"
# echo "${EDCRTURI}"
# this requires OpenSC 0.26.0, which is not available in Ubuntu and CentOS 9
if [[ -f /etc/debian_version ]] && grep Ubuntu /etc/lsb-release; then
echo "Ed448 not supported in Ubuntu's OpenSC version"
elif [[ -f /etc/redhat-release ]] && grep "release 9" /etc/redhat-release; then
echo "Ed448 not supported in EL9's OpenSC version"
else
# generate ED448
KEYID='0009'
URIKEYID="%00%09"
ED2CRTN="ed2Cert"

pkcs11-tool "${P11DEFARGS[@]}" --keypairgen --key-type="EC:Ed448" \
--label="${ED2CRTN}" --id="$KEYID"
ca_sign $ED2CRTN "My ED448 Cert" $KEYID

ED2BASEURIWITHPINVALUE="pkcs11:id=${URIKEYID};pin-value=${PINVALUE}"
ED2BASEURIWITHPINSOURCE="pkcs11:id=${URIKEYID};pin-source=file:${PINFILE}"
ED2BASEURI="pkcs11:id=${URIKEYID}"
ED2PUBURI="pkcs11:type=public;id=${URIKEYID}"
ED2PRIURI="pkcs11:type=private;id=${URIKEYID}"
ED2CRTURI="pkcs11:type=cert;object=${ED2CRTN}"

title LINE "ED448 PKCS11 URIS"
echo "${ED2BASEURIWITHPINVALUE}"
echo "${ED2BASEURIWITHPINSOURCE}"
echo "${ED2BASEURI}"
echo "${ED2PUBURI}"
echo "${ED2PRIURI}"
echo "${ED2CRTURI}"
fi
fi

title PARA "generate RSA key pair, self-signed certificate, remove public key"
KEYID='0005'
Expand Down Expand Up @@ -454,6 +458,18 @@ export EDCRTURI="${EDCRTURI}"
DBGSCRIPT
fi

if [ -n "${ED2BASEURI}" ]; then
cat >> "${TMPPDIR}/testvars" <<DBGSCRIPT

export ED2BASEURIWITHPINVALUE="${ED2BASEURIWITHPINVALUE}"
export ED2BASEURIWITHPINSOURCE="${ED2BASEURIWITHPINSOURCE}"
export ED2BASEURI="${ED2BASEURI}"
export ED2PUBURI="${ED2PUBURI}"
export ED2PRIURI="${ED2PRIURI}"
export ED2CRTURI="${ED2CRTURI}"
DBGSCRIPT
fi

if [ -n "${ECXBASEURI}" ]; then
cat >> "${TMPPDIR}/testvars" <<DBGSCRIPT

Expand Down
34 changes: 32 additions & 2 deletions tests/tedwards
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ if [[ -n $ED2BASEURI ]]; then

title PARA "Test EVP_PKEY_eq on public ED448 key via import"
$CHECKER "${TESTBLDDIR}/tcmpkeys" "$ED2PUBURI" "${TMPPDIR}"/ed2out.pub
title PARA "Match private ED key against public key"
title PARA "Match private ED448 key against public key"
$CHECKER "${TESTBLDDIR}/tcmpkeys" "$ED2PRIURI" "${TMPPDIR}"/ed2out.pub
title PARA "Match private ED key against public key (commutativity)"
title PARA "Match private ED448 key against public key (commutativity)"
$CHECKER "${TESTBLDDIR}/tcmpkeys" "${TMPPDIR}"/ed2out.pub "$ED2PRIURI"
fi

Expand All @@ -121,4 +121,34 @@ if [ $FAIL -ne 0 ]; then
exit 1
fi

ORIG_OPENSSL_CONF=${OPENSSL_CONF}
sed "s/^pkcs11-module-token-pin.*$/##nopin/" "${OPENSSL_CONF}" > "${OPENSSL_CONF}.nopin"
OPENSSL_CONF=${OPENSSL_CONF}.nopin

title PARA "Test interactive Login on key without ALWAYS AUTHENTICATE"
# shellcheck disable=SC2153 # It is correctly defined in the testvars
output=$(expect -c "spawn -noecho $CHECKER ${TESTBLDDIR}/tsession \"$EDBASEURI\";
expect \"Enter PIN for PKCS#11 Token (Slot *:\" {
send \"${PINVALUE}\r\"; exp_continue; }
expect \"ALL A-OK\";")
FAIL=0
echo "$output" | grep "Enter PIN for PKCS#11 Token (Slot .*):" > /dev/null 2>&1 || FAIL=1
prompts=$(echo "$output" | grep -c "Enter PIN for PKCS#11 Token (Slot .*):" 2>&1)
# 1 login to read key only
if [ "$prompts" -ne "1" ]; then
echo "Failed receive expected amount of prompts (got $prompts, expected 1)"
FAIL=2
fi
if [ $FAIL -eq 1 ]; then
echo "Failed to obtain expected prompt"
fi
if [ $FAIL -ne 0 ]; then
echo
echo "Original command output:"
echo "$output"
echo
exit 1
fi
OPENSSL_CONF=${ORIG_OPENSSL_CONF}

exit 0
46 changes: 43 additions & 3 deletions tests/tgenkey.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,8 @@ static void gen_keys(const char *key_type, const char *label, const char *idhex,
OSSL_STORE_close(store);
}

static void sign_test(const char *label, bool fail)
static void sign_test(const char *label, const char *mdname,
const OSSL_PARAM *params, bool fail)
{
OSSL_STORE_CTX *store;
OSSL_STORE_SEARCH *search;
Expand All @@ -281,6 +282,8 @@ static void sign_test(const char *label, bool fail)
size_t siglen = 4096;
int ret;

fprintf(stdout, "Test signature\n");

store = OSSL_STORE_open("pkcs11:", NULL, NULL, NULL, NULL);
if (store == NULL) {
fprintf(stderr, "Failed to open pkcs11 store\n");
Expand Down Expand Up @@ -324,7 +327,8 @@ static void sign_test(const char *label, bool fail)
exit(EXIT_FAILURE);
}

ret = EVP_DigestSignInit(ctx, &pctx, EVP_sha256(), NULL, privkey);
ret =
EVP_DigestSignInit_ex(ctx, &pctx, mdname, NULL, NULL, privkey, params);
if (ret == 0) {
fprintf(stderr, "Failed to init Sig Ctx\n");
exit(EXIT_FAILURE);
Expand Down Expand Up @@ -434,6 +438,9 @@ int main(int argc, char *argv[])
params[2] = OSSL_PARAM_construct_end();

gen_keys("RSA", label, idhex, params, false);

sign_test(label, "SHA256", NULL, false);

free(label);
free(uri);

Expand Down Expand Up @@ -490,6 +497,9 @@ int main(int argc, char *argv[])
params[2] = OSSL_PARAM_construct_end();

gen_keys("EC", label, idhex, params, false);

sign_test(label, "SHA256", NULL, false);

free(label);
free(uri);

Expand Down Expand Up @@ -520,7 +530,7 @@ int main(int argc, char *argv[])

gen_keys("RSA", label, idhex, params, false);

sign_test(label, true);
sign_test(label, "SHA256", NULL, true);

params[1] = OSSL_PARAM_construct_utf8_string("pkcs11_key_usage",
(char *)bad_usage, 0);
Expand All @@ -531,6 +541,13 @@ int main(int argc, char *argv[])
free(uri);
} else if (strcmp(tests[num], "ED25519") == 0
|| strcmp(tests[num], "ED448") == 0) {
const char *context = "context string";
const char *instance = "Ed25519ph";

if (strcmp(tests[num], "ED448") == 0) {
instance = "Ed448ph";
}

ret = RAND_bytes(id, 16);
if (ret != 1) {
fprintf(stderr, "Failed to generate key id\n");
Expand All @@ -552,6 +569,29 @@ int main(int argc, char *argv[])
params[1] = OSSL_PARAM_construct_end();

gen_keys(tests[num], label, idhex, params, false);

sign_test(label, NULL, NULL, false);

/* these are not defined in OpenSSL 3.0 so just skip the test */
#ifdef OSSL_SIGNATURE_PARAM_CONTEXT_STRING
/* Test again with context string */
params[0] = OSSL_PARAM_construct_octet_string(
OSSL_SIGNATURE_PARAM_CONTEXT_STRING, (void *)context,
sizeof(context));
params[1] = OSSL_PARAM_construct_end();
sign_test(label, NULL, params, false);

/* Test again with prehash */
params[0] = OSSL_PARAM_construct_utf8_string(
OSSL_SIGNATURE_PARAM_INSTANCE, (char *)instance,
strlen(instance));
params[1] = OSSL_PARAM_construct_end();
sign_test(label, NULL, params, false);
#else
(void)instance;
(void)context;
#endif

free(label);
free(uri);
} else {
Expand Down
Loading
Loading