Enip rust 3958 v7 #9991
Enip rust 3958 v7 #9991
Conversation
So that we can write enip.revision: 0x203
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
catenacyber
requested review from
jasonish,
jufajardini,
victorjulien and
a team
as code owners
|
Also #9870 should be merged before this to avoid a short-lived output-json-enip.c file ;-) |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9991 +/- ##
==========================================
- Coverage 82.45% 77.89% -4.57%
==========================================
Files 970 989 +19
Lines 271356 273570 +2214
==========================================
- Hits 223746 213096 -10650
- Misses 47610 60474 +12864
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 16954 |
| enip.cip_extendedstatus | ||
| ----------------------- | ||
|
|
||
| Match on the cip extended status if any (one of them in case of multiple service packet). |
There was a problem hiding this comment.
This sentence is a bit confusing to me. Does this mean that cip_extendedstatus will match on extended status if there is any, and that it can match on any of the seen statuses - as is, a single packet might have more than one?
There was a problem hiding this comment.
Your understanding is correct.
Do you have another way to phrase this ?
There was a problem hiding this comment.
What about...
Match on the cip extended status, if any is present. For multiple service packet, if there is more than one, can match on any of the seen statuses.
| return -1; | ||
|
|
||
| DetectU32Data *du32 = DetectU32Parse(rulestr); | ||
| if (du32 == NULL) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Curious: why not use SCReturnInt in these cases, too?
There was a problem hiding this comment.
I do not know what SCReturnInt does different... @victorjulien ?
There was a problem hiding this comment.
It's a wrapper around a SCLogDebug + return statement, so it can be helpful during debug. But we're not very consistent in using it.
| Examples:: | ||
|
|
||
| enip.status:100; | ||
| enip.status:>106; |
There was a problem hiding this comment.
For all these cases where a value is accepted, are all other modes (<, <=, >...) also accepted? Wonder if this should be made more evident, if nothing else, by using other operands, too.
There was a problem hiding this comment.
Every mode is supported is true for all integer keywords...
Maybe there should be from every integer keyword documentation a link to some special section where integers are documented ? This should be its own redmine ticket ;-)
There was a problem hiding this comment.
Fair point, and makes sense.
There was a problem hiding this comment.
|
Replaced by #10048 |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3958
Describe changes:
Alon the way, also
#9940 with rebase and fixup commit autosquashed + new S-V tests
Provide values to any of the below to override the defaults.
OISF/suricata-verify#1521
Does the first commit deserve its own redmine ticket ?
And the one in 4a49352 also ?