Re: Missing Sanity Checks in Snort-3.0.0 a2 (Build 163/165)

Thanks Bill, we'll get these cleaned up.

On 8/14/15 12:29 PM, Bill Parker wrote:
> Hello All,
>
> In reviewing source code in Snort-3.0.0a2 (Build 163), I found
> some instances where calls to calloc() and strdup() are
> not checked for a return value of NULL, indicating failure.
>
> =====================================================================
>
> In directory 'snort-3.0.0-a2/src/test', file 'sfrt_test.cc',
> a pair of calls to strdup() are not checked for a return
> value of NULL, the patch file below should address these
> issues:
>
> --- sfrt_test.cc.orig   2015-08-13 18:54:01.343000000 -0700
> +++ sfrt_test.cc        2015-08-13 18:58:42.309000000 -0700
> @@ -104,6 +104,10 @@
>              sfip_pton(ip_entry->ip_str, &ip);
>
>              ip2_str = strdup(ip_entry->ip_str);
> +           if (!ip2_str)
> +           {
> +               printf("Unable to duplicate ip_entry->ip_str\n");
> +           }
>              p = strchr(ip2_str, '/');
>              if (p)
>              {
> @@ -195,6 +199,10 @@
>              sfip_pton(ip_entry->ip_str, &ip);
>
>              ip2_str = strdup(ip_entry->ip_str);
> +           if (!ip2_str)
> +           {
> +               printf("Unable to duplicate ip_entry->ip_str\n");
> +           }
>              p = strchr(ip2_str, '/');
>              if (p)
>              {
>
> =====================================================================
>
> In directory 'snort-3.0.0-a2/tools/u2spewfoo', file 'u2spewfoo.cc',
> there is a call to strdup() which is not checked for a return value
> of NULL.  The patch file below should address this issue:
>
> --- u2spewfoo.cc.orig   2015-08-13 18:47:13.105000000 -0700
> +++ u2spewfoo.cc        2015-08-13 18:49:52.056000000 -0700
> @@ -67,6 +67,13 @@
>
>      ret->file = f;
>      ret->filename = strdup(filename);
> +    if (!ret->filename)
> +    {
> +       printf("new_iterator: Call to strdup() failed.\n");
> +       free(ret);
> +       fclose(f);
> +       return NULL;
> +    }
>      return ret;
>  }
>
> =====================================================================
>
> In directory 'snort-3.0.0-a2/src/service_inspectors/ftp_telnet', file
> 'ftp.cc', there are two calls to calloc() which are not checked for
> a return value of NULL, indicating failure.  The patch file below
> should address these issues:
>
> --- ftp.cc.orig 2015-08-13 19:05:47.698000000 -0700
> +++ ftp.cc      2015-08-13 19:08:41.172000000 -0700
> @@ -316,6 +316,7 @@
>              Fmt = (FTP_PARAM_FMT*)calloc(1, sizeof(FTP_PARAM_FMT));
>              if (Fmt == NULL)
>              {
> +               free(FTPCmd);
>                  ParseAbort("Failed to allocate memory");
>              }
>
> @@ -325,6 +326,8 @@
>              Fmt = (FTP_PARAM_FMT*)calloc(1, sizeof(FTP_PARAM_FMT));
>              if (Fmt == NULL)
>              {
> +               free(FTPCmd->param_format);
> +               free(FTPCmd);
>                  ParseAbort("Failed to allocate memory");
>              }
>
> =====================================================================
>
> In directory 'snort-3.0.0-a2/src/network_inspectors/port_scan',
> file 'ipobj.cc.patch', there is a call to strdup() which is not
> checked for a return value of NULL, indicating failure.  The patch
> file below should address this issue:
>
> Additionally, I had to bump the return values by 1 (since a -1
> was already being returned, so this might need to be checked
> from the calling routine as well)...
>
> --- ipobj.cc.orig       2015-08-13 19:24:38.989000000 -0700
> +++ ipobj.cc    2015-08-13 19:26:47.173000000 -0700
> @@ -235,6 +235,10 @@
>      char* port2;
>
>      port_begin = strdup(portstr);
> +    if (port_begin == NULL)
> +    {
> +       return -1;  /*  call to strdup() failed */
> +    }
>
>      port1 = port_begin;
>      port2 = strstr(port_begin, "-");
> @@ -243,7 +247,7 @@
>          if (*port1 == '\0')
>          {
>              free(port_begin);
> -            return -1;
> +            return -2;
>          }
>
>          if (port2)
> @@ -256,7 +260,7 @@
>          if (port_end == port1)
>          {
>              free(port_begin);
> -            return -2;
> +            return -3;
>          }
>
>          if (port2)
> @@ -265,7 +269,7 @@
>              if (port_end == port2)
>              {
>                  free(port_begin);
> -                return -3;
> +                return -4;
>              }
>          }
>          else
> @@ -277,7 +281,7 @@
>          if ( port_hi > MAXPORTS-1 || port_lo > MAXPORTS-1)
>          {
>              free(port_begin);
> -            return -4;
> +            return -5;
>          }
>
>          /* swap ports if necessary */
>
> =====================================================================
>
> I am attaching the patch files to this bug report...
>
> Bill Parker (wp02855 at gmail dot com)
>
>
>
> ------------------------------------------------------------------------------
>
>
> _______________________________________________
> 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!