[ https://bro-tracker.atlassian.net/browse/BIT-1045?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14213#comment-14213 ]
Robin Sommer commented on BIT-1045: ----------------------------------- Ok, good job on this. I went through and put a few small tweaks into topic/robin/internal-errors-merge. If you could double-check I'd appreciate (and it remains all fuzzy, I actually changed my mind a few times back and forth in some cases …) Also, one remaining (or new) concern is that now methods which previously aborted, may now return a value that's not usable upstream (null pointers in particular). I saw that you adapted a few locations, but it's hard to tell if there are more. I'm wondering though if Coverity might help us here and flag, e.g., potential null pointer usages. > Review usage of InternalError when parsing network traffic > ---------------------------------------------------------- > > Key: BIT-1045 > URL: https://bro-tracker.atlassian.net/browse/BIT-1045 > Project: Bro Issue Tracker > Issue Type: Task > Components: Bro > Affects Versions: git/master, 2.1 > Reporter: Vlad Grigorescu > Assignee: Robin Sommer > > Creating issue for tracking purposes. > Reporter->InternalError denotes a fatal error, and will cause Bro to stop. > Calling this function when parsing network traffic creates the possibility > for an attacker using a "packet of death," which could stop Bro. > I suspect that in most cases, a weird should be generated instead, and Bro > should just move on to the next packet. A quick grep shows some likely > candidates for incorrect use of InternalError: > src/Sessions.cc: reporter->InternalError("Bad IP protocol > version in DoNextInnerPacket"); > src/Sessions.cc: reporter->InternalError("fragment block not in > dictionary"); > src/Sessions.cc: reporter->InternalError("fragment block > missing"); > src/Sessions.cc: reporter->InternalError("unknown > transport protocol"); > src/Frag.cc: reporter->InternalError("bad IP version in fragment > reassembly"); > src/IP.cc: reporter->InternalError("IPv6_HdrChain::Init with > truncated IP header"); > src/IP.cc: reporter->InternalError("IPv6_Hdr_Chain bad > header %d", type); > src/IP.h: reporter->InternalError("bad IP version in > IP_Hdr ctor"); > src/RSH.cc: reporter->InternalError("multiple rsh client names"); > src/RSH.cc: reporter->InternalError("multiple rsh initial client > names"); > src/POP3.cc: reporter->InternalError("command not known"); > src/Rlogin.cc: reporter->InternalError("multiple rlogin client > names"); > src/ICMP.cc: reporter->InternalError("unexpected IP proto in > ICMP analyzer: %d", > src/ICMP.cc: reporter->InternalError("unexpected next protocol in > ICMP::DeliverPacket()"); > src/SMB.cc: reporter->InternalError("command mismatch for > ParseTransaction"); > src/HTTP.cc: reporter->InternalError("unrecognized HTTP message > event"); > src/HTTP.cc: reporter->InternalError("HTTP ParseRequest failed"); > src/DPM.cc: reporter->InternalError("unknown protocol"); > src/RPC.cc: reporter->InternalError("RPC underflow"); > src/RPC.cc: reporter->InternalError("RPC resync: skipping > over data failed"); > src/RPC.cc: > reporter->InternalError("inconsistent RPC record marker extraction"); -- This message was sent by Atlassian JIRA (v6.1-OD-09-WN#6144) _______________________________________________ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev