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

2012-10-30 Thread David Howells 
Kees Cook  wrote:

> > +   memcpy(, prep->data + ctx->sig_offset, 8);
> 
> Instead of the literal 8, sizeof(wrapper)?

Reasonable.  It was originally an array of bytes until I found out that it had
structure.  Even so, I should probably have used sizeof() then.

> > +   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?

Certainly the 0x80 can.  I have a patch to define ASN1_INDEFINITE_LENGTH.  The
question is how to define the 0x82 since it's a flag plus a value.  Maybe by
using something like:

#define ASN1_LONG_LENGTH(x) (0x80 | (x))

Anyway, thanks for the reviews!

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-10-30 Thread Kees Cook 
On Tue, Oct 30, 2012 at 12:21 PM, David Howells  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 
> ---
>
>  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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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(, 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, );
> +   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-10-30 Thread David Howells 
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 
---

 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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(, prep->data + ctx->sig_offset, 8);
+   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;
+   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;
+   }
+
+   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, );
+   if (ret < 0)
+   return ret;
+
return -ENOANO; // Not yet complete
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-10-30 Thread David Howells 
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 dhowe...@redhat.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);
+   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;
+   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;
+   }
+
+   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
 }
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-10-30 Thread Kees Cook 
On Tue, Oct 30, 2012 at 12:21 PM, David Howells dhowe...@redhat.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 dhowe...@redhat.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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-10-30 Thread David Howells 
Kees Cook keesc...@chromium.org wrote:

  +   memcpy(wrapper, prep-data + ctx-sig_offset, 8);
 
 Instead of the literal 8, sizeof(wrapper)?

Reasonable.  It was originally an array of bytes until I found out that it had
structure.  Even so, I should probably have used sizeof() then.

  +   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?

Certainly the 0x80 can.  I have a patch to define ASN1_INDEFINITE_LENGTH.  The
question is how to define the 0x82 since it's a flag plus a value.  Maybe by
using something like:

#define ASN1_LONG_LENGTH(x) (0x80 | (x))

Anyway, thanks for the reviews!

David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/