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 <[email protected]> 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
