From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web12.6864.1578981008799183286 for ; Mon, 13 Jan 2020 21:50:08 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: jian.j.wang@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Jan 2020 21:50:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,431,1571727600"; d="scan'208";a="247926138" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga004.fm.intel.com with ESMTP; 13 Jan 2020 21:50:07 -0800 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 13 Jan 2020 21:50:06 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 13 Jan 2020 21:50:05 -0800 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.210]) by shsmsx102.ccr.corp.intel.com ([169.254.2.202]) with mapi id 14.03.0439.000; Tue, 14 Jan 2020 13:50:03 +0800 From: "Wang, Jian J" To: "devel@edk2.groups.io" , "lersek@redhat.com" CC: "Lu, XiaoyuX" Subject: Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface Thread-Topic: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface Thread-Index: AQHVxg3FwUti2CW6eku6rPZVfSIeR6fhmHeAgABFAQCAB9Lm0A== Date: Tue, 14 Jan 2020 05:50:02 +0000 Message-ID: References: <20200108072650.1353-1-jian.j.wang@intel.com> <74e5fa4b-9932-c25d-f71f-699000eaaff9@redhat.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMmEwOTNlMDAtNDMwYS00YmVjLWJmZjQtY2IxNGIzZWRkN2VhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiUTlVdDh1akpMV2VqUldYeFBHUnFqRWNSSFp3MkN4eVM4cFVTaEVFbkxMTlFoNWlMZFJIaHpTYzd5WG5pYjN2MyJ9 x-ctpclassification: CTP_NT 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 Laszlo, Good suggestions. I'll send v2 patch soon. Regards, Jian > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Laszlo Er= sek > Sent: Thursday, January 09, 2020 10:20 PM > To: Wang, Jian J ; devel@edk2.groups.io > Cc: Lu, XiaoyuX > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate > HmacXxxGetContextSize interface >=20 > On 01/09/20 03:40, Wang, Jian J wrote: > >> -----Original Message----- > >> From: Laszlo Ersek > >> Sent: Wednesday, January 08, 2020 6:24 PM > >> To: Wang, Jian J ; devel@edk2.groups.io > >> Cc: Lu, XiaoyuX > >> Subject: Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate > >> HmacXxxGetContextSize interface > >> > >> On 01/08/20 08:26, Jian J Wang wrote: >=20 > >>> /** > >>> Initializes user-supplied memory pointed by HmacSha256Context as H= MAC- > >> SHA256 context for > >>> subsequent use. > >>> > >>> If HmacSha256Context is NULL, then return FALSE. > >>> > >>> @param[out] HmacSha256Context Pointer to HMAC-SHA256 context > being > >> initialized. > >>> @param[in] Key Pointer to the user-supplied key. > >>> @param[in] KeySize Key size in bytes. > >>> > >>> @retval TRUE HMAC-SHA256 context initialization succeeded. > >>> @retval FALSE HMAC-SHA256 context initialization failed. > >>> > >>> **/ > >>> BOOLEAN > >>> EFIAPI > >>> HmacSha256Init ( > >>> OUT VOID *HmacSha256Context, > >>> IN CONST UINT8 *Key, > >>> IN UINTN KeySize > >>> ) > >>> { > >>> // > >>> // Check input parameters. > >>> // > >>> if (HmacSha256Context =3D=3D NULL || KeySize > INT_MAX) { > >>> return FALSE; > >>> } > >>> > >>> // > >>> // OpenSSL HMAC-SHA256 Context Initialization > >>> // > >>> memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE); > >>> if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) !=3D 1) { > >>> return FALSE; > >>> } > >>> if (HMAC_Init_ex ((HMAC_CTX *)HmacSha256Context, Key, (UINT32) > KeySize, > >> EVP_sha256(), NULL) !=3D 1) { > >>> return FALSE; > >>> } > >>> > >>> return TRUE; > >>> } > >> > >> As the leading comment says, "HmacSha256Context" is user-supplied > >> memory. If you remove the memset() call from the function, then > >> HMAC_CTX_reset() will be invoked on user-supplied memory that may not > >> have been cleared. Then HMAC_CTX_reset() will be called on garbage. > >> > > > > You're right, if the user can supply a chunk of memory with *appropria= te* > > size as HmacContext. Since we deleted the macro HMAC_XXX_CTX_SIZE, > > it's impossible for user to do that now. HMAC_CTX is a forward declara= tion. > > MSVC refuses to give result of sizeof (HMAC_CTX). The user cannot know > > how many bytes needed by HMAC_CTX. Therefore there's no such use cases > > any longer. I think we could update the comments to enforce the use of > > HmacXxxNew() to get context. User supplied-memory is not acceptable. > > > > We can still keep the HMAC_CTX_reset line so that the user can still r= e-use > > the context got before by HmacXxxNew(). I think HMAC_CTX_reset works > > well with an empty Context or init-ed Context. > > > >> (2) The only way that I can see for fixing this problem is to remove = the > >> Hmac(Md5|Sha1|Sha256)Init functions too. > >> > >> I think that is safe to do, because I can't see any callers in the ed= k2 > >> codebase. > >> > >> One tricky part is that the leading comments of the > >> Hmac(Md5|Sha1|Sha256)(Update|Final) functions refer to > >> Hmac(Md5|Sha1|Sha256)Init. In other words, we do not have code > >> references to Hmac(Md5|Sha1|Sha256)Init, but we have documentation > >> references. This means that those comments should be updated as well = -- > >> they should refer to Hmac(Md5|Sha1|Sha256)New instead. > >> > > > > The Init interface is needed to supply user's key for HMAC. It seems t= he only > > way to do that. I suggest to keep it. > > > >> (3) In case we'd like to continue providing functions that accept "Ke= y" > >> and "KeySize", for HMAC context initialization, then those functions > >> will have to call HMAC_CTX_new() internally. Meaning that they can no > >> longer take user-supplied memory; the context will have to be allocat= ed > >> inside OpenSSL, and returned to the caller. > > > > Yes, the variable encryption feature I'm working on needs to supply us= er > > supplied key. I think it'd be better to keep it. Like I suggested abov= e, we > > should not allow user-supplied context and it's almost impossible for = use > > to supply correct size of context. >=20 > Assuming I understand your response correctly, I would suggest: >=20 > (1) Renaming "Init" to "SetKey", >=20 > (2) deleting both the memset() and the HMAC_CTX_reset() calls from "SetK= ey" >=20 > (3) updating the comment on "SetKey" so that it does not refer to "user > supplied memory"; instead, it should say that "SetKey" can only be > called on context returned by the appropriate "New" call, and only > immediately after the "New" call (no intervening operations permitted on > the context). >=20 > The goal of (1) is to clearly distinguish the key setting action from > allocation/initialization. >=20 > The goal of (2) and (3) together is to have a pristine (zalloc, reset, > Init_ex) triplet, with no repeated actions, when getting a new context, > and setting a key in it (that is, when the caller invokes New and then > SetKey). >=20 > Thanks, > Laszlo >=20 >=20 >=20