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: Ensure app-layer decode events are logged #6710

Closed
wants to merge 9 commits into from

Conversation

jlucovsky
Copy link
Contributor

Continuation of #6709

This PR ensures that app-layer decode events are logged when anomaly logging is enabled.

Link to redmine ticket: 4898

Updates

  • Address commit-order build failure.

Describe changes:

  • Move un-exposed events to app-layer events table
  • Propagate app-layer decode events to packet
  • Eliminates detection context event as events are now added to decoder event element of packet.

suricata-verify-pr: 589
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch:

This commit moves events for file decode and buffer count limits to a
centralized location for easier handling.

Issue: OISF#4482
This commit increases the duration of availability for packet pointer
within the detect context.

Issue: OISF#4482
This commit includes the packet decoder events.

Issue: OISF#4482
This commit records the app-layer decode event for anomaly reporting.

Issue: OISF#4482
Ticket: OISF#4898

This commit adds a brief description of tags used for events that occur
during protocol layer decoding, e.g., SWF file handling.
@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #6710 (4b917c3) into master (e93dc24) will increase coverage by 0.08%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #6710      +/-   ##
==========================================
+ Coverage   77.12%   77.20%   +0.08%     
==========================================
  Files         613      613              
  Lines      185666   185671       +5     
==========================================
+ Hits       143194   143350     +156     
+ Misses      42472    42321     -151     
Flag Coverage Δ
fuzzcorpus 53.08% <50.00%> (+0.07%) ⬆️
suricata-verify 52.73% <100.00%> (+0.59%) ⬆️
unittests 63.08% <50.00%> (+<0.01%) ⬆️

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

@@ -408,7 +408,9 @@ PacketCreateMask(Packet *p, SignatureMask *mask, AppProto alproto,
(*mask) |= SIG_MASK_REQUIRE_NO_PAYLOAD;
}

if (p->events.cnt > 0 || app_decoder_events != 0 || p->app_layer_events != NULL) {
if (p->events.cnt > 0 || app_decoder_events ||
(p->app_layer_events != NULL && p->app_layer_events->cnt) ||
Copy link
Member

Choose a reason for hiding this comment

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

wonder if we should set a flag in Packet::flags on setting an event in either of these, to avoid the overhead of these checks (that also happen in logging I think)
PKT_HAS_APP_EVENTS or something like it

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes -- good idea.

@suricata-qa
Copy link

Information:

field test baseline %
tlpr1_stats_chk
.flow.memuse 543533888 505916608 107.44%

Pipeline 5292

@catenacyber catenacyber added the needs rebase Needs rebase to master label Mar 10, 2022
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.

See comment by Victor + needed rebase ;-)

@catenacyber
Copy link
Contributor

(just adding the review for GitHub status filleting)

@suricata-qa suricata-qa added the needs ticket Needs (link to) redmine ticket label Apr 26, 2022
@suricata-qa
Copy link

Note: please make sure all tickets in the commits are mentioned in the PR:

Please update PR body and remove the 'needs ticket' label when done.

@suricata-qa
Copy link

Information:

field test baseline %
tlpr1_stats_chk
.flow.memuse 543533888 505916608 107.44%

Pipeline 5292

@catenacyber catenacyber closed this Jul 6, 2022
jasonish added a commit to jasonish/suricata that referenced this pull request Jan 24, 2024
"sigerror_ok" and "sigerror_requires" were not being reset after each
rule which could lead to a rule load error being incorrectly tracked
as skipped rather than failed.

Also initialize "skippedsigs" to 0 along with "goodsigs" and
"badsigs", while not directly related to this issue, could also throw
off some stats.

Ticket: OISF#6710
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Jan 25, 2024
"sigerror_ok" and "sigerror_requires" were not being reset after each
rule which could lead to a rule load error being incorrectly tracked
as skipped rather than failed.

Also initialize "skippedsigs" to 0 along with "goodsigs" and
"badsigs", while not directly related to this issue, could also throw
off some stats.

Ticket: OISF#6710
(cherry picked from commit de3cbe4)
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Jan 25, 2024
"sigerror_ok" and "sigerror_requires" were not being reset after each
rule which could lead to a rule load error being incorrectly tracked
as skipped rather than failed.

Also initialize "skippedsigs" to 0 along with "goodsigs" and
"badsigs", while not directly related to this issue, could also throw
off some stats.

Ticket: OISF#6710
(cherry picked from commit de3cbe4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master needs ticket Needs (link to) redmine ticket
Development

Successfully merging this pull request may close these issues.

4 participants