| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGXu5jLi=Nw0EmiMzCyZN9=0o4ubknK_fixwYJnaU0UrrqnUug@mail.gmail.com>
Date: Tue, 30 Oct 2012 14:44:10 -0700
From: Kees Cook <keescook@...omium.org>
To: David Howells <dhowells@...hat.com>
Cc: rusty@...tcorp.com.au, pjones@...hat.com, jwboyer@...hat.com,
mjg@...hat.com, dmitry.kasatkin@...el.com,
zohar@...ux.vnet.ibm.com, keyrings@...ux-nfs.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 20/23] pefile: Digest the PE binary and compare to the
PKCS#7 data
On Tue, Oct 30, 2012 at 12:22 PM, David Howells <dhowells@...hat.com> wrote:
> Digest the signed parts of the PE binary, canonicalising the section table
> before we need it, and then compare the the resulting digest to the one in the
> PKCS#7 signed content.
>
> Signed-off-by: David Howells <dhowells@...hat.com>
> ---
>
> crypto/asymmetric_keys/pefile_parser.c | 198 ++++++++++++++++++++++++++++++++
> 1 file changed, 198 insertions(+)
>
>
> diff --git a/crypto/asymmetric_keys/pefile_parser.c b/crypto/asymmetric_keys/pefile_parser.c
> index 42b9696..9ede195 100644
> --- a/crypto/asymmetric_keys/pefile_parser.c
> +++ b/crypto/asymmetric_keys/pefile_parser.c
> @@ -194,6 +194,193 @@ static int pefile_strip_sig_wrapper(struct key_preparsed_payload *prep,
> }
>
> /*
> + * Compare two sections for canonicalisation.
> + */
> +static int pefile_compare_shdrs(const void *a, const void *b)
> +{
> + const struct section_header *shdra = a;
> + const struct section_header *shdrb = b;
> + int rc;
> +
> + if (shdra->data_addr > shdrb->data_addr)
> + return 1;
> + if (shdrb->data_addr > shdra->data_addr)
> + return -1;
> +
> + if (shdra->virtual_address > shdrb->virtual_address)
> + return 1;
> + if (shdrb->virtual_address > shdra->virtual_address)
> + return -1;
> +
> + rc = strcmp(shdra->name, shdrb->name);
> + if (rc != 0)
> + return rc;
> +
> + if (shdra->virtual_size > shdrb->virtual_size)
> + return 1;
> + if (shdrb->virtual_size > shdra->virtual_size)
> + return -1;
> +
> + if (shdra->raw_data_size > shdrb->raw_data_size)
> + return 1;
> + if (shdrb->raw_data_size > shdra->raw_data_size)
> + return -1;
> +
> + return 0;
> +}
> +
> +/*
> + * Load the contents of the PE binary into the digest, leaving out the image
> + * checksum and the certificate data block.
> + */
> +static int pefile_digest_pe_contents(struct key_preparsed_payload *prep,
> + struct pefile_context *ctx,
> + struct shash_desc *desc)
> +{
> + unsigned *canon, tmp, loop, i, hashed_bytes;
> + int ret;
> +
> + /* Digest the header and data directory, but leave out the image
> + * checksum and the data dirent for the signature.
> + */
> + ret = crypto_shash_update(desc, prep->data, ctx->image_checksum_offset);
> + if (ret < 0)
> + return ret;
> +
> + tmp = ctx->image_checksum_offset + sizeof(uint32_t);
> + ret = crypto_shash_update(desc, prep->data + tmp,
> + ctx->cert_dirent_offset - tmp);
> + if (ret < 0)
> + return ret;
> +
> + tmp = ctx->cert_dirent_offset + sizeof(struct data_dirent);
> + ret = crypto_shash_update(desc, prep->data + tmp,
> + ctx->header_size - tmp);
header_size is not verified, so this update can walk past datalen, or
be truncated, which is probably worse here, IIUC. I think it might be
better to make sure every field that goes into ctx is sanity-checked.
> + if (ret < 0)
> + return ret;
> +
> + canon = kcalloc(ctx->n_sections, sizeof(unsigned), GFP_KERNEL);
n_sections is unverified too, so this and the loops after are troublesome.
> + if (!canon)
> + return -ENOMEM;
> +
> + /* We have to canonicalise the section table, so we perform an
> + * insertion sort.
> + */
> + canon[0] = 0;
> + for (loop = 1; loop < ctx->n_sections; loop++) {
> + for (i = 0; i < loop; i++) {
> + if (pefile_compare_shdrs(&ctx->secs[canon[i]],
> + &ctx->secs[loop]) > 0) {
> + memmove(&canon[i + 1], &canon[i],
> + (loop - i) * sizeof(canon[0]));
> + break;
> + }
> + }
> + canon[i] = loop;
> + }
> +
> + hashed_bytes = ctx->header_size;
> + for (loop = 0; loop < ctx->n_sections; loop++) {
> + i = canon[loop];
> + if (ctx->secs[i].raw_data_size == 0)
> + continue;
> + ret = crypto_shash_update(desc,
> + prep->data + ctx->secs[i].data_addr,
> + ctx->secs[i].raw_data_size);
> + if (ret < 0) {
> + kfree(canon);
> + return ret;
> + }
> + hashed_bytes += ctx->secs[i].raw_data_size;
> + }
> + kfree(canon);
> +
> + if (prep->datalen > hashed_bytes) {
> + tmp = hashed_bytes + ctx->certs_size;
> + ret = crypto_shash_update(desc,
> + prep->data + hashed_bytes,
> + prep->datalen - tmp);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Digest the contents of the PE binary, leaving out the image checksum and the
> + * certificate data block.
> + */
> +static int pefile_digest_pe(struct key_preparsed_payload *prep,
> + struct pefile_context *ctx)
> +{
> + struct crypto_shash *tfm;
> + struct shash_desc *desc;
> + size_t digest_size, desc_size;
> + void *digest;
> + int ret;
> +
> + kenter(",%u", ctx->digest_algo);
> +
> + /* Allocate the hashing algorithm we're going to need and find out how
> + * big the hash operational data will be.
> + */
> + tfm = crypto_alloc_shash(pkey_hash_algo_name[ctx->digest_algo], 0, 0);
> + if (IS_ERR(tfm))
> + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> +
> + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> + digest_size = crypto_shash_digestsize(tfm);
> +
> + if (digest_size != ctx->digest_len) {
> + pr_debug("Digest size mismatch (%zx != %x)\n",
> + digest_size, ctx->digest_len);
> + ret = -EBADMSG;
> + goto error_no_desc;
> + }
> + pr_devel("Digest: desc=%zu size=%zu\n", desc_size, digest_size);
> +
> + ret = -ENOMEM;
> + desc = kzalloc(desc_size + digest_size, GFP_KERNEL);
> + if (!desc)
> + goto error_no_desc;
> +
> + desc->tfm = tfm;
> + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> + ret = crypto_shash_init(desc);
> + if (ret < 0)
> + goto error;
> +
> + ret = pefile_digest_pe_contents(prep, ctx, desc);
> + if (ret < 0)
> + goto error;
> +
> + digest = (void *)desc + desc_size;
> + ret = crypto_shash_final(desc, digest);
> + if (ret < 0)
> + goto error;
> +
> + pr_devel("Digest calc = [%*ph]\n", ctx->digest_len, digest);
> +
> + /* Check that the PE file digest matches that in the MSCODE part of the
> + * PKCS#7 certificate.
> + */
> + if (memcmp(digest, ctx->digest, ctx->digest_len) != 0) {
> + pr_debug("Digest mismatch\n");
> + ret = -EKEYREJECTED;
> + } else {
> + pr_debug("The digests match!\n");
> + }
> +
> +error:
> + kfree(desc);
> +error_no_desc:
> + crypto_free_shash(tfm);
> + kleave(" = %d", ret);
> + return ret;
> +}
> +
> +/*
> * Parse a PE binary.
> */
> static int pefile_key_preparse(struct key_preparsed_payload *prep)
> @@ -230,6 +417,17 @@ static int pefile_key_preparse(struct key_preparsed_payload *prep)
>
> pr_devel("Digest: %u [%*ph]\n", ctx.digest_len, ctx.digest_len, ctx.digest);
>
> + /* Generate the digest and check against the PKCS7 certificate
> + * contents.
> + */
> + ret = pefile_digest_pe(prep, &ctx);
> + if (ret < 0)
> + goto error;
> +
> + ret = pkcs7_verify(pkcs7);
> + if (ret < 0)
> + goto error;
> +
> ret = -ENOANO; // Not yet complete
>
> error:
>
-Kees
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists