[mod_gnutls-devel] msva validation error handling

[Daniel Kahn Gillmor]

Hi Ramkumar--

On Wed 2014-03-12 00:35:26 -0400, Ramkumar Chinchani wrote:
> In mgs_cert_verify() when using the msva validation path, there is a free()
> of unallocated memory when the connection to the msva validation agent
> fails.

Thanks for noticing this!

> Kindly be advised of a possible patch [1] to libmsv [2]
>
> [1] https://gitorious.org/libmsv/libmsv/commit/0915099e639fee8f90737ecb217931f07b4e6f7c
> [2] http://anonscm.debian.org/gitweb/?p=users/clint/libmsv.git

It's easier to comment on a  patch when it's in the e-mail, so i'm including it here:

-----------------
--- a/msv.c
+++ b/msv.c
@@ -231,7 +231,7 @@
   pkc = json_object ();
   peer = json_object ();
 
-  response = malloc (sizeof (struct msv_response));
+  response = calloc (1, sizeof (struct msv_response));
   *response_ptr = response;
 
   if ((j_pkcdata = json_string (q.pkcdata)) &&

--------------

The concern here is that we want to ensure the message member of the
response object is NULL, right?

i agree that the fix belongs in libmsv.  i also note that libmsv doesn't
gracefully handle an out-of-memory condition here either, though.  the
following patch to libmsv should handle these cases more robustly.

Clint, what do you think?

       --dkg

commit 425528fb2324ed3bbf105d58e029d9fe35d497ae
Author: Daniel Kahn Gillmor <dkg at fifthhorseman.net>
Date:   Wed Mar 12 15:00:41 2014 -0400

    handle out of memory errors more gracefully

diff --git a/msv.c b/msv.c
index bea92f4..858a31c 100644
--- a/msv.c
+++ b/msv.c
@@ -231,9 +231,6 @@ msv_query_agent (msv_ctxt_t ctx, struct msv_query q,
   pkc = json_object ();
   peer = json_object ();
 
-  response = malloc (sizeof (struct msv_response));
-  *response_ptr = response;
-
   if ((j_pkcdata = json_string (q.pkcdata)) &&
       (j_pkctype = json_string (q.pkctype)) &&
       (j_peername = json_string (q.peername)) &&
@@ -346,6 +343,15 @@ msv_query_agent (msv_ctxt_t ctx, struct msv_query q,
       return LIBMSV_ERROR_CURLCODE;
     }
 
+  if (! (response = malloc (sizeof (struct msv_response)))) {
+    curl_easy_cleanup (c);
+    free (req);
+    curl_slist_free_all (slist);
+    return LIBMSV_ERROR_NOMEM;
+  }
+  response->message = NULL;
+  *response_ptr = response;
+
   j = json_loads (buf, 0, &je);
   free (buf);
 
@@ -366,6 +372,14 @@ msv_query_agent (msv_ctxt_t ctx, struct msv_query q,
   curl_easy_cleanup (c);
   free (req);
 
+  if (!response->message) {
+    /* strdup returned NULL, which suggests a memory allocation
+       failure */
+    msv_response_destroy(response);
+    *response_ptr = NULL;
+    return LIBMSV_ERROR_NOMEM;
+  }
+
   return !(response->valid);
 }
 
diff --git a/msv/msv.h b/msv/msv.h
index 2080043..622037d 100644
--- a/msv/msv.h
+++ b/msv/msv.h
@@ -51,6 +51,12 @@
 #define LIBMSV_ERROR_UNEXPECTED_RESPONSE 7
 
 /**
+ * @def LIBMSV_ERROR_NOMEM
+ * @brief necessary memory allocation failed
+ */
+#define LIBMSV_ERROR_NOMEM 8
+
+/**
  * @typedef msv_ctxt_t
  * @brief opaque MSVA context
  */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 948 bytes
Desc: not available
URL: </pipermail/attachments/20140312/6415dd96/attachment.sig>