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

Remove special handling of some TCP flows without SYN #1965

Merged
merged 1 commit into from
May 9, 2023

Conversation

IvanNardi
Copy link
Collaborator

This piece of code has multiple problems:

  • nDPI is able to detect some TCP protocols even with mid-flows (i.e. without the initial packets of the session); TLS is the most significative example
  • since e6b332a it is perfectly valid to not pass the TCP Handshake packets to nDPI
  • in any case, we shouldn't call ndpi_detection_giveup(). That function is usually called by the application and we end up calling it twice in some cases.

The simple solution is to completely remove that code: process these kinds of flows like everyone else.

Note that the application can always avoid to pass to nDPI any TCP flows without the initial handshake; the flow managemnt is always up to the application.

Looking at the CI results, some rare flows are now processed significantly longer. As a follow-up we could look into that.

This piece of code has multiple problems:
* nDPI is able to detect some TCP protocols even with mid-flows (i.e.
without the initial packets of the session); TLS is the most
significative example
* since e6b332a it is perfectly valid
to not pass the TCP Handshake packets to nDPI
* in any case, we shouldn't call `ndpi_detection_giveup()`. That
function is usually called by the application and we end up calling it
twice in some cases.

The simple solution is to completely remove that code: process these
kinds of flows like everyone else.

Note that the application can always avoid to pass to nDPI any TCP flows
without the initial handshake; the flow managemnt is always up to the
application.

Looking at the CI results, some rare flows are now processed significantly
longer. As a follow-up we could look into that.
@sonarcloud
Copy link

sonarcloud bot commented May 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@utoni utoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange. Some flows require 20~30 more packets till classification.

@IvanNardi
Copy link
Collaborator Author

Strange. Some flows require 20~30 more packets till classification.

Yeah, I noticed (and reported it in the commit message).
Basically those flows pass from 1 pkt to the upper limit 33 (see NDPI_DEFAULT_MAX_NUM_PKTS_PER_FLOW_TO_DISSECT ;we process one more pkt than expected...)
Where you see 20, it is basically because the flow is made by only 20 pkts...

Patches will be pushed soon to reduce those numbers

@IvanNardi IvanNardi merged commit 99d7066 into ntop:dev May 9, 2023
@IvanNardi IvanNardi deleted the tcp-syn-giveup branch May 9, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants