Re: Subj: [snort-devel] lack of sanity checks for strdup/strndup() calls in 2.9.7.0-alpha

["Costas Kleopa (ckleopa)" <ckleopa () cisco com>]

Bill,

Thanks for the updates. We will add this in the list of bugs for this feature.

Thanks
Costas


From: Bill Parker <wp02855 () gmail com<mailto:wp02855 () gmail com>>
Date: Thursday, March 20, 2014 at 5:32 PM
To: "snort-devel () lists sourceforge net<mailto:snort-devel () lists sourceforge net>" <snort-devel () lists 
sourceforge net<mailto:snort-devel () lists sourceforge net>>
Subject: [Snort-devel] Subj: [snort-devel] lack of sanity checks for strdup/strndup() calls in 2.9.7.0-alpha

Hello All,

   In reviewing code in snort-2.9.7.0-alpha, I ran into some instances
where calls to strdup()/strndup() were not checked for a return value
of NULL, indicating failure.  The patch file(s) below add the needed
checks and error messages, if appropriate:

Directory '/src/dynamic-preprocessors/appid', file 'appInfoTable.c':

--- appInfoTable.c.orig 2014-03-19 18:03:17.899352333 -0700
+++ appInfoTable.c      2014-03-19 18:05:30.862251234 -0700
@@ -131,6 +131,10 @@
             entry->payloadId = entry->appId;
 #ifdef DEBUG_APP_NAME
             entry->appName = strdup(appName);
+           if (entry->appName == NULL) {
+                _dpd.errMsg("Could not allocate space for entry->appName\n");
+           }
+
 #endif

             AppInfoTableDyn.table[AppInfoTableDyn.usedCount++] = entry;

In file 'fw_appid.c', many calls to strdup() are made, but no sanity checks
are made against the return value (though it might be advisable to perhaps
have a warning message indicating failure)?

In file 'luaDetectorApi.c', I found several instances of strdup() calls
without sanity checks and failure to free previously allocated memory.

This file was previously submitted to correct lack of sanity checks on
calloc() calls, but now has patches for strdup() calls as well:

--- luaDetectorApi.c.orig       2014-03-09 18:35:09.650653721 -0700
+++ luaDetectorApi.c    2014-03-19 18:23:49.369865903 -0700
@@ -106,7 +106,7 @@
 #ifdef LUA_DETECTOR_DEBUG
         _dpd.debugMsg(DEBUG_LOG,"DetectorUserData %p: allocated\n\n",bar);
 #endif
-      bzero(bar, sizeof(*bar));
+      memset(bar, 0, sizeof(*bar)); /* bzero() deprecated, replaced with memset() */

       if ((bar->pDetector = (Detector *)calloc(1, sizeof(Detector))) == NULL)
       {
@@ -2069,6 +2069,11 @@
         return 0;
     }
     hostPattern = (u_int8_t *)strdup(tmpString);
+    if (!hostPattern)
+    {
+       _dpd.errMsg( "Invalue host pattern string.");
+       return 0;
+    }

     /* Verify that path pattern is a valid string */
     size_t pathPatternSize = 0;
@@ -2081,6 +2086,12 @@
         return 0;
     }
     pathPattern = (u_int8_t *)strdup(tmpString);
+    if (!pathPattern)
+    {
+       _dpd.errMsg( "Invalid path pattern string.");
+       free(hostPattern);
+       return 0;
+    }

     /* Verify that scheme pattern is a valid string */
     size_t schemePatternSize;
@@ -2094,6 +2105,13 @@
         return 0;
     }
     schemePattern = (u_int8_t*) strdup(tmpString);
+    if (!schemePattern)
+    {
+       _dpd.errMsg ( "Invalid scheme pattern string,");
+       free(pathPattern);
+       free(hostPattern);
+       return 0;
+    }

     /* Verify that query pattern is a valid string */
     size_t queryPatternSize;
