sizeof (HMAC_CTX) changes with update, breaks binary compatibility

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Dan McDonald
I noticed that a new field was added to HMAC_CTX in the 1.0.2a->b or 1.0.1m->n update:

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;

This breaks binary compatibility.  I found this out the hard way during an attempt to update OmniOS's OpenSSL to 1.0.2b ('014, bloody) or 1.0.1n (006, 012).  Observe our use of HMAC_CTX in illumos (which OmniOS is a distribution of):

struct Mac {
        char            *name;
        int             enabled;
        u_int           mac_len;
        u_char          *key;
        u_int           key_len;
        int             type;
        const EVP_MD    *evp_md;
        HMAC_CTX        evp_ctx;
};
struct Comp {
        int     type;
        int     enabled;
        char    *name;
};
struct Newkeys {
        Enc     enc;
        Mac     mac;
        Comp    comp; /* XXX KEBE SAYS THIS GETS CLOBBERED!!! */
};

You can see the code here:

        http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/ssh/include/kex.h#100

What is supposed to happen in this situation?  I was under the impression that letter releases don't break binary compatibility.  The SSH in illumos breaks because of this, but it appears OpenSSH has worked around such a situation.

Clues are welcome.

Thanks,
Dan McDonald -- OmniOS Engineering

_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Dan McDonald

> On Jun 11, 2015, at 9:07 PM, Dan McDonald <[hidden email]> 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;


Dan

_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Peter Waltenberg

Which is exactly why our hacked version of OpenSSL has
allocators/deallocators for all these private struct's.

It'd be really nice if OpenSSL would fix this, adding them won't break
backwards compatibility (i.e. API breakage isn't an excuse for not fixing
these)  and going forwards problems like this would stop occuring.

Peter

_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

mancha
In reply to this post by Dan McDonald
On Thu, Jun 11, 2015 at 09:07:18PM -0400, Dan McDonald wrote:

> I noticed that a new field was added to HMAC_CTX in the 1.0.2a->b or
> 1.0.1m->n update:
>
> 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;
>
> This breaks binary compatibility.  I found this out the hard way
> during an attempt to update OmniOS's OpenSSL to 1.0.2b ('014, bloody)
> or 1.0.1n (006, 012).  Observe our use of HMAC_CTX in illumos (which
> OmniOS is a distribution of):
>
> struct Mac { char            *name; int             enabled; u_int
> mac_len; u_char          *key; u_int           key_len; int
> type; const EVP_MD    *evp_md; HMAC_CTX        evp_ctx; }; struct Comp
> { int     type; int     enabled; char    *name; }; struct Newkeys {
> Enc     enc; Mac     mac; Comp    comp; /* XXX KEBE SAYS THIS GETS
> CLOBBERED!!! */ };
>
> You can see the code here:
>
> http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/ssh/include/kex.h#100
>
> What is supposed to happen in this situation?  I was under the
> impression that letter releases don't break binary compatibility.  The
> SSH in illumos breaks because of this, but it appears OpenSSH has
> worked around such a situation.
>
> Clues are welcome.
>
> Thanks, Dan McDonald -- OmniOS Engineering
Hi Dan. Many thanks for your report. I've checked and the issue you've
identified potentially affects OpenSSH 4.7 through 6.5, inclusive.

OpenSSH 6.6 replaces the OpenSSL HMAC with its own implementation making
the ABI change a NOP for OpenSSH 6.6 onwards.  

Cheers.

--mancha

_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

attachment0 (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Timo Teras
In reply to this post by Dan McDonald
On Thu, 11 Jun 2015 21:09:59 -0400
Dan McDonald <[hidden email]> wrote:

>
> > On Jun 11, 2015, at 9:07 PM, Dan McDonald <[hidden email]> 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?
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Timo Teras
On Fri, 12 Jun 2015 11:27:42 +0300
Timo Teras <[hidden email]> wrote:

> On Thu, 11 Jun 2015 21:09:59 -0400
> Dan McDonald <[hidden email]> wrote:
>
> >
> > > On Jun 11, 2015, at 9:07 PM, Dan McDonald <[hidden email]>
> > > 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))

_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Tomas Mraz-2
On Pá, 2015-06-12 at 11:49 +0300, Timo Teras wrote:

