[Snort-devel] RE: Snort 1.9.1 fix for stream4 reassembly integer overflow

Dave Greenstein dave at ...1918...
Wed Apr 16 06:00:16 EDT 2003


Here is the full diff of the 1.9.1 code base with the 2.0.0 back-port if
anyone else wants to verify the patch. I'm still concerned about the
differences in logic between the 2.0.0 RC1 TraverseFunc and the original
1.9.1 TraverseFunc.

Dave

#diff ./src ../origsnort/snort-1.9.1/src
diff ./src/decode.h ../origsnort/snort-1.9.1/src/decode.h
168,171d167
< #ifndef IP_MAXPACKET
< #define       IP_MAXPACKET    65535           /* maximum packet size
*/
< #endif /* IP_MAXPACKET */
< 
O

Only in ./src: bounds.h

#diff ./src/preprocessors/ ../origsnort/snort-1.9.1/src/preprocessors/
Only in ./src/preprocessors/: Makefile
diff ./src/preprocessors/spp_stream4.c
../origsnort/snort-1.9.1/src/preprocessors/spp_stream4.c
40,50d39
< #ifndef DEBUG
<     #ifndef INLINE
<         #define INLINE inline
<     #endif
< #else
<     #ifdef INLINE
<         #undef INLINE
<     #endif
<     #define INLINE   
< #endif /* DEBUG */
< 
67d55
< #include "bounds.h"
90,92d77
< 
< static INLINE int isBetween(u_int32_t low, u_int32_t high, u_int32_t
cur);
< 
145,146d129
< #define MAX_STREAM_SIZE (IP_MAXPACKET - IP_HEADER_LEN -
TCP_HEADER_LEN)
<                                        
362,366d344
< static INLINE int isBetween(u_int32_t low, u_int32_t high, u_int32_t
cur)
< {
<     return (cur - low) <= (high - low);
< }
< 
487,488c465
<     if(!isBetween(s->base_seq, s->last_ack, spd->seq_num)) {
<         //    if(spd->seq_num < s->base_seq || spd->seq_num >
s->last_ack) {
---
>     if(spd->seq_num < s->base_seq || spd->seq_num > s->last_ack) {
497,500c474,475
<     if(isBetween(s->base_seq, s->last_ack, spd->seq_num) &&
<        isBetween(s->base_seq, s->last_ack,
(spd->seq_num+spd->payload_size)))
<         //    if(spd->seq_num >= s->base_seq && spd->seq_num >=
s->next_seq &&
<         //       (spd->seq_num+spd->payload_size) <= s->last_ack)
<            // NOT THE SAME: old comparisons have s->next_seq---
<
>     if(spd->seq_num >= s->base_seq && spd->seq_num >= s->next_seq &&
>        (spd->seq_num+spd->payload_size) <= s->last_ack)
515,519c490
< 
<         SafeMemcpy(buf+offset, spd->payload, spd->payload_size,
<                    stream_pkt->data, stream_pkt->data +
MAX_STREAM_SIZE);
< 
<               //        memcpy(buf+offset, spd->payload,
spd->payload_size);
---
>         memcpy(buf+offset, spd->payload, spd->payload_size);
526,530c497,499
<     else if(isBetween(s->base_seq, s->last_ack, spd->seq_num) &&
<             ((spd->seq_num + spd->payload_size) > s->last_ack))
<         //    else if(spd->seq_num >= s->base_seq && 
<         //            spd->seq_num < s->last_ack &&
<         //            spd->seq_num + spd->payload_size > s->last_ack)
>            // NOT THE SAME: OLD upper bound exclusive,
>            //               NEW upper bound inclusive
---
>     else if(spd->seq_num >= s->base_seq && 
>             spd->seq_num < s->last_ack &&
>             spd->seq_num + spd->payload_size > s->last_ack)
552,554c521
<             SafeMemcpy(buf+offset, spd->payload, trunc_size,
<                        stream_pkt->data, stream_pkt->data +
MAX_STREAM_SIZE);            
<                       //            memcpy(buf+offset, spd->payload,
trunc_size);
---
>             memcpy(buf+offset, spd->payload, trunc_size);
567,569c534
<             isBetween(s->base_seq, s->last_ack,
(spd->seq_num+spd->payload_size)))
<         //    else if(spd->seq_num < s->base_seq && 
<         //            spd->seq_num+spd->payload_size > s->base_seq)
          // NOT THE SAME: else if(spd->seq_num < s->base_seq &&
s_base_seq < spd->seq_num+spd->payload_size)
---
>             spd->seq_num+spd->payload_size > s->base_seq)
588,590c553
<             SafeMemcpy(buf, spd->payload+offset, trunc_size,
<                        stream_pkt->data, stream_pkt->data +
MAX_STREAM_SIZE);            
<                       //            memcpy(buf, spd->payload+offset,
trunc_size);
---
>             memcpy(buf, spd->payload+offset, trunc_size);


-----Original Message-----
From: Dirk Geschke [mailto:Dirk_Geschke at ...802...] 
Sent: Wednesday, April 16, 2003 4:27 AM
To: Dave Greenstein
Cc: snort-devel at lists.sourceforge.net; Dirk_Geschke at ...802...
Subject: Re: [Snort-devel] RE: Snort 1.9.1 fix for stream4 reassembly
integer overflow


Hi Dave,

> Just noticed Matt Callaway had a simpler fix for the 1.9.1 code 
> base... not sure which is preferable.

I think the backport from snort-2.0.0 is the better solution.

Matt Callaway's patch simply discards packets which would result in an
overrun of sequence numbers due to the size of the payload. 
But within a TCP session this should be allowed, there is no restriction
forbidding an overrun in sequence numbers.

Indeed the isBetween function takes an overrun implicitly
into account.

Best regards

Dirk Geschke

-- 
+-------------------------------------------------------------+
| Dr. Dirk Geschke            | E-mail: geschke at ...802...      |
| Gesellschaft fuer Netzwerk  | Tel.  : +49-(0)-89-991950-131 |
| und Unix Administration mbH | Fax   : +49-(0)-89-991950-999 |
| 85551 Kirchheim / Germany   | Domagkstrasse 7               |
+-------------------------------------------------------------+






More information about the Snort-devel mailing list