From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web11.1885.1587975418237238971 for ; Mon, 27 Apr 2020 01:16:58 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: jian.j.wang@intel.com) IronPort-SDR: 79c2g8gYuV0Rx3Iy/ZEBCQ6im77JnLPMW7bpx5iZ5ey2DpOghG2HNhnYex49IvnhjmG7OvQJgl rAF8MNNuTVqg== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Apr 2020 01:16:57 -0700 IronPort-SDR: gFJlTE+NqoA2pvFcGvA7W7m1cH4Jw9LvK7Xjy/mYM3vLq1sQzlHLUvZZoPiZWOUf+uazjeFmx0 Hs3gFYYpJVNQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,323,1583222400"; d="scan'208";a="336195466" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga001.jf.intel.com with ESMTP; 27 Apr 2020 01:16:56 -0700 Received: from fmsmsx123.amr.corp.intel.com (10.18.125.38) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 27 Apr 2020 01:16:56 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx123.amr.corp.intel.com (10.18.125.38) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 27 Apr 2020 01:16:56 -0700 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.191]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.7]) with mapi id 14.03.0439.000; Mon, 27 Apr 2020 16:16:53 +0800 From: "Wang, Jian J" To: "Jiang, Guomin" , "devel@edk2.groups.io" CC: "Lu, XiaoyuX" Subject: Re: [PATCH v4] CryptoPkg/Pkcs7: Extend support for other OID types Thread-Topic: [PATCH v4] CryptoPkg/Pkcs7: Extend support for other OID types Thread-Index: AQHWG40qz1J5lSuHWEWqmsjU26sMY6iMm9yg Date: Mon, 27 Apr 2020 08:16:52 +0000 Message-ID: References: <20200426054009.792-1-guomin.jiang@intel.com> In-Reply-To: <20200426054009.792-1-guomin.jiang@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jian.j.wang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Guomin, A few comments below. > -----Original Message----- > From: Jiang, Guomin > Sent: Sunday, April 26, 2020 1:40 PM > To: devel@edk2.groups.io > Cc: Wang, Jian J ; Lu, XiaoyuX > Subject: [PATCH v4] CryptoPkg/Pkcs7: Extend support for other OID types >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2539 >=20 > Microsoft signtool supports creation of attached P7's with any OID payloa= d > via the "/p7co" parameter. It is necessary to check the data before get > the string. >=20 > Cc: Jian J Wang > Cc: Xiaoyu Lu > Signed-off-by: Guomin Jiang > --- > .../BaseCryptLib/Pk/CryptPkcs7VerifyBase.c | 65 ++++++++++++++++++- > 1 file changed, 64 insertions(+), 1 deletion(-) >=20 > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c > index 313f459b11..70d9880897 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c > @@ -13,6 +13,65 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include >=20 > #include >=20 >=20 >=20 > +/** >=20 > + Check the contents of PKCS7 is not data. >=20 > + >=20 > + It is copied from PKCS7_type_is_other() in pk7_doit.c. >=20 > + >=20 > + @param P7 Pointer to the location which the PKCS7 is located at. >=20 > + >=20 > + @return UINT8 The content type. >=20 > +**/ >=20 > +static Please use STATIC instead, according to ch5.6.1.2 of "EDK II Coding Standards 2.1". https://github.com/tianocore-docs/Docs/raw/master/Specifications/CCS_2_1_Dr= aft.pdf >=20 > +UINT8 >=20 > +Pkcs7TypeIsOther ( >=20 > + PKCS7 *P7 Missing IN/OUT modifier for parameter, according to ch5.7.1.11 of "EDK II Coding Standards 2.1". Add in/out in function parameter description comment as well. https://github.com/tianocore-docs/Docs/raw/master/Specifications/CCS_2_1_Dr= aft.pdf >=20 > + ) >=20 > +{ >=20 > + UINT8 Others =3D 1; >=20 > + UINT8 Nid =3D (UINT8) OBJ_obj2nid (P7->type); >=20 > + >=20 > + switch (Nid) { >=20 > + case NID_pkcs7_data: >=20 > + case NID_pkcs7_signed: >=20 > + case NID_pkcs7_enveloped: >=20 > + case NID_pkcs7_signedAndEnveloped: >=20 > + case NID_pkcs7_encrypted: >=20 > + Others =3D 0; >=20 > + break; >=20 > + default: >=20 > + Others =3D 1; >=20 > + } >=20 > + >=20 > + return Others; >=20 > +} >=20 > + >=20 > +/** >=20 > + Get the ASN.1 string for the PKCS7. >=20 > + >=20 > + It is copied from PKCS7_get_octet_string() in pk7_doit.c. >=20 > + @param P7 Pointer to the location which the PKCS7 is located at. >=20 > + >=20 > + @return ASN1_OCTET_STRING ASN.1 string. >=20 > +**/ >=20 > +static Same as above, use STATIC instead. >=20 > +ASN1_OCTET_STRING* >=20 > +Pkcs7GetOctetString ( >=20 > + PKCS7 *P7 Missing IN/OUT modifier for parameter, according to ch5.7.1.11 of "EDK II Coding Standards 2.1". Add in/out in function parameter description comment as well. Regards, Jian >=20 > + ) >=20 > +{ >=20 > + if (PKCS7_type_is_data (P7)) { >=20 > + return P7->d.data; >=20 > + } >=20 > + >=20 > + if ((Pkcs7TypeIsOther(P7) =3D=3D 1) && P7->d.other && >=20 > + (P7->d.other->type =3D=3D V_ASN1_OCTET_STRING)) { >=20 > + return P7->d.other->value.octet_string; >=20 > + } >=20 > + >=20 > + return NULL; >=20 > +} >=20 > + >=20 > /** >=20 > Extracts the attached content from a PKCS#7 signed data if existed. Th= e input > signed >=20 > data could be wrapped in a ContentInfo structure. >=20 > @@ -98,7 +157,11 @@ Pkcs7GetAttachedContent ( > // >=20 > // Retrieve the attached content in PKCS7 signedData >=20 > // >=20 > - OctStr =3D Pkcs7->d.sign->contents->d.data; >=20 > + OctStr =3D Pkcs7GetOctetString (Pkcs7->d.sign->contents); >=20 > + if (OctStr =3D=3D NULL) { >=20 > + goto _Exit; >=20 > + } >=20 > + >=20 > if ((OctStr->length > 0) && (OctStr->data !=3D NULL)) { >=20 > *ContentSize =3D OctStr->length; >=20 > *Content =3D AllocatePool (*ContentSize); >=20 > -- > 2.25.1.windows.1