Added DPD signatures.

[keithjjones]

🗣 Description

Added DPD sigs instead of static port.

💭 Motivation and context

Looks like any UDP port can be used, so this will hopefully generalize the detections.

🧪 Testing

The tests pass.

✅ Pre-approval checklist

• [x] This PR has an informative and human-readable title.
• [x] Changes are limited to a single goal - eschew scope creep!
• [x] I have read the CONTRIBUTING document.
• [x] These code changes follow cisagov code standards.
• [x] All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
• [x] All new and existing tests pass.

✅ Pre-merge checklist

• [ ] Revert dependencies to default branches.
• [ ] Finalize version.

✅ Post-merge checklist

• [ ] Add a tag or create a release.

[Kleinspider]

While BACnet devices can technically use any port other than UDP/47808, we have not see any evidence of this in practice, and have always seen BACnet communicate on that port. Overall, signatures are a better way to handle traffic identification rather than static ports, however, I believe this signature is too broad and would lead to a significant amount of false positives.

One potential workaround, would be to add your dpd.sig file and then add the line # @load-sigs ./dpd.sig line into load.zeek so it can be easily enabled at any time, but not enabled by default. I could then add instructions or troubleshooting section into the README regarding handling BACnet traffic that occurs on a port other than UDP/47808.

Please let me know your thoughts on this, I appreciate the feedback and assistance in improving this parser!

[keithjjones]

I have tested this on just 1 of my networks and I see bacnet on 49000, but that could be an FP for all I know right now. Generally what I do with my spicy parsers is write a dpd signature that will capture most of the protocol and then put constraints in my parsing logic to reject anything not in the protocol. It's been a year or maybe more since I've dabbled with binpac - but there is probably something similar in there to a &requires in spicy. Or if you know of a better signature, we can use it. This signature is working great for me on 2 of my very large university sites and I haven't seen any false positives that I've been able to tell so far, but I'm still pretty new to this protocol. I don't have any specs, unfortunately.

[keithjjones]

Ah it looks like it's &enforce: https://github.com/zeek/binpac#enforce

There is one use of it here: https://github.com/cisagov/icsnpp-bacnet/search?q=enforce

Would it be possible to use &enforce more liberally to throw out false positives in the parsing code? I'm not sure what constraints the specs give for bacnet, so I don't know how much help I can be in specific suggestions for that protocol. Usually what I do is put in constraints anywhere that seem logical for throwing out false positives. Things like enums have been great for that in my spicy work. Or version number ranges. The plus side of using more &enforce is throwing out any traffic on 47808 that isn't bacnet, too.

[Kleinspider]

That sounds like a perfect solution, I will look into the protocol and see where else I can additional constraints to prevent false positives and then replace the port detection with the signature. Thanks!

[keithjjones]

That sounds like a perfect solution, I will look into the protocol and see where else I can additional constraints to prevent false positives and then replace the port detection with the signature. Thanks!

Absolutely - hey I'm happy to test this stuff on my networks too because I can now see stuff I couldn't before. I appreciate very much this being open source. Sorry I don't have specs to be more of a help - I take it you have access to the official specs? If there is anything else I can do, holler! Thanks!

[Kleinspider]

Looking through the code, it looks like there are a lot of checks already in the parsing via various switch-case statements to ensure variables such as bvlc_function, pdu_type, and services match the known BACnet values. I added an additional check for protocol_version as that is a static value, but besides that I am going to merge in your changes to utilize these signatures instead of ports.

If you happen to come across any false positives, please send them my way and I can see if there are any additional checks I can add.

[keithjjones]

@Kleinspider I haven't loaded this specific new commit yet, but I looked at my data overnight with the original code and signatures and there are false positives. I'm seeing some on dns (port 53), and a bunch of other ports. Looking through the code, there may be other spots for &enforce, such as:

https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L41 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L339 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L409 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L489 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L546 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L586 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L588 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L636 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L690 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L715 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L719 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L741 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L743 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L766 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L768 https://github.com/cisagov/icsnpp-bacnet/blob/main/src/bacnet-protocol.pac#L817

This is my best guess of properties that have known ranges we could enforce, but take them with a grain of salt because I don't fully know this protocol inside and out yet. They are best guesses on my part.

I might not be able to test any changes until next week as I have a few things going on this week, but I will be testing to help work this out as soon as I get a chance. Let me know if you need anything else!

Thanks!

[keithjjones]

I would aim to put an &enforce on all structures in bacnet-protocol.pac that use these constants, as that will add more constraints and throw out additional false positives:

https://github.com/cisagov/icsnpp-bacnet/blob/791dd31fbca6e85b89a9b3dd5c0f8a49d5bad5e2/src/consts.pac

[Kleinspider]

I was able to spend some time this morning looking into what was generating most of these false positives, and unfortunately it is the more basic/simple BVLC functions within BACnet that generate these false positives (Read/Write Broadcast Table, Register Foreign Device, Foreign Device tables, etc.). These functions are extremely basic and don't have a lot of areas where &enforce could be added in.

I have added &enforce wherever I could (that made sense within the protocol and parsing logic) in bacnet-protocol.pac, but sadly with these more basic functions, there wasn't a lot I could do with those to prevent false positives.

Because of this, we are going to leave the dpd.sig inactive by default and process BACnet only on UDP/47808. However, I have left the dpd.sig file you generated in the scripts directory, and instructions on how to switch over to utilizing signatures instead of ports in the Troubleshooting section of the README. Thanks for all your help in working with this, we are always happy to improve this parser any way we can!

[keithjjones]

That sounds good. I'd say if the sigs don't work, we don't pollute the code having them around, but that's up to you. I don't know that I would use sigs if I can't get the FP rate to almost zero.