[openssl-dev] sizeof (HMAC_CTX) changes with update, breaks binary compatibility
Timo Teras
timo.teras at iki.fi
Fri Jun 12 10:16:25 UTC 2015
On Fri, 12 Jun 2015 10:38:02 +0100
Matt Caswell <matt at openssl.org> wrote:
>
>
> 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?
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.
More information about the openssl-dev
mailing list