[openssl-dev] sizeof (HMAC_CTX) changes with update, breaks binary compatibility

[Matt Caswell]

On 12/06/15 09:49, Timo Teras wrote:
> On Fri, 12 Jun 2015 11:27:42 +0300
> Timo Teras <timo.teras at iki.fi> wrote:
> 
>> On Thu, 11 Jun 2015 21:09:59 -0400
>> Dan McDonald <danmcd at omniti.com> wrote:
>>
>>>
>>>> On Jun 11, 2015, at 9:07 PM, Dan McDonald <danmcd at omniti.com>
>>>> wrote:
>>>>
>>>> typedef struct hmac_ctx_st {
>>>>   const EVP_MD *md;
>>>>   EVP_MD_CTX md_ctx;
>>>>   EVP_MD_CTX i_ctx;
>>>>   EVP_MD_CTX o_ctx;
>>>>   unsigned int key_length;
>>>>   unsigned char key[HMAC_MAX_MD_CBLOCK];
>>>> + int key_init;
>>>> } HMAC_CTX;
>>>
>>> A cheesy, but binary compatible, fix might be:
>>>
>>> typedef struct hmac_ctx_st {
>>>   const EVP_MD *md;
>>>   EVP_MD_CTX md_ctx;
>>>   EVP_MD_CTX i_ctx;
>>>   EVP_MD_CTX o_ctx;
>>>   unsigned int key_init:1;   /* Ewww, cheesy use of bitfields... */
>>>   unsigned int key_length:31;  /* but the sizeof (HMAC_CTX) doesn't
>>> change! */ unsigned char key[HMAC_MAX_MD_CBLOCK];
>>> } HMAC_CTX;
>>
>> Why is separate key_init needid? Could we not use md!=NULL or
>> key_length!=0 checks to see if the context is initialized?
> 
> Seems it'd be along with the line to just use key_length instead.
> 
> Something along the lines of:

Your patch does introduce a change in behaviour if key != NULL but len
== 0. Previously this would set ctx->key to all 0s, and key_init to 1,
and would then continue to use that all zero key. A subsequent
invocation of HMAC_Init_ex with key == NULL would reuse that all zero
key. Your patch would allow the first invocation, but error out on the
second.

Should it be a valid use case to allow an all zero key in this way?

Matt



> 
> diff --git a/crypto/hmac/hmac.c b/crypto/hmac/hmac.c
> index 5925467..e6876bf 100644
> --- a/crypto/hmac/hmac.c
> +++ b/crypto/hmac/hmac.c
> @@ -97,7 +97,7 @@ int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, int len,
>          return 0;
>      }
>  
> -    if (!ctx->key_init && key == NULL)
> +    if (!ctx->key_length && key == NULL)
>          return 0;
>  
>      if (key != NULL) {
> @@ -121,7 +121,6 @@ int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, int len,
>          if (ctx->key_length != HMAC_MAX_MD_CBLOCK)
>              memset(&ctx->key[ctx->key_length], 0,
>                     HMAC_MAX_MD_CBLOCK - ctx->key_length);
> -        ctx->key_init = 1;
>      }
>  
>      if (reset) {
> @@ -159,7 +158,7 @@ int HMAC_Update(HMAC_CTX *ctx, const unsigned char *data, size_t len)
>      if (FIPS_mode() && !ctx->i_ctx.engine)
>          return FIPS_hmac_update(ctx, data, len);
>  #endif
> -    if (!ctx->key_init)
> +    if (!ctx->key_length)
>          return 0;
>  
>      return EVP_DigestUpdate(&ctx->md_ctx, data, len);
> @@ -174,7 +173,7 @@ int HMAC_Final(HMAC_CTX *ctx, unsigned char *md, unsigned int *len)
>          return FIPS_hmac_final(ctx, md, len);
>  #endif
>  
> -    if (!ctx->key_init)
> +    if (!ctx->key_length)
>          goto err;
>  
>      if (!EVP_DigestFinal_ex(&ctx->md_ctx, buf, &i))
> @@ -195,7 +194,7 @@ void HMAC_CTX_init(HMAC_CTX *ctx)
>      EVP_MD_CTX_init(&ctx->i_ctx);
>      EVP_MD_CTX_init(&ctx->o_ctx);
>      EVP_MD_CTX_init(&ctx->md_ctx);
> -    ctx->key_init = 0;
> +    ctx->key_length = 0;
>      ctx->md = NULL;
>  }
>  
> @@ -207,11 +206,8 @@ int HMAC_CTX_copy(HMAC_CTX *dctx, HMAC_CTX *sctx)
>          goto err;
>      if (!EVP_MD_CTX_copy(&dctx->md_ctx, &sctx->md_ctx))
>          goto err;
> -    dctx->key_init = sctx->key_init;
> -    if (sctx->key_init) {
> -        memcpy(dctx->key, sctx->key, HMAC_MAX_MD_CBLOCK);
> -        dctx->key_length = sctx->key_length;
> -    }
> +    memcpy(dctx->key, sctx->key, HMAC_MAX_MD_CBLOCK);
> +    dctx->key_length = sctx->key_length;
>      dctx->md = sctx->md;
>      return 1;
>   err:
> diff --git a/crypto/hmac/hmac.h b/crypto/hmac/hmac.h
> index f8e9f5e..b8b55cd 100644
> --- a/crypto/hmac/hmac.h
> +++ b/crypto/hmac/hmac.h
> @@ -79,7 +79,6 @@ typedef struct hmac_ctx_st {
>      EVP_MD_CTX o_ctx;
>      unsigned int key_length;
>      unsigned char key[HMAC_MAX_MD_CBLOCK];
> -    int key_init;
>  } HMAC_CTX;
>  
>  # define HMAC_size(e)    (EVP_MD_size((e)->md))
> 
> _______________________________________________
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>