@@ -2102,6 +2120,13 @@
     if(tmpString  && queryPatternSize)
     {
         queryPattern = (u_int8_t*) strdup(tmpString);
+       if (!queryPattern)
+       {
+           _dpd.errMsg( "Invalid query pattern string.");
+           free(pathPattern);
+           free(hostPattern);
+           return 0;
+       }
     }

     u_int32_t appId           = lua_tointeger(L, index++);
@@ -2452,6 +2477,11 @@
         return 0;
     }
     hostPattern = (u_int8_t *)strdup(tmpString);
+    if (!hostPattern)
+    {
+       _dpd.errMsg( "Inavlue host pattern string.");
+       return 0;
+    }

     /* Verify that path pattern is a valid string */
     size_t pathPatternSize = 0;
@@ -2464,6 +2494,12 @@
         return 0;
     }
     pathPattern = (u_int8_t *)strdup(tmpString);
+    if (!pathPattern)
+    {
+       _dpd.errMsg( "Inavlid path pattern string.");
+       free(hostPattern);
+       return 0;
+    }

     /* Verify that scheme pattern is a valid string */
     size_t schemePatternSize;
@@ -2477,6 +2513,13 @@
         return 0;
     }
     schemePattern = (u_int8_t*) strdup(tmpString);
+    if (!schemePattern)
+    {
+       _dpd.errMsg ("Invalid scheme pattern string.");
+       free(pathPattern);
+       free(hostPattern);
+       return 0;
+    }

     /* Allocate memory for data structures */
     DetectorAppUrlPattern *pattern = malloc(sizeof(DetectorAppUrlPattern));

In file 'luaDetectorModule.c', many calls to strdup() are made, but no
sanity checks are made against the return value (though it might be
advisable to perhaps have a warning message indicating failure)?

In directory 'src/dynamic_preprocessors/appid/client_plugins',
file 'client_app_sip.c', I found some instances of strdup() and
strndup() which lack sanity checking for a return value of NULL
indicating failure.  The patch file below adds the checks for
calls to strdup() and calls free() in the event of failure:

--- client_app_sip.c.orig       2014-03-19 19:22:07.219424595 -0700
+++ client_app_sip.c    2014-03-19 19:27:32.900817025 -0700
@@ -467,7 +467,17 @@

     pattern->userData.clientAppId = clientAppId;
     pattern->userData.clientVersion = strdup(clientVersion);
+    if (!pattern->userData.clientVersion)
+    {
+       free(pattern);
+       return 0;
+    }
     pattern->pattern.pattern = (uint8_t *)strdup(serverPattern);
+    if (!pattern->pattern.pattern)
+    {
+       free(pattern);
+       return 0;
+    }
     pattern->pattern.patternSize = (int) strlen(serverPattern);

     pattern->next = *patternList;

Calls are also made to strndup(), but it would appear that a
return value of NULL isn't fatal in these instances (perhaps
a warning should be logged?).

In directory 'src/dynamic-preprocessors/appid/client_plugins',
file 'client_app_base.c', I found calls to strdup() which
fail to check for a return value of NULL, indicating failure.

The patch file below adds a error/warning message to the strdup()
calls:

--- client_app_base.c.orig      2014-03-19 19:33:19.813907895 -0700
+++ client_app_base.c   2014-03-19 19:35:35.703178723 -0700
@@ -787,6 +787,12 @@
             flowp->clientVersion = strdup(version);
     }

+    if (!flowp->clientVersion)
+    {
+       _dpd.fatalMsg( "Failed to add a flowp client version value");
+       return;
+    }
+
     flowp->clientServiceAppId = service_id;
     flowp->clientAppId = id;
 }

A 'make' of the above patch files results in a clean compile
for snort-2.9.7.0-alpha.

I am attaching the patch files to this email.

Bill Parker (wp02855 at gmail dot com)

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech

_______________________________________________
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!