diff mbox series

[Openvpn-devel] EVP_DigestSignFinal siglen parameter correction

Message ID 20210312150629.57302-1-juliusz@wolfssl.com
State Accepted
Headers
Series [Openvpn-devel] EVP_DigestSignFinal siglen parameter correction |

Commit Message

Juliusz Sosinowicz March 12, 2021, 4:06 a.m. UTC 
  In the EVP_DigestSignFinal API, "before the call the siglen parameter should contain the length of the sig buffer".

Signed-off-by: Juliusz Sosinowicz <juliusz@wolfssl.com>
---
 src/openvpn/crypto_openssl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Arne Schwabe March 15, 2021, 6:11 a.m. UTC | #1 
Am 12.03.21 um 16:06 schrieb Juliusz Sosinowicz:
> In the EVP_DigestSignFinal API, "before the call the siglen parameter should contain the length of the sig buffer".

This is probably correct but I need to take a closer look as this
function is directly taken from OpenSSL's internal function and only has
minimal adaptations. Note also that we plan to remove this compatibility
path as soon as we drop OpenSSL 1.0.2 support.

Arne
Arne Schwabe March 17, 2021, 2:52 a.m. UTC | #2 
Am 12.03.21 um 16:06 schrieb Juliusz Sosinowicz:
> In the EVP_DigestSignFinal API, "before the call the siglen parameter should contain the length of the sig buffer".
> 
> Signed-off-by: Juliusz Sosinowicz <juliusz@wolfssl.com>
> ---
>  src/openvpn/crypto_openssl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 49698e4b3..4486d246d 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -1195,7 +1195,7 @@ tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
>      EVP_MD_CTX ctx, ctx_tmp, ctx_init;
>      EVP_PKEY *mac_key;
>      unsigned char A1[EVP_MAX_MD_SIZE];
> -    size_t A1_len;
> +    size_t A1_len = EVP_MAX_MD_SIZE;
>      int ret = false;
>  
>      chunk = EVP_MD_size(md);
> @@ -1249,6 +1249,7 @@ tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
>  
>          if (olen > chunk)
>          {
> +            j = olen;
>              if (!EVP_DigestSignFinal(&ctx, out, &j))
>              {
>                  goto err;
> @@ -1263,6 +1264,7 @@ tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
>          }
>          else
>          {
> +            A1_len = EVP_MAX_MD_SIZE;
>              /* last one */
>              if (!EVP_DigestSignFinal(&ctx, A1, &A1_len))
>              {
> 

Acked-By: Arne Schwabe <arne@rfc2549.org>

The uninitialised parameter should not work and the OpenSSL
documentation says that it should contain the size of the buffer but the
internal code of OpenVPN seems to ignore that and somehow it still
works. But I cannot blame wolfSSL being lax about the OpenSSL API :)
Gert Doering March 17, 2021, 8:04 a.m. UTC | #3 
Your patch has been applied to the master branch.

Mildly tested on an OpenSSL 1.0.2u box, client side only.  Works :-)

commit 476990d41ad78ac4419a3743cdab55c85c41b041
Author: Juliusz Sosinowicz
Date:   Fri Mar 12 16:06:29 2021 +0100

     EVP_DigestSignFinal siglen parameter correction

     Signed-off-by: Juliusz Sosinowicz <juliusz@wolfssl.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20210312150629.57302-1-juliusz@wolfssl.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21663.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 49698e4b3..4486d246d 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1195,7 +1195,7 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
     EVP_MD_CTX ctx, ctx_tmp, ctx_init;
     EVP_PKEY *mac_key;
     unsigned char A1[EVP_MAX_MD_SIZE];
-    size_t A1_len;
+    size_t A1_len = EVP_MAX_MD_SIZE;
     int ret = false;
 
     chunk = EVP_MD_size(md);
@@ -1249,6 +1249,7 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
 
         if (olen > chunk)
         {
+            j = olen;
             if (!EVP_DigestSignFinal(&ctx, out, &j))
             {
                 goto err;
@@ -1263,6 +1264,7 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
         }
         else
         {
+            A1_len = EVP_MAX_MD_SIZE;
             /* last one */
             if (!EVP_DigestSignFinal(&ctx, A1, &A1_len))
             {