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