Re: [PATCHES] pgcrypto: fix memory leak in openssl.c

2006-02-20 Thread Marko Kreen
On 2/18/06, Marko Kreen [EMAIL PROTECTED] wrote:
 pgcrypto crypt()/md5 and hmac() leak memory when compiled against
 OpenSSL as openssl.c digest -reset will do two DigestInit calls
 against a context.  This happened to work with OpenSSL 0.9.6
 but not with 0.9.7+.

Ugh, seems I read the old code slightly wrong.  The leak happens
also with regular digest(), although it will leak only 1 context
instance, not the 1000+ as the crypt-md5 does.  And on 8.1 there
is pgp_sym_encrypt that also does lots of resets on one context,
like crypt-md5.  In addition it does regular digest() in several
places.  So if compiled against OpenSSL, its leaking everywhere.

The positive side is that only 8.1 has openssl autoconfiguration,
older versions default to builtin algorithms that can be changed
only by editing Makefile, thus most packages are hopefully safe.

--
marko

---(end of broadcast)---
TIP 6: explain analyze is your friend


Re: [PATCHES] pgcrypto: fix memory leak in openssl.c

2006-02-20 Thread Tom Lane
Marko Kreen [EMAIL PROTECTED] writes:
 On 2/18/06, Marko Kreen [EMAIL PROTECTED] wrote:
 pgcrypto crypt()/md5 and hmac() leak memory when compiled against
 OpenSSL as openssl.c digest -reset will do two DigestInit calls
 against a context.  This happened to work with OpenSSL 0.9.6
 but not with 0.9.7+.

 Ugh, seems I read the old code slightly wrong.  The leak happens
 also with regular digest(), although it will leak only 1 context
 instance, not the 1000+ as the crypt-md5 does.

I'm confused --- does this mean that the patch you sent recently
needs further work?

regards, tom lane

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [PATCHES] pgcrypto: fix memory leak in openssl.c

2006-02-20 Thread Marko Kreen
On 2/20/06, Tom Lane [EMAIL PROTECTED] wrote:
 Marko Kreen [EMAIL PROTECTED] writes:
  On 2/18/06, Marko Kreen [EMAIL PROTECTED] wrote:
  pgcrypto crypt()/md5 and hmac() leak memory when compiled against
  OpenSSL as openssl.c digest -reset will do two DigestInit calls
  against a context.  This happened to work with OpenSSL 0.9.6
  but not with 0.9.7+.

  Ugh, seems I read the old code slightly wrong.  The leak happens
  also with regular digest(), although it will leak only 1 context
  instance, not the 1000+ as the crypt-md5 does.

 I'm confused --- does this mean that the patch you sent recently
 needs further work?

No, it's fine.  As I did not 'fix' old code but replaced it.

It's just that I gave wrong answer to the question 'who is affected?'

--
marko

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [PATCHES] pgcrypto: fix memory leak in openssl.c

2006-02-18 Thread Neil Conway
On Sat, 2006-02-18 at 02:23 +0200, Marko Kreen wrote:
 Attached are one patch for 7.3, 7.4, 8.0 branches and another
 for 8.1 and HEAD.

Thanks, patches applied to the appropriate branches.

-Neil



---(end of broadcast)---
TIP 6: explain analyze is your friend


[PATCHES] pgcrypto: fix memory leak in openssl.c

2006-02-17 Thread Marko Kreen
pgcrypto crypt()/md5 and hmac() leak memory when compiled against
OpenSSL as openssl.c digest -reset will do two DigestInit calls
against a context.  This happened to work with OpenSSL 0.9.6
but not with 0.9.7+.

Reason for the messy code was that I tried to avoid creating
wrapper structure to transport algorithm info and tried to use
OpenSSL context for it.  The fix is to create wrapper structure.

It also uses newer digest API to avoid memory allocations
on reset with newer OpenSSLs.

Attached are one patch for 7.3, 7.4, 8.0 branches and another
for 8.1 and HEAD.

Thanks to Daniel Blaisdell for reporting it.

--
marko
Index: contrib/pgcrypto/openssl.c
===
RCS file: /opt/cvs/pgsql/contrib/pgcrypto/openssl.c,v
retrieving revision 1.12.4.1
diff -u -c -r1.12.4.1 openssl.c
*** contrib/pgcrypto/openssl.c	12 Mar 2005 06:55:14 -	1.12.4.1
--- contrib/pgcrypto/openssl.c	17 Feb 2006 23:31:29 -
***
*** 36,95 
  #include openssl/evp.h
  
  /*
   * Hashes
   */
  static unsigned
  digest_result_size(PX_MD * h)
  {
! 	return EVP_MD_CTX_size((EVP_MD_CTX *) h-p.ptr);
  }
  
  static unsigned
  digest_block_size(PX_MD * h)
  {
! 	return EVP_MD_CTX_block_size((EVP_MD_CTX *) h-p.ptr);
  }
  
  static void
  digest_reset(PX_MD * h)
  {
! 	EVP_MD_CTX *ctx = (EVP_MD_CTX *) h-p.ptr;
! 	const EVP_MD *md;
  
! 	md = EVP_MD_CTX_md(ctx);
! 
! 	EVP_DigestInit(ctx, md);
  }
  
  static void
  digest_update(PX_MD * h, const uint8 *data, unsigned dlen)
  {
! 	EVP_MD_CTX *ctx = (EVP_MD_CTX *) h-p.ptr;
  
! 	EVP_DigestUpdate(ctx, data, dlen);
  }
  
  static void
  digest_finish(PX_MD * h, uint8 *dst)
  {
! 	EVP_MD_CTX *ctx = (EVP_MD_CTX *) h-p.ptr;
! 	const EVP_MD *md = EVP_MD_CTX_md(ctx);
! 
! 	EVP_DigestFinal(ctx, dst, NULL);
  
! 	/*
! 	 * Some builds of 0.9.7x clear all of ctx in EVP_DigestFinal.
! 	 * Fix it by reinitializing ctx.
! 	 */
! 	EVP_DigestInit(ctx, md);
  }
  
  static void
  digest_free(PX_MD * h)
  {
! 	EVP_MD_CTX *ctx = (EVP_MD_CTX *) h-p.ptr;
  
! 	px_free(ctx);
  	px_free(h);
  }
  
