Snort mailing list archives
Subj: [snort-devel] lack of sanity checks for strdup/strndup() calls in 2.9.7.0-alpha
From: Bill Parker <wp02855 () gmail com>
Date: Thu, 20 Mar 2014 14:32:10 -0700
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)
Attachment:
appInfoTable.c.patch
Description:
Attachment:
client_app_base.c.patch
Description:
Attachment:
luaDetectorApi.c.patch
Description:
Attachment:
client_app_sip.c.patch
Description:
------------------------------------------------------------------------------ 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!
Current thread:
- Subj: [snort-devel] lack of sanity checks for strdup/strndup() calls in 2.9.7.0-alpha Bill Parker (Mar 20)
- Re: Subj: [snort-devel] lack of sanity checks for strdup/strndup() calls in 2.9.7.0-alpha Costas Kleopa (ckleopa) (Mar 21)