Dcerpc incomplete 5699/v12 #12025
Closed
Dcerpc incomplete 5699/v12 #12025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TCP data can be presented to the protocol parser in any way e.g. one byte at a time, single complete PDU, fragmented PDU, multiple PDUs at once. A limit of 1MB can be easily reached in some of such scenarios. Remove the check that rejects data that is more than 1MB.
to make it available for logging.
Instead of own internal mechanism of buffering in case of fragmented data, use AppLayerResult::incomplete API to let the AppLayer Parser take care of it. This makes the memory use more efficient. Remove any unneeded variables and code with the introduction of this API. Ticket 5699
With the introduction of AppLayerResult::incomplete API, fragmented data is no longer handled fully in the dcerpc code. Given that these code paths are already covered by the following s-v tests, these tests can now be safely removed. - dce-gap-handling - dcerpc-dce-iface-* Ticket 5699
- remove unneeded variables - remove unnecessary tracking of bytes in state - modify calculations as indicated by failing tests
Test AppLayerTest08 has an invalid DCERPC request in many ways. - header type is invalid - fragment length is invalid With the new changes of using the applayer::incomplete API in dcerpc parser, this test failed due to dcerpc parser asking for more data which does not lead to any condition that would ask for disabling applayer. Applayer can be disabled from two code paths: 1. StreamTcpDisableAppLayer 2. StreamTcpSetSessionNoReassemblyFlag These fns are called in the cases of: - applayer parser not requiring inspection - applayer parser asking to not reassemble data - stream depth having been reached - errors encountered by applayer during parsing Since none of the these conditions are the aim of the given test, adjust the byte array to actually contain valid data that goes through all the conditions as expected in the FAIL/PASS macros.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12025 +/- ##
==========================================
- Coverage 83.24% 83.24% -0.01%
==========================================
Files 910 910
Lines 258136 257861 -275
==========================================
- Hits 214895 214665 -230
+ Misses 43241 43196 -45
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 23172 |
|
Needs investigation of the stats difference and unittest converted to s-v |
I would retry running QA to see if it persists |
|
Did you forget to use here OISF/suricata-verify#2127 ? |
Not really. I did that s-v test much later. That s-v test needs fixing and then I'll remove that unittest in this PR |
|
Ok, could you remind us the order of PRs in your DCERPC rabbit hole ? frames, incomplete, unit test, etc... |
yes. 😭
|
|
Good luck on this 3-level rabbit hole :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5699
Previous PR: #11848
Changes since v11: