[openssl-dev] sizeof (HMAC_CTX) changes with update, breaks binary compatibility
Matt Caswell
matt at openssl.org
Fri Jun 12 12:18:48 UTC 2015
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 <matt at openssl.org>
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
More information about the openssl-dev
mailing list