On Thu, Dec 10, 2015 at 01:27:38PM +0100, Kurt Roeckx wrote: > On Thu, Dec 10, 2015 at 01:16:48PM +0100, Kurt Roeckx wrote: > > On Mon, Dec 07, 2015 at 03:47:56PM +0000, Michel via RT wrote: > > > Hi, > > > > > > Following my previous mail, here attached is an updated patch against > > > 1.02e > > > to fix the SRP VBASE memory leaks. > > > > Can you confirm that this would be the correct patch for master? > > The following patch should at least compile.
I fixed a few more things, cleaned up some things. New patch attached. Kurt
>From fb9bb48cf49c6513c95c8dff8faa9a09c5ff6529 Mon Sep 17 00:00:00 2001 From: Kurt Roeckx <[email protected]> Date: Thu, 10 Dec 2015 13:17:08 +0100 Subject: [PATCH] Fix SRP VBASE memory leak Based on patch by [email protected] --- crypto/srp/srp_vfy.c | 76 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 27 deletions(-) diff --git a/crypto/srp/srp_vfy.c b/crypto/srp/srp_vfy.c index 1be68f2..c13b0ae 100644 --- a/crypto/srp/srp_vfy.c +++ b/crypto/srp/srp_vfy.c @@ -72,6 +72,8 @@ static char b64table[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz./"; +static void SRP_gN_cache_free(SRP_gN_cache *gN_cache); + /* * the following two conversion routines have been inspired by code from * Stanford @@ -253,34 +255,39 @@ SRP_VBASE *SRP_VBASE_new(char *seed_key) if (vb == NULL) return NULL; - if ((vb->users_pwd = sk_SRP_user_pwd_new_null()) == NULL - || (vb->gN_cache = sk_SRP_gN_cache_new_null()) == NULL) { - OPENSSL_free(vb); - return NULL; - } + + vb->users_pwd = NULL; + vb->gN_cache = NULL; vb->default_g = NULL; vb->default_N = NULL; vb->seed_key = NULL; + + if ((vb->users_pwd = sk_SRP_user_pwd_new_null()) == NULL + || (vb->gN_cache = sk_SRP_gN_cache_new_null()) == NULL) { + goto err; + } if ((seed_key != NULL) && (vb->seed_key = BUF_strdup(seed_key)) == NULL) { - sk_SRP_user_pwd_free(vb->users_pwd); - sk_SRP_gN_cache_free(vb->gN_cache); - OPENSSL_free(vb); - return NULL; + goto err; } return vb; + + err: + SRP_VBASE_free(vb); + return NULL; } void SRP_VBASE_free(SRP_VBASE *vb) { - if (!vb) + if (vb == NULL) return; + sk_SRP_user_pwd_pop_free(vb->users_pwd, SRP_user_pwd_free); - sk_SRP_gN_cache_free(vb->gN_cache); + sk_SRP_gN_cache_pop_free(vb->gN_cache, SRP_gN_cache_free); OPENSSL_free(vb->seed_key); OPENSSL_free(vb); } -static SRP_gN_cache *SRP_gN_new_init(const char *ch) +static SRP_gN_cache *SRP_gN_cache_new_init(const char *ch) { unsigned char tmp[MAX_LEN]; int len; @@ -302,7 +309,7 @@ static SRP_gN_cache *SRP_gN_new_init(const char *ch) return NULL; } -static void SRP_gN_free(SRP_gN_cache *gN_cache) +static void SRP_gN_cache_free(SRP_gN_cache *gN_cache) { if (gN_cache == NULL) return; @@ -311,6 +318,30 @@ static void SRP_gN_free(SRP_gN_cache *gN_cache) OPENSSL_free(gN_cache); } +static SRP_gN *SRP_gN_new(void) +{ + SRP_gN *gN = OPENSSL_malloc(sizeof(*gN)); + + if (gN == NULL) + return NULL; + + gN->id = NULL; + gN->g = NULL; + gN->N = NULL; + + return gN; +} + +static void SRP_gN_free(SRP_gN *gN) +{ + if (gN == NULL) + return; + OPENSSL_free(gN->id); + BN_free(gN->g); + BN_free(gN->N); + OPENSSL_free(gN); +} + static SRP_gN *SRP_get_gN_by_id(const char *id, STACK_OF(SRP_gN) *gN_tab) { int i; @@ -339,11 +370,11 @@ static BIGNUM *SRP_gN_place_bn(STACK_OF(SRP_gN_cache) *gN_cache, char *ch) return cache->bn; } { /* it is the first time that we find it */ - SRP_gN_cache *newgN = SRP_gN_new_init(ch); + SRP_gN_cache *newgN = SRP_gN_cache_new_init(ch); if (newgN) { if (sk_SRP_gN_cache_insert(gN_cache, newgN, 0) > 0) return newgN->bn; - SRP_gN_free(newgN); + SRP_gN_cache_free(newgN); } } return NULL; @@ -391,7 +422,7 @@ int SRP_VBASE_init(SRP_VBASE *vb, char *verifier_file) * we add this couple in the internal Stack */ - if ((gN = OPENSSL_malloc(sizeof(*gN))) == NULL) + if ((gN = SRP_gN_new()) == NULL) goto err; if ((gN->id = BUF_strdup(pp[DB_srpid])) == NULL @@ -447,22 +478,13 @@ int SRP_VBASE_init(SRP_VBASE *vb, char *verifier_file) error_code = SRP_NO_ERROR; err: - /* - * there may be still some leaks to fix, if this fails, the application - * terminates most likely - */ - - if (gN != NULL) { - OPENSSL_free(gN->id); - OPENSSL_free(gN); - } - + SRP_gN_free(gN); SRP_user_pwd_free(user_pwd); TXT_DB_free(tmpdb); BIO_free_all(in); - sk_SRP_gN_free(SRP_gN_tab); + sk_SRP_gN_pop_free(SRP_gN_tab, SRP_gN_free); return error_code; -- 2.6.2
_______________________________________________ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
