Skip to contentSkip to conversation

Use the garbage collector when retrieving x509 fields

PATCH In review Adriaan de Jong 3 participants 11 messages Feb 14, 2012 – Apr 2, 2012

lore
Share & tools
lore
#0 lore raw

cover Use the garbage collector when retrieving x509 fields

A number of the x509 functions allocated memory directly, instead of using the default OpenVPN convention: the garbage collector. This is fixed in this series of patches.

Adriaan

#1 lore raw

PATCH 1/3 Migrated x509_get_subject to use of the garbage collector

This also cleans up a messy call in pkcs11.c to _openssl_get_subject, as discussed at FOSDEM.

---
 pkcs11.c              |   10 ++++++----
 pkcs11_backend.h      |    8 +++-----
 pkcs11_openssl.c      |   16 +++++++---------
 pkcs11_polarssl.c     |   12 +++++++-----
 ssl_verify.c          |    7 ++++---
 ssl_verify_backend.h  |   11 ++---------
 ssl_verify_openssl.c  |   35 +++++++++++------------------------
 ssl_verify_polarssl.c |   26 +++++++++++---------------
 8 files changed, 51 insertions(+), 74 deletions(-)
pkcs11.c +6 -4
index 7c0b90a..3fa5197 100644
@@ -827,7 +827,8 @@ show_pkcs11_ids (
 	);
 	for (current = user_certificates;current != NULL; current = current->next) {
 		pkcs11h_certificate_t certificate = NULL;
-		char dn[1024] = {0};
+		struct gc_arena gc = gc_new();
+		char *dn = NULL;
 		char serial[1024] = {0};
 		char *ser = NULL;
 		size_t ser_len = 0;
@@ -876,10 +877,9 @@ show_pkcs11_ids (
 		}
 
 		if (
-		      (pkcs11_certificate_dn (
+		      (dn = pkcs11_certificate_dn (
 				certificate,
-				dn,
-				sizeof(dn)
+				&gc
 		      ))
 		) {
 			goto cleanup1;
@@ -920,6 +920,8 @@ show_pkcs11_ids (
 			free (ser);
 			ser = NULL;
 		}
+
+		gc_free (&gc);
 	}
 
 cleanup:
pkcs11_backend.h +3 -5
index 6b5ad31..7b7ec45 100644
@@ -42,13 +42,11 @@
  * Retrieve PKCS #11 Certificate's DN in a printable format.
  *
  * @param certificate 	The PKCS #11 helper certificate object
- * @param dn		Buffer that the certificate subject DN will be placed in.
- * @param dn_len	Size of said buffer.
+ * @param gc		Garbage collection pool to allocate memory in
  *
- * @return 		1 on failure, 0 on success
+ * @return 		Certificate's DN on success, NULL on failure
  */
-int pkcs11_certificate_dn (pkcs11h_certificate_t certificate, char *dn,
-    size_t dn_len);
+char * pkcs11_certificate_dn (pkcs11h_certificate_t certificate, struct gc_arena *gc);
 
 /**
  * Retrieve PKCS #11 Certificate's serial number in a printable format.
pkcs11_openssl.c +7 -9
index aa1eccc..797772a 100644
@@ -33,6 +33,7 @@
 
 #include "errlevel.h"
 #include "pkcs11_backend.h"
+#include "ssl_verify.h"
 #include <pkcs11-helper-1.0/pkcs11h-openssl.h>
 
 int
@@ -115,23 +116,20 @@ cleanup:
   return ret;
 }
 
-int
-pkcs11_certificate_dn (pkcs11h_certificate_t certificate, char *dn,
-    size_t dn_len)
+char *
+pkcs11_certificate_dn (pkcs11h_certificate_t certificate, struct gc_arena *gc)
 {
   X509 *x509 = NULL;
-  int ret = 1;
+
+  char *dn = NULL;
 
   if ((x509 = pkcs11h_openssl_getX509 (certificate)) == NULL)
     {
       msg (M_FATAL, "PKCS#11: Cannot get X509");
-      ret = 1;
       goto cleanup;
     }
 
-  _openssl_get_subject (x509, dn, dn_len);
-
-  ret = 0;
+  dn = x509_get_subject (x509, gc);
 
 cleanup:
   if (x509 != NULL)
@@ -140,7 +138,7 @@ cleanup:
       x509 = NULL;
     }
 
-  return ret;
+  return dn;
 }
 
 int
pkcs11_polarssl.c +7 -5
index 0f9daab..2c8ced6 100644
@@ -66,11 +66,11 @@ cleanup:
   return ret;
 }
 
-int
-pkcs11_certificate_dn (pkcs11h_certificate_t cert, char *dn,
-    size_t dn_len)
+char *
+pkcs11_certificate_dn (pkcs11h_certificate_t cert, struct gc_arena *gc)
 {
   int ret = 1;
+  char dn[1024] = {0};
 
   x509_cert polar_cert = {0};
 
@@ -79,7 +79,7 @@ pkcs11_certificate_dn (pkcs11h_certificate_t cert, char *dn,
       goto cleanup;
   }
 
-  if (-1 == x509parse_dn_gets (dn, dn_len, &polar_cert.subject)) {
+  if (-1 == x509parse_dn_gets (dn, sizeof(dn), &polar_cert.subject)) {
       msg (M_FATAL, "PKCS#11: PolarSSL cannot parse subject");
       goto cleanup;
   }
@@ -89,7 +89,9 @@ pkcs11_certificate_dn (pkcs11h_certificate_t cert, char *dn,
 cleanup:
   x509_free(&polar_cert);
 
-  return ret;
+  if (ret == 0)
+    return string_alloc(dn, gc);
+  return NULL;
 }
 
 int
ssl_verify.c +4 -3
index a7b361f..06585d8 100644
@@ -565,6 +565,7 @@ verify_cert(struct tls_session *session, x509_cert_t *cert, int cert_depth)
   char *subject = NULL;
   char common_name[TLS_USERNAME_LEN] = {0};
   const struct tls_options *opt;
+  struct gc_arena gc = gc_new();
 
   opt = session->opt;
   ASSERT (opt);
@@ -572,7 +573,7 @@ verify_cert(struct tls_session *session, x509_cert_t *cert, int cert_depth)
   session->verified = false;
 
   /* get the X509 name */
-  subject = x509_get_subject(cert);
+  subject = x509_get_subject(cert, &gc);
   if (!subject)
     {
 	msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, could not extract X509 "
@@ -670,13 +671,13 @@ verify_cert(struct tls_session *session, x509_cert_t *cert, int cert_depth)
   msg (D_HANDSHAKE, "VERIFY OK: depth=%d, %s", cert_depth, subject);
   session->verified = true;
 
-  x509_free_subject (subject);
+  gc_free(&gc);
   return SUCCESS;
 
  err:
   tls_clear_error();
   session->verified = false;
-  x509_free_subject (subject);
+  gc_free(&gc);
   return FAILURE;
 }
 
ssl_verify_backend.h +2 -9
index 2ba3723..a1fd682 100644
@@ -80,20 +80,13 @@ void cert_hash_remember (struct tls_session *session, const int cert_depth,
 /*
  * Retrieve certificate's subject name.
  *
- * The returned string must be freed with \c verify_free_subject()
- *
  * @param cert		Certificate to retrieve the subject from.
+ * @param gc		Garbage collection arena to use when allocating string.
  *
  * @return 		a string containing the subject
  */
-char *x509_get_subject (x509_cert_t *cert);
+char *x509_get_subject (x509_cert_t *cert, struct gc_arena *gc);
 
-/*
- * Free a subject string as returned by \c verify_get_subject()
- *
- * @param subject	The subject to be freed.
- */
-void x509_free_subject (char *subject);
 
 /* Retrieve the certificate's SHA1 hash.
  *
ssl_verify_openssl.c +11 -24
index 200a570..473835e 100644
@@ -42,6 +42,7 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
 {
   struct tls_session *session;
   SSL *ssl;
+  struct gc_arena gc = gc_new();
   unsigned char *sha1_hash = NULL;
 
   /* get the tls_session pointer */
@@ -58,7 +59,7 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
   if (!preverify_ok)
     {
       /* get the X509 name */
-      char *subject = x509_get_subject(ctx->current_cert);
+      char *subject = x509_get_subject(ctx->current_cert, &gc);
 
       if (subject)
 	{
@@ -67,16 +68,18 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
 	      ctx->error_depth,
 	      X509_verify_cert_error_string (ctx->error),
 	      subject);
-	  x509_free_subject(subject);
 	}
 
       ERR_clear_error();
 
+      gc_free(&gc);
       session->verified = false;
 
       return 0;
     }
 
+  gc_free(&gc);
+
   if (SUCCESS == verify_cert(session, ctx->current_cert, ctx->error_depth))
     return 1;
   return 0;
@@ -247,12 +250,12 @@ x509_free_sha1_hash (unsigned char *hash)
 }
 
 char *
-_openssl_get_subject (X509 *cert, char *buf, int size)
+x509_get_subject (X509 *cert, struct gc_arena *gc)
 {
   BIO *subject_bio = NULL;
   BUF_MEM *subject_mem;
-  char *subject = buf;
-  int maxlen = size;
+  char *subject = NULL;
+  int maxlen = 0;
 
   subject_bio = BIO_new (BIO_s_mem ());
   if (subject_bio == NULL)
@@ -266,12 +269,9 @@ _openssl_get_subject (X509 *cert, char *buf, int size)
     goto err;
 
   BIO_get_mem_ptr (subject_bio, &subject_mem);
-  if (subject == NULL)
-    {
-      maxlen = subject_mem->length + 1;
-      subject = malloc (maxlen);
-      check_malloc_return (subject);
-    }
+
+  maxlen = subject_mem->length + 1;
+  subject = gc_malloc (maxlen, false, gc);
 
   memcpy (subject, subject_mem->data, maxlen);
   subject[maxlen - 1] = '\0';
@@ -283,19 +283,6 @@ err:
   return subject;
 }
 
-char *
-x509_get_subject (X509 *cert)
-{
-  return _openssl_get_subject (cert, NULL, 0);
-}
-
-void
-x509_free_subject (char *subject)
-{
-  if (subject)
-    free(subject);
-}
-
 
 #ifdef ENABLE_X509_TRACK
 
ssl_verify_polarssl.c +11 -15
index 699eb47..c4afb24 100644
@@ -41,6 +41,7 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
     int preverify_ok)
 {
   struct tls_session *session = (struct tls_session *) session_obj;
+  struct gc_arena gc = gc_new();
   unsigned char *sha1_hash = NULL;
 
   ASSERT (cert);
@@ -56,21 +57,23 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
   /* did peer present cert which was signed by our root cert? */
   if (!preverify_ok)
     {
-      char subject[MAX_SUBJECT_LENGTH] = {0};
+      char *subject = x509_get_subject(cert, &gc);
 
-      /* get the X509 name */
-      if (x509parse_dn_gets( subject, MAX_SUBJECT_LENGTH, &cert->subject ) < 0)
-	  msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, could not extract X509 "
-	      "subject string from certificate", cert_depth);
-      else
+      if (subject)
 	msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, %s", cert_depth, subject);
+      else
+	msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, could not extract X509 "
+	      "subject string from certificate", cert_depth);
 
+      gc_free(&gc);
       return 1;
     }
 
   /*
    * PolarSSL expects 1 on failure, 0 on success
    */
+  gc_free(&gc);
+
   if (SUCCESS == verify_cert(session, cert, cert_depth))
     return 0;
   return 1;
@@ -158,7 +161,7 @@ x509_free_sha1_hash (unsigned char *hash)
 }
 
 char *
-x509_get_subject(x509_cert *cert)
+x509_get_subject(x509_cert *cert, struct gc_arena *gc)
 {
   char tmp_subject[MAX_SUBJECT_LENGTH] = {0};
   char *subject = NULL;
@@ -169,19 +172,12 @@ x509_get_subject(x509_cert *cert)
   if (ret > 0)
     {
       /* Allocate the required space for the subject */
-      subject = malloc(ret + 1);
-      strncpy(subject, tmp_subject, ret+1);
+      subject = string_alloc(tmp_subject, gc);
     }
 
   return subject;
 }
 
-void
-x509_free_subject (char *subject)
-{
-  if (subject)
-    free(subject);
-}
 
 /*
  * Save X509 fields to environment, using the naming convention:
Signature · 1 line
1.7.5.4
#2 lore raw

PATCH 2/3 Migrated x509_get_serial to use the garbage collector

In reply to Adriaan de Jong
---
 ssl_verify.c          |   28 ++++++++++++++--------------
 ssl_verify_backend.h  |   11 ++---------
 ssl_verify_openssl.c  |   17 +++++++----------
 ssl_verify_polarssl.c |   17 +++--------------
 4 files changed, 26 insertions(+), 47 deletions(-)
ssl_verify.c +14 -14
index 06585d8..352118a 100644
@@ -377,6 +377,8 @@ verify_cert_set_env(struct env_set *es, x509_cert_t *peer_cert, int cert_depth,
     )
 {
   char envname[64];
+  char *serial = NULL;
+  struct gc_arena gc = gc_new ();
 
   /* Save X509 fields in environment */
 #ifdef ENABLE_X509_TRACK
