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 <dejong@fox-it.com>
Acked-by: James Yonan <james@openvpn.net>
Acked-by: David Sommerseth <davids@redhat.com>
Signed-off-by: David Sommerseth <davids@redhat.com>

diff --git a/src/openvpn/pkcs11.c b/src/openvpn/pkcs11.c
index fd609d493142dc88502e6fd419eb626a6bf9d279..a6b8db5020874e9d0c78275eb27102c284aac079 100644 (file)
--- a/src/openvpn/pkcs11.c
+++ b/src/openvpn/pkcs11.c
@@ -834,7 +834,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;
@@ -883,10 +884,9 @@ show_pkcs11_ids (
                }
 
                if (
-                     (pkcs11_certificate_dn (
+                     (dn = pkcs11_certificate_dn (
                                certificate,
-                               dn,
-                               sizeof(dn)
+                               &gc
                      ))
                ) {
                        goto cleanup1;
@@ -927,6 +927,8 @@ show_pkcs11_ids (
                        free (ser);
                        ser = NULL;
                }
+
+               gc_free (&gc);
        }
 
 cleanup:

diff --git a/src/openvpn/pkcs11_backend.h b/src/openvpn/pkcs11_backend.h
index 6b5ad319515c1bea2782c4b56420577a317e1882..7b7ec45baf3d78bc8098f13ae450023f7fd7861a 100644 (file)
--- a/src/openvpn/pkcs11_backend.h
+++ b/src/openvpn/pkcs11_backend.h
@@ -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.

diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 18651fd9203e08c7221388ad01c1de8f55e0b89c..af843b7b9382b17b744c8615fe8fec60dc137ac4 100644 (file)
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -39,6 +39,7 @@
 
 #include "errlevel.h"
 #include "pkcs11_backend.h"
+#include "ssl_verify.h"
 #include <pkcs11-helper-1.0/pkcs11h-openssl.h>
 
 int
@@ -121,23 +122,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)
@@ -146,7 +144,7 @@ cleanup:
       x509 = NULL;
     }
 
-  return ret;
+  return dn;
 }
 
 int

diff --git a/src/openvpn/pkcs11_polarssl.c b/src/openvpn/pkcs11_polarssl.c
index ecef4d391b0658908e1510516b535e57626bbf56..f5b7b8b388efcf75d48fc7d4b604b48e1203b916 100644 (file)
--- a/src/openvpn/pkcs11_polarssl.c
+++ b/src/openvpn/pkcs11_polarssl.c
@@ -72,11 +72,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};
 
@@ -85,7 +85,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;
   }
@@ -95,7 +95,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

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index e837e39c6f8bd2e7634b82ae17fcec7cbb86a54b..c4612f97cb44d6af5191e2405e4ff08f2c1dee10 100644 (file)
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -571,6 +571,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
   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);
@@ -578,7 +579,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
   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 "
@@ -676,13 +677,13 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
   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;
 }
 

diff --git a/src/openvpn/ssl_verify_backend.h b/src/openvpn/ssl_verify_backend.h
index cc67cb99d95c06a0fe77dd61fcc278ccca27da6b..ac71d182187a6c8a6259debd2c1b5a44ac9baeb2 100644 (file)
--- a/src/openvpn/ssl_verify_backend.h
+++ b/src/openvpn/ssl_verify_backend.h
@@ -80,20 +80,12 @@ 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 (openvpn_x509_cert_t *cert);
-
-/*
- * Free a subject string as returned by \c verify_get_subject()
- *
- * @param subject      The subject to be freed.
- */
-void x509_free_subject (char *subject);
+char *x509_get_subject (openvpn_x509_cert_t *cert, struct gc_arena *gc);
 
 /* Retrieve the certificate's SHA1 hash.
  *

diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 1ccfc60a5e4efa463089cd92a03095f27c12cb23..f5fe17a9dbc9101fc2b7a23dcc54134cee83a544 100644 (file)
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -48,6 +48,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 */
@@ -64,7 +65,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)
        {
@@ -73,16 +74,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;
@@ -253,12 +256,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)
@@ -272,12 +275,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';
@@ -289,19 +289,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
 

diff --git a/src/openvpn/ssl_verify_polarssl.c b/src/openvpn/ssl_verify_polarssl.c
index 85c44dd11d3c3d03ed1ef4dd0cc7cce124bcc462..e151e87288c62c15c1f91ce57ee97363fa618f95 100644 (file)
--- a/src/openvpn/ssl_verify_polarssl.c
+++ b/src/openvpn/ssl_verify_polarssl.c
@@ -47,6 +47,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);
@@ -62,21 +63,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;
@@ -164,7 +167,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;
@@ -175,19 +178,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: