Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface

["Wang, Jian J" <jian.j.wang@intel.com>]

Laszlo,

Good suggestions. I'll send v2 patch soon.

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Thursday, January 09, 2020 10:20 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate
> HmacXxxGetContextSize interface
> 
> 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
> 
> 
>