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: log app-layer metadata in alert with single tx #12158

Closed

Conversation

catenacyber
Copy link
Contributor

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

May also solve https://redmine.openinfosecfoundation.org/issues/7406 and https://redmine.openinfosecfoundation.org/issues/7350

Describe changes:

  • detect: log app-layer metadata in alert with single tx

SV_BRANCH=OISF/suricata-verify#2141

Is this the right way to solve most of the cases as I remember discussing with someone ?

#12153 with unit test fix by checking the packet has a flow with app-layer state

Ticket: 7199

When there is a single transaction, we cannot pick a wrong
transaction to log, even if the rule does not use app-layer
keywords.
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.84%. Comparing base (bd7d38e) to head (9cdbb91).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12158      +/-   ##
==========================================
+ Coverage   49.81%   49.84%   +0.03%     
==========================================
  Files         909      909              
  Lines      257904   257905       +1     
==========================================
+ Hits       128467   128557      +90     
+ Misses     129437   129348      -89     
Flag Coverage Δ
fuzzcorpus 61.03% <100.00%> (+0.08%) ⬆️
livemode 19.43% <100.00%> (+<0.01%) ⬆️
pcap 44.44% <100.00%> (+0.03%) ⬆️
suricata-verify 62.75% <100.00%> (+<0.01%) ⬆️
unittests 8.98% <0.00%> (-0.01%) ⬇️

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

@tugtugtug
Copy link

@catenacyber as I replied to https://redmine.openinfosecfoundation.org/issues/7406?issue_count=370&issue_position=4&next_issue_id=7398&prev_issue_id=7407, I think it is a lot more readable with the
if (pflow && pflow->alstate) in the outer if, so,

        if (pflow && pflow->alstate) {
            if ((alert_flags & PACKET_ALERT_FLAG_STREAM_MATCH) ||
                (s->alproto != ALPROTO_UNKNOWN && pflow->proto == IPPROTO_UDP) ||
                (AppLayerParserGetTxCnt(pflow, pflow->alstate) == 1))
            {
                // if there is a stream match (TCP), or
                // a UDP specific app-layer signature,
                // or only one transaction
                // try to use the good tx for the packet direction
                ....
            }
        }

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23573

@catenacyber
Copy link
Contributor Author

Right, I have an even better fix coming in #12161 removing the second later check to pflow->alstate

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