[openssl.org #3226] [PATCH] crypto/srp/srp_lib.c: add/correct some error handling

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[openssl.org #3226] [PATCH] crypto/srp/srp_lib.c: add/correct some error handling

Rich Salz via RT
---
 crypto/srp/srp_lib.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/crypto/srp/srp_lib.c b/crypto/srp/srp_lib.c
index 18d1bb9..a95fb0e 100644
--- a/crypto/srp/srp_lib.c
+++ b/crypto/srp/srp_lib.c
@@ -82,6 +82,7 @@
 static BIGNUM *srp_Calc_k(BIGNUM *N, BIGNUM *g)
  {
  /* k = SHA1(N | PAD(g)) -- tls-srp draft 8 */
+ BIGNUM *ret = NULL;
 
  unsigned char digest[SHA_DIGEST_LENGTH];
  unsigned char *tmp;
@@ -94,19 +95,26 @@ static BIGNUM *srp_Calc_k(BIGNUM *N, BIGNUM *g)
  BN_bn2bin(N,tmp) ;
 
  EVP_MD_CTX_init(&ctxt);
- EVP_DigestInit_ex(&ctxt, EVP_sha1(), NULL);
- EVP_DigestUpdate(&ctxt, tmp, longN);
+ if (!EVP_DigestInit_ex(&ctxt, EVP_sha1(), NULL))
+ goto err;
+ if (!EVP_DigestUpdate(&ctxt, tmp, longN))
+ goto err;
 
  memset(tmp, 0, longN);
  longg = BN_bn2bin(g,tmp) ;
         /* use the zeros behind to pad on left */
- EVP_DigestUpdate(&ctxt, tmp + longg, longN-longg);
- EVP_DigestUpdate(&ctxt, tmp, longg);
- OPENSSL_free(tmp);
+ if (!EVP_DigestUpdate(&ctxt, tmp + longg, longN-longg))
+ goto err;
+ if (!EVP_DigestUpdate(&ctxt, tmp, longg))
+ goto err;
 
- EVP_DigestFinal_ex(&ctxt, digest, NULL);
+ if (!EVP_DigestFinal_ex(&ctxt, digest, NULL))
+ goto err;
  EVP_MD_CTX_cleanup(&ctxt);
- return BN_bin2bn(digest, sizeof(digest), NULL);
+ ret = BN_bin2bn(digest, sizeof(digest), NULL);
+err:
+ OPENSSL_free(tmp);
+ return ret;
  }
 
 BIGNUM *SRP_Calc_u(BIGNUM *A, BIGNUM *B, BIGNUM *N)
@@ -287,7 +295,12 @@ BIGNUM *SRP_Calc_client_key(BIGNUM *N, BIGNUM *B, BIGNUM *g, BIGNUM *x, BIGNUM *
  if (!BN_mod_exp(K,tmp,tmp2,N,bn_ctx))
  goto err;
 
+ if (0)
+ {
 err :
+ BN_clear_free(K);
+ K = NULL;
+ }
  BN_CTX_free(bn_ctx);
  BN_clear_free(tmp);
  BN_clear_free(tmp2);
--
1.7.2.5

______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3226] [PATCH] crypto/srp/srp_lib.c: add/correct some error handling

michel-60
With this patch, I am afraid in case of error, the context will not be
cleaned up.
Shouldn't the line :
EVP_MD_CTX_cleanup(&ctxt);
be moved inside the 'err:' block ?

Le 10/01/2014 09:54, Florian Zumbiehl via RT a écrit :

> ---
>   crypto/srp/srp_lib.c |   27 ++++++++++++++++++++-------
>   1 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/crypto/srp/srp_lib.c b/crypto/srp/srp_lib.c
> index 18d1bb9..a95fb0e 100644
> --- a/crypto/srp/srp_lib.c
> +++ b/crypto/srp/srp_lib.c
> @@ -82,6 +82,7 @@
>   static BIGNUM *srp_Calc_k(BIGNUM *N, BIGNUM *g)
>   {
>   /* k = SHA1(N | PAD(g)) -- tls-srp draft 8 */
> + BIGNUM *ret = NULL;
>  
>   unsigned char digest[SHA_DIGEST_LENGTH];
>   unsigned char *tmp;
> @@ -94,19 +95,26 @@ static BIGNUM *srp_Calc_k(BIGNUM *N, BIGNUM *g)
>   BN_bn2bin(N,tmp) ;
>  
>   EVP_MD_CTX_init(&ctxt);
> - EVP_DigestInit_ex(&ctxt, EVP_sha1(), NULL);
> - EVP_DigestUpdate(&ctxt, tmp, longN);
> + if (!EVP_DigestInit_ex(&ctxt, EVP_sha1(), NULL))
> + goto err;
> + if (!EVP_DigestUpdate(&ctxt, tmp, longN))
> + goto err;
>  
>   memset(tmp, 0, longN);
>   longg = BN_bn2bin(g,tmp) ;
>           /* use the zeros behind to pad on left */
> - EVP_DigestUpdate(&ctxt, tmp + longg, longN-longg);
> - EVP_DigestUpdate(&ctxt, tmp, longg);
> - OPENSSL_free(tmp);
> + if (!EVP_DigestUpdate(&ctxt, tmp + longg, longN-longg))
> + goto err;
> + if (!EVP_DigestUpdate(&ctxt, tmp, longg))
> + goto err;
>  
> - EVP_DigestFinal_ex(&ctxt, digest, NULL);
> + if (!EVP_DigestFinal_ex(&ctxt, digest, NULL))
> + goto err;
>   EVP_MD_CTX_cleanup(&ctxt);
> - return BN_bin2bn(digest, sizeof(digest), NULL);
> + ret = BN_bin2bn(digest, sizeof(digest), NULL);
> +err:
> + OPENSSL_free(tmp);
> + return ret;
>   }
>  
>   BIGNUM *SRP_Calc_u(BIGNUM *A, BIGNUM *B, BIGNUM *N)
> @@ -287,7 +295,12 @@ BIGNUM *SRP_Calc_client_key(BIGNUM *N, BIGNUM *B, BIGNUM *g, BIGNUM *x, BIGNUM *
>   if (!BN_mod_exp(K,tmp,tmp2,N,bn_ctx))
>   goto err;
>  
> + if (0)
> + {
>   err :
> + BN_clear_free(K);
> + K = NULL;
> + }
>   BN_CTX_free(bn_ctx);
>   BN_clear_free(tmp);
>   BN_clear_free(tmp2);


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3226] [PATCH] crypto/srp/srp_lib.c: add/correct some error handling

Rich Salz via RT
Hi,

> With this patch, I am afraid in case of error, the context will not
> be cleaned up.
> Shouldn't the line :
> EVP_MD_CTX_cleanup(&ctxt);
> be moved inside the 'err:' block ?

Good catch!

New version below ...

Regards, Florian
---

diff --git a/crypto/srp/srp_lib.c b/crypto/srp/srp_lib.c
index 18d1bb9..782f23a 100644
--- a/crypto/srp/srp_lib.c
+++ b/crypto/srp/srp_lib.c
@@ -82,6 +82,7 @@
 static BIGNUM *srp_Calc_k(BIGNUM *N, BIGNUM *g)
  {
  /* k = SHA1(N | PAD(g)) -- tls-srp draft 8 */
+ BIGNUM *ret = NULL;
 
  unsigned char digest[SHA_DIGEST_LENGTH];
  unsigned char *tmp;
@@ -94,19 +95,26 @@ static BIGNUM *srp_Calc_k(BIGNUM *N, BIGNUM *g)
  BN_bn2bin(N,tmp) ;
 
  EVP_MD_CTX_init(&ctxt);
- EVP_DigestInit_ex(&ctxt, EVP_sha1(), NULL);
- EVP_DigestUpdate(&ctxt, tmp, longN);
+ if (!EVP_DigestInit_ex(&ctxt, EVP_sha1(), NULL))
+ goto err;
+ if (!EVP_DigestUpdate(&ctxt, tmp, longN))
+ goto err;
 
  memset(tmp, 0, longN);
  longg = BN_bn2bin(g,tmp) ;
         /* use the zeros behind to pad on left */
- EVP_DigestUpdate(&ctxt, tmp + longg, longN-longg);
- EVP_DigestUpdate(&ctxt, tmp, longg);
- OPENSSL_free(tmp);
+ if (!EVP_DigestUpdate(&ctxt, tmp + longg, longN-longg))
+ goto err;
+ if (!EVP_DigestUpdate(&ctxt, tmp, longg))
+ goto err;
 
- EVP_DigestFinal_ex(&ctxt, digest, NULL);
+ if (!EVP_DigestFinal_ex(&ctxt, digest, NULL))
+ goto err;
+ ret = BN_bin2bn(digest, sizeof(digest), NULL);
+err:
  EVP_MD_CTX_cleanup(&ctxt);
- return BN_bin2bn(digest, sizeof(digest), NULL);
+ OPENSSL_free(tmp);
+ return ret;
  }
 
 BIGNUM *SRP_Calc_u(BIGNUM *A, BIGNUM *B, BIGNUM *N)
@@ -287,7 +295,12 @@ BIGNUM *SRP_Calc_client_key(BIGNUM *N, BIGNUM *B, BIGNUM *g, BIGNUM *x, BIGNUM *
  if (!BN_mod_exp(K,tmp,tmp2,N,bn_ctx))
  goto err;
 
+ if (0)
+ {
 err :
+ BN_clear_free(K);
+ K = NULL;
+ }
  BN_CTX_free(bn_ctx);
  BN_clear_free(tmp);
  BN_clear_free(tmp2);
--


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]