diff mbox series

[kmod] libkmod-signature: implement pkcs7 parsing with openssl

Message ID 20190125133808.27363-1-yauheni.kaliuta@redhat.com (mailing list archive)
State New, archived
Headers show
Series [kmod] libkmod-signature: implement pkcs7 parsing with openssl | expand

Commit Message

Yauheni Kaliuta Jan. 25, 2019, 1:38 p.m. UTC
The patch adds data fetching from the PKCS#7 certificate using
openssl library (which is used by scripts/sign-file.c in the linux
kernel to sign modules).

In general the certificate can contain many signatures, but since
kmod (modinfo) supports only one signature at the moment, only first
one is taken.

With the current sign-file.c certificate doesn't contain signer
key's fingerprint, so "serial number" is used for the key id.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 Makefile.am                 |   4 +-
 configure.ac                |  11 ++
 libkmod/libkmod-internal.h  |   3 +
 libkmod/libkmod-module.c    |   3 +
 libkmod/libkmod-signature.c | 197 +++++++++++++++++++++++++++++++++++-
 5 files changed, 213 insertions(+), 5 deletions(-)

Comments

Lucas De Marchi Jan. 25, 2019, 6:33 p.m. UTC | #1
On Fri, Jan 25, 2019 at 5:39 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> The patch adds data fetching from the PKCS#7 certificate using
> openssl library (which is used by scripts/sign-file.c in the linux
> kernel to sign modules).
>
> In general the certificate can contain many signatures, but since
> kmod (modinfo) supports only one signature at the moment, only first
> one is taken.
>
> With the current sign-file.c certificate doesn't contain signer
> key's fingerprint, so "serial number" is used for the key id.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>  Makefile.am                 |   4 +-
>  configure.ac                |  11 ++
>  libkmod/libkmod-internal.h  |   3 +
>  libkmod/libkmod-module.c    |   3 +
>  libkmod/libkmod-signature.c | 197 +++++++++++++++++++++++++++++++++++-
>  5 files changed, 213 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 1ab1db585316..de1026f8bd46 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -35,6 +35,8 @@ SED_PROCESS = \
>         -e 's,@liblzma_LIBS\@,${liblzma_LIBS},g' \
>         -e 's,@zlib_CFLAGS\@,${zlib_CFLAGS},g' \
>         -e 's,@zlib_LIBS\@,${zlib_LIBS},g' \
> +       -e 's,@openssl_CFLAGS\@,${openssl_CFLAGS},g' \
> +       -e 's,@openssl_LIBS\@,${openssl_LIBS},g' \
>         < $< > $@ || rm $@
>
>  %.pc: %.pc.in Makefile
> @@ -87,7 +89,7 @@ libkmod_libkmod_la_DEPENDENCIES = \
>         ${top_srcdir}/libkmod/libkmod.sym
>  libkmod_libkmod_la_LIBADD = \
>         shared/libshared.la \
> -       ${liblzma_LIBS} ${zlib_LIBS}
> +       ${liblzma_LIBS} ${zlib_LIBS} ${openssl_LIBS}
>
>  noinst_LTLIBRARIES += libkmod/libkmod-internal.la
>  libkmod_libkmod_internal_la_SOURCES = $(libkmod_libkmod_la_SOURCES)
> diff --git a/configure.ac b/configure.ac
> index fbc7391b2d1b..2e33380a0cc2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -106,6 +106,17 @@ AS_IF([test "x$with_zlib" != "xno"], [
>  ])
>  CC_FEATURE_APPEND([with_features], [with_zlib], [ZLIB])
>
> +AC_ARG_WITH([openssl],
> +       AS_HELP_STRING([--with-openssl], [handle PKCS7 signatures @<:@default=disabled@:>@]),
> +       [], [with_openssl=no])
> +AS_IF([test "x$with_openssl" != "xno"], [
> +       PKG_CHECK_MODULES([openssl], [openssl])
> +       AC_DEFINE([ENABLE_OPENSSL], [1], [Enable openssl for modinfo.])
> +], [
> +       AC_MSG_NOTICE([openssl support not requested])
> +])
> +CC_FEATURE_APPEND([with_features], [with_openssl], [OPENSSL])
> +
>  AC_ARG_WITH([bashcompletiondir],
>         AS_HELP_STRING([--with-bashcompletiondir=DIR], [Bash completions directory]),
>         [],
> diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
> index 346579c71aab..a65ddd156f18 100644
> --- a/libkmod/libkmod-internal.h
> +++ b/libkmod/libkmod-internal.h
> @@ -188,5 +188,8 @@ struct kmod_signature_info {
>         const char *algo, *hash_algo, *id_type;
>         const char *sig;
>         size_t sig_len;
> +       void (*free)(void *);
> +       void *private;
>  };
>  bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info) _must_check_ __attribute__((nonnull(1, 2)));
> +void kmod_module_signature_info_free(struct kmod_signature_info *sig_info) __attribute__((nonnull));
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 889f26479a98..bffe715cdef4 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -2357,6 +2357,9 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
>         ret = count;
>
>  list_error:
> +       /* aux structures freed in normal case also */
> +       kmod_module_signature_info_free(&sig_info);
> +
>         if (ret < 0) {
>                 kmod_module_info_free_list(*list);
>                 *list = NULL;
> diff --git a/libkmod/libkmod-signature.c b/libkmod/libkmod-signature.c
> index 429ffbd8a957..48d0145a7552 100644
> --- a/libkmod/libkmod-signature.c
> +++ b/libkmod/libkmod-signature.c
> @@ -19,6 +19,10 @@
>
>  #include <endian.h>
>  #include <inttypes.h>
> +#ifdef ENABLE_OPENSSL
> +#include <openssl/cms.h>
> +#include <openssl/ssl.h>
> +#endif
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -115,15 +119,194 @@ static bool fill_default(const char *mem, off_t size,
>         return true;
>  }
>
> -static bool fill_unknown(const char *mem, off_t size,
> -                        const struct module_signature *modsig, size_t sig_len,
> -                        struct kmod_signature_info *sig_info)
> +#ifdef ENABLE_OPENSSL
> +
> +struct pkcs7_private {
> +       CMS_ContentInfo *cms;
> +       unsigned char *key_id;
> +       BIGNUM *sno;
> +};
> +
> +static void pkcs7_free(void *s)
> +{
> +       struct kmod_signature_info *si = s;
> +       struct pkcs7_private *pvt = si->private;
> +
> +       CMS_ContentInfo_free(pvt->cms);
> +       BN_free(pvt->sno);
> +       free(pvt->key_id);
> +       free(pvt);
> +       si->private = NULL;
> +}
> +
> +static int obj_to_hash_algo(const ASN1_OBJECT *o)
> +{
> +       int nid;
> +
> +       nid = OBJ_obj2nid(o);
> +       switch (nid) {
> +       case NID_md4:
> +               return PKEY_HASH_MD4;
> +       case NID_md5:
> +               return PKEY_HASH_MD5;
> +       case NID_sha1:
> +               return PKEY_HASH_SHA1;
> +       case NID_ripemd160:
> +               return PKEY_HASH_RIPE_MD_160;
> +       case NID_sha256:
> +               return PKEY_HASH_SHA256;
> +       case NID_sha384:
> +               return PKEY_HASH_SHA384;
> +       case NID_sha512:
> +               return PKEY_HASH_SHA512;
> +       case NID_sha224:
> +               return PKEY_HASH_SHA224;
> +       default:
> +               return -1;
> +       }
> +       return -1;
> +}
> +
> +static const char *x509_name_to_str(X509_NAME *name)
> +{
> +       int i;
> +       X509_NAME_ENTRY *e;
> +       ASN1_STRING *d;
> +       ASN1_OBJECT *o;
> +       int nid = -1;
> +       const char *str;
> +
> +       for (i = 0; i < X509_NAME_entry_count(name); i++) {
> +               e = X509_NAME_get_entry(name, i);
> +               o = X509_NAME_ENTRY_get_object(e);
> +               nid = OBJ_obj2nid(o);
> +               if (nid == NID_commonName)
> +                       break;
> +       }
> +       if (nid == -1)
> +               return NULL;
> +
> +       d = X509_NAME_ENTRY_get_data(e);
> +       str = (const char *)ASN1_STRING_get0_data(d);
> +
> +       return str;
> +}
> +
> +static bool fill_pkcs7(const char *mem, off_t size,
> +                      const struct module_signature *modsig, size_t sig_len,
> +                      struct kmod_signature_info *sig_info)
> +{
> +       const char *pkcs7_raw;
> +       CMS_ContentInfo *cms;
> +       STACK_OF(CMS_SignerInfo) *sis;
> +       CMS_SignerInfo *si;
> +       int rc;
> +       ASN1_OCTET_STRING *key_id;
> +       X509_NAME *issuer;
> +       ASN1_INTEGER *sno;
> +       ASN1_OCTET_STRING *sig;
> +       BIGNUM *sno_bn;
> +       X509_ALGOR *dig_alg;
> +       X509_ALGOR *sig_alg;
> +       const ASN1_OBJECT *o;
> +       BIO *in;
> +       int len;
> +       unsigned char *key_id_str;
> +       struct pkcs7_private *pvt;
> +       const char *issuer_str;
> +
> +       size -= sig_len;
> +       pkcs7_raw = mem + size;
> +
> +       in = BIO_new_mem_buf(pkcs7_raw, sig_len);
> +
> +       cms = d2i_CMS_bio(in, NULL);
> +       if (cms == NULL) {
> +               BIO_free(in);

_cleanup_() on `in` would make this neater. Same for all variables in
err, err2, err3, etc.
Just remember to set it to NULL on !error-path.

> +               return false;
> +       }
> +
> +       BIO_free(in);
> +
> +       sis = CMS_get0_SignerInfos(cms);
> +       if (sis == NULL)
> +               goto err;
> +
> +       si = sk_CMS_SignerInfo_value(sis, 0);
> +       if (si == NULL)
> +               goto err;
> +
> +       rc = CMS_SignerInfo_get0_signer_id(si, &key_id, &issuer, &sno);
> +       if (rc == 0)
> +               goto err;
> +
> +       sig = CMS_SignerInfo_get0_signature(si);
> +       if (sig == NULL)
> +               goto err;
> +
> +       CMS_SignerInfo_get0_algs(si, NULL, NULL, &dig_alg, &sig_alg);
> +
> +       sig_info->sig = (const char *)ASN1_STRING_get0_data(sig);
> +       sig_info->sig_len = ASN1_STRING_length(sig);
> +
> +       sno_bn = ASN1_INTEGER_to_BN(sno, NULL);
> +       if (sno_bn == NULL)
> +               goto err;
> +
> +       len = BN_num_bytes(sno_bn);
> +       key_id_str = malloc(len);
> +       if (key_id_str == NULL)
> +               goto err2;
> +       BN_bn2bin(sno_bn, key_id_str);
> +
> +       sig_info->key_id = (const char *)key_id_str;
> +       sig_info->key_id_len = len;
> +
> +       issuer_str = x509_name_to_str(issuer);
> +       if (issuer_str != NULL) {
> +               sig_info->signer = issuer_str;
> +               sig_info->signer_len = strlen(issuer_str);
> +       }
> +
> +       X509_ALGOR_get0(&o, NULL, NULL, dig_alg);
> +
> +       sig_info->hash_algo = pkey_hash_algo[obj_to_hash_algo(o)];
> +       sig_info->id_type = pkey_id_type[modsig->id_type];
> +
> +       pvt = malloc(sizeof(*pvt));
> +       if (pvt == NULL)
> +               goto err3;
> +
> +       pvt->cms = cms;
> +       pvt->key_id = key_id_str;
> +       pvt->sno = sno_bn;
> +       sig_info->private = pvt;

why do you keep pvt around if the only thing you will do with it later
is to free it?
AFAICS the only thing that needs to remain around is the str so we can
free it after
the user used it (because normal signature is backed in memory by the
mem object,
while these are openssl structs)

I miss a simple module in the testsuite to make sure this is working.

thanks
Lucas De Marchi

> +
> +       sig_info->free = pkcs7_free;
> +
> +       return true;
> +err3:
> +       free(key_id_str);
> +err2:
> +       BN_free(sno_bn);
> +err:
> +       CMS_ContentInfo_free(cms);
> +       return false;
> +}
> +
> +#else /* ENABLE OPENSSL */
> +
> +static bool fill_pkcs7(const char *mem, off_t size,
> +                      const struct module_signature *modsig, size_t sig_len,
> +                      struct kmod_signature_info *sig_info)
>  {
>         sig_info->hash_algo = "unknown";
>         sig_info->id_type = pkey_id_type[modsig->id_type];
>         return true;
>  }
>
> +#endif /* ENABLE OPENSSL */
> +
>  #define SIG_MAGIC "~Module signature appended~\n"
>
>  /*
> @@ -167,8 +350,14 @@ bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signat
>
>         switch (modsig->id_type) {
>         case PKEY_ID_PKCS7:
> -               return fill_unknown(mem, size, modsig, sig_len, sig_info);
> +               return fill_pkcs7(mem, size, modsig, sig_len, sig_info);
>         default:
>                 return fill_default(mem, size, modsig, sig_len, sig_info);
>         }
>  }
> +
> +void kmod_module_signature_info_free(struct kmod_signature_info *sig_info)
> +{
> +       if (sig_info->free)
> +               sig_info->free(sig_info);
> +}
> --
> 2.20.1
>
Yauheni Kaliuta Jan. 26, 2019, 11:01 a.m. UTC | #2
Hi, Lucas!

>>>>> On Fri, 25 Jan 2019 10:33:44 -0800, Lucas De Marchi  wrote:

 > On Fri, Jan 25, 2019 at 5:39 AM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:

[...]

 >> +
 >> +static bool fill_pkcs7(const char *mem, off_t size,
 >> +                      const struct module_signature *modsig, size_t sig_len,
 >> +                      struct kmod_signature_info *sig_info)
 >> +{
 >> +       const char *pkcs7_raw;
 >> +       CMS_ContentInfo *cms;
 >> +       STACK_OF(CMS_SignerInfo) *sis;
 >> +       CMS_SignerInfo *si;
 >> +       int rc;
 >> +       ASN1_OCTET_STRING *key_id;
 >> +       X509_NAME *issuer;
 >> +       ASN1_INTEGER *sno;
 >> +       ASN1_OCTET_STRING *sig;
 >> +       BIGNUM *sno_bn;
 >> +       X509_ALGOR *dig_alg;
 >> +       X509_ALGOR *sig_alg;
 >> +       const ASN1_OBJECT *o;
 >> +       BIO *in;
 >> +       int len;
 >> +       unsigned char *key_id_str;
 >> +       struct pkcs7_private *pvt;
 >> +       const char *issuer_str;
 >> +
 >> +       size -= sig_len;
 >> +       pkcs7_raw = mem + size;
 >> +
 >> +       in = BIO_new_mem_buf(pkcs7_raw, sig_len);
 >> +
 >> +       cms = d2i_CMS_bio(in, NULL);
 >> +       if (cms == NULL) {
 >> +               BIO_free(in);

 > _cleanup_() on `in` would make this neater. Same for all variables in
 > err, err2, err3, etc.
 > Just remember to set it to NULL on !error-path.

You mean

pvt->key_id = key_id_str;
key_id_str = NULL;

?

not sure that it is neater, but I can do it.

 >> +               return false;
 >> +       }
 >> +
 >> +       BIO_free(in);
 >> +
 >> +       sis = CMS_get0_SignerInfos(cms);
 >> +       if (sis == NULL)
 >> +               goto err;
 >> +
 >> +       si = sk_CMS_SignerInfo_value(sis, 0);
 >> +       if (si == NULL)
 >> +               goto err;
 >> +
 >> +       rc = CMS_SignerInfo_get0_signer_id(si, &key_id, &issuer, &sno);
 >> +       if (rc == 0)
 >> +               goto err;
 >> +
 >> +       sig = CMS_SignerInfo_get0_signature(si);
 >> +       if (sig == NULL)
 >> +               goto err;
 >> +
 >> +       CMS_SignerInfo_get0_algs(si, NULL, NULL, &dig_alg, &sig_alg);
 >> +
 >> +       sig_info->sig = (const char *)ASN1_STRING_get0_data(sig);
 >> +       sig_info->sig_len = ASN1_STRING_length(sig);
 >> +
 >> +       sno_bn = ASN1_INTEGER_to_BN(sno, NULL);
 >> +       if (sno_bn == NULL)
 >> +               goto err;
 >> +
 >> +       len = BN_num_bytes(sno_bn);
 >> +       key_id_str = malloc(len);
 >> +       if (key_id_str == NULL)
 >> +               goto err2;
 >> +       BN_bn2bin(sno_bn, key_id_str);
 >> +
 >> +       sig_info->key_id = (const char *)key_id_str;
 >> +       sig_info->key_id_len = len;
 >> +
 >> +       issuer_str = x509_name_to_str(issuer);
 >> +       if (issuer_str != NULL) {
 >> +               sig_info->signer = issuer_str;
 >> +               sig_info->signer_len = strlen(issuer_str);
 >> +       }
 >> +
 >> +       X509_ALGOR_get0(&o, NULL, NULL, dig_alg);
 >> +
 >> +       sig_info->hash_algo = pkey_hash_algo[obj_to_hash_algo(o)];
 >> +       sig_info->id_type = pkey_id_type[modsig->id_type];
 >> +
 >> +       pvt = malloc(sizeof(*pvt));
 >> +       if (pvt == NULL)
 >> +               goto err3;
 >> +
 >> +       pvt->cms = cms;
 >> +       pvt->key_id = key_id_str;
 >> +       pvt->sno = sno_bn;
 >> +       sig_info->private = pvt;

 > why do you keep pvt around if the only thing you will do with
 > it later is to free it?
 > AFAICS the only thing that needs to remain around is the str
 > so we can free it after the user used it (because normal
 > signature is backed in memory by the mem object, while these
 > are openssl structs)

I should keep them until kmod_module_get_info() makes the copies.

cms is openssl struct
sno_bn is allocated by openssl and must be freed later
key_id_str is allocated here since the size in unknown in advance
and must be freed later.

Or what did I miss?

 > I miss a simple module in the testsuite to make sure this is
 > working.

I'll think what can I do.

 > thanks
 > Lucas De Marchi

 >> +
 >> +       sig_info->free = pkcs7_free;
 >> +
 >> +       return true;
 >> +err3:
 >> +       free(key_id_str);
 >> +err2:
 >> +       BN_free(sno_bn);
 >> +err:
 >> +       CMS_ContentInfo_free(cms);
 >> +       return false;
 >> +}
 >> +
 >> +#else /* ENABLE OPENSSL */
 >> +
 >> +static bool fill_pkcs7(const char *mem, off_t size,
 >> +                      const struct module_signature *modsig, size_t sig_len,
 >> +                      struct kmod_signature_info *sig_info)
 >> {
 sig_info-> hash_algo = "unknown";
 sig_info-> id_type = pkey_id_type[modsig->id_type];
 >> return true;
 >> }
 >> 
 >> +#endif /* ENABLE OPENSSL */
 >> +
 >> #define SIG_MAGIC "~Module signature appended~\n"
 >> 
 >> /*
 >> @@ -167,8 +350,14 @@ bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signat
 >> 
 >> switch (modsig->id_type) {
 >> case PKEY_ID_PKCS7:
 >> -               return fill_unknown(mem, size, modsig, sig_len, sig_info);
 >> +               return fill_pkcs7(mem, size, modsig, sig_len, sig_info);
 >> default:
 >> return fill_default(mem, size, modsig, sig_len, sig_info);
 >> }
 >> }
 >> +
 >> +void kmod_module_signature_info_free(struct kmod_signature_info *sig_info)
 >> +{
 >> +       if (sig_info->free)
 >> +               sig_info->free(sig_info);
 >> +}
 >> --
 >> 2.20.1
 >> 


 > -- 
 > Lucas De Marchi
Lucas De Marchi Jan. 28, 2019, 6:05 p.m. UTC | #3
On Sat, Jan 26, 2019 at 3:01 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Lucas!
>
> >>>>> On Fri, 25 Jan 2019 10:33:44 -0800, Lucas De Marchi  wrote:
>
>  > On Fri, Jan 25, 2019 at 5:39 AM Yauheni Kaliuta
>  > <yauheni.kaliuta@redhat.com> wrote:
>
> [...]
>
>  >> +
>  >> +static bool fill_pkcs7(const char *mem, off_t size,
>  >> +                      const struct module_signature *modsig, size_t sig_len,
>  >> +                      struct kmod_signature_info *sig_info)
>  >> +{
>  >> +       const char *pkcs7_raw;
>  >> +       CMS_ContentInfo *cms;
>  >> +       STACK_OF(CMS_SignerInfo) *sis;
>  >> +       CMS_SignerInfo *si;
>  >> +       int rc;
>  >> +       ASN1_OCTET_STRING *key_id;
>  >> +       X509_NAME *issuer;
>  >> +       ASN1_INTEGER *sno;
>  >> +       ASN1_OCTET_STRING *sig;
>  >> +       BIGNUM *sno_bn;
>  >> +       X509_ALGOR *dig_alg;
>  >> +       X509_ALGOR *sig_alg;
>  >> +       const ASN1_OBJECT *o;
>  >> +       BIO *in;
>  >> +       int len;
>  >> +       unsigned char *key_id_str;
>  >> +       struct pkcs7_private *pvt;
>  >> +       const char *issuer_str;
>  >> +
>  >> +       size -= sig_len;
>  >> +       pkcs7_raw = mem + size;
>  >> +
>  >> +       in = BIO_new_mem_buf(pkcs7_raw, sig_len);
>  >> +
>  >> +       cms = d2i_CMS_bio(in, NULL);
>  >> +       if (cms == NULL) {
>  >> +               BIO_free(in);
>
>  > _cleanup_() on `in` would make this neater. Same for all variables in
>  > err, err2, err3, etc.
>  > Just remember to set it to NULL on !error-path.
>
> You mean
>
> pvt->key_id = key_id_str;
> key_id_str = NULL;
>
> ?
>
> not sure that it is neater, but I can do it.

The assignment to NULL should be in the one place it exits the
function successfully.

>
>  >> +               return false;
>  >> +       }
>  >> +
>  >> +       BIO_free(in);
>  >> +
>  >> +       sis = CMS_get0_SignerInfos(cms);
>  >> +       if (sis == NULL)
>  >> +               goto err;
>  >> +
>  >> +       si = sk_CMS_SignerInfo_value(sis, 0);
>  >> +       if (si == NULL)
>  >> +               goto err;
>  >> +
>  >> +       rc = CMS_SignerInfo_get0_signer_id(si, &key_id, &issuer, &sno);
>  >> +       if (rc == 0)
>  >> +               goto err;
>  >> +
>  >> +       sig = CMS_SignerInfo_get0_signature(si);
>  >> +       if (sig == NULL)
>  >> +               goto err;
>  >> +
>  >> +       CMS_SignerInfo_get0_algs(si, NULL, NULL, &dig_alg, &sig_alg);
>  >> +
>  >> +       sig_info->sig = (const char *)ASN1_STRING_get0_data(sig);
>  >> +       sig_info->sig_len = ASN1_STRING_length(sig);
>  >> +
>  >> +       sno_bn = ASN1_INTEGER_to_BN(sno, NULL);
>  >> +       if (sno_bn == NULL)
>  >> +               goto err;
>  >> +
>  >> +       len = BN_num_bytes(sno_bn);
>  >> +       key_id_str = malloc(len);
>  >> +       if (key_id_str == NULL)
>  >> +               goto err2;
>  >> +       BN_bn2bin(sno_bn, key_id_str);
>  >> +
>  >> +       sig_info->key_id = (const char *)key_id_str;
>  >> +       sig_info->key_id_len = len;
>  >> +
>  >> +       issuer_str = x509_name_to_str(issuer);
>  >> +       if (issuer_str != NULL) {
>  >> +               sig_info->signer = issuer_str;
>  >> +               sig_info->signer_len = strlen(issuer_str);
>  >> +       }
>  >> +
>  >> +       X509_ALGOR_get0(&o, NULL, NULL, dig_alg);
>  >> +
>  >> +       sig_info->hash_algo = pkey_hash_algo[obj_to_hash_algo(o)];
>  >> +       sig_info->id_type = pkey_id_type[modsig->id_type];
>  >> +
>  >> +       pvt = malloc(sizeof(*pvt));
>  >> +       if (pvt == NULL)
>  >> +               goto err3;
>  >> +
>  >> +       pvt->cms = cms;
>  >> +       pvt->key_id = key_id_str;
>  >> +       pvt->sno = sno_bn;
>  >> +       sig_info->private = pvt;
>
>  > why do you keep pvt around if the only thing you will do with
>  > it later is to free it?
>  > AFAICS the only thing that needs to remain around is the str
>  > so we can free it after the user used it (because normal
>  > signature is backed in memory by the mem object, while these
>  > are openssl structs)
>
> I should keep them until kmod_module_get_info() makes the copies.
>
> cms is openssl struct
> sno_bn is allocated by openssl and must be freed later
> key_id_str is allocated here since the size in unknown in advance
> and must be freed later.
>
> Or what did I miss?

we could just duplicate the information that we want stored and keep
the openssl context contained
to just this function. I thought the only one would be key_str_id, but
missed that sig and signer
also need to have their backing object around.

Lucas De Marchi
>
>  > I miss a simple module in the testsuite to make sure this is
>  > working.
>
> I'll think what can I do.
>
>  > thanks
>  > Lucas De Marchi
>
>  >> +
>  >> +       sig_info->free = pkcs7_free;
>  >> +
>  >> +       return true;
>  >> +err3:
>  >> +       free(key_id_str);
>  >> +err2:
>  >> +       BN_free(sno_bn);
>  >> +err:
>  >> +       CMS_ContentInfo_free(cms);
>  >> +       return false;
>  >> +}
>  >> +
>  >> +#else /* ENABLE OPENSSL */
>  >> +
>  >> +static bool fill_pkcs7(const char *mem, off_t size,
>  >> +                      const struct module_signature *modsig, size_t sig_len,
>  >> +                      struct kmod_signature_info *sig_info)
>  >> {
>  sig_info-> hash_algo = "unknown";
>  sig_info-> id_type = pkey_id_type[modsig->id_type];
>  >> return true;
>  >> }
>  >>
>  >> +#endif /* ENABLE OPENSSL */
>  >> +
>  >> #define SIG_MAGIC "~Module signature appended~\n"
>  >>
>  >> /*
>  >> @@ -167,8 +350,14 @@ bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signat
>  >>
>  >> switch (modsig->id_type) {
>  >> case PKEY_ID_PKCS7:
>  >> -               return fill_unknown(mem, size, modsig, sig_len, sig_info);
>  >> +               return fill_pkcs7(mem, size, modsig, sig_len, sig_info);
>  >> default:
>  >> return fill_default(mem, size, modsig, sig_len, sig_info);
>  >> }
>  >> }
>  >> +
>  >> +void kmod_module_signature_info_free(struct kmod_signature_info *sig_info)
>  >> +{
>  >> +       if (sig_info->free)
>  >> +               sig_info->free(sig_info);
>  >> +}
>  >> --
>  >> 2.20.1
>  >>
>
>
>  > --
>  > Lucas De Marchi
>
> --
> WBR,
> Yauheni Kaliuta
Yauheni Kaliuta Jan. 29, 2019, 9:50 a.m. UTC | #4
Hi, Lucas!

>>>>> On Mon, 28 Jan 2019 10:05:24 -0800, Lucas De Marchi  wrote:
 > On Sat, Jan 26, 2019 at 3:01 AM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 

[...]
 >> >> +
 >> >> +       pvt->cms = cms;
 >> >> +       pvt->key_id = key_id_str;
 >> >> +       pvt->sno = sno_bn;
 >> >> +       sig_info->private = pvt;
 >> 
 >> > why do you keep pvt around if the only thing you will do with
 >> > it later is to free it?
 >> > AFAICS the only thing that needs to remain around is the str
 >> > so we can free it after the user used it (because normal
 >> > signature is backed in memory by the mem object, while these
 >> > are openssl structs)
 >> 
 >> I should keep them until kmod_module_get_info() makes the copies.
 >> 
 >> cms is openssl struct
 >> sno_bn is allocated by openssl and must be freed later
 >> key_id_str is allocated here since the size in unknown in advance
 >> and must be freed later.
 >> 
 >> Or what did I miss?

 > we could just duplicate the information that we want stored and keep
 > the openssl context contained
 > to just this function. I thought the only one would be key_str_id, but
 > missed that sig and signer
 > also need to have their backing object around.

If I duplicate it here then without cleanup I'll have memory
leak, no?

In the old code they were pointers inside the module image and
freed with the image itself.
Lucas De Marchi Jan. 29, 2019, 4:50 p.m. UTC | #5
On Tue, Jan 29, 2019 at 11:50:05AM +0200, Yauheni Kaliuta wrote:
>Hi, Lucas!
>
>>>>>> On Mon, 28 Jan 2019 10:05:24 -0800, Lucas De Marchi  wrote:
> > On Sat, Jan 26, 2019 at 3:01 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> >>
>
>[...]
> >> >> +
> >> >> +       pvt->cms = cms;
> >> >> +       pvt->key_id = key_id_str;
> >> >> +       pvt->sno = sno_bn;
> >> >> +       sig_info->private = pvt;
> >>
> >> > why do you keep pvt around if the only thing you will do with
> >> > it later is to free it?
> >> > AFAICS the only thing that needs to remain around is the str
> >> > so we can free it after the user used it (because normal
> >> > signature is backed in memory by the mem object, while these
> >> > are openssl structs)
> >>
> >> I should keep them until kmod_module_get_info() makes the copies.
> >>
> >> cms is openssl struct
> >> sno_bn is allocated by openssl and must be freed later
> >> key_id_str is allocated here since the size in unknown in advance
> >> and must be freed later.
> >>
> >> Or what did I miss?
>
> > we could just duplicate the information that we want stored and keep
> > the openssl context contained
> > to just this function. I thought the only one would be key_str_id, but
> > missed that sig and signer
> > also need to have their backing object around.
>
>If I duplicate it here then without cleanup I'll have memory
>leak, no?

yes, my idea was to just leave it simpler and add a

if (pkcs7)
        free(key_id)

Lucas De Marchi

>
>In the old code they were pointers inside the module image and
>freed with the image itself.
>
>-- 
>WBR,
>Yauheni Kaliuta
Yauheni Kaliuta Jan. 29, 2019, 5:22 p.m. UTC | #6
Hi, Lucas!

>>>>> On Tue, 29 Jan 2019 08:50:10 -0800, Lucas De Marchi  wrote:

 > On Tue, Jan 29, 2019 at 11:50:05AM +0200, Yauheni Kaliuta wrote:
 >> Hi, Lucas!
 >> 
 >>>>>>> On Mon, 28 Jan 2019 10:05:24 -0800, Lucas De Marchi  wrote:
 >> > On Sat, Jan 26, 2019 at 3:01 AM Yauheni Kaliuta
 >> > <yauheni.kaliuta@redhat.com> wrote:
 >> >>
 >> 
 >> [...]
 >> >> >> +
 >> >> >> +       pvt->cms = cms;
 >> >> >> +       pvt->key_id = key_id_str;
 >> >> >> +       pvt->sno = sno_bn;
 >> >> >> +       sig_info->private = pvt;
 >> >>
 >> >> > why do you keep pvt around if the only thing you will do with
 >> >> > it later is to free it?
 >> >> > AFAICS the only thing that needs to remain around is the str
 >> >> > so we can free it after the user used it (because normal
 >> >> > signature is backed in memory by the mem object, while these
 >> >> > are openssl structs)
 >> >>
 >> >> I should keep them until kmod_module_get_info() makes the copies.
 >> >>
 >> >> cms is openssl struct
 >> >> sno_bn is allocated by openssl and must be freed later
 >> >> key_id_str is allocated here since the size in unknown in advance
 >> >> and must be freed later.
 >> >>
 >> >> Or what did I miss?
 >> 
 >> > we could just duplicate the information that we want stored and keep
 >> > the openssl context contained
 >> > to just this function. I thought the only one would be key_str_id, but
 >> > missed that sig and signer
 >> > also need to have their backing object around.
 >> 
 >> If I duplicate it here then without cleanup I'll have memory
 >> leak, no?

 > yes, my idea was to just leave it simpler and add a

 > if (pkcs7)
 >        free(key_id)

Ah, got it, thanks!

Well, if it's not a show stopper for you, I would rather keep it
as in the original proposal since it does not inject
implementation details to the upper level.

 > Lucas De Marchi

 >> 
 >> In the old code they were pointers inside the module image and
 >> freed with the image itself.
 >> 
 >> -- 
 >> WBR,
 >> Yauheni Kaliuta
Lucas De Marchi Jan. 29, 2019, 6:03 p.m. UTC | #7
On Tue, Jan 29, 2019 at 07:22:43PM +0200, Yauheni Kaliuta wrote:
>Hi, Lucas!
>
>>>>>> On Tue, 29 Jan 2019 08:50:10 -0800, Lucas De Marchi  wrote:
>
> > On Tue, Jan 29, 2019 at 11:50:05AM +0200, Yauheni Kaliuta wrote:
> >> Hi, Lucas!
> >>
> >>>>>>> On Mon, 28 Jan 2019 10:05:24 -0800, Lucas De Marchi  wrote:
> >> > On Sat, Jan 26, 2019 at 3:01 AM Yauheni Kaliuta
> >> > <yauheni.kaliuta@redhat.com> wrote:
> >> >>
> >>
> >> [...]
> >> >> >> +
> >> >> >> +       pvt->cms = cms;
> >> >> >> +       pvt->key_id = key_id_str;
> >> >> >> +       pvt->sno = sno_bn;
> >> >> >> +       sig_info->private = pvt;
> >> >>
> >> >> > why do you keep pvt around if the only thing you will do with
> >> >> > it later is to free it?
> >> >> > AFAICS the only thing that needs to remain around is the str
> >> >> > so we can free it after the user used it (because normal
> >> >> > signature is backed in memory by the mem object, while these
> >> >> > are openssl structs)
> >> >>
> >> >> I should keep them until kmod_module_get_info() makes the copies.
> >> >>
> >> >> cms is openssl struct
> >> >> sno_bn is allocated by openssl and must be freed later
> >> >> key_id_str is allocated here since the size in unknown in advance
> >> >> and must be freed later.
> >> >>
> >> >> Or what did I miss?
> >>
> >> > we could just duplicate the information that we want stored and keep
> >> > the openssl context contained
> >> > to just this function. I thought the only one would be key_str_id, but
> >> > missed that sig and signer
> >> > also need to have their backing object around.
> >>
> >> If I duplicate it here then without cleanup I'll have memory
> >> leak, no?
>
> > yes, my idea was to just leave it simpler and add a
>
> > if (pkcs7)
> >        free(key_id)
>
>Ah, got it, thanks!
>
>Well, if it's not a show stopper for you, I would rather keep it
>as in the original proposal since it does not inject
>implementation details to the upper level.

ok, we may revisit this later. What's the status of the test for
testsuite? Are you going to send a v2 (3?) including the test?

thanks
Lucas De Marchi

>
> > Lucas De Marchi
>
> >>
> >> In the old code they were pointers inside the module image and
> >> freed with the image itself.
> >>
> >> --
> >> WBR,
> >> Yauheni Kaliuta
>
>
>-- 
>WBR,
>Yauheni Kaliuta
Yauheni Kaliuta Jan. 29, 2019, 6:34 p.m. UTC | #8
Hi, Lucas!

>>>>> On Tue, 29 Jan 2019 10:03:45 -0800, Lucas De Marchi  wrote:

[...]

 >> > yes, my idea was to just leave it simpler and add a
 >> 
 >> > if (pkcs7)
 >> >        free(key_id)
 >> 
 >> Ah, got it, thanks!
 >> 
 >> Well, if it's not a show stopper for you, I would rather keep it
 >> as in the original proposal since it does not inject
 >> implementation details to the upper level.

 > ok, we may revisit this later. What's the status of the test
 > for testsuite?

Haven't checked yet, sorry (caught some flu).

 > Are you going to send a v2 (3?) including the test?

Yes if everything finally works.
diff mbox series

Patch

diff --git a/Makefile.am b/Makefile.am
index 1ab1db585316..de1026f8bd46 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -35,6 +35,8 @@  SED_PROCESS = \
 	-e 's,@liblzma_LIBS\@,${liblzma_LIBS},g' \
 	-e 's,@zlib_CFLAGS\@,${zlib_CFLAGS},g' \
 	-e 's,@zlib_LIBS\@,${zlib_LIBS},g' \
+	-e 's,@openssl_CFLAGS\@,${openssl_CFLAGS},g' \
+	-e 's,@openssl_LIBS\@,${openssl_LIBS},g' \
 	< $< > $@ || rm $@
 
 %.pc: %.pc.in Makefile
@@ -87,7 +89,7 @@  libkmod_libkmod_la_DEPENDENCIES = \
 	${top_srcdir}/libkmod/libkmod.sym
 libkmod_libkmod_la_LIBADD = \
 	shared/libshared.la \
-	${liblzma_LIBS} ${zlib_LIBS}
+	${liblzma_LIBS} ${zlib_LIBS} ${openssl_LIBS}
 
 noinst_LTLIBRARIES += libkmod/libkmod-internal.la
 libkmod_libkmod_internal_la_SOURCES = $(libkmod_libkmod_la_SOURCES)
diff --git a/configure.ac b/configure.ac
index fbc7391b2d1b..2e33380a0cc2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -106,6 +106,17 @@  AS_IF([test "x$with_zlib" != "xno"], [
 ])
 CC_FEATURE_APPEND([with_features], [with_zlib], [ZLIB])
 
+AC_ARG_WITH([openssl],
+	AS_HELP_STRING([--with-openssl], [handle PKCS7 signatures @<:@default=disabled@:>@]),
+	[], [with_openssl=no])
+AS_IF([test "x$with_openssl" != "xno"], [
+	PKG_CHECK_MODULES([openssl], [openssl])
+	AC_DEFINE([ENABLE_OPENSSL], [1], [Enable openssl for modinfo.])
+], [
+	AC_MSG_NOTICE([openssl support not requested])
+])
+CC_FEATURE_APPEND([with_features], [with_openssl], [OPENSSL])
+
 AC_ARG_WITH([bashcompletiondir],
 	AS_HELP_STRING([--with-bashcompletiondir=DIR], [Bash completions directory]),
 	[],
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 346579c71aab..a65ddd156f18 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -188,5 +188,8 @@  struct kmod_signature_info {
 	const char *algo, *hash_algo, *id_type;
 	const char *sig;
 	size_t sig_len;
+	void (*free)(void *);
+	void *private;
 };
 bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info) _must_check_ __attribute__((nonnull(1, 2)));
+void kmod_module_signature_info_free(struct kmod_signature_info *sig_info) __attribute__((nonnull));
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 889f26479a98..bffe715cdef4 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -2357,6 +2357,9 @@  KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 	ret = count;
 
 list_error:
+	/* aux structures freed in normal case also */
+	kmod_module_signature_info_free(&sig_info);
+
 	if (ret < 0) {
 		kmod_module_info_free_list(*list);
 		*list = NULL;
diff --git a/libkmod/libkmod-signature.c b/libkmod/libkmod-signature.c
index 429ffbd8a957..48d0145a7552 100644
--- a/libkmod/libkmod-signature.c
+++ b/libkmod/libkmod-signature.c
@@ -19,6 +19,10 @@ 
 
 #include <endian.h>
 #include <inttypes.h>
+#ifdef ENABLE_OPENSSL
+#include <openssl/cms.h>
+#include <openssl/ssl.h>
+#endif
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -115,15 +119,194 @@  static bool fill_default(const char *mem, off_t size,
 	return true;
 }
 
-static bool fill_unknown(const char *mem, off_t size,
-			 const struct module_signature *modsig, size_t sig_len,
-			 struct kmod_signature_info *sig_info)
+#ifdef ENABLE_OPENSSL
+
+struct pkcs7_private {
+	CMS_ContentInfo *cms;
+	unsigned char *key_id;
+	BIGNUM *sno;
+};
+
+static void pkcs7_free(void *s)
+{
+	struct kmod_signature_info *si = s;
+	struct pkcs7_private *pvt = si->private;
+
+	CMS_ContentInfo_free(pvt->cms);
+	BN_free(pvt->sno);
+	free(pvt->key_id);
+	free(pvt);
+	si->private = NULL;
+}
+
+static int obj_to_hash_algo(const ASN1_OBJECT *o)
+{
+	int nid;
+
+	nid = OBJ_obj2nid(o);
+	switch (nid) {
+	case NID_md4:
+		return PKEY_HASH_MD4;
+	case NID_md5:
+		return PKEY_HASH_MD5;
+	case NID_sha1:
+		return PKEY_HASH_SHA1;
+	case NID_ripemd160:
+		return PKEY_HASH_RIPE_MD_160;
+	case NID_sha256:
+		return PKEY_HASH_SHA256;
+	case NID_sha384:
+		return PKEY_HASH_SHA384;
+	case NID_sha512:
+		return PKEY_HASH_SHA512;
+	case NID_sha224:
+		return PKEY_HASH_SHA224;
+	default:
+		return -1;
+	}
+	return -1;
+}
+
+static const char *x509_name_to_str(X509_NAME *name)
+{
+	int i;
+	X509_NAME_ENTRY *e;
+	ASN1_STRING *d;
+	ASN1_OBJECT *o;
+	int nid = -1;
+	const char *str;
+
+	for (i = 0; i < X509_NAME_entry_count(name); i++) {
+		e = X509_NAME_get_entry(name, i);
+		o = X509_NAME_ENTRY_get_object(e);
+		nid = OBJ_obj2nid(o);
+		if (nid == NID_commonName)
+			break;
+	}
+	if (nid == -1)
+		return NULL;
+
+	d = X509_NAME_ENTRY_get_data(e);
+	str = (const char *)ASN1_STRING_get0_data(d);
+
+	return str;
+}
+
+static bool fill_pkcs7(const char *mem, off_t size,
+		       const struct module_signature *modsig, size_t sig_len,
+		       struct kmod_signature_info *sig_info)
+{
+	const char *pkcs7_raw;
+	CMS_ContentInfo *cms;
+	STACK_OF(CMS_SignerInfo) *sis;
+	CMS_SignerInfo *si;
+	int rc;
+	ASN1_OCTET_STRING *key_id;
+	X509_NAME *issuer;
+	ASN1_INTEGER *sno;
+	ASN1_OCTET_STRING *sig;
+	BIGNUM *sno_bn;
+	X509_ALGOR *dig_alg;
+	X509_ALGOR *sig_alg;
+	const ASN1_OBJECT *o;
+	BIO *in;
+	int len;
+	unsigned char *key_id_str;
+	struct pkcs7_private *pvt;
+	const char *issuer_str;
+
+	size -= sig_len;
+	pkcs7_raw = mem + size;
+
+	in = BIO_new_mem_buf(pkcs7_raw, sig_len);
+
+	cms = d2i_CMS_bio(in, NULL);
+	if (cms == NULL) {
+		BIO_free(in);
+		return false;
+	}
+
+	BIO_free(in);
+
+	sis = CMS_get0_SignerInfos(cms);
+	if (sis == NULL)
+		goto err;
+
+	si = sk_CMS_SignerInfo_value(sis, 0);
+	if (si == NULL)
+		goto err;
+
+	rc = CMS_SignerInfo_get0_signer_id(si, &key_id, &issuer, &sno);
+	if (rc == 0)
+		goto err;
+
+	sig = CMS_SignerInfo_get0_signature(si);
+	if (sig == NULL)
+		goto err;
+
+	CMS_SignerInfo_get0_algs(si, NULL, NULL, &dig_alg, &sig_alg);
+
+	sig_info->sig = (const char *)ASN1_STRING_get0_data(sig);
+	sig_info->sig_len = ASN1_STRING_length(sig);
+
+	sno_bn = ASN1_INTEGER_to_BN(sno, NULL);
+	if (sno_bn == NULL)
+		goto err;
+
+	len = BN_num_bytes(sno_bn);
+	key_id_str = malloc(len);
+	if (key_id_str == NULL)
+		goto err2;
+	BN_bn2bin(sno_bn, key_id_str);
+
+	sig_info->key_id = (const char *)key_id_str;
+	sig_info->key_id_len = len;
+
+	issuer_str = x509_name_to_str(issuer);
+	if (issuer_str != NULL) {
+		sig_info->signer = issuer_str;
+		sig_info->signer_len = strlen(issuer_str);
+	}
+
+	X509_ALGOR_get0(&o, NULL, NULL, dig_alg);
+
+	sig_info->hash_algo = pkey_hash_algo[obj_to_hash_algo(o)];
+	sig_info->id_type = pkey_id_type[modsig->id_type];
+
+	pvt = malloc(sizeof(*pvt));
+	if (pvt == NULL)
+		goto err3;
+
+	pvt->cms = cms;
+	pvt->key_id = key_id_str;
+	pvt->sno = sno_bn;
+	sig_info->private = pvt;
+
+	sig_info->free = pkcs7_free;
+
+	return true;
+err3:
+	free(key_id_str);
+err2:
+	BN_free(sno_bn);
+err:
+	CMS_ContentInfo_free(cms);
+	return false;
+}
+
+#else /* ENABLE OPENSSL */
+
+static bool fill_pkcs7(const char *mem, off_t size,
+		       const struct module_signature *modsig, size_t sig_len,
+		       struct kmod_signature_info *sig_info)
 {
 	sig_info->hash_algo = "unknown";
 	sig_info->id_type = pkey_id_type[modsig->id_type];
 	return true;
 }
 
+#endif /* ENABLE OPENSSL */
+
 #define SIG_MAGIC "~Module signature appended~\n"
 
 /*
@@ -167,8 +350,14 @@  bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signat
 
 	switch (modsig->id_type) {
 	case PKEY_ID_PKCS7:
-		return fill_unknown(mem, size, modsig, sig_len, sig_info);
+		return fill_pkcs7(mem, size, modsig, sig_len, sig_info);
 	default:
 		return fill_default(mem, size, modsig, sig_len, sig_info);
 	}
 }
+
+void kmod_module_signature_info_free(struct kmod_signature_info *sig_info)
+{
+	if (sig_info->free)
+		sig_info->free(sig_info);
+}