linux-kernel - Re: [PATCH 17/23] pefile: Strip the wrapper off of the cert data block

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jL6hqy-5aZQnLetxDh+XyDr5HUDYKCuPw_dcS7RQB77_w@mail.gmail.com>
Date:	Tue, 30 Oct 2012 14:14:42 -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 17/23] pefile: Strip the wrapper off of the cert data block

On Tue, Oct 30, 2012 at 12:21 PM, David Howells <dhowells@...hat.com> wrote:
> The certificate data block in a PE binary has a wrapper around the PKCS#7
> signature we actually want to get at.  Strip this off and check that we've got
> something that appears to be a PKCS#7 signature.
>
> Signed-off-by: David Howells <dhowells@...hat.com>
> ---
>
>  crypto/asymmetric_keys/pefile_parser.c |   60 ++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
>
> diff --git a/crypto/asymmetric_keys/pefile_parser.c b/crypto/asymmetric_keys/pefile_parser.c
> index efae278..24c117e 100644
> --- a/crypto/asymmetric_keys/pefile_parser.c
> +++ b/crypto/asymmetric_keys/pefile_parser.c
> @@ -15,6 +15,7 @@
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  #include <linux/pe.h>
> +#include <linux/asn1.h>
>  #include <keys/asymmetric-subtype.h>
>  #include <keys/asymmetric-parser.h>
>  #include <crypto/hash.h>
> @@ -138,6 +139,61 @@ static int pefile_parse_binary(struct key_preparsed_payload *prep,
>  }
>
>  /*
> + * Check and strip the PE wrapper from around the signature and check that the
> + * remnant looks something like PKCS#7.
> + */
> +static int pefile_strip_sig_wrapper(struct key_preparsed_payload *prep,
> +                                   struct pefile_context *ctx)
> +{
> +       struct win_certificate wrapper;
> +       const u8 *pkcs7;
> +
> +       if (ctx->sig_len < 8) {
> +               pr_devel("Signature wrapper too short\n");
> +               return -ELIBBAD;
> +       }
> +
> +       memcpy(&wrapper, prep->data + ctx->sig_offset, 8);

Instead of the literal 8, sizeof(wrapper)?

> +       pr_devel("sig wrapper = { %x, %x, %x }\n",
> +                wrapper.length, wrapper.revision, wrapper.cert_type);
> +       if (wrapper.length != ctx->sig_len) {
> +               pr_devel("Signature wrapper len wrong\n");
> +               return -ELIBBAD;
> +       }
> +       if (wrapper.revision != WIN_CERT_REVISION_2_0) {
> +               pr_devel("Signature is not revision 2.0\n");
> +               return -ENOTSUPP;
> +       }
> +       if (wrapper.cert_type != WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> +               pr_devel("Signature certificate type is not PKCS\n");
> +               return -ENOTSUPP;
> +       }
> +
> +       ctx->sig_offset += 8;
> +       ctx->sig_len -= 8;

Same for these?

> +       if (ctx->sig_len == 0) {
> +               pr_devel("Signature data missing\n");
> +               return -EKEYREJECTED;
> +       }
> +
> +       /* What's left should a PKCS#7 cert */
> +       pkcs7 = prep->data + ctx->sig_offset;
> +       if (pkcs7[0] == (ASN1_CONS_BIT | ASN1_SEQ)) {
> +               if (pkcs7[1] == 0x82 &&
> +                   pkcs7[2] == (((ctx->sig_len - 4) >> 8) & 0xff) &&
> +                   pkcs7[3] ==  ((ctx->sig_len - 4)       & 0xff))
> +                       return 0;
> +               if (pkcs7[1] == 0x80)
> +                       return 0;
> +               if (pkcs7[1] > 0x82)
> +                       return -EMSGSIZE;

Can these literals be replaced with something more meaningful?

> +       }
> +
> +       pr_devel("Signature data not PKCS#7\n");
> +       return -ELIBBAD;
> +}
> +
> +/*
>   * Parse a PE binary.
>   */
>  static int pefile_key_preparse(struct key_preparsed_payload *prep)
> @@ -152,6 +208,10 @@ static int pefile_key_preparse(struct key_preparsed_payload *prep)
>         if (ret < 0)
>                 return ret;
>
> +       ret = pefile_strip_sig_wrapper(prep, &ctx);
> +       if (ret < 0)
> +               return ret;
> +
>         return -ENOANO; // Not yet complete
>  }
>
>



-- 
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/