Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size

["Laszlo Ersek" <lersek@redhat.com>]

On 04/30/19 03:16, Wang, Jian J wrote:
> Laszlo,
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, April 30, 2019 2:31 AM
>> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata
>> HMAC_ctx size
>>
>> On 04/29/19 10:15, Xiaoyu lu wrote:
>>> From: Xiaoyu Lu <xiaoyux.lu@intel.com>
>>>
>>> Openssl internally redefines the size of HMAC_CTX,
>>> but there is no external definition.
>>> So add an additional nubmer.
>>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Ting Ye <ting.ye@intel.com>
>>> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
>>> ---
>>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c    | 11 ++++++++++-
>>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c   | 12 ++++++++++--
>>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12 ++++++++++-
>> -
>>>  3 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
>> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
>>> index 3134806..3ffb8e2 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
>>> @@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>  #include "InternalCryptLib.h"
>>>  #include <openssl/hmac.h>
>>>
>>> +//
>>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
>>> +//       #define HMAC_MAX_MD_CBLOCK 128
>>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
>>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
>>> +//       But we need to compatible with previous API.
>>> +//       So fix it with correct size 144-128 = 16.
>>> +//
>>>  #define HMAC_MD5_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
>>> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
>>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>>> +
>>>
>>>  /**
>>>    Retrieves the size, in bytes, of the context buffer required for HMAC-MD5
>> operations.
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
>> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
>>> index bbe3df4..e59602e 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
>>> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>  #include "InternalCryptLib.h"
>>>  #include <openssl/hmac.h>
>>>
>>> -#define HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
>>> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
>>> +//
>>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
>>> +//       #define HMAC_MAX_MD_CBLOCK 128
>>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
>>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
>>> +//       But we need to compatible with previous API.
>>> +//       So fix it with correct size 144-128 = 16.
>>> +//
>>> +#define  HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
>>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>>>
>>>  /**
>>>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1
>> operations.
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
>> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
>>> index ac9084f..8d0570b 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
>>> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>  #include "InternalCryptLib.h"
>>>  #include <openssl/hmac.h>
>>>
>>> -#define HMAC_SHA256_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
>>> -                               sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
>>> +//
>>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
>>> +//       #define HMAC_MAX_MD_CBLOCK 128
>>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
>>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
>>> +//       But we need to compatible with previous API.
>>> +//       So fix it with correct size 144-128 = 16.
>>> +//
>>> +#define HMAC_SHA256_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) +
>> \
>>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>>>
>>>  /**
>>>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256
>> operations.
>>>
>>
>> I believe I disagree with this patch.
>>
>> The issue is well described here:
>>
>>   https://github.com/openssl/openssl/pull/4338
>>
>> And the real solution, for edk2, is apparently described in edk2
>> already:
>>
>> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c]:
>>
>>> /**
>>>   Retrieves the size, in bytes, of the context buffer required for HMAC-MD5
>> operations.
>>>   (NOTE: This API is deprecated.
>>>          Use HmacMd5New() / HmacMd5Free() for HMAC-MD5 Context
>> operations.)
>>>
>>>   @return  The size, in bytes, of the context buffer required for HMAC-MD5
>> operations.
>>>
>>> **/
>>> UINTN
>>> EFIAPI
>>> HmacMd5GetContextSize (
>>>   VOID
>>>   )
>>> {
>>>   //
>>>   // Retrieves the OpenSSL HMAC-MD5 Context Size
>>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
>> use the
>>>   //       fixed size as a workaround to make this API work for compatibility.
>>>   //       We should retire HmacMd5GetContextSize() in future, and use
>> HmacMd5New()
>>>   //       and HmacMd5Free() for context allocation and release.
>>>   //
>>>   return (UINTN) HMAC_MD5_CTX_SIZE;
>>> }
>>
>> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c]:
>>
>>> /**
>>>   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1
>> operations.
>>>   (NOTE: This API is deprecated.
>>>          Use HmacSha1New() / HmacSha1Free() for HMAC-SHA1 Context
>> operations.)
>>>
>>>   @return  The size, in bytes, of the context buffer required for HMAC-SHA1
>> operations.
>>>
>>> **/
>>> UINTN
>>> EFIAPI
>>> HmacSha1GetContextSize (
>>>   VOID
>>>   )
>>> {
>>>   //
>>>   // Retrieves the OpenSSL HMAC-SHA1 Context Size
>>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
>> use the
>>>   //       fixed size as a workaround to make this API work for compatibility.
>>>   //       We should retire HmacSha15GetContextSize() in future, and use
>> HmacSha1New()
>>>   //       and HmacSha1Free() for context allocation and release.
>>>   //
>>>   return (UINTN) HMAC_SHA1_CTX_SIZE;
>>> }
>>
>> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c]:
>>
>>> /**
>>>   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256
>> operations.
>>>   (NOTE: This API is deprecated.
>>>          Use HmacSha256New() / HmacSha256Free() for HMAC-SHA256 Context
>> operations.)
>>>
>>>   @return  The size, in bytes, of the context buffer required for HMAC-SHA256
>> operations.
>>>
>>> **/
>>> UINTN
>>> EFIAPI
>>> HmacSha256GetContextSize (
>>>   VOID
>>>   )
>>> {
>>>   //
>>>   // Retrieves the OpenSSL HMAC-SHA256 Context Size
>>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
>> use the
>>>   //       fixed size as a workaround to make this API work for compatibility.
>>>   //       We should retire HmacSha256GetContextSize() in future, and use
>> HmacSha256New()
>>>   //       and HmacSha256Free() for context allocation and release.
>>>   //
>>>   return (UINTN)HMAC_SHA256_CTX_SIZE;
>>> }
>>
>> Is there any reason why we can't clean up this code first, in a separate
>> patch series, before moving to OpenSSL 1.1.1?
>>
>> Because, the "NOTE"s are more than two years old now... They come from
>> commit 4c270243995a ("CryptoPkg: Update HMAC Wrapper with opaque
>> HMAC_CTX object.", 2017-03-29).
>>
>> This has been our technical debt ever since, violating the OpenSSL
>> effort to hide implementation details. I think we should eliminate this
>> debt, rather than make it worse.
>>
>> (Obviously this is just my opinion, and I'm not a CryptoPkg
>> co-maintainer.)
>>
> 
> I agree that we need to clean up the deprecated macros and api. The problem
> is that XxxGetContextSize() are public APIs and there're dozens of modules which
> still call them. This can't be done in short time. What about we file a BZ to track
> this issue and target to next stable tag?

I don't see any calls to either of HmacMd5GetContextSize,
HmacSha1GetContextSize, and HmacSha256GetContextSize in the open source
edk2 tree.

Do you mean those dozens of calls are in the edk2-platforms project, or
in proprietary platfoms?

I do see that the change affects the <BaseCryptLib.h> lib class header,
so it seems justified to postpone the real fix to the next stable tag.
Please file a BZ for this.

Regarding this patch:

(1) The commit message should explain the situation a lot more clearly.
It should reference both

- https://github.com/openssl/openssl/pull/4338

and

- OpenSSL commit e0810e3502bb ("Fix HMAC SHA3-224 and HMAC SHA3-256.",
2018-09-04)

(2) The commit message should also reference the new TianoCore BZ (about
removing these functions from the lib class).

(3) The code comments are confusing, and should be deleted, in my
opinion. A good commit message will suffice.

In particular, the following part of the comment:

  "we need to compatible with previous API"

is very confusing, as it first made me think that we should stick with
the *previous* value, i.e. 128, for compatibility with callers of these
functions.

(4) In the patch (= the new code), HMAC_MAX_MD_CBLOCK should be
eliminated completely, and the sum (HMAC_MAX_MD_CBLOCK + 16) should be
replaced with the constant 144.

The fact that the new context size is 16 bytes larger than what it used
to be (due to the introduction of SHA3-224) is circumstantial. The old
value (128) is irrelevant, so we shouldn't express the new value
relative to the old value. The new size is 144 bytes because the new
size is 144 bytes.

Thanks
Laszlo