Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Michel via RT]

Hello Kurt,

I finally managed to compile a debug 1.1.0 version.
(I manually modified the makefile)
:-(

Anyway, I can confirm the patch you send to me fixes the first memory leak case.
BUT,
You are right, we should not free g and N, because they are shared/referenced 
by the VBASE cache.
They will be freed by SRP_VBASE_free();
BUT (again) we need to free the *ID* of g and N, which was allocated and no 
longer in use or referenced elsewhere.
I believe the data structures should have been made differently such that they 
do not share only part of internal data.

That's why I did not call sk_SRP_gN_pop_free() in my first patch.

Here attached is an updated version of your patch that, I hope, can meet our 
first requirement.

As long as the memory is in a stable state, I think it is not mandatory to free 
immediatly the VBASE data in case of error.
We can let the caller decide what he wants to do. But it is just my opinion.

Thanks again,

Regards,

Michel
 
-Message d'origine-
De : openssl-dev [mailto:openssl-dev-boun...@openssl.org] De la part de Michel 
via RT
Envoyé : jeudi 10 décembre 2015 23:37
Cc : openssl-dev@openssl.org
Objet : [openssl-dev] TR: [openssl.org #4172] SRP VBASE stuff still leaking 
memory

Hello Kurt,

I was not able to 'configure' the master branch for debug-VC-WIN32.
I got the error message 'pick os/compiler from: ...
However I succeeded with VC-WIN32.
I guess this is something related to the new configure perl script and 
debug/non-debug options, but I am lost with perl.

Could you please help for this ?

Michel.

-Message d'origine-
De : openssl-dev [mailto:openssl-dev-boun...@openssl.org] De la part de Michel 
via RT Envoy  : jeudi 10 d cembre 2015 17:00 Cc : openssl-dev@openssl.org Objet 
: Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

Hi Kurt,

At first glance, it's a fact that your patch is better.
:-)
I should have thought to some of your improvement, like SRP_gN_new().

I will test it tonight and come back to you.

Many for thanks for your interrest in this matter,

Michel.

-Message d'origine-
De : Kurt Roeckx via RT [mailto:r...@openssl.org] Envoy  : jeudi 10 d cembre 
2015 15:38   : michel.sa...@free.fr Cc : openssl-dev@openssl.org Objet : Re: 
[openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

On Thu, Dec 10, 2015 at 03:19:54PM +0100, Kurt Roeckx wrote:
> 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 +, 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.

I think there is something wrong with new SRP_gN_free().  You now also free g 
and N, and it's not clear to me who the owner of those is.  I think the cache 
is, in which case we should not free them.
I think the cache also isn't cleared, we should probably call
SRP_VBASE_free() when SRP_VBASE_init() fails.


Kurt



___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev



srp_vfy-1.1.0.patch
Description: Binary data
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Kurt Roeckx via RT]

On Thu, Dec 10, 2015 at 12:17:04PM +, Kurt Roeckx via RT wrote:
> On Mon, Dec 07, 2015 at 03:47:56PM +, 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?
> 
> I still need to look at it.

It doesn't even compile ...


Kurt


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Kurt Roeckx]

On Thu, Dec 10, 2015 at 12:17:04PM +, Kurt Roeckx via RT wrote:
> On Mon, Dec 07, 2015 at 03:47:56PM +, 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?
> 
> I still need to look at it.

It doesn't even compile ...


Kurt

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Kurt Roeckx via RT]

On Thu, Dec 10, 2015 at 01:16:48PM +0100, Kurt Roeckx wrote:
> On Mon, Dec 07, 2015 at 03:47:56PM +, 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.


Kurt


>From cf23497a5e6db5d43f0d7a5efc5aefcb8590 Mon Sep 17 00:00:00 2001
From: Kurt Roeckx 
Date: Thu, 10 Dec 2015 13:17:08 +0100
Subject: [PATCH] Fix SRP VBASE memory leak

Based on patch by michel.sa...@free.fr
---
 crypto/srp/srp_vfy.c | 35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/crypto/srp/srp_vfy.c b/crypto/srp/srp_vfy.c
index 1be68f2..172976b 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
@@ -275,7 +277,7 @@ void SRP_VBASE_free(SRP_VBASE *vb)
 if (!vb)
 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);
 }
@@ -302,7 +304,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 +313,16 @@ static void SRP_gN_free(SRP_gN_cache *gN_cache)
 OPENSSL_free(gN_cache);
 }
 