@@ -399,25 +401,21 @@ verify_cert_set_env(struct env_set *es, x509_cert_t *peer_cert, int cert_depth,
 #ifdef ENABLE_EUREPHIA
   /* export X509 cert SHA1 fingerprint */
   {
-    struct gc_arena gc = gc_new ();
     unsigned char *sha1_hash = x509_get_sha1_hash(peer_cert);
 
     openvpn_snprintf (envname, sizeof(envname), "tls_digest_%d", cert_depth);
     setenv_str (es, envname, format_hex_ex(sha1_hash, SHA_DIGEST_LENGTH, 0, 1,
 					  ":", &gc));
     x509_free_sha1_hash(sha1_hash);
-    gc_free(&gc);
   }
 #endif
 
-  /* export serial number as environmental variable,
-     use bignum in case serial number is large */
-  {
-    char *serial = x509_get_serial(peer_cert);
-    openvpn_snprintf (envname, sizeof(envname), "tls_serial_%d", cert_depth);
-    setenv_str (es, envname, serial);
-    x509_free_serial(serial);
-  }
+  /* export serial number as environmental variable */
+  serial = x509_get_serial(peer_cert, &gc);
+  openvpn_snprintf (envname, sizeof(envname), "tls_serial_%d", cert_depth);
+  setenv_str (es, envname, serial);
+
+  gc_free(&gc);
 }
 
 /*
@@ -537,24 +535,26 @@ verify_check_crl_dir(const char *crl_dir, x509_cert_t *cert)
 {
   char fn[256];
   int fd;
-  char *serial = x509_get_serial(cert);
+  struct gc_arena gc = gc_new();
+
+  char *serial = x509_get_serial(cert, &gc);
 
   if (!openvpn_snprintf(fn, sizeof(fn), "%s%c%s", crl_dir, OS_SPECIFIC_DIRSEP, serial))
     {
       msg (D_HANDSHAKE, "VERIFY CRL: filename overflow");
-      x509_free_serial(serial);
+      gc_free(&gc);
       return FAILURE;
     }
   fd = openvpn_open (fn, O_RDONLY, 0);
   if (fd >= 0)
     {
       msg (D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
-      x509_free_serial(serial);
       close(fd);
+      gc_free(&gc);
       return FAILURE;
     }
 
-  x509_free_serial(serial);
+  gc_free(&gc);
 
   return SUCCESS;
 }
ssl_verify_backend.h +2 -9
index a1fd682..5c69b11 100644
@@ -125,20 +125,13 @@ result_t x509_get_username (char *common_name, int cn_len,
  * Return the certificate's serial number.
  *
  * The serial number is returned as a string, since it might be a bignum.
- * The returned string must be freed with \c verify_free_serial()
  *
  * @param cert		Certificate to retrieve the serial number from.
+ * @param gc		Garbage collection arena to use when allocating string.
  *
  * @return 		The certificate's serial number.
  */
