Date: Wed, 14 Sep 2005 13:47:16 -0400
From: Martin Roesch <roesch@...rcefire.com>
To: "Ferguson, Justin (IARC)" <FergusonJ@...doe.gov>
Cc: "'snort-devel@...ts.sourceforge.net'" <snort-devel@...ts.sourceforge.net>,
	"'snort-users@...ts.sourceforge.net'" <snort-users@...ts.sourceforge.net>,
	"'bugtraq@...urityfocus.com'" <bugtraq@...urityfocus.com>
Subject: Re: Re: [Snort-users] Snort DoS Fallacies

I think your response can be safely classified as FUD as it is filled  
with inaccuracies and serves only to cast doubt on the code, the  
ability of the development team and (strangely) the quality of  
Sourcefire's products.  Let me just net it out:

Q1) This is a replay of the milw0rm TCP Option processing DoS.
A1) Wrong, this is a different issue

Q2) How did this make it back into the codebase?
A2) It didn't, your research was incorrect.

Q3) Does this problem affect Sourcefire as well?
A3) No, Sourcefire appliances are configured for robustness and  
performance, we don't use ASCII mode logging or alerting ever for any  
reason from the Snort engines on our products.

Q4) If the rest of the code doesn't have this vulnerability why were  
the additional NULL pointer checks added?
A4) Safe coding practices, Steve Sturges thought that it wouldn't be  
a bad idea to put those checks in there and I agreed.  I'm 100% sure  
they aren't vulnerable because I actually tested it.  Personally I  
don't like all those memcpy()'s in that function and when I (or  
someone here) get the time I'd like to go back and change how that  
function works.

Q5) Frag3 has the problem in the snapshot I downloaded, why won't you  
admit it?
A5) Because you're wrong.  The snapshot you're referring to has the  
fixes in PrintTcpOptions(), so even with the call to PrintIPPkt() in  
there the DoS doesn't work.  Version 2.4.0 did not have the code you  
are referring to.

This is the code checked out of cvs.snort.org from last night.

    3050 #ifdef DEBUG_FRAG3
    3051     /*
    3052      * Note, that this won't print out the IP Options or any  
other
    3053      * data that is established when the packet is decoded.
    3054      */
    3055     if (DEBUG_FRAG & GetDebugLevel())
    3056     {
    3057         ClearDumpBuf();
    3058         printf("++++++++++++++++++Frag3 DEFRAG'd PACKET++++++ 
++++++++\n");
    3059         PrintIPPkt(stdout, defrag_pkt->iph->ip_proto,  
defrag_pkt);
    3060         printf("++++++++++++++++++Frag3 DEFRAG'd PACKET++++++ 
++++++++\n");
    3061         ClearDumpBuf();
    3062     }
    3063 #endif
    3064     ProcessPacket(NULL, defrag_pkt->pkth, defrag_pkt->pkt, ft);
    3065
    3066     DEBUG_WRAP(DebugMessage(DEBUG_FRAG,
    3067                 "Done with rebuilt packet, marking rebuilt... 
\n"););
    3068
    3069     ft->frag_flags = ft->frag_flags | FRAG_REBUILT;
    3070 }
    3071

Here's the code from the 2.4.0 stable release tarball on snort.org:

    2595 #ifdef DEBUG_FRAG3
    2596     /*
    2597      * Note, that this won't print out the IP Options or any  
other
    2598      * data that is established when the packet is decoded.
    2599      */
    2600     if (DEBUG_FRAG & GetDebugLevel())
    2601     {
    2602         ClearDumpBuf();
    2603         printf("++++++++++++++++++Frag3 DEFRAG'd PACKET++++++ 
++++++++\n");
    2604         PrintIPPkt(stdout, defrag_pkt->iph->ip_proto,  
defrag_pkt);
    2605         printf("++++++++++++++++++Frag3 DEFRAG'd PACKET++++++ 
++++++++\n");
    2606         ClearDumpBuf();
    2607     }
    2608 #endif
    2609     ProcessPacket(NULL, defrag_pkt->pkth, defrag_pkt->pkt, ft);
    2610
    2611     DEBUG_WRAP(DebugMessage(DEBUG_FRAG,
    2612                 "Done with rebuilt packet, marking rebuilt... 
\n"););
    2613
    2614     ft->frag_flags = ft->frag_flags | FRAG_REBUILT;
    2615 }

Note that the code in question isn't in there in either version.  It  
looks to me like some debug code snuck into the snapshot, we'll have  
to update that.

Q6) Blackboxing/fuzzing "are of little value", why did you use that  
method?
A6) I tested every possible input to the system (a simple test since  
we're dealing only with  two 8-bit values in the test matrix) by  
modifying the PoC and generating all the potential value pairs.  This  
method clearly showed the DoS condition for option 5 with length 2  
and that it did not occur under any other input set.  I would  
consider this a valid test for the purposes of analyzing this code  
path and vulnerability.

Q7) "And your team, the vendor missed all of the above on your first  
*two* passes through, once in Dec 2004, and once in Aug/Sep 2005,  
does this mean what you've said and/or done is without merit as well?"
A7) You are incorrect, these were two different issues and you are  
confusing them.  The merits of our analysis is obvious because it is  
independently verifiable as being correct.

Q8) How can you say I was wrong about the "-A fast" switch when the  
DoS can effect fast output mode?
A8) Because your analysis was incorrect, the conditions under which  
the DoS could be manifested was different than your assertion.  There  
is a DoS condition for manually configured fast mode with a specific  
argument only.

Q9) Why did you keep the ASCII logging problem "quiet"?
A9) If you're running ASCII logging mode you are now subject to two  
DoS conditions, I don't consider the risk of one to be greater than  
the other because inode exhaustion can happen far before a disk is  
actually full.

