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

applayer: add tests for bug 7044 #1946

Closed
wants to merge 1 commit into from

Conversation

inashivb
Copy link
Member

Ticket

If your pull request is related to a Suricata ticket, please provide
the full URL to the ticket here so this pull request can monitor
changes to the ticket status:

Redmine ticket: https://redmine.openinfosecfoundation.org/issues/7044

Previous PR: #1936

Changes since v1:

  • dcerpc event was added
  • a test w/o stream depth bounds was added

@victorjulien
Copy link
Member

We'll need to understand the difference in behavior between 6 and 7, and determine which one is correct.

@inashivb
Copy link
Member Author

inashivb commented Jul 2, 2024

We'll need to understand the difference in behavior between 6 and 7, and determine which one is correct.

ok so 6 is def correct.
Following is the diff b/w 6 and 7/master when stream depth is reached:

6.0.x 7.0.x
parser marks the request as done as a part of the call to truncate fn. Triggers reassembly of raw data until this point and returns nothing happens on Rust side as dcerpc truncate fn is set to None making request never being marked as completed <- this var req_done is used by the logger to check whether a request should be logged

However, as mentioned by Philippe, things that the truncate fn does (marking the status of the direction we're in completed based on certain conditions like depth was reached or gaps were seen) are already done by generic fns like AppLayerParserGetStateProgress so we can get rid of these truncate fns while making sure logging does not depend on vars that needed to be updated from C applayer routines.

Conclusions:

  1. Based on other parsers, as long as a request exists, it should be logged. (This is how smb/dcerpc also does it)
  2. Local Rust variables like req_done should be sparingly used esp when the state that they represent could be updated from the generic applayer code. Otherwise, we need an extra communication step (like a call to the truncate fn) from C to Rust just to make sure logging happens right.

wdyt?

@inashivb
Copy link
Member Author

inashivb commented Jul 3, 2024

Tests and corresponding PR are being updated.

@inashivb inashivb closed this Jul 3, 2024
@inashivb inashivb deleted the truncate-test-7044/v2 branch July 3, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants