[Snort-devel] patch for sp_pattern_match.c::ParsePattern (BAD JUJU)

Martin Roesch roesch at ...402...
Sun Sep 28 18:18:04 EDT 2003


Thanks for the patch, I'll check it out.

It should be noted that we make no guarantees for the functionality of  
the rule parsing subsystem or a lot of the non-critical startup code.   
In a better world I would have been smart in the first place and just  
done some lex/yacc foo to eliminate about 20k lines of code in Snort,  
but that's a fight for another day...

      -Marty


On Friday, September 26, 2003, at 07:46  PM, Pavel Zeldin wrote:

> There is piece of code in ParsePattern() marked "BAD JUJU" which has a
> potential buffer overflow while parsing rules.
> Here is a patch to avoid buffer overflow. It does not improve parsing
> code in any other way, therefore "BAD JUJU" is still bad :-)
>
> The patch is against snort-2.0.2
> /* $Id: sp_pattern_match.c,v 1.56 2003/07/28 17:35:22 chris_reid Exp $  
> */
>
>
> Pavel.
> --- sp_pattern_match.c.orig	Fri Sep 26 19:35:19 2003
> +++ sp_pattern_match.c	Fri Sep 26 19:35:09 2003
> @@ -838,6 +838,50 @@
>
>
>   
> / 
> *********************************************************************** 
> *****
> + * Helper for ParsePattern()
> + * ByteBuffer set of functions is tightly integrated with  
> ParsePattern(),
> + * so it does no error checking (other than putting too much data),  
> and
> + * even swears in ParsePattern() terms.
> +  
> *********************************************************************** 
> ****/
> +typedef struct _ByteBuffer
> +{
> +  u_int8_t *buf_start;       /*< start of the buffer */
> +  u_int8_t *buf_current_pos; /*< where next insertion goes */
> +  u_int8_t *buf_end;         /*< one past last byte of the buffer */
> +} ByteBuffer;
> +
> +
> +static ByteBuffer ByteBufferInit(u_int8_t *store, unsigned long  
> buf_size)
> +{
> +  ByteBuffer buf;
> +  buf.buf_start = buf.buf_end = buf.buf_current_pos = store;
> +  buf.buf_end += buf_size;
> +  memset(store, 0, buf_size);
> +  return buf;
> +}
> +
> +
> +static void ByteBufferAppend(ByteBuffer *buf, u_int8_t octet)
> +{
> +  if(buf->buf_current_pos < buf->buf_end) {
> +    *buf->buf_current_pos++ = octet;
> +  }
> +  else
> +  {
> +    FatalError("ParsePattern() dummy "
> +               "buffer overflow, make a smaller "
> +               "pattern please! (Max size = 2048)\n");
> +  };
> +}
> +
> +
> +static unsigned long ByteBufferSize(const ByteBuffer *buf)
> +{
> +  return (unsigned long)(buf->buf_current_pos - buf->buf_start);
> +}
> +
> +
> +/ 
> *********************************************************************** 
> *****
>   *
>   * Function: ParsePattern(char *)
>   *
> @@ -852,16 +896,13 @@
>  static void ParsePattern(char *rule, OptTreeNode * otn, int type)
>  {
>      unsigned char tmp_buf[2048];
> -
> +    ByteBuffer byte_buf = ByteBufferInit(tmp_buf, sizeof(tmp_buf));
> +
>      /* got enough ptrs for you? */
>      char *start_ptr;
>      char *end_ptr;
>      char *idx;
> -    char *dummy_idx;
> -    char *dummy_end;
>      char hex_buf[3];
> -    u_int dummy_size = 0;
> -    int size;
>      int hexmode = 0;
>      int hexsize = 0;
>      int pending = 0;
> @@ -870,9 +911,6 @@
>      int exception_flag = 0;
>      PatternMatchData *ds_idx;
>
> -    /* clear out the temp buffer */
> -    bzero(tmp_buf, 2048);
> -
>      if(rule == NULL)
>      {
>          FatalError("%s(%d) => ParsePattern Got Null "
> @@ -913,22 +951,9 @@
>      /* Move the null termination up a bit more */
>      *end_ptr = '\0';
>
> -    /* how big is it?? */
> -    size = end_ptr - start_ptr;
> -
> -    /* uh, this shouldn't happen */
> -    if(size <= 0)
> -    {
> -        FatalError("%s(%d) => Bad pattern length!\n",
> -                   file_name, file_line);
> -    }
>      /* set all the pointers to the appropriate places... */
>      idx = start_ptr;
>
> -    /* set the indexes into the temp buffer */
> -    dummy_idx = tmp_buf;
> -    dummy_end = (dummy_idx + size);
> -
>      /* why is this buffer so small? */
>      bzero(hex_buf, 3);
>      memset(hex_buf, '0', 2);
> @@ -963,8 +988,7 @@
>                  {
>                      DEBUG_WRAP(DebugMessage(DEBUG_PARSER, "literal  
> set, Clearing\n"););
>                      literal = 0;
> -                    tmp_buf[dummy_size] = start_ptr[cnt];
> -                    dummy_size++;
> +                    ByteBufferAppend(&byte_buf, start_ptr[cnt]);
>                  }
>
>                  break;
> @@ -981,9 +1005,8 @@
>                  else
>                  {
>                      DEBUG_WRAP(DebugMessage(DEBUG_PATTERN_MATCH,  
> "Clearing literal\n"););
> -                    tmp_buf[dummy_size] = start_ptr[cnt];
> +                    ByteBufferAppend(&byte_buf, start_ptr[cnt]);
>                      literal = 0;
> -                    dummy_size++;
>                  }
>
>                  break;
> @@ -1010,21 +1033,10 @@
>                              hex_buf[1] = *idx;
>                              pending--;
>
> -                            if(dummy_idx < dummy_end)
> -                            {
> -                                tmp_buf[dummy_size] = (u_char)
> -                                    strtol(hex_buf, (char **) NULL,  
> 16)&0xFF;
> -
> -                                dummy_size++;
> -                                bzero(hex_buf, 3);
> -                                memset(hex_buf, '0', 2);
> -                            }
> -                            else
> -                            {
> -                                FatalError("ParsePattern() dummy "
> -                                        "buffer overflow, make a  
> smaller "
> -                                        "pattern please! (Max size =  
> 2048)\n");
> -                            }
> +                            ByteBufferAppend(&byte_buf, (u_char)
> +                                    strtol(hex_buf, (char **) NULL,  
> 16)&0xFF);
> +                            bzero(hex_buf, 3);
> +                            memset(hex_buf, '0', 2);
>                          }
>                      }
>                      else
> @@ -1044,17 +1056,8 @@
>                  {
>                      if(*idx >= 0x1F && *idx <= 0x7e)
>                      {
> -                        if(dummy_idx < dummy_end)
> -                        {
> -                            tmp_buf[dummy_size] = start_ptr[cnt];
> -                            dummy_size++;
> -                        }
> -                        else
> -                        {
> -                            FatalError("%s(%d)=> ParsePattern() "
> -                                    "dummy buffer overflow!\n",  
> file_name, file_line);
> -                        }
> -
> +                        ByteBufferAppend(&byte_buf, start_ptr[cnt]);
> +
>                          if(literal)
>                          {
>                              literal = 0;
> @@ -1064,8 +1067,7 @@
>                      {
>                          if(literal)
>                          {
> -                            tmp_buf[dummy_size] = start_ptr[cnt];
> -                            dummy_size++;
> +                            ByteBufferAppend(&byte_buf,  
> start_ptr[cnt]);
>                              DEBUG_WRAP(DebugMessage(DEBUG_PARSER,  
> "Clearing literal\n"););
>                              literal = 0;
>                          }
> @@ -1081,7 +1083,6 @@
>                  break;
>          }
>
> -        dummy_idx++;
>          idx++;
>          cnt++;
>      }
> @@ -1103,16 +1104,16 @@
>      while(ds_idx->next != NULL)
>          ds_idx = ds_idx->next;
>
> -    if((ds_idx->pattern_buf = (char *) calloc(dummy_size+1,  
> sizeof(char)))
> -       == NULL)
> +    ds_idx->pattern_size = ByteBufferSize(&byte_buf);
> +    ds_idx->search = uniSearch;
> +
> +    if((ds_idx->pattern_buf = (char *) calloc(ds_idx->pattern_size+1,
> +                                              sizeof(char))) == NULL)
>      {
>          FatalError("ParsePattern() pattern_buf malloc failed!\n");
>      }
>
> -    memcpy(ds_idx->pattern_buf, tmp_buf, dummy_size);
> -
> -    ds_idx->pattern_size = dummy_size;
> -    ds_idx->search = uniSearch;
> +    memcpy(ds_idx->pattern_buf, tmp_buf, ds_idx->pattern_size);
>
>      make_precomp(ds_idx);
>      ds_idx->exception_flag = exception_flag;





More information about the Snort-devel mailing list