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

detect/http: fix progress for headers keywords #12056

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • detect/http: fix progress for headers keywords

SV_BRANCH=OISF/suricata-verify#2094

Should we have the generic fix of DetectAppLayerMultiRegister using only one tx progress ?

#11977 with yet longer improved commit message

Ticket: 7326

Having a lower progress than one where we actually can get
occurences of the multibuffer made prefilter
bail out too early, not having found a buffer in the multi-buffer
that matiched the prefilter.

For example, we registered http_request_header with progress 0
instad of progress HTP_REQUEST_HEADERS==2, and if the first
packet had only the request line, we would consider
that signatures with http_request_header as prefilter/fast_pattern
could not match for this transaction, even if they in fact
could have a later packet with matching headers.

Hence, we got false negatives, if http.request_header or
http.response_header was used as fast pattern, and if the request
or response came in multiple packets, and the first of these packets
did not have enough data (like only http request line),
and the next packets did have the matching data.
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.41%. Comparing base (3a7eef8) to head (2bc4004).
Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12056      +/-   ##
==========================================
- Coverage   83.42%   83.41%   -0.02%     
==========================================
  Files         910      910              
  Lines      257642   257642              
==========================================
- Hits       214949   214913      -36     
- Misses      42693    42729      +36     
Flag Coverage Δ
fuzzcorpus 61.56% <100.00%> (-0.09%) ⬇️
livemode 19.41% <100.00%> (-0.01%) ⬇️
pcap 44.47% <100.00%> (-0.02%) ⬇️
suricata-verify 62.78% <100.00%> (+0.02%) ⬆️
unittests 59.37% <100.00%> (ø)

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23206

@victorjulien victorjulien added this to the 8.0 milestone Nov 4, 2024
@catenacyber
Copy link
Contributor Author

@victorjulien Should we have the generic fix of DetectAppLayerMultiRegister using only one tx progress ?

@victorjulien
Copy link
Member

@victorjulien Should we have the generic fix of DetectAppLayerMultiRegister using only one tx progress ?

I don't understand the question. Can you explain more?

@catenacyber
Copy link
Contributor Author

The prototype is like
void DetectAppLayerMultiRegister(..., int progress, ..., int tx_min_progress);

progress is used by inspect logic AppLayerInspectEngineRegisterInternal and tx_min_progress is used by prefilter logic DetectAppLayerMpmMultiRegister

Should we change the prototype of DetectAppLayerMultiRegister so that it uses only one same tx progress for both prefilter and inspect logic ?

@victorjulien
Copy link
Member

The prototype is like void DetectAppLayerMultiRegister(..., int progress, ..., int tx_min_progress);

progress is used by inspect logic AppLayerInspectEngineRegisterInternal and tx_min_progress is used by prefilter logic DetectAppLayerMpmMultiRegister

Should we change the prototype of DetectAppLayerMultiRegister so that it uses only one same tx progress for both prefilter and inspect logic ?

Hmmm I don't know w/o studying the code more. I guess it can be investigated.

@victorjulien victorjulien removed this from the 8.0 milestone Nov 5, 2024
@catenacyber
Copy link
Contributor Author

Hmmm I don't know w/o studying the code more. I guess it can be investigated.

I don't know after having studied the code.
As a matter of facts, I did not anticipate this bug (setting too early progress for prefilter causes FN)...

@victorjulien victorjulien added this to the 8.0 milestone Nov 5, 2024
@victorjulien
Copy link
Member

Merged in #12088, thanks!

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.

3 participants