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

[Timo Teras]

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:

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