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.

Sign up for GitHub

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

Jump to bottom

Fixed syslog false positives. #1577

Merged
merged 1 commit into from
Jun 3, 2022
Merged

Fixed syslog false positives. #1577

merged 1 commit into from
Jun 3, 2022

Conversation

utoni
Copy link
Collaborator

@utoni utoni commented Jun 3, 2022

  • syslog: removed unnecessary/unreliable printable string check
  • added ndpi_isalnum()
  • splitted ndpi_is_printable_string() into ndpi_is_printable_buffer() and ndpi_normalize_printable_string()

Signed-off-by: lns matzeton@googlemail.com

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2022

This pull request introduces 1 alert when merging 5df615d into 6149c0f - view on LGTM.com

new alerts:

  • 1 for Comparison of narrow type with wide type in loop condition

@utoni utoni force-pushed the fix/syslog-false-positive branch from 5df615d to 434731f Compare June 3, 2022 14:39
@IvanNardi
Copy link
Collaborator

Do you have any example of false positive with the old code?
AFAIK, all the flows in the removed syslog.pcapng file were real syslog messages.
BTW, with the new code and the new syslog.pcap 2 sessions are classified only "by-port", even if it seems they are real syslog flows

utoni
utoni commented Jun 3, 2022
View reviewed changes
src/include/ndpi_utils.h Outdated Show resolved Hide resolved
@utoni utoni force-pushed the fix/syslog-false-positive branch from 434731f to c058026 Compare June 3, 2022 15:00
@utoni
Copy link
Collaborator Author

utoni commented Jun 3, 2022

Do you have any example of false positive with the old code? AFAIK, all the flows in the removed syslog.pcapng file were real syslog messages. BTW, with the new code and the new syslog.pcap 2 sessions are classified only "by-port", even if it seems they are real syslog flows

I accidentally deleted the rsh false-positive unfortunately. 👎
syslog.pcap is a merge of the old syslog.pcapng and some newer flows.

@IvanNardi
Copy link
Collaborator

This is an example of a flow classified "by-dpi" with the old code and "by-port" with the new one.
syslog.zip
Could you take a look, please?

@utoni
Fixed syslog false positives.
b836674 
 * syslog: removed unnecessary/unreliable printable string check
 * added `ndpi_isalnum()`
 * splitted `ndpi_is_printable_string()` into `ndpi_is_printable_buffer()` and `ndpi_normalize_printable_string()`

Signed-off-by: lns <matzeton@googlemail.com>
@utoni utoni force-pushed the fix/syslog-false-positive branch from c058026 to b836674 Compare June 3, 2022 15:16
@sonarcloud
Copy link

sonarcloud bot commented Jun 3, 2022

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

@IvanNardi
Copy link
Collaborator

This is an example of a flow classified "by-dpi" with the old code and "by-port" with the new one. syslog.zip Could you take a look, please?

Thanks!

@IvanNardi IvanNardi merged commit 09fbe0a into ntop:dev Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanNardi IvanNardi IvanNardi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@utoni @IvanNardi