> On Fri, 12 Jun 2015 11:27:42 +0300
> Timo Teras <[hidden email]> wrote:
>
> > On Thu, 11 Jun 2015 21:09:59 -0400
> > Dan McDonald <[hidden email]> wrote:
> >
> > >
> > > > On Jun 11, 2015, at 9:07 PM, Dan McDonald <[hidden email]>
> > > > 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.

Yes, this needs to be reverted and key_length or some other workaround
used instead. I had a hope that breaking ABI compatibility on 1.0.x
branches is no-go.

--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
(You'll never know whether the road is wrong though.)


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Matt Caswell-2


On 12/06/15 09:52, Tomas Mraz wrote:

> On Pá, 2015-06-12 at 11:49 +0300, Timo Teras wrote:
>> On Fri, 12 Jun 2015 11:27:42 +0300
>> Timo Teras <[hidden email]> wrote:
>>
>>> On Thu, 11 Jun 2015 21:09:59 -0400
>>> Dan McDonald <[hidden email]> wrote:
>>>
>>>>
>>>>> On Jun 11, 2015, at 9:07 PM, Dan McDonald <[hidden email]>
>>>>> 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.
>
> Yes, this needs to be reverted and key_length or some other workaround
> used instead. I had a hope that breaking ABI compatibility on 1.0.x
> branches is no-go.
>

Breaking ABI compatibility on 1.0.x is (supposed to be) a no-go. We're
just debating what the appropriate response is to this issue.

Matt
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Matt Caswell-2
In reply to this post by Timo Teras


On 12/06/15 09:49, Timo Teras wrote:

> On Fri, 12 Jun 2015 11:27:42 +0300
> Timo Teras <[hidden email]> wrote:
>
>> On Thu, 11 Jun 2015 21:09:59 -0400
>> Dan McDonald <[hidden email]> wrote:
>>
>>>
>>>> On Jun 11, 2015, at 9:07 PM, Dan McDonald <[hidden email]>
>>>> 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
>
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Timo Teras
On Fri, 12 Jun 2015 10:38:02 +0100
Matt Caswell <[hidden email]> wrote:

>
>
> On 12/06/15 09:49, Timo Teras wrote:
> > On Fri, 12 Jun 2015 11:27:42 +0300
> > Timo Teras <[hidden email]> wrote:
> >
> >> On Thu, 11 Jun 2015 21:09:59 -0400
> >> Dan McDonald <[hidden email]> wrote:
> >>
> >>>
> >>>> On Jun 11, 2015, at 9:07 PM, Dan McDonald <[hidden email]>
> >>>> 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?

This raises another concern. If md is changed, but key is not, things
go wrong anyway. I think we should just disallow chaning md without
key.

The problem is that if md is changed, we need to rehash using the new
md (in case they key >= HMAC_MAX_MD_CBLOCK). This was not allowed
earlier. So let's just require specifying key if md changes.

We can in fact remove using key_length altogether then. I think
key_length should be assigned to EVP_MD_block_size(md) always. Because
they key is technically zero-padded anyway to this length.
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Matt Caswell-2


On 12/06/15 11:16, Timo Teras wrote:

>>>> 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?
>
> This raises another concern. If md is changed, but key is not, things
> go wrong anyway.

Hmmm...yes, this is a problem.

> I think we should just disallow chaning md without
> key.
>
> The problem is that if md is changed, we need to rehash using the new
> md (in case they key >= HMAC_MAX_MD_CBLOCK). This was not allowed
> earlier. So let's just require specifying key if md changes.
>
> We can in fact remove using key_length altogether then. I think
> key_length should be assigned to EVP_MD_block_size(md) always. Because
> they key is technically zero-padded anyway to this length.
>

Previously, it would work to do this:

HMAC_Init_ex(ctx, NULL, 0, md, NULL);
HMAC_Init_ex(ctx, key, len, NULL, NULL);

The first call above would actually read uninitialised ctx->key
data...but then throw away any results in the second call.

I'm not sure we could get rid of key_length altogether and deal with the
above?

Matt
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: sizeof (HMAC_CTX) changes with update, breaks binary compatibility

Matt Caswell-2


On 12/06/15 12:26, Matt Caswell wrote:

>
>
> On 12/06/15 11:16, Timo Teras wrote:
>>>>> 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?
>>
>> This raises another concern. If md is changed, but key is not, things
>> go wrong anyway.
>
> Hmmm...yes, this is a problem.
>
>> I think we should just disallow chaning md without
>> key.
>>
>> The problem is that if md is changed, we need to rehash using the new
>> md (in case they key >= HMAC_MAX_MD_CBLOCK). This was not allowed
>> earlier. So let's just require specifying key if md changes.
>>
>> We can in fact remove using key_length altogether then. I think
>> key_length should be assigned to EVP_MD_block_size(md) always. Because
>> they key is technically zero-padded anyway to this length.
>>
>
> Previously, it would work to do this:
>
> HMAC_Init_ex(ctx, NULL, 0, md, NULL);
> HMAC_Init_ex(ctx, key, len, NULL, NULL);
>
> The first call above would actually read uninitialised ctx->key
> data...but then throw away any results in the second call.

Actually thinking about it, I think that might be a corner case too
far...and I now remember that that was one of the corner cases that the
key_init change prevented.

So that would make the change look more like this:

From d392a99d2d7910a3aea8b7fc33e52dfcb115c2b5 Mon Sep 17 00:00:00 2001
From: Matt Caswell <[hidden email]>
Date: Fri, 12 Jun 2015 13:08:04 +0100
Subject: [PATCH] Fix ABI break with HMAC

Recent HMAC changes broke ABI compatibility due to a new field in HMAC_CTX.
This backs that change out, and does it a different way.

Thanks to Timo Teras for the concept.
---
 crypto/hmac/hmac.c     | 20 ++++++++------------
 include/openssl/hmac.h |  1 -
 test/hmactest.c        |  7 ++++++-
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/crypto/hmac/hmac.c b/crypto/hmac/hmac.c
index d50fabb..7699b0b 100644
--- a/crypto/hmac/hmac.c
+++ b/crypto/hmac/hmac.c
@@ -68,6 +68,10 @@ int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, int len,
     int i, j, reset = 0;
     unsigned char pad[HMAC_MAX_MD_CBLOCK];

+    /* If we are changing MD then we must have a key */
+    if (md != NULL && md != ctx->md && (key == NULL || len < 0))
+        return 0;
+
     if (md != NULL) {
         reset = 1;
         ctx->md = md;
@@ -77,9 +81,6 @@ int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, int len,
         return 0;
     }

-    if (!ctx->key_init && key == NULL)
-        return 0;
-
     if (key != NULL) {
         reset = 1;
         j = M_EVP_MD_block_size(md);
@@ -101,7 +102,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) {
@@ -137,7 +137,7 @@ int HMAC_Init(HMAC_CTX *ctx, const void *key, int
len, const EVP_MD *md)

 int HMAC_Update(HMAC_CTX *ctx, const unsigned char *data, size_t len)
 {
-    if (!ctx->key_init)
+    if (!ctx->md)
         return 0;
     return EVP_DigestUpdate(&ctx->md_ctx, data, len);
 }
@@ -147,7 +147,7 @@ int HMAC_Final(HMAC_CTX *ctx, unsigned char *md,
unsigned int *len)
     unsigned int i;
     unsigned char buf[EVP_MAX_MD_SIZE];

-    if (!ctx->key_init)
+    if (!ctx->md)
         goto err;

     if (!EVP_DigestFinal_ex(&ctx->md_ctx, buf, &i))
@@ -168,7 +168,6 @@ 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->md = NULL;
 }

@@ -181,11 +180,8 @@ int HMAC_CTX_copy(HMAC_CTX *dctx, HMAC_CTX *sctx)
         goto err;
     if (!EVP_MD_CTX_copy_ex(&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/include/openssl/hmac.h b/include/openssl/hmac.h
index 61946fc..81aa49d 100644
--- a/include/openssl/hmac.h
+++ b/include/openssl/hmac.h
@@ -75,7 +75,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))
diff --git a/test/hmactest.c b/test/hmactest.c
index 13344d6..a9b829d 100644
--- a/test/hmactest.c
+++ b/test/hmactest.c
@@ -226,7 +226,12 @@ test5:
         err++;
         goto test6;
     }
-    if (!HMAC_Init_ex(&ctx, NULL, 0, EVP_sha256(), NULL)) {
+    if (HMAC_Init_ex(&ctx, NULL, 0, EVP_sha256(), NULL)) {
+        printf("Should disallow changing MD without a new key (test 5)\n");
+        err++;
+        goto test6;
+    }
+    if (!HMAC_Init_ex(&ctx, test[4].key, test[4].key_len, EVP_sha256(),
NULL)) {
         printf("Failed to reinitialise HMAC (test 5)\n");
         err++;
         goto test6;
--
2.1.4

Matt


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev