Re: [edk2-devel] [PATCH v5] CryptoPkg/Pkcs7: Extend support for other OID types

["Wang, Jian J" <jian.j.wang@intel.com>]

Guomin,

Sorry I didn't check carefully on the return type of function Pkcs7TypeIsOther().
I think BOOLEAN should be more appropriate. INTN looks strange, although it
works. It's actually used to check if the PKCS7 is data or not. Son in this function
TRUE or FALSE should be returned instead of 1 or 0.

 If using BOOLEAN, you can also use this function directly in following expression,
without comparing to '1', which looks strange to me too.

+  if (Pkcs7TypeIsOther(P7) && P7->d.other &&

In addition, 'P7->d.other' should use explicit check against NULL.

With above addressed,

	Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin
> Jiang
> Sent: Tuesday, April 28, 2020 8:50 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: [edk2-devel] [PATCH v5] CryptoPkg/Pkcs7: Extend support for other OID
> types
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2539
> 
> Microsoft signtool supports creation of attached P7's with any OID payload
> via the "/p7co" parameter. It is necessary to check the data before get
> the string.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> ---
>  .../BaseCryptLib/Pk/CryptPkcs7VerifyBase.c    | 66 ++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c
> b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c
> index 313f459b11..1b23ec99af 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c
> @@ -13,6 +13,66 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <openssl/x509v3.h>
> 
>  #include <openssl/pkcs7.h>
> 
> 
> 
> +/**
> 
> +  Check the contents of PKCS7 is not data.
> 
> +
> 
> +  It is copied from PKCS7_type_is_other() in pk7_doit.c.
> 
> +
> 
> +  @param[in] P7 Pointer to the location at which the PKCS7 is located.
> 
> +
> 
> +  @return INTN The content type.
> 
> +**/
> 
> +STATIC
> 
> +INTN
> 
> +Pkcs7TypeIsOther (
> 
> +  IN PKCS7 *P7
> 
> +  )
> 
> +{
> 
> +  INTN Others = 1;
> 
> +  INTN Nid = OBJ_obj2nid (P7->type);
> 
> +
> 
> +  switch (Nid) {
> 
> +    case NID_pkcs7_data:
> 
> +    case NID_pkcs7_signed:
> 
> +    case NID_pkcs7_enveloped:
> 
> +    case NID_pkcs7_signedAndEnveloped:
> 
> +    case NID_pkcs7_encrypted:
> 
> +      Others = 0;
> 
> +      break;
> 
> +    default:
> 
> +      Others = 1;
> 
> +  }
> 
> +
> 
> +  return Others;
> 
> +}
> 
> +
> 
> +/**
> 
> +  Get the ASN.1 string for the PKCS7.
> 
> +
> 
> +  It is copied from PKCS7_get_octet_string() in pk7_doit.c.
> 
> +
> 
> +  @param[in] P7 Pointer to the location at which the PKCS7 is located.
> 
> +
> 
> +  @return ASN1_OCTET_STRING ASN.1 string.
> 
> +**/
> 
> +STATIC
> 
> +ASN1_OCTET_STRING*
> 
> +Pkcs7GetOctetString (
> 
> +  IN PKCS7 *P7
> 
> +  )
> 
> +{
> 
> +  if (PKCS7_type_is_data (P7)) {
> 
> +    return P7->d.data;
> 
> +  }
> 
> +
> 
> +  if ((Pkcs7TypeIsOther(P7) == 1) && P7->d.other &&
> 
> +      (P7->d.other->type == V_ASN1_OCTET_STRING)) {
> 
> +    return P7->d.other->value.octet_string;
> 
> +  }
> 
> +
> 
> +  return NULL;
> 
> +}
> 
> +
> 
>  /**
> 
>    Extracts the attached content from a PKCS#7 signed data if existed. The input
> signed
> 
>    data could be wrapped in a ContentInfo structure.
> 
> @@ -98,7 +158,11 @@ Pkcs7GetAttachedContent (
>      //
> 
>      // Retrieve the attached content in PKCS7 signedData
> 
>      //
> 
> -    OctStr = Pkcs7->d.sign->contents->d.data;
> 
> +    OctStr = Pkcs7GetOctetString (Pkcs7->d.sign->contents);
> 
> +    if (OctStr == NULL) {
> 
> +      goto _Exit;
> 
> +    }
> 
> +
> 
>      if ((OctStr->length > 0) && (OctStr->data != NULL)) {
> 
>        *ContentSize = OctStr->length;
> 
>        *Content     = AllocatePool (*ContentSize);
> 
> --
> 2.25.1.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#58197): https://edk2.groups.io/g/devel/message/58197
> Mute This Topic: https://groups.io/mt/73318347/1768734
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jian.j.wang@intel.com]
> -=-=-=-=-=-=