+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;
@@ -343,7 +355,7 @@ static BIGNUM *SRP_gN_place_bn(STACK_OF(SRP_gN_cache) *gN_cache, char *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 +403,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 = OPENSSL_zalloc(sizeof(*gN))) == NULL)
 goto err;
 
 if ((gN->id = BUF_strdup(pp[DB_srpid])) == NULL
@@ -447,21 +459,16 @@ 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);
 
+for (i=0; i < sk_SRP_gN_num(SRP_gN_tab); i++) {
+OPENSSL_free(sk_SRP_gN_value(SRP_gN_tab, i)->id);
+OPENSSL_free(sk_SRP_gN_value(SRP_gN_tab, i));
+}
 sk_SRP_gN_free(SRP_gN_tab);
 
 return error_code;
-- 
2.6.2

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Kurt Roeckx via RT]

On Mon, Dec 07, 2015 at 03:47:56PM +, 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?

I still need to look at it.


Kurt


diff --git a/crypto/srp/srp_vfy.c b/crypto/srp/srp_vfy.c
index 1be68f2..dc649c8 100644
--- a/crypto/srp/srp_vfy.c
+++ b/crypto/srp/srp_vfy.c
@@ -275,7 +275,7 @@ void SRP_VBASE_free(SRP_VBASE *vb)
 if (!vb)
 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);
 }
@@ -302,13 +302,14 @@ 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_free(SRP_gN *gN)
 {
-if (gN_cache == NULL)
+if (gN == NULL)
 return;
-OPENSSL_free(gN_cache->b64_bn);
-BN_free(gN_cache->bn);
-OPENSSL_free(gN_cache);
+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)
@@ -343,7 +344,7 @@ static BIGNUM *SRP_gN_place_bn(STACK_OF(SRP_gN_cache) *gN_cache, char *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 +392,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 = OPENSSL_zalloc(sizeof(*gN))) == NULL)
 goto err;
 
 if ((gN->id = BUF_strdup(pp[DB_srpid])) == NULL
@@ -447,21 +448,16 @@ 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);
 
+for (i=0; i < sk_SRP_gN_num(SRP_gN_tab); i++) {
+OPENSSL_free(sk_SRP_gN_value(SRP_gN_tab, i)->id);
+OPENSSL_free(sk_SRP_gN_value(SRP_gN_tab, i));
+}
 sk_SRP_gN_free(SRP_gN_tab);
 
 return error_code;
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Michel via RT]

I did not try it on master, only with 1.0.2e.

I will prepare a new one againt master to see what happened.

-Message d'origine-
De : Kurt Roeckx via RT [mailto:r...@openssl.org] 
Envoyé : jeudi 10 décembre 2015 13:28
À : michel.sa...@free.fr
Cc : openssl-dev@openssl.org
Objet : Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking 
memory

On Thu, Dec 10, 2015 at 01:16:48PM +0100, Kurt Roeckx wrote:
> On Mon, Dec 07, 2015 at 03:47:56PM +, 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.


Kurt




___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Michel via RT]

Hi Kurt,

First, thanks to look at this.

I just look at your mail.
Quick answer : I applied this patch before sending to RT, and not only it 
compiles but I tested it concerning memory leaks, consequently I am very 
surprise. So I will review this and return to you soon.

In the meantimehere some additional info :
I am working with Visual Studio 2015 community Ed., under Windows 7, OpenSSL 
1.0.2e.

Thanks again,

Regards,

Michel

-Message d'origine-
De : Kurt Roeckx via RT [mailto:r...@openssl.org] 
Envoyé : jeudi 10 décembre 2015 13:28
À : michel.sa...@free.fr
Cc : openssl-dev@openssl.org
Objet : Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking 
memory

On Thu, Dec 10, 2015 at 01:16:48PM +0100, Kurt Roeckx wrote:
> On Mon, Dec 07, 2015 at 03:47:56PM +, 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.


Kurt




___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Michel]

Hi Kurt,

At first glance, it's a fact that your patch is better.
:-)
I should have thought to some of your improvement, like SRP_gN_new().

I will test it tonight and come back to you.

Many for thanks for your interrest in this matter,

Michel.

-Message d'origine-
De : Kurt Roeckx via RT [mailto:r...@openssl.org] 
Envoyé : jeudi 10 décembre 2015 15:38
À : michel.sa...@free.fr
Cc : openssl-dev@openssl.org
Objet : Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking 
memory

