cisagov / icsnpp-bacnet

Zeek BACnet Parser - CISA ICSNPP
BSD 3-Clause "New" or "Revised" License
15 stars 11 forks source link

Bacnet segfaults in ACK logic #15

Closed keithjjones closed 1 year ago

keithjjones commented 1 year ago

🐛 Summary

I'm seeing the following 2 segfaults on a network I monitor. I can't tell if they are related. Feel free to make a separate issue for the second error if they are not related. Unfortunately I can't pull/share pcaps or see what packets cause these errors, but I thought I'd flag the errors here. Thanks!

Error 1:

binpac::BACNET::Complex_ACK_PDU::Parse(unsigned char const*, unsigned char const*, binpac::BACNET::ContextBACNET*, int)
binpac::BACNET::APDU_Header::Parse(unsigned char const*, unsigned char const*, binpac::BACNET::ContextBACNET*, int)
binpac::BACNET::NPDU_Header::Parse(unsigned char const*, unsigned char const*, binpac::BACNET::ContextBACNET*, int)
binpac::BACNET::BVLC_Header::Parse(unsigned char const*, unsigned char const*, binpac::BACNET::ContextBACNET*, int)
binpac::BACNET::BACNET_Flow::NewData(unsigned char const*, unsigned char const*)
analyzer::BACNET::BACNET_Analyzer::DeliverPacket(int, unsigned char const*, bool, unsigned long, zeek::IP_Hdr const*, int)
zeek::analyzer::Analyzer::NextPacket(int, unsigned char const*, bool, unsigned long, zeek::IP_Hdr const*, int)
zeek::analyzer::Analyzer::ForwardPacket(int, unsigned char const*, bool, unsigned long, zeek::IP_Hdr const*, int)
zeek::packet_analysis::UDP::UDPAnalyzer::DeliverPacket(zeek::Connection*, double, bool, int, zeek::Packet*)
zeek::packet_analysis::IP::IPBasedAnalyzer::AnalyzePacket(unsigned long, unsigned char const*, zeek::Packet*)
zeek::packet_analysis::Analyzer::ForwardPacket(unsigned long, unsigned char const*, zeek::Packet*, unsigned int) const
zeek::packet_analysis::IP::IPAnalyzer::AnalyzePacket(unsigned long, unsigned char const*, zeek::Packet*)
zeek::packet_analysis::Analyzer::ForwardPacket(unsigned long, unsigned char const*, zeek::Packet*, unsigned int) const
zeek::packet_analysis::Analyzer::ForwardPacket(unsigned long, unsigned char const*, zeek::Packet*, unsigned int) const
zeek::packet_analysis::Analyzer::ForwardPacket(unsigned long, unsigned char const*, zeek::Packet*, unsigned int) const
zeek::packet_analysis::Analyzer::ForwardPacket(unsigned long, unsigned char const*, zeek::Packet*, unsigned int) const
zeek::packet_analysis::Manager::ProcessPacket(zeek::Packet*)
zeek::run_state::detail::dispatch_packet(zeek::Packet*, zeek::iosource::PktSrc*)
zeek::iosource::PktSrc::Process()
zeek::run_state::detail::run_loop()
main
__libc_start_main
_start

Error 2:

binpac::BACNET::BACNET_Flow::process_read_range_ack(bool, std::vector<binpac::BACNET::BACnet_Tag*, std::allocator<binpac::BACNET::BACnet_Tag*> >*)
binpac::BACNET::Complex_ACK_PDU::Parse(unsigned char const*, unsigned char const*, binpac::BACNET::ContextBACNET*, int)
binpac::BACNET::APDU_Header::Parse(unsigned char const*, unsigned char const*, binpac::BACNET::ContextBACNET*, int)
binpac::BACNET::NPDU_Header::Parse(unsigned char const*, unsigned char const*, binpac::BACNET::ContextBACNET*, int)
binpac::BACNET::BVLC_Header::Parse(unsigned char const*, unsigned char const*, binpac::BACNET::ContextBACNET*, int)
binpac::BACNET::BACNET_Flow::NewData(unsigned char const*, unsigned char const*)
analyzer::BACNET::BACNET_Analyzer::DeliverPacket(int, unsigned char const*, bool, unsigned long, zeek::IP_Hdr const*, int)
Kleinspider commented 1 year ago

I have added a variety of checks in different places that hopefully is preventing out of bound reads that I believe may have been causing the segfaults. I don't have a way to confirm this will fix this issue, so if you are still seeing this issue after the most recent commit, please let me know and I will look into it more.

keithjjones commented 1 year ago

Thanks! I loaded it up on my sensors and will watch it for the next few days.

keithjjones commented 1 year ago

No errors in 2 days.