-char *x509_get_serial (x509_cert_t *cert);
-
-/*
- * Free a serial number string as returned by \c verify_get_serial()
- *
- * @param serial	The string to be freed.
- */
-void x509_free_serial (char *serial);
+char *x509_get_serial (x509_cert_t *cert, struct gc_arena *gc);
 
 /*
  * Save X509 fields to environment, using the naming convention:
ssl_verify_openssl.c +7 -10
index 473835e..6d31bb3 100644
@@ -213,25 +213,22 @@ x509_get_username (char *common_name, int cn_len,
 }
 
 char *
-x509_get_serial (x509_cert_t *cert)
+x509_get_serial (x509_cert_t *cert, struct gc_arena *gc)
 {
   ASN1_INTEGER *asn1_i;
   BIGNUM *bignum;
-  char *serial;
+  char *openssl_serial, *serial;
 
   asn1_i = X509_get_serialNumber(cert);
   bignum = ASN1_INTEGER_to_BN(asn1_i, NULL);
-  serial = BN_bn2dec(bignum);
+  openssl_serial = BN_bn2dec(bignum);
+
+  serial = string_alloc(openssl_serial, gc);
 
   BN_free(bignum);
-  return serial;
-}
+  OPENSSL_free(openssl_serial);
 
-void
-x509_free_serial (char *serial)
-{
-  if (serial)
-    OPENSSL_free(serial);
+  return serial;
 }
 
 unsigned char *
ssl_verify_polarssl.c +3 -14
index c4afb24..065b30d 100644
@@ -119,32 +119,21 @@ x509_get_username (char *cn, int cn_len,
 }
 
 char *
-x509_get_serial (x509_cert *cert)
+x509_get_serial (x509_cert *cert, struct gc_arena *gc)
 {
   int ret = 0;
   int i = 0;
   char *buf = NULL;
   size_t len = cert->serial.len * 3;
 
-  buf = malloc(len);
-  ASSERT(buf);
+  buf = gc_malloc(len, true, gc);
 
   if(x509parse_serial_gets(buf, len-1, &cert->serial) < 0)
-    {
-      free(buf);
-      buf = NULL;
-    }
+    buf = NULL;
 
   return buf;
 }
 
-void
-x509_free_serial (char *serial)
-{
-  if (serial)
-    free(serial);
-}
-
 unsigned char *
 x509_get_sha1_hash (x509_cert *cert)
 {
Signature · 1 line
1.7.5.4
#3 lore raw

PATCH 3/3 Migrated x509_get_sha1_hash to use the garbage collector

In reply to Adriaan de Jong
---
 ssl_verify.c          |    7 ++-----
 ssl_verify_backend.h  |   11 ++---------
 ssl_verify_openssl.c  |   17 ++++-------------
 ssl_verify_polarssl.c |   17 +++--------------
 4 files changed, 11 insertions(+), 41 deletions(-)
ssl_verify.c +2 -5
index 352118a..018278e 100644
@@ -401,12 +401,11 @@ verify_cert_set_env(struct env_set *es, x509_cert_t *peer_cert, int cert_depth,
 #ifdef ENABLE_EUREPHIA
   /* export X509 cert SHA1 fingerprint */
   {
-    unsigned char *sha1_hash = x509_get_sha1_hash(peer_cert);
+    unsigned char *sha1_hash = x509_get_sha1_hash(peer_cert, &gc);
 
     openvpn_snprintf (envname, sizeof(envname), "tls_digest_%d", cert_depth);
     setenv_str (es, envname, format_hex_ex(sha1_hash, SHA_DIGEST_LENGTH, 0, 1,
 					  ":", &gc));
-    x509_free_sha1_hash(sha1_hash);
   }
 #endif
 
