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

Costas Kleopa (ckleopa) ckleopa at ...5...
Fri Mar 21 09:33:36 EDT 2014


Bill,

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

Thanks
Costas


From: Bill Parker <wp02855 at ...8...<mailto:wp02855 at ...8...>>
Date: Thursday, March 20, 2014 at 5:32 PM
To: "snort-devel at lists.sourceforge.net<mailto:snort-devel at ...10....net>" <snort-devel at lists.sourceforge.net<mailto:snort-devel at ...12...rge.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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.snort.org/pipermail/snort-openappid/attachments/20140321/d817650d/attachment.html>


More information about the Snort-openappid mailing list