While testing the new Broker code in master I came across this a bit unexpectedly when trying to run our full production policy stack:
2.5-544 | 2018-05-01 17:57:15 -0500
- Rewrite the DHCP analyzer and accompanying script-layer API.
I’m all for analyzer updates and improvements, but what I’m honestly not sure about is this:
- Reduced all DHCP events into a single dhcp_message event.
(removed legacy events since they weren’t widely used anyway)
How was the determination made that it’s not widely used? I don’t recall a survey on the bro/bro-dev lists and there’s clearly instances of it’s use when searching github.
-Dop
I didn't dig around much which I made that change. Generally the DHCP analyzer hasn't been something that I've heard of much use (although Vlad did point out that I broke one of my own scripts!). How many scripts do you think it impacted? I suspect it's fairly few and we probably just need to put a change note in the release to say that developers will need to handle a new event to get the data. On the upside, you can handle both the old events and the new and they shouldn't impact each other (if you want to make a script work on multiple releases).
.Seth
I ran into this on a script I got from somewhere, bash-cve-2014-6271.bro
The fix is a little trickier, you can't handle both events because the DHCP::Msg type no longer exists and you need to wrap the old event with
@ifdef (DHCP::Msg)
@endif
So for that script I ended up with
@ifdef (DHCP::Msg)
event dhcp_message(c: connection, is_orig: bool, msg: DHCP::Msg, options: DHCP::Options)
{
if ( options?$host_name && shellshock in options$host_name )
NOTICE([$note=Bash::DHCP_hostname_Attack,
$conn=c,
$msg=fmt("%s may have attempted to exploit CVE-2014-6271, bash environment variable attack, via dhcp hostname against %s submitting \"hostname\"=\"%s\"",c$id$orig_h, c$id$resp_h, options$host_name),
$identifier=c$uid]);
}
@else
event dhcp_offer(c: connection, msg: dhcp_msg, mask: addr, router: dhcp_router_list, lease: interval, serv_addr: addr, host_name: string)
{
if ( shellshock in host_name )
NOTICE([$note=Bash::DHCP_hostname_Attack,
$conn=c,
$msg=fmt("%s may have attempted to exploit CVE-2014-6271, bash environment variable attack, via dhcp hostname against %s submitting \"hostname\"=\"%s\"",c$id$orig_h, c$id$resp_h, host_name),
$identifier=c$uid]);
}
@endif
Yeah, I’ve mainly seen it used for shellshock. On top of that, I saw some scripts in GitHub that used it from:
(There were a few others, like IVRE, but they’ve already updated).
Even if it’s not widely used, I think it’d be a nicer user experience if we were to ship a script that handled dhcp_message, and raised the old events. We could mark the old events as deprecated, and remove them in the next version. That way, people have at least one cycle to upgrade.
Hopefully, as we see more published Bro packages, we have a better idea of which events are/aren’t being used.
–Vlad
Hey, I use the dhcp analyzer because i cannot count on our dhcp logs. Not just that, I do some detection around it.
How much trouble is it to migrate your scripts to what's in Bro master?
.Seth
Ugh, I didn't consider that. Bro has re-reached the point where touching any number of things can set off an avalanche of problems like this.
Anyone on this thread up for submitting a patch which makes Bro cope with the changes automatically? You can then even mark the old events as deprecated. 
.Seth
Yep, already working on it.
No idea what’s in master. It should be trivial if you don’t remove the dhcp analyzer completely.
The other aspect of Bro changes is in the logging format. This comment isn’t specifically tied the DHCP change, but since we’re talking about breaking things…
With the default TSV, any change can break export into the various back-end log stores and SIEMs. When adding new fields, it would be nice to see them added to the end of the Info structure.
…alan
This was a complete rework on the logs and scripts so the structure is completely different. Unfortunately it wasn't just one of the cases where a field or two was added.
I don't think that assuming the order of fields is ever a safe assumption. It's why we shipped a version of bro-cut with Bro 2.0. We wanted to encourage people to refer to fields by the field name rather than the ordinal position of the field.
.Seth
I have a branch that implements this, topic/vladg/dhcp_event_deprecation.
You would need to load policy/protocols/dhcp/deprecated_events.bro.
--Vlad