[
https://bro-tracker.atlassian.net/browse/BIT-1045?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14211#comment-14211
]
Robin Sommer commented on BIT-1045:
-----------------------------------
Going through, I see number of places where I'd argue it's actually a
programming/logic error that's not something that can be directly/just
triggered by crafted network traffic. Examples are the RefCnt() checks in
~ConnectionTimer() and the indent_level check in ODesc. I'm inclined to leave
them as they were, with the argument being that those kinds of error actually
*are* best to trigger an abort. E.g, if the reference counting goes awry,
pretty much all bets are off anyways, and I'd rather have Bro terminate than
trying to continue.
So I think the guideline should be avoiding internal errors that happen
*directly* because of broken network input; not because of (for lack of a
better term) "infrastructure problems" in other parts of Bro. (Although I'm
sure as I go further, I'll find more cases where that definition is ambiguous
as well.)
What's your opinion on cases like the above?
What I could do is go through your diffs and adapt with the above in mind, and
then we can do another iteration and see if/where we agree.
> 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