@@ -614,14 +613,12 @@ verify_cert(struct tls_session *session, x509_cert_t *cert, int cert_depth)
   /* verify level 1 cert, i.e. the CA that signed our leaf cert */
   if (cert_depth == 1 && opt->verify_hash)
     {
-      unsigned char *sha1_hash = x509_get_sha1_hash(cert);
+      unsigned char *sha1_hash = x509_get_sha1_hash(cert, &gc);
       if (memcmp (sha1_hash, opt->verify_hash, SHA_DIGEST_LENGTH))
       {
 	msg (D_TLS_ERRORS, "TLS Error: level-1 certificate hash verification failed");
-	x509_free_sha1_hash(sha1_hash);
 	goto err;
       }
-      x509_free_sha1_hash(sha1_hash);
     }
 
   /* save common name in session object */
ssl_verify_backend.h +2 -9
index 5c69b11..ae64061 100644
@@ -90,20 +90,13 @@ char *x509_get_subject (x509_cert_t *cert, struct gc_arena *gc);
 
 /* Retrieve the certificate's SHA1 hash.
  *
- * The returned string must be freed with \c verify_free_sha1_hash()
- *
  * @param cert		Certificate to retrieve the hash from.
+ * @param gc		Garbage collection arena to use when allocating string.
  *
  * @return 		a string containing the SHA1 hash of the certificate
  */
-unsigned char *x509_get_sha1_hash (x509_cert_t *cert);
+unsigned char *x509_get_sha1_hash (x509_cert_t *cert, struct gc_arena *gc);
 
-/*
- * Free a hash as returned by \c verify_get_hash()
- *
- * @param hash		The subject to be freed.
- */
-void x509_free_sha1_hash (unsigned char *hash);
 
 /*
  * Retrieve the certificate's username from the specified field.
ssl_verify_openssl.c +4 -13
index 6d31bb3..f7e7d52 100644
@@ -43,7 +43,6 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
   struct tls_session *session;
   SSL *ssl;
   struct gc_arena gc = gc_new();
-  unsigned char *sha1_hash = NULL;
 
   /* get the tls_session pointer */
   ssl = X509_STORE_CTX_get_ex_data (ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
@@ -51,9 +50,8 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
   session = (struct tls_session *) SSL_get_ex_data (ssl, mydata_index);
   ASSERT (session);
 
-  sha1_hash = x509_get_sha1_hash(ctx->current_cert);
-  cert_hash_remember (session, ctx->error_depth, sha1_hash);
-  x509_free_sha1_hash(sha1_hash);
+  cert_hash_remember (session, ctx->error_depth,
+      x509_get_sha1_hash(ctx->current_cert, &gc));
 
   /* did peer present cert which was signed by our root cert? */
   if (!preverify_ok)
@@ -232,20 +230,13 @@ x509_get_serial (x509_cert_t *cert, struct gc_arena *gc)
 }
 
 unsigned char *
-x509_get_sha1_hash (X509 *cert)
+x509_get_sha1_hash (X509 *cert, struct gc_arena *gc)
 {
-  char *hash = malloc(SHA_DIGEST_LENGTH);
+  char *hash = gc_malloc(SHA_DIGEST_LENGTH, false, gc);
   memcpy(hash, cert->sha1_hash, SHA_DIGEST_LENGTH);
   return hash;
 }
 
