protodetect: improve DCERPC UDP probing parser and simplify app-layer-detect-proto.c code #11541
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11541 +/- ##
==========================================
- Coverage 82.51% 82.48% -0.03%
==========================================
Files 923 923
Lines 248732 248727 -5
==========================================
- Hits 205232 205170 -62
- Misses 43500 43557 +57
Flags with carried forward coverage won't be shown. Click here to find out more. |
| 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.
Why this condition leftover_bytes.len() >= hdr.fraglen as usize ?
There was a problem hiding this comment.
Please see the conventional parser code at rust/src/dcerpc/dcerpc_udp.rs:207
The probe must do at least the same since the DCERPC PM is extremely short.
There was a problem hiding this comment.
Ok indeed, we are on UDP here :-)
| } | ||
| } | ||
|
|
||
| /* Look if flow can be found in expectation list */ | ||
| if (!FLOW_IS_PE_DONE(f, flags)) { | ||
| *reverse_flow = false; /* to be safe if it was changed by PP */ |
There was a problem hiding this comment.
Not sure about this safety check... not sure it is needed...
Maybe we should rather have a DEBUG_VALIDATION like DEBUG_VALIDATE_BUG_ON(*reverse_flow); // should not be set to true, otherwise we returned a valid alproto already
There was a problem hiding this comment.
i'll inspect if the reversed flag is never set by the previous kind of the proto detection in the case of an unknown/failed result
There was a problem hiding this comment.
I've made an inspection of the corresponding code. Direction may be reported reversed only in a case of valid protocol returned. So the "to be safe" lines are not necessary. Now i believe even debug validation is not necessary.
At the same time i found a case when incorrect direction may be technically reported by the PM.
It's not in the scope of the current work. And most likely the case is not possible whith the currently registered set of protocol detection patterns. I can not provide a reproducer. The following is a result of code review only.
The PM technically detects a set of protos but only first one is used. Let's see at app-layer-detect-proto.c from line 1407
uint16_t pm_matches = AppLayerProtoDetectPMGetProto(
tctx, f, buf, buflen, flags, pm_results, reverse_flow);
if (pm_matches > 0) {
DEBUG_VALIDATE_BUG_ON(pm_matches > 1);
alproto = pm_results[0];
DEBUG_VALIDATE_BUG_ON exists but the whole part of code looks confusing. Why do we use an array of matches when only first is expected? And i believe without debug a combination of patterns is possible when more than one is returned and some of the following damaged the 'reverse_flow'.
It looks a bit confusing why PMGetProtoInspect tries to traverse all patterns (from line 294 of app-layer-detect-proto.c) while its client expects the one and only. And it has even DEBUG_VALIDATE_BUG_ON check.
There was a problem hiding this comment.
Thanks for the investigation
Now i believe even debug validation is not necessary.
It may be useful in the future, if some new code tries to break this assertion
Why do we use an array of matches when only first is expected?
Because we use some generic code for multi-pattern matching. The debug validation is here so that fuzzing can find if there is one way to find mutliple protocols with patterns... which would lead likely to bad things...
There was a problem hiding this comment.
I see.
I prepared a version with the debug validation - #11644
Regarding the array of matches - I still don't quite understand why we do not break from the loop started at app-layer-detect-proto.c:294 when the pattern is found.
Well, I did not inspect the behavior fully and it's not in the scope of current change. Moreover, I hope the condition with multiple patterns detected is impossible with the current set of registered protocol detection patterns.
There was a problem hiding this comment.
Moreover, I hope the condition with multiple patterns detected is impossible with the current set of registered protocol detection patterns.
That is the important point indeed...
|
Also @ilya-bakhtin the SV PR needs some simple fix ;-) |
|
SV update is at OISF/suricata-verify#2008 |
|
Continued in #11644 |
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
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7111
Describe changes:
This is a replacement of #11100
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 which combined with 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#1989
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=