commit 6b35262272e435aa5a6366b3d846fc9125984261
author Adam Langley <agl@google.com> Tue Sep 19 09:53:19 2017 -0700
committer Adam Langley <agl@google.com> Wed Sep 20 18:43:21 2017 +0000
tree 5e9845d0a87d1d9db247cd162368e3aa9b125afe
parent 40b24c81546f44cfc840715e8a1c1b5060d313f3

Maintain EVP_MD_CTX invariants.

Thanks to Lennart Beringer for pointing that that malloc failures could
lead to invalid EVP_MD_CTX states. This change cleans up the code in
general so that fallible operations are all performed before mutating
objects. Thus failures should leave objects in a valid state.

Also, |ctx_size| is never zero and a hash with no context is not
sensible, so stop handling that case and simply assert that it doesn't
occur.

Change-Id: Ia60c3796dcf2f772f55e12e49431af6475f64d52
Reviewed-on: https://boringssl-review.googlesource.com/20544
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>

crypto/fipsmodule/digest/digest.c

diff --git a/crypto/fipsmodule/digest/digest.c b/crypto/fipsmodule/digest/digest.c
index 886c910..8afd1e1 100644
--- a/crypto/fipsmodule/digest/digest.c
+++ b/crypto/fipsmodule/digest/digest.c
@@ -90,9 +90,7 @@
 }
 
 int EVP_MD_CTX_cleanup(EVP_MD_CTX *ctx) {
-  if (ctx->digest && ctx->digest->ctx_size && ctx->md_data) {
-    OPENSSL_free(ctx->md_data);
-  }
+  OPENSSL_free(ctx->md_data);
 
   assert(ctx->pctx == NULL || ctx->pctx_ops != NULL);
   if (ctx->pctx_ops) {
@@ -114,17 +112,35 @@
 }
 
 int EVP_MD_CTX_copy_ex(EVP_MD_CTX *out, const EVP_MD_CTX *in) {
-  uint8_t *tmp_buf = NULL;
-
   if (in == NULL || in->digest == NULL) {
     OPENSSL_PUT_ERROR(DIGEST, DIGEST_R_INPUT_NOT_INITIALIZED);
     return 0;
   }
 
-  if (out->digest == in->digest) {
-    // |md_data| will be the correct size in this case so it's removed from
-    // |out| at this point so that |EVP_MD_CTX_cleanup| doesn't free it and
-    // then it's reused.
+  EVP_PKEY_CTX *pctx = NULL;
+  assert(in->pctx == NULL || in->pctx_ops != NULL);
+  if (in->pctx) {
+    pctx = in->pctx_ops->dup(in->pctx);
+    if (!pctx) {
+      OPENSSL_PUT_ERROR(DIGEST, ERR_R_MALLOC_FAILURE);
+      return 0;
+    }
+  }
+
+  uint8_t *tmp_buf;
+  if (out->digest != in->digest) {
+    assert(in->digest->ctx_size != 0);
+    tmp_buf = OPENSSL_malloc(in->digest->ctx_size);
+    if (tmp_buf == NULL) {
+      if (pctx) {
+        in->pctx_ops->free(pctx);
+      }
+      OPENSSL_PUT_ERROR(DIGEST, ERR_R_MALLOC_FAILURE);
+      return 0;
+    }
+  } else {
+    // |md_data| will be the correct size in this case. It's removed from |out|
+    // so that |EVP_MD_CTX_cleanup| doesn't free it, and then it's reused.
     tmp_buf = out->md_data;
     out->md_data = NULL;
   }
@@ -132,28 +148,11 @@
   EVP_MD_CTX_cleanup(out);
 
   out->digest = in->digest;
-  if (in->md_data && in->digest->ctx_size) {
-    if (tmp_buf) {
-      out->md_data = tmp_buf;
-    } else {
-      out->md_data = OPENSSL_malloc(in->digest->ctx_size);
-      if (!out->md_data) {
-        OPENSSL_PUT_ERROR(DIGEST, ERR_R_MALLOC_FAILURE);
-        return 0;
-      }
-    }
-    OPENSSL_memcpy(out->md_data, in->md_data, in->digest->ctx_size);
-  }
-
-  assert(in->pctx == NULL || in->pctx_ops != NULL);
+  out->md_data = tmp_buf;
+  OPENSSL_memcpy(out->md_data, in->md_data, in->digest->ctx_size);
+  out->pctx = pctx;
   out->pctx_ops = in->pctx_ops;
-  if (in->pctx && in->pctx_ops) {
-    out->pctx = in->pctx_ops->dup(in->pctx);
-    if (!out->pctx) {
-      EVP_MD_CTX_cleanup(out);
-      return 0;
-    }
-  }
+  assert(out->pctx == NULL || out->pctx_ops != NULL);
 
   return 1;
 }
@@ -165,18 +164,16 @@
 
 int EVP_DigestInit_ex(EVP_MD_CTX *ctx, const EVP_MD *type, ENGINE *engine) {
   if (ctx->digest != type) {
-    if (ctx->digest && ctx->digest->ctx_size > 0) {
-      OPENSSL_free(ctx->md_data);
-      ctx->md_data = NULL;
+    assert(type->ctx_size != 0);
+    uint8_t *md_data = OPENSSL_malloc(type->ctx_size);
+    if (md_data == NULL) {
+      OPENSSL_PUT_ERROR(DIGEST, ERR_R_MALLOC_FAILURE);
+      return 0;
     }
+
+    OPENSSL_free(ctx->md_data);
+    ctx->md_data = md_data;
     ctx->digest = type;
-    if (type->ctx_size > 0) {
-      ctx->md_data = OPENSSL_malloc(type->ctx_size);
-      if (ctx->md_data == NULL) {
-        OPENSSL_PUT_ERROR(DIGEST, ERR_R_MALLOC_FAILURE);
-        return 0;
-      }
-    }
   }
 
   assert(ctx->pctx == NULL || ctx->pctx_ops != NULL);