[Snort-devel] snort-1.9.1: patch for stream4 and frag2

Matthew Callaway matt-snort at ...806...
Tue Apr 29 15:49:14 EDT 2003


Several days ago, I posted a patch to the stream4 preprocessor that
addressed the integer overflow identified by the Core team.  This is a
final revision of that patch that also fixes frag2.

In addition to the application of the isBetween() and SafeMemcpy()
functions, I also check the return code of SafeMemcpy() to see if we
skip the memcpy() due to the integer overflow.  We now log an error
message when we return a corrupted reassembled packet, instead of simply
returning silently.

Chris Green again encourages us to make the move to 2.0, but for those
of us who are, for the moment, stuck with 1.9.1, this should help.

Matt

P.S. You could change ErrorMessage to LogMessage to send the warning to
syslog.

o-------------------------------o
| Matthew Callaway              |
| Project Manager             o-------------------------o
| Firewall and VPN Technology | matt at ...806...     |
| SecurePipe, Inc.            | Tel: 608.294.6940       |
o-----------------------------| Fax: 608.294.6950       |
                              | Web: www.securepipe.com |
                              o-------------------------o


diff -ruN snort-1.9.1.orig/src/bounds.h snort-1.9.1/src/bounds.h
--- snort-1.9.1.orig/src/bounds.h	Wed Dec 31 18:00:00 1969
+++ snort-1.9.1/src/bounds.h	Tue Apr 29 14:06:35 2003
@@ -0,0 +1,127 @@
+#ifndef _BOUNDS_H
+#define _BOUNDS_H
+/*
+** Copyright (C) 2003, Sourcefire, Inc.
+**               Chris Green <cmg at ...402...>
+**
+** This program is free software; you can redistribute it and/or modify
+** it under the terms of the GNU General Public License as published by
+** the Free Software Foundation; either version 2 of the License, or
+** (at your option) any later version.
+**
+** This program is distributed in the hope that it will be useful,
+** but WITHOUT ANY WARRANTY; without even the implied warranty of
+** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+** GNU General Public License for more details.
+**
+** You should have received a copy of the GNU General Public License
+** along with this program; if not, write to the Free Software
+** Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+**
+*/
+
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "snort.h"
+
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <assert.h>
+#include <unistd.h>
+
+/* This INLINE is conflicting with the INLINE defined in bitop.h.
+ * So, let's just add a little sanity check here.
+ */
+#ifndef DEBUG
+    #ifndef INLINE
+        #define INLINE inline
+    #endif
+    #define ERRORRET return 0;
+#else
+    #ifdef INLINE
+        #undef INLINE
+    #endif
+    #define INLINE
+    #define ERRORRET assert(0==1)
+#endif /* DEBUG */
+
+/*
+ * Check to make sure that p is less than or equal to the ptr range
+ * pointers
+ *
+ * 1 means it's in bounds, 0 means it's not
+ */
+static INLINE int inBounds(u_int8_t *start, u_int8_t *end, u_int8_t *p)
+{
+    if(p >= start && p < end)
+    {
+        return 1;
+    }
+
+    return 0;
+}
+
+/**
+ * A Safer Memcpy
+ *
+ * @param dst where to copy to
+ * @param src where to copy from
+ * @param n number of bytes to copy
+ * @param start start of the dest buffer
+ * @param end end of the dst buffer
+ *
+ * @return 0 on failure, 1 on success
+ */
+static INLINE int SafeMemcpy(void *dst, void *src, size_t n, void *start, void *end)
+{
+     if(n < 1)
+     {
+         ERRORRET;
+     }
+
+     if(!inBounds(start,end, dst) || !inBounds(start,end,((u_int8_t*)dst)+n))
+     {
+         ERRORRET;
+     }
+
+     memcpy(dst, src, n);
+     return 1;
+}
+
+/**
+ * A Safer *a = *b
+ *
+ * @param start start of the dst buffer
+ * @param end end of the dst buffer
+ * @param dst the location to write to
+ * @param src the source to read from
+ *
+ * @return 0 on failure, 1 on success
+ */
+static INLINE int SafeWrite(u_int8_t *start, u_int8_t *end, u_int8_t *dst, u_int8_t *src)
+{
+    if(!inBounds(start, end, dst))
+    {
+        ERRORRET;
+    }
+
+    *dst = *src;
+    return 1;
+}
+
+static inline int SafeRead(u_int8_t *start, u_int8_t *end, u_int8_t *src, u_int8_t *read)
+{
+    if(!inBounds(start,end, src))
+    {
+        ERRORRET;
+    }
+
+    *read = *start;
+    return 1;
+}
+
+#endif /* _BOUNDS_H */
diff -ruN snort-1.9.1.orig/src/decode.h snort-1.9.1/src/decode.h
--- snort-1.9.1.orig/src/decode.h	Fri Feb 14 13:32:26 2003
+++ snort-1.9.1/src/decode.h	Tue Apr 29 14:06:35 2003
@@ -165,6 +165,10 @@
 #define UDP_HEADER_LEN          8
 #define ICMP_HEADER_LEN         4

+#ifndef IP_MAXPACKET
+#define IP_MAXPACKET            65535   /* maximum packet size */
+#endif /* IP_MAXPACKET */
+
 #define TH_FIN  0x01
 #define TH_SYN  0x02
 #define TH_RST  0x04
diff -ruN snort-1.9.1.orig/src/preprocessors/spp_frag2.c snort-1.9.1/src/preprocessors/spp_frag2.c
--- snort-1.9.1.orig/src/preprocessors/spp_frag2.c	Wed Aug 21 08:02:01 2002
+++ snort-1.9.1/src/preprocessors/spp_frag2.c	Tue Apr 29 14:06:54 2003
@@ -60,6 +60,7 @@
 #include <ctype.h>
 #include <rpc/types.h>

+#include "bounds.h"
 #include "generators.h"
 #include "log.h"
 #include "detect.h"
@@ -98,6 +99,8 @@
 #define SPARC_TWIDDLE       0
 #endif

+#define DATASIZE (ETHERNET_HEADER_LEN+65536)
+
 /*  D A T A   S T R U C T U R E S  **********************************/
 typedef struct _Frag2Data
 {
@@ -302,7 +305,9 @@

     if((frag->offset + frag->size) < 65516)
     {
-        memcpy(buf+frag->offset, frag->data, frag->size);
+        if (! SafeMemcpy(buf+frag->offset, frag->data, frag->size,
+                   defrag_pkt->pkt, defrag_pkt->pkt + DATASIZE))
+           ErrorMessage("Unsafe memcpy in frag2, reassembled packet corrupted.\n");
         pc.rebuild_element++;
     }
     else
@@ -779,7 +784,9 @@

     newfrag->data = (u_int8_t *) Frag2Alloc(ft, p->pkth->ts.tv_sec, p->dsize);

-    memcpy(newfrag->data, p->data, p->dsize);
+    if (! SafeMemcpy(newfrag->data, p->data, p->dsize,
+               defrag_pkt->pkt, defrag_pkt->pkt + DATASIZE))
+        ErrorMessage("Unsafe memcpy in frag2, reassembled packet corrupted.\n");

     newfrag->offset = p->frag_offset << 3;
     newfrag->size = p->dsize;
diff -ruN snort-1.9.1.orig/src/preprocessors/spp_stream4.c snort-1.9.1/src/preprocessors/spp_stream4.c
--- snort-1.9.1.orig/src/preprocessors/spp_stream4.c	Fri Feb 14 13:32:27 2003
+++ snort-1.9.1/src/preprocessors/spp_stream4.c	Tue Apr 29 14:08:05 2003
@@ -37,6 +37,17 @@
 #include "config.h"
 #endif

+#ifndef DEBUG
+    #ifndef INLINE
+        #define INLINE inline
+    #endif
+#else
+    #ifdef INLINE
+        #undef INLINE
+    #endif
+    #define INLINE���
+#endif /* DEBUG */
+
 #include <sys/types.h>
 #include <stdlib.h>
 #include <string.h>
@@ -65,6 +76,7 @@
 #include "generators.h"
 #include "detect.h"
 #include "perf.h"
+#include "bounds.h"

 #include "ubi_SplayTree.h"

@@ -143,6 +155,8 @@
 #define SPARC_TWIDDLE       0
 #endif

+#define MAX_STREAM_SIZE (IP_MAXPACKET - IP_HEADER_LEN - TCP_HEADER_LEN)
+
 /* random array of flush points */

 #define FCOUNT 64
@@ -325,6 +339,7 @@
 void WriteSsnStats(BinStats *);
 void OpenStatsFile();
 static int RetransTooFast(struct timeval *a, struct timeval *b);
+static INLINE int isBetween(u_int32_t low, u_int32_t high, u_int32_t cur);

 /*
   Here is where we separate which functions will be called in the
@@ -342,6 +357,10 @@



+static INLINE int isBetween(u_int32_t low, u_int32_t high, u_int32_t cur)
+{
+        return (cur - low) <= (high - low);
+}

 static int CompareFunc(ubi_trItemPtr ItemPtr, ubi_trNodePtr NodePtr)
 {
@@ -462,7 +481,7 @@
     /* don't reassemble if we're before the start sequence number or
      * after the last ack'd byte
      */
-    if(spd->seq_num < s->base_seq || spd->seq_num > s->last_ack) {
+    if(!isBetween(s->base_seq, s->last_ack, spd->seq_num)) {
         DEBUG_WRAP(DebugMessage(DEBUG_STREAM,
                                 "not reassembling because"
                                 " we're (%u) before isn(%u) or after last_ack(%u)\n",
@@ -471,8 +490,8 @@
     }

     /* if it's in bounds... */
-    if(spd->seq_num >= s->base_seq && spd->seq_num >= s->next_seq &&
-       (spd->seq_num+spd->payload_size) <= s->last_ack)
+    if(isBetween(s->base_seq, s->last_ack, spd->seq_num) &&
+       isBetween(s->base_seq, s->last_ack, (spd->seq_num+spd->payload_size)))
     {
         offset = spd->seq_num - s->base_seq;

@@ -487,16 +506,17 @@
                                 spd->seq_num, s->last_ack, s->base_seq,
                                 spd->payload_size, s->next_seq, offset));

-        memcpy(buf+offset, spd->payload, spd->payload_size);
+        if(! SafeMemcpy(buf+offset, spd->payload, spd->payload_size,
+                   stream_pkt->data, stream_pkt->data + MAX_STREAM_SIZE))
+           ErrorMessage("Unsafe memcpy in stream4, reassembled stream corrupted.\n");

         pc.rebuilt_segs++;

         spd->chuck = 1;
         bd->total_size += spd->payload_size;
     }
-    else if(spd->seq_num >= s->base_seq &&
-            spd->seq_num < s->last_ack &&
-            spd->seq_num + spd->payload_size > s->last_ack)
+    else if(isBetween(s->base_seq, s->last_ack, spd->seq_num) &&
+            ((spd->seq_num + spd->payload_size) > s->last_ack))
     {
         /*
          *  if it starts in bounds and hasn't been completely ack'd,
@@ -518,7 +538,10 @@
             DEBUG_WRAP(DebugMessage(DEBUG_STREAM, "Copying %d bytes into buffer, "
                                     "offset %d, buf %p\n", trunc_size, offset,
                                     buf););
-            memcpy(buf+offset, spd->payload, trunc_size);
+            if (! SafeMemcpy(buf+offset, spd->payload, trunc_size,
+                       stream_pkt->data, stream_pkt->data + MAX_STREAM_SIZE))
+                ErrorMessage("Unsafe memcpy in stream4, reassembled stream corrupted.\n");
+
             pc.rebuilt_segs++;
             bd->total_size += trunc_size;
         }
@@ -530,8 +553,7 @@

         spd->chuck = 1;
     }
-    else if(spd->seq_num < s->base_seq &&
-            spd->seq_num+spd->payload_size > s->base_seq)
+    else if(isBetween(s->base_seq, s->last_ack, (spd->seq_num+spd->payload_size)))
     {
         /* case where we've got a segment that wasn't completely ack'd
          * last time it was processed, do a partial copy into the buffer
@@ -544,13 +566,16 @@

         /* figure out where in the original data payload to start copying */
         offset = s->base_seq - spd->seq_num;
-
+
         if(trunc_size < 65500 && trunc_size > 0)
         {
             DEBUG_WRAP(DebugMessage(DEBUG_STREAM, "Copying %d bytes into buffer, "
                                     "offset %d, buf %p\n", trunc_size, offset,
                                     buf););
-            memcpy(buf, spd->payload+offset, trunc_size);
+            if (! SafeMemcpy(buf, spd->payload+offset, trunc_size,
+                       stream_pkt->data, stream_pkt->data + MAX_STREAM_SIZE))
+                ErrorMessage("Unsafe memcpy in stream4, reassembled stream corrupted.\n");
+
             pc.rebuilt_segs++;
             bd->total_size += trunc_size;
         }




More information about the Snort-devel mailing list