[edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Wrap OpenSSL HKDF algorithm
Gary West
gary.west at intel.com
Mon Jul 29 16:38:47 UTC 2019
I will update and resubmit for item a (add NULL) and item b (remove duplicate EVP_PKEY_CTX_free) but the changing the code as suggested looks more complex 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 circuit seems much more complex to me. There are several other ways I could implement 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 not after second one.
b) The duplicate calling of EVP_PKEY_CTX_free() can be saved with a local variable 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
Sent: Thursday, July 25, 2019 7:59 PM
To: West, Gary <gary.west at intel.com>; devel at edk2.groups.io
Cc: Ye, Ting <ting.ye at intel.com>
Subject: RE: [PATCH 1/1] CryptoPkg/BaseCryptLib: Wrap OpenSSL HKDF algorithm
Gary,
See my comment below.
> -----Original Message-----
> From: West, Gary
> Sent: Tuesday, July 23, 2019 8:04 PM
> To: devel at edk2.groups.io
> Cc: West, Gary <gary.west at intel.com>; West, Gary
> <gary.west at intel.com>; Wang, Jian J <jian.j.wang at intel.com>; Ye, Ting
> <ting.ye at intel.com>
> Subject: [PATCH 1/1] CryptoPkg/BaseCryptLib: Wrap OpenSSL HKDF
> algorithm
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1928
>
> 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
>
> Signed-off-by: Gary West <Gary.West at intel.com>
> Cc: Jian Wang <jian.j.wang at intel.com>
> Cc: Ting Ye <ting.ye at intel.com>
> ---
> .../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
> 100644 CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
> create mode 100644 CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c
>
> 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
>
> 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
> );
>
> +//========================================================
> =============================
> +// Key Derivation Function Primitive
> +//========================================================
> =============================
> +
> +/**
> + 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
> +reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/BaseCryptLib.h>
> +#include <openssl/evp.h>
> +#include <openssl/kdf.h>
> +
> +/**
> + 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 == NULL || Salt == NULL || Info == NULL || Out == NULL ||
> + KeySize > INT_MAX || SaltSize > INT_MAX || InfoSize > INT_MAX ||
> OutSize > INT_MAX ) {
> + return FALSE;
> + }
> +
> + pHkdfCtx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL); if (pHkdfCtx
> + == NULL) {
> + goto _Error;
> + }
> +
> + if (EVP_PKEY_derive_init(pHkdfCtx) <= 0) {
> + goto _Error;
> + }
> + if (EVP_PKEY_CTX_set_hkdf_md(pHkdfCtx, EVP_sha256()) <= 0) {
> + goto _Error;
> + }
> + if (EVP_PKEY_CTX_set1_hkdf_salt(pHkdfCtx, Salt, (UINT32)SaltSize)
> + <= 0)
> {
> + goto _Error;
> + }
> + if (EVP_PKEY_CTX_set1_hkdf_key(pHkdfCtx, Key, (UINT32)KeySize) <=
> + 0)
> {
> + goto _Error;
> + }
> + if (EVP_PKEY_CTX_add1_hkdf_info(pHkdfCtx, Info, (UINT32)InfoSize)
> + <= 0)
> {
> + goto _Error;
> + }
> + if (EVP_PKEY_derive(pHkdfCtx, Out, &OutSize) <= 0) {
> + goto _Error;
> + }
> +
> + EVP_PKEY_CTX_free(pHkdfCtx);
> + pHkdfCtx = 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 not after second one.
b) The duplicate calling of EVP_PKEY_CTX_free() can be saved with a local variable 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 = FALSE;
pHkdfCtx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL);
if (pHkdfCtx != 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 = TRUE;
}
EVP_PKEY_CTX_free(pHkdfCtx);
pHkdfCtx = 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
> +reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/BaseCryptLib.h>
> +#include <Library/DebugLib.h>
> +
> +/**
> + 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44565): https://edk2.groups.io/g/devel/message/44565
Mute This Topic: https://groups.io/mt/32572735/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list