protodetect: improve DCERPC UDP probing parser and simplify app-layer-detect-proto.c code v3 #11679
Conversation
Several additional checks are added to the probing parser to avoid false detection of DNS as DCERPC Ticket - 7111
…detection is improved Protocol detection code is simplified. Removed dependency on explicit alproto constants from the common part of code that must not be aware of the each specific protocol features. Ticket - 7111
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11679 +/- ##
==========================================
- Coverage 82.61% 82.61% -0.01%
==========================================
Files 919 919
Lines 248997 248992 -5
==========================================
- Hits 205717 205695 -22
- Misses 43280 43297 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I did not do a full review, but this looks good to me |
There was a problem hiding this comment.
Thanks for the work.
CI : ✅
Code : good
Commits segmentation : ok
Commit messages : thanks for the improvement
Git ID set : looks fine for me
CLA : you already contributed
Doc update : not needed
Redmine ticket : ok
Rustfmt : looks ok
Tests : nice, thanks
Dependencies added: none
|
@inashivb can you review the DCERPC change? |
|
Commits guideline is here https://docs.suricata.io/en/latest/devguide/contributing/code-submission-process.html#commits |
| let is_request = hdr.pkt_type == 0x00; | ||
| let is_dcerpc = hdr.rpc_vers == 0x04 && | ||
| hdr.fragnum == 0 && | ||
| leftover_bytes.len() >= hdr.fraglen as usize && |
There was a problem hiding this comment.
This looks good. This alone seems good enough to detect your test pcap correctly.
In addition to this, following wireshark's detection logic, possible header field to make probing stronger that isn't already covered:
pkt_type must be <= CANCEL_ACK (10) [this update alone fails the detection test on your s-v pcap though]
lmk wdyt?
There was a problem hiding this comment.
There seem to be more cf https://github.com/secdev/scapy/blob/master/scapy/layers/dcerpc.py#L138
10: "cancel_ack",
11: "bind",
12: "bind_ack",
13: "bind_nak",
14: "alter_context",
15: "alter_context_resp",
16: "auth3",
17: "shutdown",
18: "co_cancel",
19: "orphaned",
There was a problem hiding this comment.
Indeed there are. That condition check was used by wireshark for the detection so only the first pkt I think (probing). All the other types you mentioned are after a connection is established and some of these are even handled by our dcerpc parser like bind + bindack. This is why I thought of suggesting it.
There was a problem hiding this comment.
I would go without it in case we probe midstream...
|
friendly ping @ilya-bakhtin will you finish this work ? |
|
Yes, i will complete it most likely tomorrow (19th of Nov) |
|
Closing as replaced by #12134 |
Make sure these boxes are signed before submitting your Pull Request -- thank you.
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
(if applicable)
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7111
Describe changes:
This is a replacement of #11644
There are 2 commits.
The first one is intended to improve DCERPC UDP detection. False positives resulted in improper work of the detection framework.
The second commit simplifies the detection framework function AppLayerProtoDetectGetProto.
It previously contained a bug that combined with a false positive in DCERPC resulted in incorrect reporting of DNS flow direction.
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_REPO=
SV_BRANCH=OISF/suricata-verify#2008
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=