From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: gary.west@intel.com) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by groups.io with SMTP; Mon, 29 Jul 2019 09:38:48 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jul 2019 09:38:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,323,1559545200"; d="scan'208";a="322924034" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga004.jf.intel.com with ESMTP; 29 Jul 2019 09:38:48 -0700 Received: from fmsmsx126.amr.corp.intel.com ([169.254.1.198]) by FMSMSX105.amr.corp.intel.com ([169.254.4.116]) with mapi id 14.03.0439.000; Mon, 29 Jul 2019 09:38:47 -0700 From: "Gary West" To: "Wang, Jian J" , "devel@edk2.groups.io" CC: "Ye, Ting" Subject: Re: [PATCH 1/1] CryptoPkg/BaseCryptLib: Wrap OpenSSL HKDF algorithm Thread-Topic: [PATCH 1/1] CryptoPkg/BaseCryptLib: Wrap OpenSSL HKDF algorithm Thread-Index: AQHVQLqdkxrSGborZkmtuDZ8x4O9jqbcsAsAgAUkg0A= Date: Mon, 29 Jul 2019 16:38:47 +0000 Message-ID: <6A95350B14795443A16CB18A60A1467D7280089F@FMSMSX126.amr.corp.intel.com> References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNGU2YTA3ZTgtNjdmYy00ZDU3LTlhOTAtN2ZkOTU2MmZkZjYwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoibHFUU1BCSkNHOFdQSVlGRWx5bTdxeXN0SStcLytkdDAxa0hcL2VoN2xkazkxbTV1eFlzSUlxdVNuMytoNkFZVm16In0= dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.1.200.107] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I will update and resubmit for item a (add NULL) and item b (remove duplica= te EVP_PKEY_CTX_free) but the changing the code as suggested looks more com= plex to me. I think the user of the label follows the pattern of other EDK2= code. A single if with multiple function calls and relying on short circui= t seems much more complex to me. There are several other ways I could imple= ment without the label if that is the requirement. Some coding style comment: a) pHkdfCtx is set to NULL after first calling of EVP_PKEY_CTX_free() but n= ot after second one. b) The duplicate calling of EVP_PKEY_CTX_free() can be saved with a local v= ariable to store the return value (see below example). c) The whole function is not complex. Maybe the label _Error can be saved. -----Original Message----- From: Wang, Jian J=20 Sent: Thursday, July 25, 2019 7:59 PM To: West, Gary ; devel@edk2.groups.io Cc: Ye, Ting Subject: RE: [PATCH 1/1] CryptoPkg/BaseCryptLib: Wrap OpenSSL HKDF algorith= m Gary, See my comment below. > -----Original Message----- > From: West, Gary > Sent: Tuesday, July 23, 2019 8:04 PM > To: devel@edk2.groups.io > Cc: West, Gary ; West, Gary=20 > ; Wang, Jian J ; Ye, Ting=20 > > Subject: [PATCH 1/1] CryptoPkg/BaseCryptLib: Wrap OpenSSL HKDF=20 > algorithm >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1928 >=20 > 1. Implement OpenSSL HKDF wrapped function in CryptHkdf.c file. > 2. Implement stub implementation function in CryptHkdfNull.c file. > 3. Add wrapped HKDF function declaration to BaseCryptLib.h file. > 4. Add CryptHkdf.c to module information BaseCryptLib.inf file. > 5. Add CryptHkdfNull.c to module information PeiCryptLib.inf, > RuntimeCryptLib.inf and SmmCryptLib.inf >=20 > Signed-off-by: Gary West > Cc: Jian Wang > Cc: Ting Ye > --- > .../Library/BaseCryptLib/BaseCryptLib.inf | 1 + > .../Library/BaseCryptLib/PeiCryptLib.inf | 4 +- > .../Library/BaseCryptLib/RuntimeCryptLib.inf | 1 + > .../Library/BaseCryptLib/SmmCryptLib.inf | 1 + > CryptoPkg/Include/Library/BaseCryptLib.h | 33 ++++++++ > .../Library/BaseCryptLib/Kdf/CryptHkdf.c | 80 +++++++++++++++++++ > .../Library/BaseCryptLib/Kdf/CryptHkdfNull.c | 43 ++++++++++ > 7 files changed, 160 insertions(+), 3 deletions(-) create mode=20 > 100644 CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c > create mode 100644 CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c >=20 > diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > index 020df3c19b3c..8d4988e8c6b4 100644 > --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > @@ -37,6 +37,7 @@ [Sources] > Hmac/CryptHmacMd5.c > Hmac/CryptHmacSha1.c > Hmac/CryptHmacSha256.c > + Kdf/CryptHkdf.c > Cipher/CryptAes.c > Cipher/CryptTdes.c > Cipher/CryptArc4.c > diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > index 4c4353747622..d26161d79ae5 100644 > --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > @@ -43,10 +43,10 @@ [Sources] > Hmac/CryptHmacMd5Null.c > Hmac/CryptHmacSha1Null.c > Hmac/CryptHmacSha256Null.c > + Kdf/CryptHkdfNull.c > Cipher/CryptAesNull.c > Cipher/CryptTdesNull.c > Cipher/CryptArc4Null.c > - > Pk/CryptRsaBasic.c > Pk/CryptRsaExtNull.c > Pk/CryptPkcs1OaepNull.c > @@ -55,13 +55,11 @@ [Sources] > Pk/CryptPkcs7VerifyCommon.c > Pk/CryptPkcs7VerifyBase.c > Pk/CryptPkcs7VerifyEku.c > - > Pk/CryptDhNull.c > Pk/CryptX509Null.c > Pk/CryptAuthenticodeNull.c > Pk/CryptTsNull.c > Pem/CryptPemNull.c > - > Rand/CryptRandNull.c >=20 > SysCall/CrtWrapper.c > diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > index a59079d99e05..e99c046be29b 100644 > --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > @@ -42,6 +42,7 @@ [Sources] > Hmac/CryptHmacMd5Null.c > Hmac/CryptHmacSha1Null.c > Hmac/CryptHmacSha256Null.c > + Kdf/CryptHkdfNull.c > Cipher/CryptAesNull.c > Cipher/CryptTdesNull.c > Cipher/CryptArc4Null.c > diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > index 3fd7d65abfca..fc217938825d 100644 > --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > @@ -42,6 +42,7 @@ [Sources] > Hmac/CryptHmacMd5Null.c > Hmac/CryptHmacSha1Null.c > Hmac/CryptHmacSha256.c > + Kdf/CryptHkdfNull.c > Cipher/CryptAes.c > Cipher/CryptTdesNull.c > Cipher/CryptArc4Null.c > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h > b/CryptoPkg/Include/Library/BaseCryptLib.h > index 19d1afe3c8c0..da32bb2444fd 100644 > --- a/CryptoPkg/Include/Library/BaseCryptLib.h > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h > @@ -3122,4 +3122,37 @@ RandomBytes ( > IN UINTN Size > ); >=20 > +//=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > +// Key Derivation Function Primitive > +//=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > + > +/** > + Derive key data using HMAC-SHA256 based KDF. > + > + @param[in] Key Pointer to the user-supplied key. > + @param[in] KeySize Key size in bytes. > + @param[in] Salt Pointer to the salt(non-secret) value. > + @param[in] SaltSize Salt size in bytes. > + @param[in] Info Pointer to the application specific info= . > + @param[in] InfoSize Info size in bytes. > + @param[Out] Out Pointer to buffer to receive hkdf value. > + @param[in] OutSize Size of hkdf bytes to generate. > + > + @retval TRUE Hkdf generated successfully. > + @retval FALSE Hkdf generation failed. > + > +**/ > +BOOLEAN > +EFIAPI > +HkdfSha256ExtractAndExpand ( > + IN CONST UINT8 *Key, > + IN UINTN KeySize, > + IN CONST UINT8 *Salt, > + IN UINTN SaltSize, > + IN CONST UINT8 *Info, > + IN UINTN InfoSize, > + OUT UINT8 *Out, > + IN UINTN OutSize > + ); > + > #endif // __BASE_CRYPT_LIB_H__ > diff --git a/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c > b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c > new file mode 100644 > index 000000000000..c0b307806232 > --- /dev/null > +++ b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c > @@ -0,0 +1,80 @@ > +/** @file > + HMAC-SHA256 KDF Wrapper Implementation over OpenSSL. > + > +Copyright (c) 2018 - 2019, Intel Corporation. All rights=20 > +reserved.
> +SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > + > +/** > + Derive HMAC-based Extract-and-Expand Key Derivation Function (HKDF). > + > + @param[in] Key Pointer to the user-supplied key. > + @param[in] KeySize Key size in bytes. > + @param[in] Salt Pointer to the salt(non-secret) value. > + @param[in] SaltSize Salt size in bytes. > + @param[in] Info Pointer to the application specific info= . > + @param[in] InfoSize Info size in bytes. > + @param[Out] Out Pointer to buffer to receive hkdf value. > + @param[in] OutSize Size of hkdf bytes to generate. > + > + @retval TRUE Hkdf generated successfully. > + @retval FALSE Hkdf generation failed. > + > +**/ > +BOOLEAN > +EFIAPI > +HkdfSha256ExtractAndExpand ( > + IN CONST UINT8 *Key, > + IN UINTN KeySize, > + IN CONST UINT8 *Salt, > + IN UINTN SaltSize, > + IN CONST UINT8 *Info, > + IN UINTN InfoSize, > + OUT UINT8 *Out, > + IN UINTN OutSize > + ) > +{ > + EVP_PKEY_CTX *pHkdfCtx; > + > + if (Key =3D=3D NULL || Salt =3D=3D NULL || Info =3D=3D NULL || Out =3D= =3D NULL || > + KeySize > INT_MAX || SaltSize > INT_MAX || InfoSize > INT_MAX || > OutSize > INT_MAX ) { > + return FALSE; > + } > + > + pHkdfCtx =3D EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL); if (pHkdfCtx=20 > + =3D=3D NULL) { > + goto _Error; > + } > + > + if (EVP_PKEY_derive_init(pHkdfCtx) <=3D 0) { > + goto _Error; > + } > + if (EVP_PKEY_CTX_set_hkdf_md(pHkdfCtx, EVP_sha256()) <=3D 0) { > + goto _Error; > + } > + if (EVP_PKEY_CTX_set1_hkdf_salt(pHkdfCtx, Salt, (UINT32)SaltSize)=20 > + <=3D 0) > { > + goto _Error; > + } > + if (EVP_PKEY_CTX_set1_hkdf_key(pHkdfCtx, Key, (UINT32)KeySize) <=3D=20 > + 0) > { > + goto _Error; > + } > + if (EVP_PKEY_CTX_add1_hkdf_info(pHkdfCtx, Info, (UINT32)InfoSize)=20 > + <=3D 0) > { > + goto _Error; > + } > + if (EVP_PKEY_derive(pHkdfCtx, Out, &OutSize) <=3D 0) { > + goto _Error; > + } > + > + EVP_PKEY_CTX_free(pHkdfCtx); > + pHkdfCtx =3D NULL; > + return TRUE; > + > +_Error: > + EVP_PKEY_CTX_free(pHkdfCtx); > + return FALSE; Some coding style comment: a) pHkdfCtx is set to NULL after first calling of EVP_PKEY_CTX_free() but n= ot after second one. b) The duplicate calling of EVP_PKEY_CTX_free() can be saved with a local v= ariable to store the return value (see below example). c) The whole function is not complex. Maybe the label _Error can be saved. Following is just an example. You don't have to do the same way. --------------- Result =3D FALSE; pHkdfCtx =3D EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL); if (pHkdfCtx !=3D NULL && EVP_PKEY_derive_init(pHkdfCtx) > 0 && EVP_PKEY_CTX_set_hkdf_md(pHkdfCtx, EVP_sha256()) > 0 && EVP_PKEY_CTX_set1_hkdf_salt(pHkdfCtx, Salt, (UINT32)SaltSize) > 0 && EVP_PKEY_CTX_set1_hkdf_key(pHkdfCtx, Key, (UINT32)KeySize) > 0 && EVP_PKEY_CTX_add1_hkdf_info(pHkdfCtx, Info, (UINT32)InfoSize) > 0 && EVP_PKEY_derive(pHkdfCtx, Out, &OutSize) > 0) { Result =3D TRUE; } EVP_PKEY_CTX_free(pHkdfCtx); pHkdfCtx =3D NULL; return Result; --------------- Regards, Jian > +} > diff --git a/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c > b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c > new file mode 100644 > index 000000000000..73deb5bc3614 > --- /dev/null > +++ b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c > @@ -0,0 +1,43 @@ > +/** @file > + HMAC-SHA256 KDF Wrapper Implementation which does not provide > real capabilities. > + > +Copyright (c) 2018 - 2019, Intel Corporation. All rights=20 > +reserved.
> +SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > + > +/** > + Derive key data using HMAC-SHA256 based KDF. > + > + @param[in] Key Pointer to the user-supplied key. > + @param[in] KeySize Key size in bytes. > + @param[in] Salt Pointer to the salt(non-secret) value. > + @param[in] SaltSize Salt size in bytes. > + @param[in] Info Pointer to the application specific info= . > + @param[in] InfoSize Info size in bytes. > + @param[Out] Out Pointer to buffer to receive hkdf value. > + @param[in] OutSize Size of hkdf bytes to generate. > + > + @retval TRUE Hkdf generated successfully. > + @retval FALSE Hkdf generation failed. > + > +**/ > +BOOLEAN > +EFIAPI > +HkdfSha256ExtractAndExpand ( > + IN CONST UINT8 *Key, > + IN UINTN KeySize, > + IN CONST UINT8 *Salt, > + IN UINTN SaltSize, > + IN CONST UINT8 *Info, > + IN UINTN InfoSize, > + OUT UINT8 *Out, > + IN UINTN OutSize > + ) > +{ > + ASSERT (FALSE); > + return FALSE; > +} > -- > 2.19.1.windows.1