[Bro-Dev] #351: Incorrect bounds checking with truncated TCP options

Bro Tracker bro at tracker.icir.org
Tue Jan 18 08:57:14 PST 2011


#351: Incorrect bounds checking with truncated TCP options
---------------------+------------------------
 Reporter:  gregor   |      Owner:
     Type:  Problem  |     Status:  new
 Priority:  Normal   |  Milestone:  Bro1.6
Component:  Bro      |    Version:  git/master
 Keywords:           |
---------------------+------------------------
 {{{
 #!rst

 (from an e-mail I sent a while ago)
 (setting milestone to 1.6. Can probably be pushed back)

 Hi,

 there is a potential problem in Bro when it receives a packet with
 truncated TCP options (i.e., the packet isn't long enough to accommodate
 all options).

 This can happen:

 a) in the ConnCompressor: it calls ParseTCPOptions without checking
    whether the packet is long enough to contain all options.
    ConnCompressor needs to parse the TCP options to know the window
    scaling factor.


 b) in TCP.cc, when caplen < len and len is long enough for the TCP
    options but caplen is not.

 ExtractTCPHeader() ensures that the len is long enough to contain the
 options and that caplen is long enough to contain a struct tcphdr (but
 doesn't check for options). Presumably this is done to enable parsing of
 header only traces that truncate options.

 Nevertheless, the TCP Analyzer correctly checks caplen before
 ParseTCPOptions().

 But there are also situations when options are parsed without checking
 for caplen:

 * BuildSYNVal(), which is called on every SYN to get the window
   scaling options.

 * BuildOSVal(), which is only called when the OS_version_found event
   has a handler

 * TCP TraceRewriter (presumably we can ignore this, as we were going
   to remove it anyway)


 So, question is: what's the best way to tackle this? One option is to
 not parse packets that are truncated. But that's probably not a good
 idea wrt header traces.

 The other option is to check for the caplen whenever we parse options.
 That might be cumbersome as this information needs to be passed to many
 functions, e.g. in TCP_Analyzer: ProcessFlags -> ProcessSYN ->
 BuildSYNPacketVal.

 (In any case, truncated packets mean that we can't learn the window
 scaling....)


 }}}

-- 
Ticket URL: <http://tracker.icir.org/bro/ticket/351>
Bro Tracker <http://tracker.icir.org/bro>
Bro Issue Tracker



More information about the bro-dev mailing list