Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface

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

On 01/09/20 03:40, Wang, Jian J wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, January 08, 2020 6:24 PM
>> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
>> Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com>
>> Subject: Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate
>> HmacXxxGetContextSize interface
>>
>> On 01/08/20 08:26, Jian J Wang wrote:

>>> /**
>>>   Initializes user-supplied memory pointed by HmacSha256Context as HMAC-
>> 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 == 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) != 1) {
>>>     return FALSE;
>>>   }
>>>   if (HMAC_Init_ex ((HMAC_CTX *)HmacSha256Context, Key, (UINT32) KeySize,
>> EVP_sha256(), NULL) != 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 *appropriate*
> 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 declaration.
> 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 re-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 edk2
>> 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 the only
> way to do that. I suggest to keep it.
> 
>> (3) In case we'd like to continue providing functions that accept "Key"
>> 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 allocated
>> inside OpenSSL, and returned to the caller.
> 
> Yes, the variable encryption feature I'm working on needs to supply user
> supplied key. I think it'd be better to keep it. Like I suggested above, we
> should not allow user-supplied context and it's almost impossible for use
> to supply correct size of context.

Assuming I understand your response correctly, I would suggest:

(1) Renaming "Init" to "SetKey",

(2) deleting both the memset() and the HMAC_CTX_reset() calls from "SetKey"

(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).

The goal of (1) is to clearly distinguish the key setting action from
allocation/initialization.

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).

Thanks,
Laszlo