Lack of sanity checks and possible memory leak in 2.9.7.x

[Bill Parker <wp02855 () gmail com>]

Hello All,

    In reviewing code in directory 'src/detection-plugins', file
file 'sp_byte_jump.c', I found a pair of calls to strdup() which
are not tested for a return value of NULL, indicating failure.
The patch file below corrects this issue:

--- sp_byte_jump.c.orig 2015-04-11 16:00:38.317385324 -0700
+++ sp_byte_jump.c      2015-04-11 16:03:23.829421053 -0700
@@ -201,7 +201,13 @@
     ByteJumpOverrideData *new = SnortAlloc(sizeof(ByteJumpOverrideData));

     new->keyword = strdup(keyword);
+    if (new->keyword == NULL) /* should we throw fatal error here or just
return? */
+       return;
     new->option = strdup(option);
+    if (new->option == NULL) { /* should we throw fatal error here or just
return? */
+       free(new->keyword);
+       return;
+    }
     new->func = roo_func;

     new->next = byteJumpOverrideFuncs;

In directory 'src/dynamic-plugins', file 'sf_convert_dynamic.c', I found
a call to 'strdup()' which is not checked for a return value of NULL,
indicating failure.  The patch file below corrects this issue:

--- sf_convert_dynamic.c.orig   2015-04-11 16:17:17.987438247 -0700
+++ sf_convert_dynamic.c        2015-04-11 16:17:25.803431620 -0700
@@ -877,6 +877,9 @@
     snort_byte->offset = so_byte->offset;
     snort_byte->align = so_byte->align;
     snort_byte->name = strdup(so_byte->refId);
+    if (snort_byte->name == NULL) { /* couldn't allocate memory, now what?
*/
+       return -1;                  /* what should be returned here? */
+    }

     /* In an SO rule, setting multiplier to 0 means that the multiplier is
        ignored. This is not the case in the text rule version of
byte_extract. */

In directory 'src/dynamic-preprocessors/appid/service_plugins', file
'service_rpc.c', there is a call to strdup(), which upon failure, does
not free the previously allocated memory assigned to variable 'prog'
above it.  The patch file below corrects this issue:

--- service_rpc.c.orig  2015-04-11 16:55:21.199622775 -0700
+++ service_rpc.c       2015-04-11 16:56:47.197552670 -0700
@@ -241,8 +241,10 @@
                     prog->next = rpc_programs;
                     rpc_programs = prog;
                     prog->name = strdup(rpc->r_name);
-                    if (!prog->name)
+                    if (!prog->name) {
+                       free(prog); /* free previously alloc'd memory? */
                         _dpd.errMsg("failed to allocate rpc program name");
+                   }
                 }
             }
         }

In directory 'src/dynamic-preprocessors/appid/service_plugins', file
'service_tftp.c', I found an instance where a call to calloc() is
made for variable 'tmp_td', but upon failure does not release the
memory allocated to variable 'td' in function
'MakeRNAServiceValidationPrototype(tftp_validate)', should this memory
not be released if return SERVICE_ENOMEM is called from the function?

Patch file is below:

--- service_tftp.c.orig 2015-04-11 17:08:32.411181696 -0700
+++ service_tftp.c      2015-04-11 17:11:32.209033078 -0700
@@ -194,7 +194,11 @@
             goto bail;

         tmp_td = calloc(1, sizeof(ServiceTFTPData));
-        if (tmp_td == NULL) return SERVICE_ENOMEM;
+        if (tmp_td == NULL)
+       {
+           free(td);
+           return SERVICE_ENOMEM;
+       }
         tmp_td->state = TFTP_STATE_TRANSFER;

         dip = GET_DST_IP(pkt);

In directory 'src/dynamic-preprocessors/ftptelnet', file 'snort_ftptelnet.c'
there is a call to strdup() which is not checked for NULL, indicating
failure.  The patch file is below:

--- snort_ftptelnet.c.orig      2015-04-11 17:27:55.717031887 -0700
+++ snort_ftptelnet.c   2015-04-11 17:29:01.636409901 -0700
@@ -3394,6 +3394,11 @@
         ftp_conf = GlobalConf->default_ftp_server;
         ConfigParseResumePtr = server+strlen(server);
         GlobalConf->default_ftp_server->serverAddr = strdup("default");
+       if (GlobalConf->default_ftp_server->serverAddr == NULL)
+       {
+           DynamicPreprocessorFatalMessage("Out of memory trying to create
"
+                   "default ftp server configuration.\n");
+       }
         iRet = parseFtpServerConfigStr( ftp_conf, ConfigParseResumePtr,
ip_list, ErrorString, ErrStrLen );
         if (iRet)
         {

In directory 'src/dynamic-preprocessors/ssl_common', file 'ssl_ha.c',
there are some instances of calls to calloc() without a check for
a return value of NULL, indicating failure.  The patch file which
corrects this issue is below:

--- ssl_ha.c.orig       2015-04-11 18:17:32.320342862 -0700
+++ ssl_ha.c    2015-04-11 18:21:07.955749013 -0700
@@ -184,6 +184,10 @@
         n_ssl_ha_funcs++;

     node = (SSLHAFuncsNode *)calloc(1, sizeof(SSLHAFuncsNode));
+    if (node == NULL)
+    {
+       DynamicPreprocessorFatalMessage("Failed to allocate storage for SSL
HA functions.\n");
+    }
     node->id = idx;
     node->mask = (1 << idx);
     node->preproc_id = (uint8_t) preproc_id;
@@ -408,6 +412,10 @@
     }

     pDefaultPolicyConfig->ssl_ha_config = (SSLHAConfig*)calloc(1, sizeof(
SSLHAConfig ));
+    if (pDefaultPolicyConfig->ssl_ha_config == NULL)
+    {
+       DynamicPreprocessorFatalMessage("Failed to allocate storage for SSL
HA configuration.\n");
+    }

     SSLParseHAArgs(sc, pDefaultPolicyConfig->ssl_ha_config, args);

in directory 'src', file 'log.c', it would appear that the call to
calloc() is commented out, which would make the following code appear
to do nothing (unless data_dump_buffer is declared and initialized
elsewhere, which does not appear to be the case):

    /* allocate a buffer to print the data to */
    //data_dump_buffer = (char *) calloc(data_len + (data_len >> 6) + 2,
sizeof(char));
    if (data_dump_buffer == NULL)
    {
        AllocDumpBuf();
    }

I am attaching the patch file(s) to this bug report...

Bill Parker (wp02855 at gmail dot com)

Attachment: ssl_ha.c.patch
Description:

Attachment: service_rpc.c.patch
Description:

Attachment: service_tftp.c.patch
Description:

Attachment: sf_convert_dynamic.c.patch
Description:

Attachment: snort_ftptelnet.c.patch
Description:

Attachment: sp_byte_jump.c.patch
Description:

------------------------------------------------------------------------------
BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT
Develop your own process in accordance with the BPMN 2 standard
Learn Process modeling best practices with Bonita BPM through live exercises
http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_
source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF

_______________________________________________
Snort-devel mailing list
Snort-devel () lists sourceforge net
https://lists.sourceforge.net/lists/listinfo/snort-devel
Archive:
http://sourceforge.net/mailarchive/forum.php?forum_name=snort-devel

Please visit http://blog.snort.org for the latest news about Snort!