On Wed, Jan 26, 2022 at 6:50 AM Arne Schwabe <a...@rfc2549.org> wrote:
> Am 25.01.22 um 03:51 schrieb selva.n...@gmail.com:
> > From: Selva Nair <selva.n...@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.n...@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.
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
