RE: Snort DoS Fallacies

["Ferguson, Justin (IARC)" <FergusonJ () nv doe gov>]

Martin,

I would hardly call this "analysis", as I took the time to look through the
code which obviously you guys originally did not do- as the report came
across as this is only a problem if you are using -v, which as you stated is
not true. As for whether the other tcp options can cause the same problem I
am not sure, you are correct in that I did not go that far into it, however
because the pointer was not checked, it feasibly *could* happen, providing
there was no other checks done before it was called, seeing as I didn't see
any such checks in the calling PrintTCPHeader(), then one would guess that
it would be possible to cause the same issues with other options-- however
no, I did not spend a lot of time figuring out the logistics of it, just
that the null pointer wasn't checked for.

Comments inline.


> That's not right.  This will only happen if you give Snort a config  
> directive in the snort.conf file with a specific argument.  You  
> didn't bother to check where the data->packet_flag is set, when you  
> specify fast as a command line option the alerting plugin is  
> automatically loaded with a NULL argument string.  Here's the config  
> line you need to make it happen:

> output alert_fast: packet

I will take your word on it, and you are correct I did not bother to check
through the entire source tree, believe it or not, that's your job, not
mine-- but the point was that here is another *possible* means of getting
into that function without using -v. The underlying point you will find in
this email is that the released advisories tell people that its only an
issue if you use -v, which is not true.



> If "a lot of people" are logging in ASCII mode then nobody is reading  
> the docs, the books, the mailing list archives or thinking about how  
> ASCII mode logging works with real file systems.  
[...]
> For the record, NO PRODUCTION SNORT DEPLOYMENT SHOULD EVER  
> (EVER!!!) RUN WITH ASCII LOGGING!!!  Everyone should be using  
> unified, database or binary (pcap) logging for production sensors,  
> period end of story.  ASCII logging is suitable only for testing and  
> lab environments, that's it.

While I will not disagree with you, I have seen this in several production
environments. Regardless of whether someone should be doing this or not is
beside the point, here is another execution path that could lead to the same
result. You arguing that no one should be doing this is akin to DJB arguing
that because people should be using ulimits that there isn't a bug in
qmail-- the point still remains that there is/was a bug.

> That's debug code there, we (developers) only enable that when we're  
> testing stuff out.  If you turn on all Snort's debug code you aren't  
> running an IDS anymore, you're running chargen. :)  

   snort/src/preprocessors/spp_frag3.c
2929 Frag3Rebuild()
     [...]
3117 #ifdef DEBUG_FRAG3
     [...]
3122     if (DEBUG_FRAG & GetDebugLevel())
3123     {
     [...]
3126         PrintIPPkt(stdout, defrag_pkt->iph->ip_proto, defrag_pkt);
     [...]
3129     }
3130 #endif
      [...]
3133         PrintIPPkt(stdout, defrag_pkt->iph->ip_proto, defrag_pkt);

Re-read the code snippet, developer. Notice the closing curly bracket
follwed by the #endif. You have two identical pieces of code, one is wrapped
in a #ifdef and a if (DEBUG ...) and the other isn't. So, one is only there
if debugging is enabled, the other exists in the preprocessor regardless.

> Actually, if you had done the research you would have seen that this  
> DoS condition doesn't work for:
> TCPOPT_MAXSEG
> TCPOPT_WSCALE
> TCPOPT_ECHO
> TCPOPT_ECHOREPLY
> TCPOPT_TIMESTAMP
> TCPOPT_CC
> TCPOPT_CCNEW
> TCPOPT_CCECHO
> or _any_ unrecognized or invalid option.
>
> While we were cleaning up the code for the SACK problem we thought  
> we'd make sure that there could never be another NULL ptr dereference  
> in that code.  Whether or not these are "bugs" (as you term them) is  
> open to interpretation because they don't look like you can exercise  
> them, but they certainly weren't as solid as they could have been so  
> we cleaned them up.

This may be the case, and I won't argue the point as I didn't research into
the feasibility of whether it was possible to cause a null pointer there,
just that it *could* happen, as I said. 

> Wow, we did almost as well as you!!

My line was originally incorrect as I was under the impression that this was
the result of an internal audit of some sort, whereas this was the result of
someone fuzzing snort. 


> BTW, you missed that we also call PrintTCPHeader in spo_alert_full.c,  
> which is actually done in the default config case, so this is  
> something you might want to worry about if you're using full alerting  
> for whatever reason.  For the record, the recommended alerting modes  
> for a production sensor are unified, syslog or database.

Thank you for adding to my point. This makes what 3 possible routes of
execution + the -v route for a total of 4 without debugging, and 6 if
debugging was to be enabled. Still quite a long ways from the 'only if you
are using -v'. 

> I *strongly* recommend that people use unified logging/alerting for  
> the foreseeable future, this is The Right Way (and the high  
> performance way) to run a Snort sensor.
>
> So, to summarize, there's an additional problem if you're using ASCII  
> mode logging, but if you've been running Snort for any time you  
> should know never to do that on a production sensor.  There is an  
> actual real live issue if you run with Full-mode alerting, but you  
> should typically be using a more robust alerting mode for production  
> sensors anyway.  If you're the one person on the planet that's  
> running Fast-mode alerting with the 'packet' config option turned on,  
> you should probably just switch to Full and get it over with since  
> they're effectively the same thing.  There are no additional TCP  
> Options processing that have this issue at this time that I'm aware  
> of, if anyone knows otherwise please feel free to submit a report to  
> me or snort-team () sourcefire com.

Is this about saving face or what? I mean, I can understand writing a
function that somewhere in it doesn't check that a pointer is not null, it
happens from time to time. What I cannot see is telling everyone that uses
your product that there is only one way to get into the bug when in fact
there is at least a few.
 
Best Regards,

J. Ferguson
Instrusion Analyst
NNSA Information Assurance Response Center (IARC)
fergusonj () nv doe gov



-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Snort-users mailing list
Snort-users () lists sourceforge net
Go to this URL to change user options or unsubscribe:
https://lists.sourceforge.net/lists/listinfo/snort-users
Snort-users list archive:
http://www.geocrawler.com/redir-sf.php3?list=snort-users