-void
-x509_free_sha1_hash (unsigned char *hash)
-{
-  if (hash)
-    free(hash);
-}
-
 char *
 x509_get_subject (X509 *cert, struct gc_arena *gc)
 {
ssl_verify_polarssl.c +3 -14
index 065b30d..2ba0a5c 100644
@@ -42,7 +42,6 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
 {
   struct tls_session *session = (struct tls_session *) session_obj;
   struct gc_arena gc = gc_new();
-  unsigned char *sha1_hash = NULL;
 
   ASSERT (cert);
   ASSERT (session);
@@ -50,9 +49,7 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
   session->verified = false;
 
   /* Remember certificate hash */
-  sha1_hash = x509_get_sha1_hash(cert);
-  cert_hash_remember (session, cert_depth, sha1_hash);
-  x509_free_sha1_hash(sha1_hash);
+  cert_hash_remember (session, cert_depth, x509_get_sha1_hash(cert, &gc));
 
   /* did peer present cert which was signed by our root cert? */
   if (!preverify_ok)
@@ -135,20 +132,13 @@ x509_get_serial (x509_cert *cert, struct gc_arena *gc)
 }
 
 unsigned char *
-x509_get_sha1_hash (x509_cert *cert)
+x509_get_sha1_hash (x509_cert *cert, struct gc_arena *gc)
 {
-  unsigned char *sha1_hash = malloc(SHA_DIGEST_LENGTH);
+  unsigned char *sha1_hash = gc_malloc(SHA_DIGEST_LENGTH, false, gc);
   sha1(cert->tbs.p, cert->tbs.len, sha1_hash);
   return sha1_hash;
 }
 
-void
-x509_free_sha1_hash (unsigned char *hash)
-{
-  if (hash)
-    free(hash);
-}
-
 char *
 x509_get_subject(x509_cert *cert, struct gc_arena *gc)
 {
@@ -167,7 +157,6 @@ x509_get_subject(x509_cert *cert, struct gc_arena *gc)
   return subject;
 }
 
-
 /*
  * Save X509 fields to environment, using the naming convention:
  *
Signature · 1 line
1.7.5.4
#4 lore raw
In reply to Adriaan de Jong
Quoted 5 lines · Adriaan de Jong

A number of the x509 functions allocated memory directly, instead of using the default OpenVPN convention: the garbage collector. This is fixed in this series of patches.

Adriaan

These patches got ACKed by James and me at the developers meeting yesterday. They are now applied to the master branch on -testing and -stable trees.

commit 8e5613c2a8545a67cab2734569a8f088100d731b

Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:26 2012 +0100
    Migrated x509_get_sha1_hash to use the garbage collector

    Signed-off-by: Adriaan de Jong <[email protected]...>
    Acked-by: James Yonan <[email protected]...>
    Acked-by: David Sommerseth <[email protected]...>
    Signed-off-by: David Sommerseth <[email protected]...>

commit 025f30d7c6434aaf0ab4af3744f76aaf8c0b71d6

Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:25 2012 +0100
    Migrated x509_get_serial to use the garbage collector

    Signed-off-by: Adriaan de Jong <[email protected]...>
    Acked-by: James Yonan <[email protected]...>
    Acked-by: David Sommerseth <[email protected]...>
    Signed-off-by: David Sommerseth <[email protected]...>

commit 00b973f8af85c3ea8fa3cef80eec55e8dc139b27

Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:24 2012 +0100

Migrated x509_get_subject to use of the garbage collector

This also cleans up a messy call in pkcs11.c to
_openssl_get_subject, as discussed at FOSDEM.

    Signed-off-by: Adriaan de Jong <[email protected]...>
    Acked-by: James Yonan <[email protected]...>
    Acked-by: David Sommerseth <[email protected]...>
    Signed-off-by: David Sommerseth <[email protected]...>

kind regards,

David Sommerseth

#5 lore raw

Hm.... I missed that one.

I don't like this patch...

the gc_new() and gc_free() should be called once per function at prolog and epilog. This is the whole point of gc, otherwise there is no difference between it and simple malloc.

So:
1. in pkcs11.c it should be moved out of the loop.
2. In verify_cert, verify_callback we should add cleanup label.

Alon.

Quoted 63 lines · David Sommerseth
Quoted 5 lines · Adriaan de Jong

A number of the x509 functions allocated memory directly, instead of using the default OpenVPN convention: the garbage collector. This is fixed in this series of patches.

Adriaan

These patches got ACKed by James and me at the developers meeting yesterday.  They are now applied to the master branch on -testing and -stable trees.

commit 8e5613c2a8545a67cab2734569a8f088100d731b
Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:26 2012 +0100

   Migrated x509_get_sha1_hash to use the garbage collector

commit 025f30d7c6434aaf0ab4af3744f76aaf8c0b71d6
Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:25 2012 +0100

   Migrated x509_get_serial to use the garbage collector

commit 00b973f8af85c3ea8fa3cef80eec55e8dc139b27
Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:24 2012 +0100

   Migrated x509_get_subject to use of the garbage collector

   This also cleans up a messy call in pkcs11.c to _openssl_get_subject, as discussed at FOSDEM.

kind regards,

David Sommerseth

------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

#6 lore raw

PATCH 1/1 cleanup: gc usage

Cleanup of "Use the garbage collector when retrieving x509 fields" patch series.

Discussed at [1].

There should be an effort to produce common function prologue and epilogue, so that cleanups will be done at single point.

[1] http://comments.gmane.org/gmane.network.openvpn.devel/5401

---
 src/openvpn/pkcs11.c              |    5 +--
 src/openvpn/pkcs11_polarssl.c     |    8 ++---
 src/openvpn/ssl_verify.c          |   53 +++++++++++++++++++++----------------
 src/openvpn/ssl_verify_openssl.c  |   15 ++++++----
 src/openvpn/ssl_verify_polarssl.c |   18 +++++++-----
 5 files changed, 55 insertions(+), 44 deletions(-)
src/openvpn/pkcs11.c +2 -3
index a6b8db5..d86e267 100644
@@ -769,6 +769,7 @@ show_pkcs11_ids (
 	const char * const provider,
 	bool cert_private
 ) {
+	struct gc_arena gc = gc_new();
 	pkcs11h_certificate_id_list_t user_certificates = NULL;
 	pkcs11h_certificate_id_list_t current = NULL;
 	CK_RV rv = CKR_FUNCTION_FAILED;
@@ -834,7 +835,6 @@ show_pkcs11_ids (
 	);
 	for (current = user_certificates;current != NULL; current = current->next) {
 		pkcs11h_certificate_t certificate = NULL;
-		struct gc_arena gc = gc_new();
 		char *dn = NULL;
 		char serial[1024] = {0};
 		char *ser = NULL;
@@ -927,8 +927,6 @@ show_pkcs11_ids (
 			free (ser);
 			ser = NULL;
 		}
-
-		gc_free (&gc);
 	}
 
 cleanup:
@@ -938,6 +936,7 @@ cleanup:
 	}
 
 	pkcs11h_terminate ();
+	gc_free (&gc);
 }
 
 #else
src/openvpn/pkcs11_polarssl.c +3 -5
index f5b7b8b..03b2bab 100644
@@ -75,7 +75,7 @@ cleanup:
 char *
 pkcs11_certificate_dn (pkcs11h_certificate_t cert, struct gc_arena *gc)
 {
-  int ret = 1;
+  char *ret = NULL;
   char dn[1024] = {0};
 
   x509_cert polar_cert = {0};
@@ -90,14 +90,12 @@ pkcs11_certificate_dn (pkcs11h_certificate_t cert, struct gc_arena *gc)
       goto cleanup;
   }
 
-  ret = 0;
+  ret = string_alloc(dn, gc);
 
 cleanup:
   x509_free(&polar_cert);
 
-  if (ret == 0)
-    return string_alloc(dn, gc);
-  return NULL;
+  return ret;
 }
 
 int
src/openvpn/ssl_verify.c +30 -23

134 lines not on this page · raw message

src/openvpn/ssl_verify_openssl.c +9 -6
index 4dfabfc..f5dce0d 100644
@@ -46,6 +46,7 @@
 int
 verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
 {
+  int ret = 0;
   struct tls_session *session;
   SSL *ssl;
   struct gc_arena gc = gc_new();
@@ -76,17 +77,19 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
 
       ERR_clear_error();
 
-      gc_free(&gc);
       session->verified = false;
-
-      return 0;
+      goto cleanup;
     }
 
+  if (SUCCESS != verify_cert(session, ctx->current_cert, ctx->error_depth))
+    goto cleanup;
+
+  ret = 1;
+
+cleanup:
   gc_free(&gc);
 
-  if (SUCCESS == verify_cert(session, ctx->current_cert, ctx->error_depth))
-    return 1;
-  return 0;
+  return ret;
 }
 
 #ifdef ENABLE_X509ALTUSERNAME
src/openvpn/ssl_verify_polarssl.c +11 -7
index d9d4fd5..a32db8d 100644
@@ -48,6 +48,7 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
 {
   struct tls_session *session = (struct tls_session *) session_obj;
   struct gc_arena gc = gc_new();
+  int ret = 1;
 
   ASSERT (cert);
   ASSERT (session);
@@ -68,18 +69,21 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
 	msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, could not extract X509 "
 	      "subject string from certificate", cert_depth);
 
-      gc_free(&gc);
-      return 1;
+      goto cleanup;
     }
 
+  if (SUCCESS != verify_cert(session, cert, cert_depth))
+    goto cleanup;
+
+  ret = 0;
+
+cleanup:
+  gc_free(&gc);
+
   /*
    * PolarSSL expects 1 on failure, 0 on success
    */
-  gc_free(&gc);
-
-  if (SUCCESS == verify_cert(session, cert, cert_depth))
-    return 0;
-  return 1;
+  return ret;
 }
 
 #ifdef ENABLE_X509ALTUSERNAME
