search

amzn / zeek-plugin-s7comm

Zeek network security monitor plugin that enables parsing of the S7 protocol
BSD 3-Clause "New" or "Revised" License
39 stars 12 forks source link

unsupported byte length for bytestring_to_count #1

Closed Mohan-Dhawan closed 4 years ago

Mohan-Dhawan commented 4 years ago

The plugin reports the following errors for certain pcaps.

1408528978.024324 error in /home/mohan/.zkg/plugin_dir//packages/zeek-plugin-s7comm/scripts/./main.zeek, line 119: unsupported byte length for bytestring_to_count (bytestring_to_count(\x00\x00 + S7comm::data[(coerce S7comm::data_index to int), (coerce S7comm::data_index + S7comm::data_length / 8 to int)], F))
1408528978.038314 error in /home/mohan/.zkg/plugin_dir//packages/zeek-plugin-s7comm/scripts/./main.zeek, line 119: unsupported byte length for bytestring_to_count (bytestring_to_count(\x00\x00 + S7comm::data[(coerce S7comm::data_index to int), (coerce S7comm::data_index + S7comm::data_length / 8 to int)], F))
1408528978.069292 error in /home/mohan/.zkg/plugin_dir//packages/zeek-plugin-s7comm/scripts/./main.zeek, line 119: unsupported byte length for bytestring_to_count (bytestring_to_count(\x00\x00 + S7comm::data[(coerce S7comm::data_index to int), (coerce S7comm::data_index + S7comm::data_length / 8 to int)], F))

Similar errors have been reported for BACNet (https://github.com/amzn/zeek-plugin-bacnet/issues) and TDS (https://github.com/amzn/zeek-plugin-tds/issues) plugins. The offending pcaps for the S7 plugin are: a) https://github.com/ITI/ICS-pcap/blob/master/S7/s7comm_varservice_libnodavedemo/s7comm_varservice_libnodavedemo.pcap b) https://github.com/ITI/ICS-pcap/blob/master/S7/s7comm_varservice_libnodavedemo_bench/s7comm_varservice_libnodavedemo_bench.pcap

NothinRandom commented 4 years ago

@Mohan-Dhawan. Thanks for reporting this issue! This is weird because the analyzer was first developed during the Bro2.5.x. Since Bro didn't have support for bytes to float, this bytes to double method was used instead. It did work for the Bro era. However, it stopped working when transitioned to Zeek. I did put in a request to make the bytecount_to_float available, so hopefully they'll incorporate it into the next revision. Unfortunately, there's not much to do for now since we chose to parse with zeek scripting language over C++. I'll still take a look, and maybe there's a work around.

Mohan-Dhawan commented 4 years ago

@NothinRandom: Thanks for the update. If I understand correctly, then there remains a possibility that the plugin may not work correctly for some corner cases when the size of the data string fed to bytestring_to_count is not a multiple of 8 (and Zeek returns a default value of 0), right?

NothinRandom commented 4 years ago

@Mohan-Dhawan, bytestring_to_count handles some scenarios, but s7comm data plays nice and should follow the 1/2/4 bytes format. Since float is only a 32b precision and zeek doesn't have anything to handle 32b, this is why padding was needed to do handle bytestring to double instead.

Mohan-Dhawan commented 4 years ago

@NothinRandom I still have a doubt here. The API that is throwing warning is bytestring_to_count, which does not deal withfloat or double values. I realize that the cause of the error is that the length of the data is not aligned to 8, 16, 2 or 64 bytes and Zeek returns a value of 0. The only workaround I see here is to manually pad the data to the correct size, right.

NothinRandom commented 4 years ago

@Mohan-Dhawan, Sorry for delayed response, was traveling. The traffic that I've seen so far only has 32b float values. Since Zeek doesn't handle 32b, I needed to pad to 64b in order to convert. Since we're using switch statement to handle different cases, it's pretty strict on this... but there could be some odd about it. It worked fine in bro 2.x era. I'll investigate the provided pcaps and get back to you.

gabry94 commented 4 years ago

Hi @NothinRandom do you have any updates? I'm facing with the same issue.

Thanks.

NothinRandom commented 4 years ago

@gabry94, Sorry for delay, I'm picking this back up again. I'll try to provide status update this week.

NothinRandom commented 4 years ago

@Mohan-Dhawan, @gabry94 hey guys. Thanks for your patience. I've made some changes that should address this issue. Give it a whirl and let me know how it goes.

NothinRandom commented 4 years ago

Closing. Please reopen, if needed