[Openvpn-devel,v2,4/5] Check return values in md_ctx_init and hmac_ctx_init

Message ID 20210129104708.7484-1-arne@rfc2549.org
State Superseded
Headers show
Series
  • Untitled series #1022
Related show

Commit Message

Arne Schwabe Jan. 29, 2021, 10:47 a.m.
Without this OpenVPN will later segfault on a FIPS enabled system due
to the algorithm available but not allowed.

Patch V2: Use (!func) instead (func != 1)

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto_openssl.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Antonio Quartulli Jan. 29, 2021, 10:57 a.m. | #1
Hi,

On 29/01/2021 11:47, Arne Schwabe wrote:
> Without this OpenVPN will later segfault on a FIPS enabled system due
> to the algorithm available but not allowed.
> 
> Patch V2: Use (!func) instead (func != 1)
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/crypto_openssl.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index c60d4a54..e124a7b6 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -954,7 +954,10 @@ md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
>      ASSERT(NULL != ctx && NULL != kt);
>  
>      EVP_MD_CTX_init(ctx);
> -    EVP_DigestInit(ctx, kt);
> +    if (!EVP_DigestInit(ctx, kt))
> +    {
> +        crypto_msg(M_FATAL, "EVP_DigestInit failed");
> +    }
>  }
>  
>  void
> @@ -1011,7 +1014,10 @@ hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int key_len,
>      ASSERT(NULL != kt && NULL != ctx);
>  
>      HMAC_CTX_reset(ctx);
> -    HMAC_Init_ex(ctx, key, key_len, kt, NULL);
> +    if  (!HMAC_Init_ex(ctx, key, key_len, kt, NULL))
> +    {
> +        crypto_msg(M_FATAL, "HMAC_Init_ex failed");
> +    }
>  
>      /* make sure we used a big enough key */
>      ASSERT(HMAC_size(ctx) <= key_len);
> @@ -1032,7 +1038,10 @@ hmac_ctx_size(const HMAC_CTX *ctx)
>  void
>  hmac_ctx_reset(HMAC_CTX *ctx)
>  {
> -    HMAC_Init_ex(ctx, NULL, 0, NULL, NULL);
> +    if(!HMAC_Init_ex(ctx, NULL, 0, NULL, NULL))
> +    {
> +        crypto_msg(M_FATAL, "HMAC_Init_ex failed");
> +    }
>  }

I saw that you missed this case earlier, but I thought that this call
cannot really fail.

Assuming it can fail under certain conditions, wouldn't the M_FATAL
somewhat become a DoS on the server side?

Regards,
Arne Schwabe Jan. 29, 2021, 2:21 p.m. | #2
> 
> I saw that you missed this case earlier, but I thought that this call
> cannot really fail.
> 
> Assuming it can fail under certain conditions, wouldn't the M_FATAL
> somewhat become a DoS on the server side?

The condition it can fail is basically that the crypto library is unable
or unwilling to create a context for that hash algorithm. If that
happens we later segfault. This basically only happend on the OpenSSL in
FIPS mode, which claim to have MD5 but will then not accept to create
MD5. So a fatal fail here is better than a segfault.

Arne

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index c60d4a54..e124a7b6 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -954,7 +954,10 @@  md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
     ASSERT(NULL != ctx && NULL != kt);
 
     EVP_MD_CTX_init(ctx);
-    EVP_DigestInit(ctx, kt);
+    if (!EVP_DigestInit(ctx, kt))
+    {
+        crypto_msg(M_FATAL, "EVP_DigestInit failed");
+    }
 }
 
 void
@@ -1011,7 +1014,10 @@  hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int key_len,
     ASSERT(NULL != kt && NULL != ctx);
 
     HMAC_CTX_reset(ctx);
-    HMAC_Init_ex(ctx, key, key_len, kt, NULL);
+    if  (!HMAC_Init_ex(ctx, key, key_len, kt, NULL))
+    {
+        crypto_msg(M_FATAL, "HMAC_Init_ex failed");
+    }
 
     /* make sure we used a big enough key */
     ASSERT(HMAC_size(ctx) <= key_len);
@@ -1032,7 +1038,10 @@  hmac_ctx_size(const HMAC_CTX *ctx)
 void
 hmac_ctx_reset(HMAC_CTX *ctx)
 {
-    HMAC_Init_ex(ctx, NULL, 0, NULL, NULL);
+    if(!HMAC_Init_ex(ctx, NULL, 0, NULL, NULL))
+    {
+        crypto_msg(M_FATAL, "HMAC_Init_ex failed");
+    }
 }
 
 void