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

From: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
To: Lucas De Marchi <lucas.de.marchi@gmail.com>
Cc: linux-modules <linux-modules@vger.kernel.org>,
	ykaliuta@redhat.com, Lucas De Marchi <lucas.demarchi@intel.com>
Subject: Re: [PATCH kmod] libkmod-signature: implement pkcs7 parsing with openssl
Date: Sat, 26 Jan 2019 13:01:51 +0200	[thread overview]
Message-ID: <xunyk1ir8yxs.fsf@redhat.com> (raw)
In-Reply-To: <CAKi4VA+Zy==+=UzzghN261tcukcPBd0B5dyTjNXbCTEYyXyY5A@mail.gmail.com> (Lucas De Marchi's message of "Fri, 25 Jan 2019 10:33:44 -0800")

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

-- 
WBR,
Yauheni Kaliuta