Avoid undefined behavior of provided macs on EVP_MAC reinitialization

When the context is reinitialized, i.e. the same key should be used we must properly reinitialize the underlying implementation. However in POLY1305 case it does not make sense as this special MAC should not reuse keys. We fail with this provided implementation when reinitialization happens. Fixes openssl#17811 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#18100) (cherry picked from commit c9ddc5a)
webpanorg · Apr 19, 2022 · 4f675d8 · 4f675d8
1 parent 8a4c3ed
commit 4f675d8
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 33 deletions.
diff --git a/providers/implementations/macs/cmac_prov.c b/providers/implementations/macs/cmac_prov.c
@@ -122,7 +122,8 @@ static int cmac_init(void *vmacctx, const unsigned char *key,
         return 0;
     if (key != NULL)
         return cmac_setkey(macctx, key, keylen);
-    return 1;
+    /* Reinitialize the CMAC context */
+    return CMAC_Init(macctx->ctx, NULL, 0, NULL, NULL);
 }
 
 static int cmac_update(void *vmacctx, const unsigned char *data,

diff --git a/providers/implementations/macs/gmac_prov.c b/providers/implementations/macs/gmac_prov.c
@@ -120,7 +120,7 @@ static int gmac_init(void *vmacctx, const unsigned char *key,
         return 0;
     if (key != NULL)
         return gmac_setkey(macctx, key, keylen);
-    return 1;
+    return EVP_EncryptInit_ex(macctx->ctx, NULL, NULL, NULL, NULL);
 }
 
 static int gmac_update(void *vmacctx, const unsigned char *data,
@@ -209,19 +209,22 @@ static int gmac_set_ctx_params(void *vmacctx, const OSSL_PARAM params[])
 
     if (params == NULL)
         return 1;
-    if (ctx == NULL
-        || !ossl_prov_cipher_load_from_params(&macctx->cipher, params, provctx))
+    if (ctx == NULL)
         return 0;
 
-    if (EVP_CIPHER_get_mode(ossl_prov_cipher_cipher(&macctx->cipher))
-        != EVP_CIPH_GCM_MODE) {
-        ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_MODE);
-        return 0;
+    if ((p = OSSL_PARAM_locate_const(params, OSSL_MAC_PARAM_CIPHER)) != NULL) {
+        if (!ossl_prov_cipher_load_from_params(&macctx->cipher, params, provctx))
+            return 0;
+        if (EVP_CIPHER_get_mode(ossl_prov_cipher_cipher(&macctx->cipher))
+            != EVP_CIPH_GCM_MODE) {
+            ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_MODE);
+            return 0;
+        }
+        if (!EVP_EncryptInit_ex(ctx, ossl_prov_cipher_cipher(&macctx->cipher),
+                                ossl_prov_cipher_engine(&macctx->cipher), NULL,
+                                NULL))
+            return 0;
     }
-    if (!EVP_EncryptInit_ex(ctx, ossl_prov_cipher_cipher(&macctx->cipher),
-                            ossl_prov_cipher_engine(&macctx->cipher), NULL,
-                            NULL))
-        return 0;
 
     if ((p = OSSL_PARAM_locate_const(params, OSSL_MAC_PARAM_KEY)) != NULL)
         if (p->data_type != OSSL_PARAM_OCTET_STRING

diff --git a/providers/implementations/macs/hmac_prov.c b/providers/implementations/macs/hmac_prov.c
@@ -152,7 +152,7 @@ static int hmac_setkey(struct hmac_data_st *macctx,
 {
     const EVP_MD *digest;
 
-    if (macctx->keylen > 0)
+    if (macctx->key != NULL)
         OPENSSL_secure_clear_free(macctx->key, macctx->keylen);
     /* Keep a copy of the key in case we need it for TLS HMAC */
     macctx->key = OPENSSL_secure_malloc(keylen > 0 ? keylen : 1);
@@ -177,9 +177,11 @@ static int hmac_init(void *vmacctx, const unsigned char *key,
     if (!ossl_prov_is_running() || !hmac_set_ctx_params(macctx, params))
         return 0;
 
-    if (key != NULL && !hmac_setkey(macctx, key, keylen))
-        return 0;
-    return 1;
+    if (key != NULL)
+        return hmac_setkey(macctx, key, keylen);
+
+    /* Just reinit the HMAC context */
+    return HMAC_Init_ex(macctx->ctx, NULL, 0, NULL, NULL);
 }
 
 static int hmac_update(void *vmacctx, const unsigned char *data,
@@ -325,22 +327,10 @@ static int hmac_set_ctx_params(void *vmacctx, const OSSL_PARAM params[])
     if ((p = OSSL_PARAM_locate_const(params, OSSL_MAC_PARAM_KEY)) != NULL) {
         if (p->data_type != OSSL_PARAM_OCTET_STRING)
             return 0;
-
-        if (macctx->keylen > 0)
-            OPENSSL_secure_clear_free(macctx->key, macctx->keylen);
-        /* Keep a copy of the key if we need it for TLS HMAC */
-        macctx->key = OPENSSL_secure_malloc(p->data_size > 0 ? p->data_size : 1);
-        if (macctx->key == NULL)
-            return 0;
-        memcpy(macctx->key, p->data, p->data_size);
-        macctx->keylen = p->data_size;
-
-        if (!HMAC_Init_ex(macctx->ctx, p->data, p->data_size,
-                          ossl_prov_digest_md(&macctx->digest),
-                          NULL /* ENGINE */))
+        if (!hmac_setkey(macctx, p->data, p->data_size))
             return 0;
-
     }
+
     if ((p = OSSL_PARAM_locate_const(params,
                                      OSSL_MAC_PARAM_TLS_DATA_SIZE)) != NULL) {
         if (!OSSL_PARAM_get_size_t(p, &macctx->tls_data_size))

diff --git a/providers/implementations/macs/poly1305_prov.c b/providers/implementations/macs/poly1305_prov.c
@@ -37,6 +37,7 @@ static OSSL_FUNC_mac_final_fn poly1305_final;
 
 struct poly1305_data_st {
     void *provctx;
+    int updated;
     POLY1305 poly1305;           /* Poly1305 data */
 };
 
@@ -98,14 +99,16 @@ static int poly1305_init(void *vmacctx, const unsigned char *key,
         return 0;
     if (key != NULL)
         return poly1305_setkey(ctx, key, keylen);
-    return 1;
+    /* no reinitialization of context with the same key is allowed */
+    return ctx->updated == 0;
 }
 
 static int poly1305_update(void *vmacctx, const unsigned char *data,
                        size_t datalen)
 {
     struct poly1305_data_st *ctx = vmacctx;
 
+    ctx->updated = 1;
     if (datalen == 0)
         return 1;
 
@@ -121,6 +124,7 @@ static int poly1305_final(void *vmacctx, unsigned char *out, size_t *outl,
 
     if (!ossl_prov_is_running())
         return 0;
+    ctx->updated = 1;
     Poly1305_Final(&ctx->poly1305, out);
     *outl = poly1305_size();
     return 1;

diff --git a/providers/implementations/macs/siphash_prov.c b/providers/implementations/macs/siphash_prov.c
@@ -39,6 +39,7 @@ static OSSL_FUNC_mac_final_fn siphash_final;
 struct siphash_data_st {
     void *provctx;
     SIPHASH siphash;             /* Siphash data */
+    SIPHASH sipcopy;             /* Siphash data copy for reinitialization */
     unsigned int crounds, drounds;
 };
 
@@ -94,9 +95,14 @@ static size_t siphash_size(void *vmacctx)
 static int siphash_setkey(struct siphash_data_st *ctx,
                           const unsigned char *key, size_t keylen)
 {
+    int ret;
+
     if (keylen != SIPHASH_KEY_SIZE)
         return 0;
-    return SipHash_Init(&ctx->siphash, key, crounds(ctx), drounds(ctx));
+    ret = SipHash_Init(&ctx->siphash, key, crounds(ctx), drounds(ctx));
+    if (ret)
+        ctx->sipcopy = ctx->siphash;
+    return ret;
 }
 
 static int siphash_init(void *vmacctx, const unsigned char *key, size_t keylen,
@@ -109,8 +115,10 @@ static int siphash_init(void *vmacctx, const unsigned char *key, size_t keylen,
     /* Without a key, there is not much to do here,
      * The actual initialization happens through controls.
      */
-    if (key == NULL)
+    if (key == NULL) {
+        ctx->siphash = ctx->sipcopy;
         return 1;
+    }
     return siphash_setkey(ctx, key, keylen);
 }