Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enip rust 3958 v4 #9850

Closed
wants to merge 2 commits into from
Closed

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • fix stats not having _tcp prefix when protocol is detection-only
  • convert enip parser to rust

Alon the way, also

  • enip_command keyword accepts now string enumeration as values.
  • transactions are now bidirectional
  • there is a enip logger
  • gap support is improved with probing for resync
  • SEQUENCE_ADDR_ITEM value is fixed to 0x8002 instead of 0xB002
  • frames
  • events

#9848 CI should be greener + enip.status keyword + enip identity parsing and logging

Draft as is this is not complete :

  • parse more, log more, add more keywords (what is logged in enip identity)...

Provide values to any of the below to override the defaults.

SV_BRANCH=pr/1485

OISF/suricata-verify#1485

Even when on detection-only mode.
So that we always have enip_tcp and enip_udp in stats
and never just `enip`.
Suricata needs to know beyond suricata.yaml configuration which
protocols can be enabled on both tcp and udp...

Ticket: 6304
@catenacyber catenacyber mentioned this pull request Nov 20, 2023
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Merging #9850 (42b5fac) into master (90c1765) will decrease coverage by 0.03%.
The diff coverage is 83.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9850      +/-   ##
==========================================
- Coverage   82.47%   82.45%   -0.03%     
==========================================
  Files         973      976       +3     
  Lines      273962   273736     -226     
==========================================
- Hits       225944   225698     -246     
- Misses      48018    48038      +20     
Flag Coverage Δ
fuzzcorpus 64.25% <73.32%> (-0.13%) ⬇️
suricata-verify 61.15% <67.17%> (+0.05%) ⬆️
unittests 62.86% <18.64%> (-0.02%) ⬇️

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

Ticket: 3958

- enip_command keyword accepts now string enumeration as values.
- transactions are now bidirectional
- there is a logger
- gap support is improved with probing for resync
- SEQUENCE_ADDR_ITEM value is fixed to 0x8002 instead of 0xB002
- frames support
- app-layer events
- add enip.status keyword
}

#[no_mangle]
pub unsafe extern "C" fn rs_enip_parse_command(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Public C API, so SCEnipParseCommand?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically apply this to any no_mange pub extern C fn.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably have a (tracking) ticket to clean this up for any existing code. Plus see if we can have a CI check?

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW2_autofp_stats_chk
.uptime 184 174 94.57%

Pipeline 16661

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 16664

@victorjulien
Copy link
Member

"SEQUENCE_ADDR_ITEM value is fixed to 0x8002 instead of 0xB002" do we need this as a fix for 6/7 too?

@catenacyber
Copy link
Contributor Author

"SEQUENCE_ADDR_ITEM value is fixed to 0x8002 instead of 0xB002" do we need this as a fix for 6/7 too?

I guess so, but I have no pcap for test, just https://github.com/wireshark/wireshark/blob/136ca4287d66b84b2bbd46d616530abb458ddfdc/epan/dissectors/packet-enip.c#L89

Also, this PR does not pretend to handle CIP_SET_ATTR_LIST (when current version does it wrong for more than one attribute)

@catenacyber catenacyber added the needs rebase Needs rebase to master label Nov 23, 2023
This was referenced Dec 1, 2023
@catenacyber
Copy link
Contributor Author

Replaced by #9937

@catenacyber catenacyber closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

4 participants