enumerated treatment beyond case 2: action, 25: limit enable, 107: segmentation support, 112: system status

[duffy-ocraven]

Curious that current code has confined the Read-Property-Response enumerated treatment to solely:

                                        switch(property_identifier) {
                                            case 2: ##! action
                                                data[data_index] = fmt("value=%s", action[value]);
                                                break;
                                            case 25: ##! limit enable
                                                data[data_index] = fmt("value=%s", limit_enable[value]);
                                                break;
                                            case 107: ##! segmentation support
                                                data[data_index] = fmt("value=%s", segmentation_supports[value]);
                                                break;
                                            case 112: ##! system status
                                                data[data_index] = fmt("value=%s", system_statuses[value]);
                                                break;
                                            default:
                                                data[data_index] = fmt("value=%d", value);
                                                break;

Those are all good choices. But I wonder by what criteria those were selected, and why only those?

[duffy-ocraven]

And why in consts.zeek, only the choice:

const segmentation_supports = {
        [3] = "No Segmentation",

instead of the full enumerated possibilities:

BACnetSegmentation ::= ENUMERATED {
	segmented-both		(0),
	segmented-transmit	(1),
	segmented-receive	(2),
	no-segmentation		(3)

and only the either/or choices:

const limit_enable = {
        [1] = "Event Low Limit Enable",
        [2] = "Event High Limit Enable",

instead of the full bitstring of possibilities that also includes:

        [3] = "Event Both Limits Enable",

[duffy-ocraven]

I am interested in discussing the motivation; doesn't have to be on this Issue thread, but I don't have other contact info for you. Feel free to message me [email protected]

[NothinRandom]

@duffy-corelight, I don't mind discussing it here at all. For both inquiries, this was done so because those are the items that was noticed often in our traffic. I took pcaps for a few days from our production traffic and noticed these in WireShark. The same goes for the enumerations. Is there a reference list that I could use to make sure that all are included? If you have pcaps that you could share, that would be wonderful. I know I pulled the list of vendors from the BACnet website at the time this plugin was developed, so I know at least that was the latest. You could also contact me using my git user name @ gmail.com if there are topics that we need to discuss offline.

I've gone ahead and updated these and will push sometime this week. Again, thanks for being the subject matter expert in BACnet protocol to help improve this plugin!

[duffy-ocraven]

If we wanted to go for completeness, an automated syntax conversion from BACnet clause 21 is warranted. The enumerated string tables in computer-readable format that I have is 3272 lines.

[duffy-ocraven]

The content which is going into bacnet.log currently leaves out the property value content, due to lack of completeness in ranging over all the 13 different datatypes. Implemented are just:

                                local data_type: count = identifier_info / 16;
                                switch(data_type) {
                                    case 2,  ##! UINT
                                         9:  ##! ENUMERATION
                                    case 7,  ##! STRING
                                         8:  ##! BIT STRING
                                    case 12: ##! OBJECT

Is the omission because the others (NULL, Boolean, Integer, OctetString, Date, Time, REAL, Double) contain data which consumers of bacnet.log wouldn't care about?

[NothinRandom]

@duffy-corelight, it was because we didn't see traffic that contained the missing datatypes, so it was deemed safer to exclude them just in case incorrect parsing could have crashed zeek.

[duffy-ocraven]

Wise choice. Deploying a cure worse than the disease would have been excess hubris.