Signature · 1 line
1.7.3.4
#7 lore raw

Re: [Openvpn-devel] [PATCH] cleanup: gc usage

Quoted 49 lines · Alon Bar-Lev

Cleanup of "Use the garbage collector when retrieving x509 fields" patch series.

Discussed at [1].

There should be an effort to produce common function prologue and epilogue, so that cleanups will be done at single point.

[1] http://comments.gmane.org/gmane.network.openvpn.devel/5401

---

int

src/openvpn/ssl_verify.c +5 -7
index 5783528..30fb05d 100644
@@ -538,8 +538,9 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
 static result_t
 verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
 {
+  result_t ret = FAILURE;
   char fn[256];
-  int fd;
+  int fd = -1;
   struct gc_arena gc = gc_new();
 
   char *serial = x509_get_serial(cert, &gc);
@@ -547,26 +548,29 @@ verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
   if (!openvpn_snprintf(fn, sizeof(fn), "%s%c%s", crl_dir, OS_SPECIFIC_DIRSEP, serial))
     {
       msg (D_HANDSHAKE, "VERIFY CRL: filename overflow");
-      gc_free(&gc);
-      return FAILURE;
+      goto cleanup;
     }
   fd = platform_open (fn, O_RDONLY, 0);
   if (fd >= 0)
     {
       msg (D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
-      close(fd);
-      gc_free(&gc);
-      return FAILURE;
+      goto cleanup;
     }
 
-  gc_free(&gc);
+  ret = SUCCESS;

src/openvpn/ssl_verify.c verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert) · line 569

Quoted 10 lines
src/openvpn/ssl_verify.c +6 -1
 
-  return SUCCESS;
+cleanup:
+
+  if (fd != -1)
+    close(fd);
+  gc_free(&gc);
+  return ret;
 }
 

