Re: Lack of Sanity Checking for calls to strndup() in Snort-2.9.7.x/2.9.8 Beta

Thanks for the suggestions. We will apply them on our next release. 

Thanks,
Costas

> On Aug 17, 2015, at 12:57 PM, Bill Parker <[email protected]> wrote:
> 
> Hello All,
> 
>     In doing some code review, I found some instances where calls to
> strndup() are not checked for a return value of NULL, indicating
> failure (memory allocation is the same for strndup() as it is for
> strdup(), btw).
> 
> =======================================================================
> 
> In directory 'src/preprocessors/HttpInspect/server', file 'hi_server.c'
> there is a call to strndup() at approximately line 675 which is missing
> a test for a return value of NULL.  The patch file below should address
> this issue:
> 
> --- hi_server.c.orig    2015-08-16 19:20:53.381000000 -0700
> +++ hi_server.c 2015-08-16 19:28:01.482000000 -0700
> @@ -673,6 +673,10 @@
>          {
>              headerLoc->len =  (end_ptr - cur_ptr);
>              headerLoc->start = (u_char *)strndup((const char *)cur_ptr, headerLoc->len);
> +           if (headerLoc->start == NULL) /* oops, strndup failed   */
> +           {   /* should we warn user that strndup() has failed?   */
> +               return NULL;
> +           }
>          }
>      }
>      else
> 
> =======================================================================
> 
> In directory 'src/dynamic-preprocessors/appid', file 'fw_appid.c', there
> are instances of strndup() being called at lines: 892, 911, 917, 923,
> 933, and 943 which are not checked for a return value of NULL, which
> would indicate failure.  The patch file below should address these
> issues:
> 
> --- fw_appid.c.orig     2015-08-16 19:42:25.817000000 -0700
> +++ fw_appid.c  2015-08-16 19:42:28.749000000 -0700
> @@ -890,6 +890,11 @@
>          {
>              free(session->host);
>              session->host = strndup((char *)headers->host.start, headers->host.len);
> +           if (session->host == NULL)
> +           {
> +               _dpd.errMsg("failed to allocate memory for session->host");
> +               return;
> +           }
>              session->scan_flags |= SCAN_HTTP_HOST_URL_FLAG;
> 
>              if (headers->url.start)
> @@ -909,18 +914,33 @@
>          {
>              free(session->useragent);
>              session->useragent  = strndup((char *)headers->userAgent.start, headers->userAgent.len);
> +           if (session->useragent == NULL)
> +           {
> +               _dpd.errMsg("failed to allocate memory for session user-agent");
> +               return;
> +           }
>              session->scan_flags |= SCAN_HTTP_USER_AGENT_FLAG;
>          }
>          if (headers->referer.start)
>          {
>              free(session->referer);
>              session->referer  = strndup((char *)headers->referer.start, headers->referer.len);
> +           if (session->referer == NULL)
> +           {
> +               _dpd.errMsg("failed to allocate memory for session referer");
> +               return;
> +           }
> 
>          }
>          if (headers->via.start)
>          {
>              free(session->via);
>              session->via  = strndup((char *)headers->via.start, headers->via.len);
> +           if (session->via == NULL)
> +           {
> +               _dpd.errMsg("failed to allocate memory for session via");
> +               return;
> +           }
>              session->scan_flags |= SCAN_HTTP_VIA_FLAG;
>          }
> 
> @@ -931,6 +951,11 @@
>          {
>              free(session->via);
>              session->via  = strndup((char *)headers->via.start, headers->via.len);
> +           if (session->via == NULL)
> +           {
> +               _dpd.errMsg("failed to allocate memory for session via");
> +               return;
> +           }
>              session->scan_flags |= SCAN_HTTP_VIA_FLAG;
>          }
>          if (headers->responseCode.start)
> @@ -941,6 +966,11 @@
>              {
>                  free(session->response_code);
>                  session->response_code  = strndup((char *)headers->responseCode.start, headers->responseCode.len);
> +               if (session->response_code == NULL)
> +               {
> +                   _dpd.errMsg("failed to allocate memory for session response code");
> +                   return;
> +               }
>              }
>          }
>      }
>      
> =======================================================================
> 
> I am attaching the patch files to this bug report.
> 
> Questions, comments, suggestions, complaints? :)
> 
> Bill Parker (wp02855 at gmail dot com)
> <hi_server.c.patch>
> <fw_appid.c.patch>
> ------------------------------------------------------------------------------
> _______________________________________________
> Snort-devel mailing list
> [email protected]
> 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!

------------------------------------------------------------------------------
_______________________________________________
Snort-devel mailing list
[email protected]
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!