Closed
Conversation
The parser could receive an input that consists of arbitrary data post gap. This is handled in the beginning of the fn handle_input_data. However, the rest of the calculation does not take into account the bytes that were consumed at this stage. Fix the indices and calculations to consider a new DCERPC fragment beginning post these consumed bytes.
So far, the fraglen defined in the header was used inconsistently in certain places to define bounds on input length. Make it consistent by making sure that only a slice up until fraglen is passed around as that is the maximum length the fragment should have. With the help of Applayer::incomplete API, the case when the stream_slice passed to the parser is smaller than the header defined fraglen is already handled. Bug 7546
Unittests test_parse_bind_pdu_infinite_loop and test_parse_bindack_pdu_infinite_loop seem to have artificially made up header which does not hold up to the strict calculations enforced by the parser now. Their headers mark the fraglens as 64 and 72 respectively which are not enough to hold the kind of bind(ack) items that are expected. It worked so far as the parser passed the entire input slice around but with the bugfix for issue 7546, the input passed around is strictly restricted to the fraglen parsed in the header. Bug 7546
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14733 +/- ##
==========================================
- Coverage 82.17% 82.16% -0.02%
==========================================
Files 1008 1008
Lines 263916 263893 -23
==========================================
- Hits 216868 216817 -51
- Misses 47048 47076 +28
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
catenacyber
reviewed
Feb 2, 2026
catenacyber
reviewed
Feb 2, 2026
Contributor
|
Should there be SV tests ? |
|
Information: QA ran without warnings. Pipeline = 29357 |
Member
Author
|
Is this ok for you then, @catenacyber ? |
victorjulien
approved these changes
Feb 4, 2026
Merged
Contributor
If it is ok for Victor, this is ok for me ;-) |
Member
|
Merged in #14750, thanks! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 ticket: https://redmine.openinfosecfoundation.org/issues/7546
Previous PR: #14731
Changes since v4: