enip: convert to rust by catenacyber · Pull Request #10901 · OISF/suricata

catenacyber · 2024-04-18T12:05:34Z

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3958

Describe changes:

convert enip parser to rust

Alon the way, also

transactions are now bidirectional
there is a enip logger
gap support is improved with probing for resync
frames
events
enip_command keyword accepts now string enumeration as values.
more keywords, to have parity with logging

#10850 with needed rebase

Ticket: 3958 - transactions are now bidirectional - there is a logger - gap support is improved with probing for resync - frames support - app-layer events - enip_command keyword accepts now string enumeration as values. - add enip.status keyword - add keywords : enip.product_name, enip.protocol_version, enip.revision, enip.identity_status, enip.state, enip.serial, enip.product_code, enip.device_type, enip.vendor_id, enip.capabilities, enip.cip_attribute, enip.cip_class, enip.cip_instance, enip.cip_status, enip.cip_extendedstatus

codecov · 2024-04-18T16:40:18Z

Codecov Report

Attention: Patch coverage is 43.60902% with 1950 lines in your changes are missing coverage. Please review.

Project coverage is 82.17%. Comparing base (2b4e102) to head (2321978).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10901      +/-   ##
==========================================
+ Coverage   77.64%   82.17%   +4.53%     
==========================================
  Files         922      941      +19     
  Lines      247806   250157    +2351     
==========================================
+ Hits       192400   205570   +13170     
+ Misses      55406    44587   -10819

Flag	Coverage Δ
fuzzcorpus	`63.35% <33.76%> (?)`
suricata-verify	`61.34% <38.80%> (-1.09%)`	⬇️
unittests	`61.65% <11.22%> (-0.56%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2024-04-20T21:30:04Z

Information:

ERROR: QA failed on SURI_TLPW2_autofp_suri_time.

ERROR: QA failed on SURI_TLPW1_files_sha256.

ERROR: QA failed on SURI_TLPR1_suri_time.

field	baseline	test	%
	SURI_TLPW2_autofp_stats_chk
.uptime	138	145	105.07%
	SURI_TLPR1_stats_chk
.uptime	644	693	107.61%

Pipeline 20220

jufajardini

Kudos on tackling keyword/log output parity :D

Not sure if possible, but wondering if we could have dedicated commits:
Maybe:

one for docs
one for logging and parsing (and schema?)
one for detection
one for new keywords added
replacement of C code with rust code into an independent commit
maybe one for unittests removal, just to keep the change contained?

Please ignore if this proposal doesn't make sense or would be too much work

catenacyber · 2024-04-23T19:57:06Z

Not sure if possible, but wondering if we could have dedicated commits: Maybe:

one for docs

I do not like separating doc commits from the code changes needing the documentation...
I do not think this was settled

one for logging and parsing (and schema?)

one for detection

Already existing keywords must be done within the same commit as parsing goes from c to rust...

one for new keywords added

replacement of C code with rust code into an independent commit

This is not independent : I add logging and parsing in rust, while moving away the C code...

maybe one for unittests removal, just to keep the change contained?

Interesting. Why so ?

Please ignore if this proposal doesn't make sense or would be too much work

This can be work... not sure if it is relevant...

jufajardini · 2024-04-25T03:01:06Z

maybe one for unittests removal, just to keep the change contained?

Interesting. Why so ?

Looking again, I can't see what made me make this comment, please ignore. >__<'

Please ignore if this proposal doesn't make sense or would be too much work

This can be work... not sure if it is relevant...

In general I see that we try to break changes into smaller commits, so that's where this comes from. But I don't have a super strong opinion about this, yet...

victorjulien

Some questions and feedback inline

victorjulien · 2024-05-24T16:43:39Z

rust/src/enip/detect.rs

+}
+
+#[no_mangle]
+pub unsafe extern "C" fn rs_enip_tx_has_cip_attribute(


Can you update all [no_mangle] functions to use the new naming style for FFI rust funcs?

With python

f = open("diff.txt") for l in f.readlines(): base = l[:-1] trans1 = base.replace("rs_enip", "SCEnip") trans = "" state = 0 for c in trans1: if c == '_': state = 1 else: if state == 1: trans += c.upper() state = 0 else: trans += c print("git grep %s | cut -d: -f1 | xargs sed -i -e 's/%s/%s/'" % (base, base, trans))

victorjulien · 2024-05-24T16:45:50Z

rust/src/enip/enip.rs

+        let input = stream_slice.as_slice();
+        match parser::parse_enip_pdu(input) {
+            Ok((_, pdu)) => {
+                process_frames(&pdu, &stream_slice, flow, input);


we should create the frame regardless of the parser result

Do you mean only for UDP ?

And which frame are you talking about ? There are many...
I think you want a frame pdu which is both the header and payload frame, right ?

I added the standard Pdu frame...

And will do so for Websocket ;-)

victorjulien · 2024-05-24T16:53:17Z

rust/src/enip/logger.rs

+    Ok(())
+}
+
+fn enip_vendorid_string(p: u16) -> Option<&'static str> {


where does this list come from?

https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-cip.c#L1579

Or https://github.com/cisagov/icsnpp-enip/blob/main/scripts/consts.zeek#L325

victorjulien · 2024-05-24T16:53:41Z

rust/src/enip/logger.rs

+    }
+}
+
+fn enip_devicetype_string(p: u16) -> Option<&'static str> {


what is the source of this info?

I bet on Wireshark

There is also https://github.com/secdev/scapy/blob/master/scapy/contrib/enipTCP.py#L64

catenacyber · 2024-05-29T11:42:31Z

Continued in #11174

catenacyber requested review from a team, jasonish, jufajardini and victorjulien as code owners April 18, 2024 12:05

catenacyber mentioned this pull request Apr 18, 2024

enip: convert to rust #10850

Closed

catenacyber force-pushed the enip-rust-3958-v17 branch from 9aff27c to 2321978 Compare April 18, 2024 13:40

jufajardini reviewed Apr 23, 2024

View reviewed changes

catenacyber mentioned this pull request May 14, 2024

detect/cip: add check for sscanf return-value - v5 #10433

Closed

victorjulien requested changes May 24, 2024

View reviewed changes

catenacyber mentioned this pull request May 29, 2024

enip: convert to rust #11174

Closed

catenacyber closed this May 29, 2024

Comments

Conversation

catenacyber commented Apr 18, 2024

Uh oh!

codecov bot commented Apr 18, 2024

Codecov Report

Uh oh!

suricata-qa commented Apr 20, 2024

Uh oh!

jufajardini left a comment

Choose a reason for hiding this comment

Uh oh!

catenacyber commented Apr 23, 2024

Uh oh!

jufajardini commented Apr 25, 2024

Uh oh!

victorjulien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catenacyber May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catenacyber commented May 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

catenacyber May 29, 2024 •

edited

Loading