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

ftp: do not set alproto if one was already found #6651

Closed
wants to merge 1 commit into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4857

Describe changes:

  • Fixes segfault caused by using a known protocol detection pattern in a file transferred over FTP-DATA

Does not solve the fact that the flow should have been detected as FTP-DATA in the first place

Ticket: 4857

If a pattern such as GET is seen ine the beginning of the
file transferred over ftp-data, this flow will get recognized
as HTTP, and a HTTP state will be created during parsing.

Thus, we cannot override directly alproto's values

This solves the segfault, but not the logical bug that the flow
should be classified as FTP-DATA instead of HTTP
@catenacyber catenacyber requested a review from a team as a code owner November 29, 2021 10:05
@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #6651 (71489d0) into master (c9d222a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6651      +/-   ##
==========================================
+ Coverage   77.19%   77.24%   +0.04%     
==========================================
  Files         613      613              
  Lines      185646   185648       +2     
==========================================
+ Hits       143310   143398      +88     
+ Misses      42336    42250      -86     
Flag Coverage Δ
fuzzcorpus 53.30% <100.00%> (+0.10%) ⬆️
suricata-verify 52.14% <100.00%> (-0.03%) ⬇️
unittests 63.09% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

ERROR: QA failed on tlpw2_single_alerts_cmp.

Pipeline 5097

@catenacyber
Copy link
Contributor Author

ERROR: QA failed on tlpw2_single_alerts_cmp.

Pipeline 5097

1 major deviations in alert counts
"2820835:4"
base 3 vs test 2

Looks like it happens regularly...
Should we exclude off by one even if this means 33 % ? @ct0br0

@victorjulien victorjulien mentioned this pull request Dec 13, 2021
@victorjulien
Copy link
Member

Merged in #6705, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants