[Snort-devel] Patch for snort-1.9.1: CORE-2003-0307: StreamReassembly Overflow

Matthew Callaway matt-snort at ...806...
Wed Apr 16 10:24:08 EDT 2003


Below is a re-posting of Dave Greenstein's patch to snort-1.9.1 that
backports snort-2.0.0 stream4 behavior with respect to the afor
mentioned integer overflow.  It's just in a different format (diff -ruN)
and fixes one typo (a missing "else if").  This patch applies to
snort-1.9.1, builds, and has been confirmed to fix the vulnerability
seen with Core's hping2 example.

Please note however that it does not address frag2, it does not replace
all occurrences of memcpy with SafeMemcpy, and we have not done any
further tests to ensure that the rest of snort's behavior remains
unchanged.

I post this only to solicit further opinion and allow the community to
test further.

Matt

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	Wed Apr 16 11:12:43 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	Wed Apr 16 10:37:58 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_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	Wed Apr 16 11:18:51 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,16 @@
                                 spd->seq_num, s->last_ack, s->base_seq,
                                 spd->payload_size, s->next_seq, offset));

-        memcpy(buf+offset, spd->payload, spd->payload_size);
+        SafeMemcpy(buf+offset, spd->payload, spd->payload_size,
+                   stream_pkt->data, stream_pkt->data + MAX_STREAM_SIZE);

         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 +537,8 @@
             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);
+            SafeMemcpy(buf+offset, spd->payload, trunc_size,
+                       stream_pkt->data, stream_pkt->data + MAX_STREAM_SIZE);
             pc.rebuilt_segs++;
             bd->total_size += trunc_size;
         }
@@ -530,8 +550,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
@@ -550,7 +569,8 @@
             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);
+            SafeMemcpy(buf, spd->payload+offset, trunc_size,
+                       stream_pkt->data, stream_pkt->data + MAX_STREAM_SIZE);
             pc.rebuilt_segs++;
             bd->total_size += trunc_size;
         }




More information about the Snort-devel mailing list