Thanks, looks good. Does platform_open return -1 on failure on all platforms? If so, ack, otherwise change that to < 0.

Adriaan

#8 lore raw

Re: [Openvpn-devel] [PATCH] cleanup: gc usage

Quoted 2 lines · Adriaan de Jong

Thanks, looks good. Does platform_open return -1 on failure on all platforms? If so, ack, otherwise change that to < 0.

As far as I know, yes.

"""open() and creat() return the new file descriptor, or -1 if an error occurred."""

#9 lore raw

Re: [Openvpn-devel] [PATCH] cleanup: gc usage

In reply to Alon Bar-Lev
Quoted 10 lines · Alon Bar-Lev

Cleanup of "Use the garbage collector when retrieving x509 fields" patch series.

Discussed at [1].

There should be an effort to produce common function prologue and epilogue, so that cleanups will be done at single point.

[1] http://comments.gmane.org/gmane.network.openvpn.devel/5401

Quoted 8 lines
---
 src/openvpn/pkcs11.c              |    5 +--
 src/openvpn/pkcs11_polarssl.c     |    8 ++---
 src/openvpn/ssl_verify.c          |   53 +++++++++++++++++++++----------------
 src/openvpn/ssl_verify_openssl.c  |   15 ++++++----
 src/openvpn/ssl_verify_polarssl.c |   18 +++++++-----
 5 files changed, 55 insertions(+), 44 deletions(-)

Applied to master branches on -testing and -stable trees

commit 75b49e406430299b187964744f82e50a9035a0d3

Author: Alon Bar-Lev <[email protected]...>
Date:   Sun Apr 1 16:46:28 2012 +0300
    Signed-off-by: Alon Bar-Lev <[email protected]...>
    Acked-by: Adriaan de Jong <[email protected]...>
    Signed-off-by: David Sommerseth <[email protected]...>

kind regards,

David Sommerseth

#10 lore raw
In reply to Alon Bar-Lev

I sent candidate. Please review.

Quoted 79 lines · Alon Bar-Lev

Hm.... I missed that one.

I don't like this patch...

the gc_new() and gc_free() should be called once per function at prolog and epilog. This is the whole point of gc, otherwise there is no difference between it and simple malloc.

So:
1. in pkcs11.c it should be moved out of the loop.
2. In verify_cert, verify_callback we should add cleanup label.

Alon.

Quoted 63 lines · David Sommerseth
Quoted 5 lines · Adriaan de Jong

A number of the x509 functions allocated memory directly, instead of using the default OpenVPN convention: the garbage collector. This is fixed in this series of patches.

Adriaan

These patches got ACKed by James and me at the developers meeting yesterday.  They are now applied to the master branch on -testing and -stable trees.

commit 8e5613c2a8545a67cab2734569a8f088100d731b
Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:26 2012 +0100

   Migrated x509_get_sha1_hash to use the garbage collector

commit 025f30d7c6434aaf0ab4af3744f76aaf8c0b71d6
Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:25 2012 +0100

   Migrated x509_get_serial to use the garbage collector

commit 00b973f8af85c3ea8fa3cef80eec55e8dc139b27
Author: Adriaan de Jong <[email protected]...>
Date:   Tue Feb 14 11:11:24 2012 +0100

   Migrated x509_get_subject to use of the garbage collector

   This also cleans up a messy call in pkcs11.c to _openssl_get_subject, as discussed at FOSDEM.

kind regards,

David Sommerseth

------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel