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.w...@intel.com>; devel@edk2.groups.io
> Cc: Lu, XiaoyuX <xiaoyux...@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 <ler...@redhat.com>
> >> Sent: Wednesday, January 08, 2020 6:24 PM
> >> To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io
> >> Cc: Lu, XiaoyuX <xiaoyux...@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
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53206): https://edk2.groups.io/g/devel/message/53206
Mute This Topic: https://groups.io/mt/69523367/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