Q10) I have no proof that other TCP options can't cause this  
condition, how can I trust your analysis?
A10) I actually setup an instrumented, repeatable test and proved  
it.  The test is easily recreated by grabbing the POC code from the  
net and applying the attached patch.

Q11) "Did you not at least get dejavu from 2004?"
A11) I did for about 2 seconds, but then I realized it was a  
different issue when I actually did some empirical testing and  
research/debugging.  If you had actually looked at the code and  
understood what you were looking at you would have seen that too.   
The issue from 2004 was an off-by-one error in an array index that  
could manifest if the TCP option decoder bailed out early.  This was  
a problem  with a NULL pointer dereference from a specific option  
setting as a side effect of the way the validation routine works.

Q12) "You do not find it odd at all, that you say 'no one should be  
doing xyz on production sensors anyways', and then you suggest that  
they run CVS code on production sensors?  Which one do you think will  
result in problems first?"
A12) Knowing the exact scope of the changes in the logging code, I  
think that patching in a new log.c file from CVS is a safe course of  
action, otherwise I wouldn't have recommended it.  It's not bad  
advice because people can trivially look at the diffs from CVS and  
see what changed.

Q13) Justin Ferguson made a patch against 2.3.3 that fixes this  
problem, should I use it?
A13) I would not under any circumstances move back to 2.3.3 or use a  
patch from an untrusted 3rd party in a production sensor, there were  
many bug fixes and new features incorporated in 2.4.0 and reverting  
to an old version is a step in the wrong direction.  It is far safer  
and advisable to use the log.c in CVS or wait for the 2.4.1 release  
if you're not running fast/full alerting or ASCII logging.

      -Marty



Download attachment "tcpoptdos-mod.diff" of type "application/octet-stream" (1979 bytes)



--
Martin Roesch - Founder/CTO, Sourcefire Inc. - +1-410-290-1616
Sourcefire - Discover.  Determine.  Defend.
roesch@...rcefire.com - http://www.sourcefire.com
Snort: Open Source Network IDS - http://www.snort.org




Download attachment "PGP.sig" of type "application/pgp-signature" (187 bytes)

Powered by blists - more mailing lists