From: Lucas De Marchi <lucas.de.marchi@gmail.com>
To: Yauheni Kaliuta <yauheni.kaliuta@redhat.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: Mon, 28 Jan 2019 10:05:24 -0800 [thread overview]
Message-ID: <CAKi4VA+YGwBqg6kDb9mrgxqm7X4UCQD-Tu9PqXhm6bbRrG5Uaw@mail.gmail.com> (raw)
In-Reply-To: <xunyk1ir8yxs.fsf@redhat.com>
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
--
Lucas De Marchi
next prev parent reply other threads:[~2019-01-28 18:05 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-25 13:38 [PATCH kmod] libkmod-signature: implement pkcs7 parsing with openssl Yauheni Kaliuta
2019-01-25 18:33 ` Lucas De Marchi
2019-01-26 11:01 ` Yauheni Kaliuta
2019-01-28 18:05 ` Lucas De Marchi [this message]
2019-01-29 9:50 ` Yauheni Kaliuta
2019-01-29 16:50 ` Lucas De Marchi
2019-01-29 17:22 ` Yauheni Kaliuta
2019-01-29 18:03 ` Lucas De Marchi
2019-01-29 18:34 ` Yauheni Kaliuta
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKi4VA+YGwBqg6kDb9mrgxqm7X4UCQD-Tu9PqXhm6bbRrG5Uaw@mail.gmail.com \
--to=lucas.de.marchi@gmail.com \
--cc=linux-modules@vger.kernel.org \
--cc=lucas.demarchi@intel.com \
--cc=yauheni.kaliuta@redhat.com \
--cc=ykaliuta@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).