--- 36,124 
  #include openssl/evp.h
  
  /*
+  * Backwards compatibility code for digest.
+  */
+ #if OPENSSL_VERSION_NUMBER  0x00907000L
+ 
+ static void EVP_MD_CTX_init(EVP_MD_CTX *ctx)
+ {
+ 	memset(ctx, 0, sizeof(*ctx));
+ }
+ 
+ static int EVP_MD_CTX_cleanup(EVP_MD_CTX *ctx)
+ {
+ 	memset(ctx, 0, sizeof(*ctx));
+ 	return 1;
+ }
+ 
+ static int EVP_DigestInit_ex(EVP_MD_CTX *ctx, const EVP_MD *md, void *engine)
+ {
+ 	EVP_DigestInit(ctx, md);
+ 	return 1;
+ }
+ 
+ static int EVP_DigestFinal_ex(EVP_MD_CTX *ctx, unsigned char *res, unsigned int *len)
+ {
+ 	EVP_DigestFinal(ctx, res, len);
+ 	return 1;
+ }
+ 
+ #endif
+ 
+ /*
   * Hashes
   */
+ 
+ typedef struct OSSLDigest {
+ 	const EVP_MD *algo;
+ 	EVP_MD_CTX ctx;
+ } OSSLDigest;
+ 
  static unsigned
  digest_result_size(PX_MD * h)
  {
! 	OSSLDigest *digest = (OSSLDigest *)h-p.ptr;
! 	return EVP_MD_CTX_size(digest-ctx);
  }
  
  static unsigned
  digest_block_size(PX_MD * h)
  {
! 	OSSLDigest *digest = (OSSLDigest *)h-p.ptr;
! 	return EVP_MD_CTX_block_size(digest-ctx);
  }
  
  static void
  digest_reset(PX_MD * h)
  {
! 	OSSLDigest *digest = (OSSLDigest *)h-p.ptr;
  
! 	EVP_DigestInit_ex(digest-ctx, digest-algo, NULL);
  }
  
  static void
  digest_update(PX_MD * h, const uint8 *data, unsigned dlen)
  {
! 	OSSLDigest *digest = (OSSLDigest *)h-p.ptr;
  
! 	EVP_DigestUpdate(digest-ctx, data, dlen);
  }
  
  static void
  digest_finish(PX_MD * h, uint8 *dst)
  {
! 	OSSLDigest *digest = (OSSLDigest *)h-p.ptr;
  
! 	EVP_DigestFinal_ex(digest-ctx, dst, NULL);
  }
  
  static void
  digest_free(PX_MD * h)
  {
! 	OSSLDigest *digest = (OSSLDigest *)h-p.ptr;
  
! 	EVP_MD_CTX_cleanup(digest-ctx);
! 	px_free(digest);
  	px_free(h);
  }
  
***
*** 101,108 
  px_find_digest(const char *name, PX_MD ** res)
  {
  	const EVP_MD *md;
- 	EVP_MD_CTX *ctx;
  	PX_MD	   *h;
  
  	if (!px_openssl_initialized)
  	{
--- 130,137 
  px_find_digest(const char *name, PX_MD ** res)
  {
  	const EVP_MD *md;
  	PX_MD	   *h;
+ 	OSSLDigest *digest;
  
  	if (!px_openssl_initialized)
  	{
***
*** 114,121 
  	if (md == NULL)
  		return -1;
  
! 	ctx = px_alloc(sizeof(*ctx));
! 	EVP_DigestInit(ctx, md);
  
  	h = px_alloc(sizeof(*h));
  	h-result_size = digest_result_size;
--- 143,154 
  	if (md == NULL)
  		return -1;
  
! 	digest = px_alloc(sizeof(*digest));
! 	digest-algo = md;
! 	
! 	EVP_MD_CTX_init(digest-ctx);
! 	if (EVP_DigestInit_ex(digest-ctx, digest-algo, NULL) == 0)
! 		return -1;
  
  	h = px_alloc(sizeof(*h));
  	h-result_size = digest_result_size;
***
*** 124,130 
  	h-update = digest_update;
  	h-finish = digest_finish;
  	h-free = digest_free;
! 	h-p.ptr = (void *) ctx;
  
  	*res = h;
  	return 0;
--- 157,163 
  	h-update = digest_update;
  	h-finish = digest_finish;
  	h-free = digest_free;
! 	h-p.ptr = (void *) digest;
  
  	*res = h;
  	return 0;