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

Backports/708/v3 #12267

Closed
wants to merge 3 commits into from
Closed

Conversation

victorjulien
Copy link
Member

#12263, with 5d22aa4

catenacyber and others added 3 commits December 11, 2024 12:08
Ticket: 7199

Uses a config parameter detect.guess-applayer-tx to enable
this behavior (off by default)

This feature is requested for use cases with signatures not
using app-layer keywords but still targetting application
layer transactions, such as pass/drop rule combination,
or lua usage.

This overrides the previous behavior of checking if the signature
has a content match, by checking if there is only one live
transaction, in addition to the config parameter being set.

(cherry picked from commit f2c3776)
The `guess-applayer-tx` work also removed the stream match condition
for adding app-layer metadata to alerts. This is a behavior change that
is not desired at this point, so this commit reverts that part of the
changes.

We keep the exising logging of app-layer metadata if the match was in
the stream.
@victorjulien victorjulien requested review from jufajardini and a team as code owners December 11, 2024 20:24
@victorjulien victorjulien mentioned this pull request Dec 11, 2024
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.18%. Comparing base (9be2eca) to head (a293a7c).

Additional details and impacted files
@@              Coverage Diff               @@
##           main-7.0.x   #12267      +/-   ##
==============================================
- Coverage       83.19%   83.18%   -0.02%     
==============================================
  Files             922      922              
  Lines          260888   260898      +10     
==============================================
- Hits           217048   217026      -22     
- Misses          43840    43872      +32     
Flag Coverage Δ
fuzzcorpus 64.19% <87.50%> (+<0.01%) ⬆️
suricata-verify 63.37% <100.00%> (-0.02%) ⬇️
unittests 62.38% <56.25%> (+<0.01%) ⬆️

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

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

Original PR ok for me, plus added doc commit ok for me

picked up.** This is to reduce the chances of logging unrelated data, and may
lead to alerts being logged without metadata, in some cases.
The alert event will have ``tx_guessed: true`` to recognize
such alerts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add there that we also log some transaction metadata for rule with stream matches ?
And that the first transaction is logged, like one TCP packet with 3 DNS requests to suricata.io oisf.net and suricon.net and rule alert tcp any any -> any any (content: "suricon"; sid: 1) will log the request to suricata.io

Copy link
Member

Choose a reason for hiding this comment

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

Seems all too much of a low level implementation detail to discuss in configuration docs. But these types of issues are making me think we need a know issues and shortcomings chapter.

Comment on lines +1680 to +1683
# try to tie an app-layer transaction for rules without app-layer keywords
# if there is only one live transaction for the flow
# allows to log app-layer metadata in alert
# but the transaction may not be the relevant one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Tried to add a bit more here (from https://github.com/OISF/suricata/pull/12260/files):

Suggested change
# try to tie an app-layer transaction for rules without app-layer keywords
# if there is only one live transaction for the flow
# allows to log app-layer metadata in alert
# but the transaction may not be the relevant one.
# Try to guess an app-layer transaction for rules without app-layer keywords,
# ONLY IF there is just one live transaction for the flow.
# This allows logging app-layer metadata in alert - the transaction may not
# be the relevant one for the alert.

Copy link
Member

Choose a reason for hiding this comment

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

Tried to add a bit more here (from https://github.com/OISF/suricata/pull/12260/files):

On suggestion...

ONLY IF there is a single live transaction for the flow,

IMO, can be cleaned up in post-release.

@jasonish
Copy link
Member

Should pass with:

SV_BRANCH=OISF/suricata-verify#2179

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

@victorjulien victorjulien mentioned this pull request Dec 11, 2024
@victorjulien
Copy link
Member Author

replaced by #12268

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.

4 participants