[Snort-devel] incorrect FDDI test in decode.c leads to reading uninitialized fields

Joel Esler jesler at ...402...
Tue Jan 15 21:32:19 EST 2013


Tavis,

Thanks for your report, we've opened a bug on our side to look into the issue.  FDDI support isn't compiled into our commercial products and in the open source world you'd need to be on a FDDI datalink to have that decoder active and linked into the execution path of Snort.

We look forward to any bugs that a man of your talents is able to provide to us, and we appreciate the review.

Feel free to contact and coordinate with me directly or via bugs at ...2256...

Thanks

--
Joel Esler
Senior Research Engineer, VRT
OpenSource Community Manager
Sourcefire

On Jan 15, 2013, at 4:05 PM, Tavis Ormandy <taviso at ...3361...> wrote:

> Hey, I've been looking at snort code recently, and have a few issues.
> I'll send more soon, but here is the first: DecodeFDDIPkt() in
> decode.c looks broken, you can see the intention of the author was to
> reject any packet that doesn't have FDDI_DSAP_IP and FDDI_SSAP_IP, but
> he incorrectly uses && instead of || in the condition at around line
> 6588.
> 
> 6583     /*
> 6584      * Now let's see if we actually care about the packet... If we don't,
> 6585      * throw it out!!!
> 6586      */
> 6587     if((p->fddisaps->dsap != FDDI_DSAP_IP) &&
> 6588             (p->fddisaps->ssap != FDDI_SSAP_IP))
> 6589     {
> 6590         DEBUG_WRAP(
> 6591                 DebugMessage(DEBUG_DECODE,
> 6592                     "This FDDI Packet isn't an IP/ARP packet...\n");
> 6593                 );
> 6594         PREPROC_PROFILE_END(decodePerfStats);
> 6595         return;
> 6596     }
> 
> This is a bug, because p->fddiiparp is only set when both fields are
> set, but that condition allows dsap or ssap to be set and still
> continue with it unintialized. The fix is trivial, just replacing '&&'
> with '||' should solve the problem.
> 
> It's trivial to produce a minimal testcase, here's an example:
> 
> $ xxd DecodeFDDIPkt.pcap
> 0000000: d4c3 b2a1 0200 0400 0000 0000 0000 0000  ................
> 0000010: 6000 0000 0a00 0000 0a4f de50 757f 0700  `........O.Pu...
> 0000020: 1000 0000 1000 0000 00aa aaaa aaaa aabb  ................
> 0000030: bbbb bbbb bb00 aa00                      ........
> 
> (use xxd -r to recover)
> 
> $ ./snort --pcap-single=- < DecodeFDDIPkt.pcap
> Running in packet dump mode
> 
>        --== Initializing Snort ==--
> Initializing Output Plugins!
> pcap DAQ configured to read-file.
> The DAQ version does not support reload.
> Acquiring network traffic from "stdin".
> 
>        --== Initialization Complete ==--
> 
>   ,,_     -*> Snort! <*-
>  o"  )~   Version 2.9.4 GRE (Build 40)
>   ''''    By Martin Roesch & The Snort Team:
> http://www.snort.org/snort/snort-team
>           Copyright (C) 1998-2012 Sourcefire, Inc., et al.
>           Using libpcap version 1.1.1
>           Using PCRE version: 8.12 2011-01-15
>           Using ZLIB version: 1.2.3.4
> 
> Commencing packet processing (pid=4154)
> Segmentation fault (core dumped)
> 
> src/decode.h:#define FDDI_DSAP_IP                    0xaa    /* IP */
> src/decode.h:#define FDDI_SSAP_IP                    0xaa    /* IP */
> 
> $ gdb -q ./snort
> (gdb) r -q --pcap-single=- < DecodeFDDIPkt.pcap
> Starting program: snort -q --pcap-single=- < DecodeFDDIPkt.pcap
> 
> Program received signal SIGSEGV, Segmentation fault.
> DecodeFDDIPkt (p=0x7fffffffd320, pkthdr=0x7fffffffdef0, pkt=0x15ce0f0
> "") at decode.c:6600
> 6600	    switch(htons(p->fddiiparp->ethertype))
> (gdb) p/x p->fddisaps->dsap
> $1 = 0x0
> (gdb) p/x p->fddisaps->ssap
> $2 = 0xaa
> (gdb)
> 
> You can see that flow incorrectly continued past the ssap/dsap check.
> 
> Hope this helps, Tavis.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.snort.org/pipermail/snort-devel/attachments/20130115/ea3798c3/attachment.html>


More information about the Snort-devel mailing list