On Thu, Dec 10, 2015 at 03:19:54PM +0100, Kurt Roeckx wrote:
> 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 +, 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.

I think there is something wrong with new SRP_gN_free().  You now also free g 
and N, and it's not clear to me who the owner of those is.  I think the cache 
is, in which case we should not free them.
I think the cache also isn't cleared, we should probably call
SRP_VBASE_free() when SRP_VBASE_init() fails.


Kurt


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Michel via RT]

Hi Kurt,

At first glance, it's a fact that your patch is better.
:-)
I should have thought to some of your improvement, like SRP_gN_new().

I will test it tonight and come back to you.

Many for thanks for your interrest in this matter,

Michel.

-Message d'origine-
De : Kurt Roeckx via RT [mailto:r...@openssl.org] 
Envoyé : jeudi 10 décembre 2015 15:38
À : michel.sa...@free.fr
Cc : openssl-dev@openssl.org
Objet : Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking 
memory

On Thu, Dec 10, 2015 at 03:19:54PM +0100, Kurt Roeckx wrote:
> 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 +, 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.

I think there is something wrong with new SRP_gN_free().  You now also free g 
and N, and it's not clear to me who the owner of those is.  I think the cache 
is, in which case we should not free them.
I think the cache also isn't cleared, we should probably call
SRP_VBASE_free() when SRP_VBASE_init() fails.


Kurt



___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Kurt Roeckx via RT]

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 +, 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 
Date: Thu, 10 Dec 2015 13:17:08 +0100
Subject: [PATCH] Fix SRP VBASE memory leak

Based on patch by michel.sa...@free.fr
---
 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

Re: [openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Kurt Roeckx via RT]

On Thu, Dec 10, 2015 at 03:19:54PM +0100, Kurt Roeckx wrote:
> 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 +, 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.

I think there is something wrong with new SRP_gN_free().  You now
also free g and N, and it's not clear to me who the owner of those
is.  I think the cache is, in which case we should not free them.
I think the cache also isn't cleared, we should probably call
SRP_VBASE_free() when SRP_VBASE_init() fails.


Kurt


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4172] SRP VBASE stuff still leaking memory

[Michel via RT]

Hi,

Following my previous mail, here attached is an updated patch against 1.02e
to fix the SRP VBASE memory leaks.
I understand the VBASE stuff is not a TLS critical component, but it is part
of the SRP command line tool.
NB : it's a pity that the base64 encoding is not the same than the one use
elsewhere by OpenSSL.

Regards,

Michel.

De : openssl-dev [mailto:openssl-dev-boun...@openssl.org] De la part de
Michel
Envoyé : lundi 23 mars 2015 12:10
À : openssl-dev@openssl.org
Objet : [openssl-dev] SRP memory leaks and more leaks

Hi,

Trying to use the 'SRP' code, I found two kinds of memory leaks in srp_vfy.c
.

1)  The first kind was due to bad use of OpenSSL 'stack' code and cumbersome
/ confusing names of structure / functions.
In this case, leaks occurs when loading a verifier file containing 'index'
lines.
Two structures are used : SRP_gN and SRP_gN_cache.
And unfortunatly, the SRP_gN_free function free a SRP_gN_cache structure.
The SRP_VBASE_free() function, line 276, should call :
sk_SRP_gN_cache_pop_free(vb->gN_cache, SRP_gN_cache_free);
instead of :
sk_SRP_gN_cache_free(vb->gN_cache);
And consequently the SRP_gN_cache_free() function, lines 305-312, has to
move before SRP_VBASE_free()

The attached patch solve this kind of leaks.

2) When simulating a 'fake user' as per RFC 5054, § '2.5.1.3.  Unknown SRP
User Name',
the SRP_VBASE_get_by_user() function returns a newly allocated SRP_user_pwd
structure whose adress is not kept anywhere else.
So, the caller should free it later, but from outside the function, there is
no way to distinguish between 'fake users' and real ones
which are managed and freed throught the use of the SRP_VBASE structure /
functions.

I am afraid there is no other solution than changing the
SRP_VBASE_get_by_user() function prototype.

It is better I do not share here my opinion about the comments below :
/* need to check whether there are memory leaks */, s_server.c line 433
or :
/* there may be still some leaks to fix, … */ srp_vfy.c line 449
>:(

Hope this will save time to other users,

Michel.



srp_vfy-1.0.2e.patch
Description: Binary data
___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev