#4342: few missing malloc return checks and free in error paths

ossl_hmac_cleanup, pkey_hmac_cleanup:
 - allow to invoke with NULL data
 - using EVP_PKEY_CTX_[get|set]_data

EVP_DigestInit_ex:
 - remove additional check for ‘type’ and doing clear free instead of
free

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>

3 files changed, 41 insertions, 16 deletions

diff --git a/crypto/engine/eng_openssl.c b/crypto/engine/eng_openssl.c
index 75fd23d28d..7e28604955 100644
--- a/crypto/engine/eng_openssl.c
+++ b/crypto/engine/eng_openssl.c
@@ -441,6 +441,10 @@ static int ossl_hmac_init(EVP_PKEY_CTX *ctx)
         return 0;
     hctx->ktmp.type = V_ASN1_OCTET_STRING;
     hctx->ctx = HMAC_CTX_new();
+    if (hctx->ctx == NULL) {
+        OPENSSL_free(hctx);
+        return 0;
+    }
     EVP_PKEY_CTX_set_data(ctx, hctx);
     EVP_PKEY_CTX_set0_keygen_info(ctx, NULL, 0);
 # ifdef TEST_ENG_OPENSSL_HMAC_INIT
@@ -449,31 +453,42 @@ static int ossl_hmac_init(EVP_PKEY_CTX *ctx)
     return 1;
 }
 
+static void ossl_hmac_cleanup(EVP_PKEY_CTX *ctx);
+
 static int ossl_hmac_copy(EVP_PKEY_CTX *dst, EVP_PKEY_CTX *src)
 {
     OSSL_HMAC_PKEY_CTX *sctx, *dctx;
+
+    /* allocate memory for dst->data and a new HMAC_CTX in dst->data->ctx */
     if (!ossl_hmac_init(dst))
         return 0;
     sctx = EVP_PKEY_CTX_get_data(src);
     dctx = EVP_PKEY_CTX_get_data(dst);
     dctx->md = sctx->md;
     if (!HMAC_CTX_copy(dctx->ctx, sctx->ctx))
-        return 0;
+        goto err;
     if (sctx->ktmp.data) {
         if (!ASN1_OCTET_STRING_set(&dctx->ktmp,
                                    sctx->ktmp.data, sctx->ktmp.length))
-            return 0;
+            goto err;
     }
     return 1;
+err:
+    /* release HMAC_CTX in dst->data->ctx and memory allocated for dst->data */
+    ossl_hmac_cleanup(dst);
+    return 0;
 }
 
 static void ossl_hmac_cleanup(EVP_PKEY_CTX *ctx)
 {
     OSSL_HMAC_PKEY_CTX *hctx = EVP_PKEY_CTX_get_data(ctx);
 
-    HMAC_CTX_free(hctx->ctx);
-    OPENSSL_clear_free(hctx->ktmp.data, hctx->ktmp.length);
-    OPENSSL_free(hctx);
+    if (hctx) {
+        HMAC_CTX_free(hctx->ctx);
+        OPENSSL_clear_free(hctx->ktmp.data, hctx->ktmp.length);
+        OPENSSL_free(hctx);
+        EVP_PKEY_CTX_set_data(ctx, NULL);
+    }
 }
 
 static int ossl_hmac_keygen(EVP_PKEY_CTX *ctx, EVP_PKEY *pkey)
diff --git a/crypto/evp/digest.c b/crypto/evp/digest.c
index 051fc7b19d..c594a0a638 100644
--- a/crypto/evp/digest.c
+++ b/crypto/evp/digest.c
@@ -68,10 +68,8 @@ int EVP_DigestInit_ex(EVP_MD_CTX *ctx, const EVP_MD *type, ENGINE *impl)
      * previous handle, re-querying for an ENGINE, and having a
      * reinitialisation, when it may all be unnecessary.
      */
-    if (ctx->engine && ctx->digest && (!type ||
-                                       (type
-                                        && (type->type ==
-                                            ctx->digest->type))))
+    if (ctx->engine && ctx->digest &&
+        (type == NULL || (type->type == ctx->digest->type)))
         goto skip_to_init;
     if (type) {
         /*
@@ -117,7 +115,7 @@ int EVP_DigestInit_ex(EVP_MD_CTX *ctx, const EVP_MD *type, ENGINE *impl)
 #endif
     if (ctx->digest != type) {
         if (ctx->digest && ctx->digest->ctx_size) {
-            OPENSSL_free(ctx->md_data);
+            OPENSSL_clear_free(ctx->md_data, ctx->digest->ctx_size);
             ctx->md_data = NULL;
         }
         ctx->digest = type;
diff --git a/crypto/hmac/hm_pmeth.c b/crypto/hmac/hm_pmeth.c
index 55493beb2a..5b98477f9c 100644
--- a/crypto/hmac/hm_pmeth.c
+++ b/crypto/hmac/hm_pmeth.c
@@ -32,6 +32,10 @@ static int pkey_hmac_init(EVP_PKEY_CTX *ctx)
         return 0;
     hctx->ktmp.type = V_ASN1_OCTET_STRING;
     hctx->ctx = HMAC_CTX_new();
+    if (hctx->ctx == NULL) {
+        OPENSSL_free(hctx);
+        return 0;
+    }
 
     ctx->data = hctx;
     ctx->keygen_info_count = 0;
@@ -39,33 +43,41 @@ static int pkey_hmac_init(EVP_PKEY_CTX *ctx)
     return 1;
 }
 
+static void pkey_hmac_cleanup(EVP_PKEY_CTX *ctx);
+
 static int pkey_hmac_copy(EVP_PKEY_CTX *dst, EVP_PKEY_CTX *src)
 {
     HMAC_PKEY_CTX *sctx, *dctx;
+
+    /* allocate memory for dst->data and a new HMAC_CTX in dst->data->ctx */
     if (!pkey_hmac_init(dst))
         return 0;
-    sctx = src->data;
-    dctx = dst->data;
+    sctx = EVP_PKEY_CTX_get_data(src);
+    dctx = EVP_PKEY_CTX_get_data(dst);
     dctx->md = sctx->md;
     if (!HMAC_CTX_copy(dctx->ctx, sctx->ctx))
-        return 0;
+        goto err;
     if (sctx->ktmp.data) {
         if (!ASN1_OCTET_STRING_set(&dctx->ktmp,
                                    sctx->ktmp.data, sctx->ktmp.length))
-            return 0;
+            goto err;
     }
     return 1;
+err:
+    /* release HMAC_CTX in dst->data->ctx and memory allocated for dst->data */
+    pkey_hmac_cleanup (dst);
+    return 0;
 }
 
 static void pkey_hmac_cleanup(EVP_PKEY_CTX *ctx)
 {
-    HMAC_PKEY_CTX *hctx = ctx->data;
+    HMAC_PKEY_CTX *hctx = EVP_PKEY_CTX_get_data(ctx);
 
     if (hctx != NULL) {
         HMAC_CTX_free(hctx->ctx);
         OPENSSL_clear_free(hctx->ktmp.data, hctx->ktmp.length);
         OPENSSL_free(hctx);
-        ctx->data = NULL;
+        EVP_PKEY_CTX_set_data(ctx, NULL);
     }
 }