Forbid reusing HMAC key without reusing the hash function.

There's no good reason to do this, and it doesn't work; HMAC checks the length
of the key and runs it through the hash function if too long. The reuse occurs
after this check.

This allows us to shave 132 bytes off HMAC_CTX as this was the only reason it
ever stored the original key. It also slightly simplifies HMAC_Init_ex's
logic.

Change-Id: Ib56aabc3630b7178f1ee7c38ef6370c9638efbab
Reviewed-on: https://boringssl-review.googlesource.com/3733
Reviewed-by: Adam Langley <agl@google.com>

crypto/hmac/hmac.c

@@ -88,7 +88,6 @@ uint8_t *HMAC(const EVP_MD *evp_md, const void *key, size_t key_len,
 
 void HMAC_CTX_init(HMAC_CTX *ctx) {
   ctx->md = NULL;
-  ctx->key_length = 0;
   EVP_MD_CTX_init(&ctx->i_ctx);
   EVP_MD_CTX_init(&ctx->o_ctx);
   EVP_MD_CTX_init(&ctx->md_ctx);
@@ -103,48 +102,44 @@ void HMAC_CTX_cleanup(HMAC_CTX *ctx) {
 
 int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, size_t key_len,
                  const EVP_MD *md, ENGINE *impl) {
-  unsigned i, reset = 0;
-  uint8_t pad[HMAC_MAX_MD_CBLOCK];
-
-  if (md != NULL) {
-    if (ctx->md == NULL && key == NULL && ctx->key_length == 0) {
-      /* TODO(eroman): Change the API instead of this hack.
-       * If a key hasn't yet been assigned to the context, then default to using
-       * an all-zero key. This is to work around callers of
-       * HMAC_Init_ex(key=NULL, key_len=0) intending to set a zero-length key.
-       * Rather than resulting in uninitialized memory reads, it will
-       * predictably use a zero key. */
-      memset(ctx->key, 0, sizeof(ctx->key));
-    }
-    reset = 1;
-    ctx->md = md;
-  } else {
+  if (md == NULL) {
     md = ctx->md;
   }
 
-  if (key != NULL) {
+  /* If either |key| is non-NULL or |md| has changed, initialize with a new key
+   * rather than rewinding the previous one.
+   *
+   * TODO(davidben,eroman): Passing the previous |md| with a NULL |key| is
+   * ambiguous between using the empty key and reusing the previous key. There
+   * exist callers which intend the latter, but the former is an awkward edge
+   * case. Fix to API to avoid this. */
+  if (md != ctx->md || key != NULL) {
+    size_t i;
+    uint8_t pad[HMAC_MAX_MD_CBLOCK];
+    uint8_t key_block[HMAC_MAX_MD_CBLOCK];
+    unsigned key_block_len;
+
     size_t block_size = EVP_MD_block_size(md);
-    reset = 1;
-    assert(block_size <= sizeof(ctx->key));
+    assert(block_size <= sizeof(key_block));
     if (block_size < key_len) {
+      /* Long keys are hashed. */
       if (!EVP_DigestInit_ex(&ctx->md_ctx, md, impl) ||
           !EVP_DigestUpdate(&ctx->md_ctx, key, key_len) ||
-          !EVP_DigestFinal_ex(&(ctx->md_ctx), ctx->key, &ctx->key_length)) {
+          !EVP_DigestFinal_ex(&ctx->md_ctx, key_block, &key_block_len)) {
         goto err;
       }
     } else {
-      assert(key_len >= 0 && key_len <= sizeof(ctx->key));
-      memcpy(ctx->key, key, key_len);
-      ctx->key_length = key_len;
+      assert(key_len >= 0 && key_len <= sizeof(key_block));
+      memcpy(key_block, key, key_len);
+      key_block_len = (unsigned)key_len;
     }
-    if (ctx->key_length != HMAC_MAX_MD_CBLOCK) {
-      memset(&ctx->key[ctx->key_length], 0, sizeof(ctx->key) - ctx->key_length);
+    /* Keys are then padded with zeros. */
+    if (key_block_len != HMAC_MAX_MD_CBLOCK) {
+      memset(&key_block[key_block_len], 0, sizeof(key_block) - key_block_len);
     }
-  }
 
-  if (reset) {
     for (i = 0; i < HMAC_MAX_MD_CBLOCK; i++) {
-      pad[i] = 0x36 ^ ctx->key[i];
+      pad[i] = 0x36 ^ key_block[i];
     }
     if (!EVP_DigestInit_ex(&ctx->i_ctx, md, impl) ||
         !EVP_DigestUpdate(&ctx->i_ctx, pad, EVP_MD_block_size(md))) {
@@ -152,12 +147,14 @@ int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, size_t key_len,
     }
 
     for (i = 0; i < HMAC_MAX_MD_CBLOCK; i++) {
-      pad[i] = 0x5c ^ ctx->key[i];
+      pad[i] = 0x5c ^ key_block[i];
     }
     if (!EVP_DigestInit_ex(&ctx->o_ctx, md, impl) ||
         !EVP_DigestUpdate(&ctx->o_ctx, pad, EVP_MD_block_size(md))) {
       goto err;
     }
+
+    ctx->md = md;
   }
 
   if (!EVP_MD_CTX_copy_ex(&ctx->md_ctx, &ctx->i_ctx)) {
@@ -200,8 +197,6 @@ int HMAC_CTX_copy_ex(HMAC_CTX *dest, const HMAC_CTX *src) {
     return 0;
   }
 
-  memcpy(dest->key, src->key, HMAC_MAX_MD_CBLOCK);
-  dest->key_length = src->key_length;
   dest->md = src->md;
   return 1;
 }

include/openssl/hmac.h

@@ -94,9 +94,14 @@ OPENSSL_EXPORT void HMAC_CTX_init(HMAC_CTX *ctx);
 OPENSSL_EXPORT void HMAC_CTX_cleanup(HMAC_CTX *ctx);
 
 /* HMAC_Init_ex sets up an initialised |HMAC_CTX| to use |md| as the hash
- * function and |key| as the key. Any of |md| or |key| can be NULL, in which
- * case the previous value will be used. It returns one on success or zero
- * otherwise. */
+ * function and |key| as the key. For a non-initial call, |md| may be NULL, in
+ * which case the previous hash function will be used. If the hash function has
+ * not changed and |key| is NULL, |ctx| reuses the previous key. It returns one
+ * on success or zero otherwise.
+ *
+ * WARNING: NULL and empty keys are ambiguous on non-initial calls. Passing NULL
+ * |key| but repeating the previous |md| reuses the previous key rather than the
+ * empty key. */
 OPENSSL_EXPORT int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, size_t key_len,
                                 const EVP_MD *md, ENGINE *impl);
 
@@ -152,8 +157,6 @@ struct hmac_ctx_st {
   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];
 } /* HMAC_CTX */;