| Message ID | 20220125025128.2117-3-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,1/3] xkey: Use a custom error level for debug messages | expand |
Am 25.01.22 um 03:51 schrieb selva.nair@gmail.com: > From: Selva Nair <selva.nair@gmail.com> > > - Call pkcs11h_certificate_signAny_ex() when available > so that the signature mechanism parameters can be pased. > (Required for RSA-PSS signature). > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > --- > src/openvpn/pkcs11_openssl.c | 123 +++++++++++++++++++++++++++++++++-- > 1 file changed, 118 insertions(+), 5 deletions(-) > > diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c > index 9cf46b2c..5d1a5de6 100644 > --- a/src/openvpn/pkcs11_openssl.c > +++ b/src/openvpn/pkcs11_openssl.c > @@ -45,10 +45,112 @@ > #ifdef HAVE_XKEY_PROVIDER > static XKEY_EXTERNAL_SIGN_fn xkey_pkcs11h_sign; > > +#if PKCS11H_VERSION > ((1<<16) | (27<<8)) /* version > 1.27 */ > + > +/* Table linking OpenSSL digest NID with CKM and CKG constants in PKCS#11 */ > +#define MD_TYPE(n) {NID_sha##n, CKM_SHA##n, CKG_MGF1_SHA##n} > +static const struct > +{ > + int nid; > + unsigned long ckm_id; > + unsigned long mgf_id; > +} mdtypes[] = {MD_TYPE(224), MD_TYPE(256), MD_TYPE(384), MD_TYPE(512), > + {NID_sha1, CKM_SHA_1, CKG_MGF1_SHA1}, /* SHA_1 naming is an oddity */ > + {NID_undef, 0, 0}}; > + > +/* From sigalg, derive parameters for pss signature and fill in pss_params. > + * Its of type CK_RSA_PKCS_PSS_PARAMS struct with three fields to be filled in: > + * {enum hashAlg, enum mgf, ulong sLen} > + * where hashAlg is CKM_SHA256 etc., mgf is CKG_MGF1_SHA256 etc. > + */ > +static int > +set_pss_params(CK_RSA_PKCS_PSS_PARAMS *pss_params, XKEY_SIGALG sigalg, > + pkcs11h_certificate_t cert) > +{ > + int ret = 0; > + X509 *x509 = NULL; > + EVP_PKEY *pubkey = NULL; > + > + if ((x509 = pkcs11h_openssl_getX509(cert)) == NULL > + || (pubkey = X509_get0_pubkey(x509)) == NULL) > + { > + msg(M_WARN, "PKCS#11: Unable get public key"); > + goto cleanup; > + } > + > + /* map mdname to CKM and CKG constants for hash and mgf algorithms */ > + int i = 0; > + int nid = OBJ_sn2nid(sigalg.mdname); > + while (mdtypes[i].nid != NID_undef && mdtypes[i].nid != nid) > + { > + i++; > + } > + pss_params->hashAlg = mdtypes[i].ckm_id; > + pss_params->mgf = mdtypes[i].mgf_id; > + > + /* determine salt length */ > + int mdsize = EVP_MD_size(EVP_get_digestbyname(sigalg.mdname)); This will break for newer hashes since it relies on nids but we have a fixed table anyway, it will break before that. But maybe we should bail out if we cannot find an entry in the translation table? Or is EVP_MD_size(NID_undef) well defined? Arne
I'm not claiming to understand any of this... so I have test compiled
on Linux/3.0.1, and also bumped my MinGW test rig to pkcs11-helper 1.28
(though only 1.1.1 yet). So maybe I tested something useful, or maybe
not... but at least these combinations still compile fine :-)
Your patch has been applied to the master branch.
commit 15ba808503113f685d254e9007b8e9937e01532d
Author: Selva Nair
Date: Mon Jan 24 21:51:28 2022 -0500
Support PSS signing using pkcs11-helper >= 1.28
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20220125025128.2117-3-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23647.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, On Wed, Jan 26, 2022 at 02:26:46PM +0100, Gert Doering wrote: > I'm not claiming to understand any of this... so I have test compiled > on Linux/3.0.1, and also bumped my MinGW test rig to pkcs11-helper 1.28 > (though only 1.1.1 yet). So maybe I tested something useful, or maybe > not... but at least these combinations still compile fine :-) > > Your patch has been applied to the master branch. Oops. I did not look closely enough, saw "Arne ACKed 1/3 and 2/3, and this one is complex, compiles fine, and Arne also sent something in reply" but it turns out it was not an ACK but concerns. (*If* it breaks, it will break for very cases unknown today, so it's not as bad as this sounds, but still, meh) Sorry. Selva, can you send a followup patch if you share Arne's concerns here? gert
On Wed, Jan 26, 2022 at 6:50 AM Arne Schwabe <arne@rfc2549.org> wrote: > Am 25.01.22 um 03:51 schrieb selva.nair@gmail.com: > > From: Selva Nair <selva.nair@gmail.com> > > > > - Call pkcs11h_certificate_signAny_ex() when available > > so that the signature mechanism parameters can be pased. > > (Required for RSA-PSS signature). > > > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > > --- > > src/openvpn/pkcs11_openssl.c | 123 +++++++++++++++++++++++++++++++++-- > > 1 file changed, 118 insertions(+), 5 deletions(-) > > > > diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c > > index 9cf46b2c..5d1a5de6 100644 > > --- a/src/openvpn/pkcs11_openssl.c > > +++ b/src/openvpn/pkcs11_openssl.c > > @@ -45,10 +45,112 @@ > > #ifdef HAVE_XKEY_PROVIDER > > static XKEY_EXTERNAL_SIGN_fn xkey_pkcs11h_sign; > > > > +#if PKCS11H_VERSION > ((1<<16) | (27<<8)) /* version > 1.27 */ > > + > > +/* Table linking OpenSSL digest NID with CKM and CKG constants in > PKCS#11 */ > > +#define MD_TYPE(n) {NID_sha##n, CKM_SHA##n, CKG_MGF1_SHA##n} > > +static const struct > > +{ > > + int nid; > > + unsigned long ckm_id; > > + unsigned long mgf_id; > > +} mdtypes[] = {MD_TYPE(224), MD_TYPE(256), MD_TYPE(384), MD_TYPE(512), > > + {NID_sha1, CKM_SHA_1, CKG_MGF1_SHA1}, /* SHA_1 naming is > an oddity */ > > + {NID_undef, 0, 0}}; > > + > > +/* From sigalg, derive parameters for pss signature and fill in > pss_params. > > + * Its of type CK_RSA_PKCS_PSS_PARAMS struct with three fields to be > filled in: > > + * {enum hashAlg, enum mgf, ulong sLen} > > + * where hashAlg is CKM_SHA256 etc., mgf is CKG_MGF1_SHA256 etc. > > + */ > > +static int > > +set_pss_params(CK_RSA_PKCS_PSS_PARAMS *pss_params, XKEY_SIGALG sigalg, > > + pkcs11h_certificate_t cert) > > +{ > > + int ret = 0; > > + X509 *x509 = NULL; > > + EVP_PKEY *pubkey = NULL; > > + > > + if ((x509 = pkcs11h_openssl_getX509(cert)) == NULL > > + || (pubkey = X509_get0_pubkey(x509)) == NULL) > > + { > > + msg(M_WARN, "PKCS#11: Unable get public key"); > > + goto cleanup; > > + } > > + > > + /* map mdname to CKM and CKG constants for hash and mgf algorithms > */ > > + int i = 0; > > + int nid = OBJ_sn2nid(sigalg.mdname); > > + while (mdtypes[i].nid != NID_undef && mdtypes[i].nid != nid) > > + { > > + i++; > > + } > > + pss_params->hashAlg = mdtypes[i].ckm_id; > > + pss_params->mgf = mdtypes[i].mgf_id; > > + > > + /* determine salt length */ > > + int mdsize = EVP_MD_size(EVP_get_digestbyname(sigalg.mdname)); > > This will break for newer hashes since it relies on nids but we have a > fixed table anyway, it will break before that. But maybe we should bail > out if we cannot find an entry in the translation table? Or is > EVP_MD_size(NID_undef) well defined? > EVP_get_digestbyname(mdname) will return NULL if mdname is not recognized (or has no NID etc), that when passed to EVP_MD_size(NULL) will trigger an error and return -1 (based on OpenSSL source). So, yes, mdsize will be wrong (-1), but we will catch it below where we check whether NID was found in the table. Not ideal, and you say, may be better to error out earlier if NID is NID_undef or not in our table, or if EVP_get_digestbyname() returns NULL. Selva P.S. If the commit is not pushed yet, I can send a v2 for review, or follow up with a fixup. <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 26, 2022 at 6:50 AM Arne Schwabe <<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Am 25.01.22 um 03:51 schrieb <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>:<br> > From: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>><br> > <br> > - Call pkcs11h_certificate_signAny_ex() when available<br> > so that the signature mechanism parameters can be pased.<br> > (Required for RSA-PSS signature).<br> > <br> > Signed-off-by: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>><br> > ---<br> > src/openvpn/pkcs11_openssl.c | 123 +++++++++++++++++++++++++++++++++--<br> > 1 file changed, 118 insertions(+), 5 deletions(-)<br> > <br> > diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c<br> > index 9cf46b2c..5d1a5de6 100644<br> > --- a/src/openvpn/pkcs11_openssl.c<br> > +++ b/src/openvpn/pkcs11_openssl.c<br> > @@ -45,10 +45,112 @@<br> > #ifdef HAVE_XKEY_PROVIDER<br> > static XKEY_EXTERNAL_SIGN_fn xkey_pkcs11h_sign;<br> > <br> > +#if PKCS11H_VERSION > ((1<<16) | (27<<8)) /* version > 1.27 */<br> > +<br> > +/* Table linking OpenSSL digest NID with CKM and CKG constants in PKCS#11 */<br> > +#define MD_TYPE(n) {NID_sha##n, CKM_SHA##n, CKG_MGF1_SHA##n}<br> > +static const struct<br> > +{<br> > + int nid;<br> > + unsigned long ckm_id;<br> > + unsigned long mgf_id;<br> > +} mdtypes[] = {MD_TYPE(224), MD_TYPE(256), MD_TYPE(384), MD_TYPE(512),<br> > + {NID_sha1, CKM_SHA_1, CKG_MGF1_SHA1}, /* SHA_1 naming is an oddity */<br> > + {NID_undef, 0, 0}};<br> > +<br> > +/* From sigalg, derive parameters for pss signature and fill in pss_params.<br> > + * Its of type CK_RSA_PKCS_PSS_PARAMS struct with three fields to be filled in:<br> > + * {enum hashAlg, enum mgf, ulong sLen}<br> > + * where hashAlg is CKM_SHA256 etc., mgf is CKG_MGF1_SHA256 etc.<br> > + */<br> > +static int<br> > +set_pss_params(CK_RSA_PKCS_PSS_PARAMS *pss_params, XKEY_SIGALG sigalg,<br> > + pkcs11h_certificate_t cert)<br> > +{<br> > + int ret = 0;<br> > + X509 *x509 = NULL;<br> > + EVP_PKEY *pubkey = NULL;<br> > +<br> > + if ((x509 = pkcs11h_openssl_getX509(cert)) == NULL<br> > + || (pubkey = X509_get0_pubkey(x509)) == NULL)<br> > + {<br> > + msg(M_WARN, "PKCS#11: Unable get public key");<br> > + goto cleanup;<br> > + }<br> > +<br> > + /* map mdname to CKM and CKG constants for hash and mgf algorithms */<br> > + int i = 0;<br> > + int nid = OBJ_sn2nid(sigalg.mdname);<br> > + while (mdtypes[i].nid != NID_undef && mdtypes[i].nid != nid)<br> > + {<br> > + i++;<br> > + }<br> > + pss_params->hashAlg = mdtypes[i].ckm_id;<br> > + pss_params->mgf = mdtypes[i].mgf_id;<br> > +<br> > + /* determine salt length */<br> > + int mdsize = EVP_MD_size(EVP_get_digestbyname(sigalg.mdname));<br> <br> This will break for newer hashes since it relies on nids but we have a <br> fixed table anyway, it will break before that. But maybe we should bail <br> out if we cannot find an entry in the translation table? Or is <br> EVP_MD_size(NID_undef) well defined?<br></blockquote><div><br></div><div>EVP_get_digestbyname(mdname) will return NULL if mdname is not recognized (or has no NID etc), that when passed to EVP_MD_size(NULL) will trigger an error and return -1 (based on OpenSSL source). So, yes, mdsize will be wrong (-1), but we will catch it below where we check whether NID was found in the table.</div><div><br></div><div>Not ideal, and you say, may be better to error out earlier if NID is NID_undef or not in our table, or if EVP_get_digestbyname() returns NULL.</div><div><br></div><div>Selva </div><div>P.S. If the commit is not pushed yet, I can send a v2 for review, or follow up with a fixup.</div></div></div>
Hi, On Wed, Jan 26, 2022 at 09:23:29AM -0500, Selva Nair wrote: > P.S. If the commit is not pushed yet, I can send a v2 for review, or follow > up with a fixup. I only noticed it after pushing, so yes, messed-up. Sorry again. gert
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c index 9cf46b2c..5d1a5de6 100644 --- a/src/openvpn/pkcs11_openssl.c +++ b/src/openvpn/pkcs11_openssl.c @@ -45,10 +45,112 @@ #ifdef HAVE_XKEY_PROVIDER static XKEY_EXTERNAL_SIGN_fn xkey_pkcs11h_sign; +#if PKCS11H_VERSION > ((1<<16) | (27<<8)) /* version > 1.27 */ + +/* Table linking OpenSSL digest NID with CKM and CKG constants in PKCS#11 */ +#define MD_TYPE(n) {NID_sha##n, CKM_SHA##n, CKG_MGF1_SHA##n} +static const struct +{ + int nid; + unsigned long ckm_id; + unsigned long mgf_id; +} mdtypes[] = {MD_TYPE(224), MD_TYPE(256), MD_TYPE(384), MD_TYPE(512), + {NID_sha1, CKM_SHA_1, CKG_MGF1_SHA1}, /* SHA_1 naming is an oddity */ + {NID_undef, 0, 0}}; + +/* From sigalg, derive parameters for pss signature and fill in pss_params. + * Its of type CK_RSA_PKCS_PSS_PARAMS struct with three fields to be filled in: + * {enum hashAlg, enum mgf, ulong sLen} + * where hashAlg is CKM_SHA256 etc., mgf is CKG_MGF1_SHA256 etc. + */ +static int +set_pss_params(CK_RSA_PKCS_PSS_PARAMS *pss_params, XKEY_SIGALG sigalg, + pkcs11h_certificate_t cert) +{ + int ret = 0; + X509 *x509 = NULL; + EVP_PKEY *pubkey = NULL; + + if ((x509 = pkcs11h_openssl_getX509(cert)) == NULL + || (pubkey = X509_get0_pubkey(x509)) == NULL) + { + msg(M_WARN, "PKCS#11: Unable get public key"); + goto cleanup; + } + + /* map mdname to CKM and CKG constants for hash and mgf algorithms */ + int i = 0; + int nid = OBJ_sn2nid(sigalg.mdname); + while (mdtypes[i].nid != NID_undef && mdtypes[i].nid != nid) + { + i++; + } + pss_params->hashAlg = mdtypes[i].ckm_id; + pss_params->mgf = mdtypes[i].mgf_id; + + /* determine salt length */ + int mdsize = EVP_MD_size(EVP_get_digestbyname(sigalg.mdname)); + + int saltlen = -1; + if (!strcmp(sigalg.saltlen, "digest")) /* same as digest size */ + { + saltlen = mdsize; + } + else if (!strcmp(sigalg.saltlen, "max")) /* maximum possible value */ + { + saltlen = xkey_max_saltlen(EVP_PKEY_get_bits(pubkey), mdsize); + } + + if (saltlen < 0 || pss_params->hashAlg == 0) + { + msg(M_WARN, "WARN: invalid RSA_PKCS1_PSS parameters: saltlen = <%s> " + "mdname = <%s>.", sigalg.saltlen, sigalg.mdname); + goto cleanup; + } + pss_params->sLen = (unsigned long) saltlen; /* saltlen >= 0 at this point */ + + msg(D_XKEY, "set_pss_params: sLen = %lu, hashAlg = %lu, mgf = %lu", + pss_params->sLen, pss_params->hashAlg, pss_params->mgf); + + ret = 1; + +cleanup: + if (x509) + { + X509_free(x509); + } + return ret; +} + +#else + +/* Make set_pss_params a no-op that always succeeds */ +#define set_pss_params(...) (1) + +/* Use a wrapper for pkcs11h_certificate_signAny_ex() for versions < 1.28 + * where its not available. + * We just call pkcs11h_certificate_signAny() unless the padding + * is PSS in which case we return an error. + */ +static CK_RV +pkcs11h_certificate_signAny_ex(const pkcs11h_certificate_t cert, + const CK_MECHANISM *mech, const unsigned char *tbs, + size_t tbslen, unsigned char *sig, size_t *siglen) +{ + if (mech->mechanism == CKM_RSA_PKCS_PSS) + { + msg(M_NONFATAL, "PKCS#11: Error: PSS padding is not supported by " + "this version of pkcs11-helper library."); + return CKR_MECHANISM_INVALID; + } + return pkcs11h_certificate_signAny(cert, mech->mechanism, tbs, tbslen, sig, siglen); +} +#endif /* PKCS11H_VERSION > 1.27 */ + /** * Sign op called from xkey provider * - * We support ECDSA, RSA_NO_PADDING, RSA_PKCS1_PADDING + * We support ECDSA, RSA_NO_PADDING, RSA_PKCS1_PADDING, RSA_PKCS_PSS_PADDING */ static int xkey_pkcs11h_sign(void *handle, unsigned char *sig, @@ -62,7 +164,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig, if (!strcmp(sigalg.op, "DigestSign")) { - dmsg(D_LOW, "xkey_pkcs11h_sign: computing digest"); + msg(D_XKEY, "xkey_pkcs11h_sign: computing digest"); if (xkey_digest(tbs, tbslen, buf, &buflen, sigalg.mdname)) { tbs = buf; @@ -77,18 +179,29 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig, if (!strcmp(sigalg.keytype, "EC")) { + msg(D_XKEY, "xkey_pkcs11h_sign: signing with EC key"); mech.mechanism = CKM_ECDSA; } else if (!strcmp(sigalg.keytype, "RSA")) { + msg(D_XKEY, "xkey_pkcs11h_sign: signing with RSA key: padmode = %s", + sigalg.padmode); if (!strcmp(sigalg.padmode,"none")) { mech.mechanism = CKM_RSA_X_509; } else if (!strcmp(sigalg.padmode, "pss")) { - msg(M_NONFATAL, "PKCS#11: Error: PSS padding is not yet supported."); - return 0; + CK_RSA_PKCS_PSS_PARAMS pss_params = {0}; + mech.mechanism = CKM_RSA_PKCS_PSS; + + if (!set_pss_params(&pss_params, sigalg, cert)) + { + return 0; + } + + mech.pParameter = &pss_params; + mech.ulParameterLen = sizeof(pss_params); } else if (!strcmp(sigalg.padmode, "pkcs1")) { @@ -114,7 +227,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig, ASSERT(0); /* coding error -- we couldnt have created any such key */ } - return CKR_OK == pkcs11h_certificate_signAny(cert, mech.mechanism, + return CKR_OK == pkcs11h_certificate_signAny_ex(cert, &mech, tbs, tbslen, sig, siglen); }