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

Pr 8341 and payload len fix/v2 #8393

Closed
wants to merge 5 commits into from

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Jan 18, 2023

Link to redmine tickets:
https://redmine.openinfosecfoundation.org/issues/5379
https://redmine.openinfosecfoundation.org/issues/5693

Previous PR: #8342

Changes since v1:

  • add new event type udp.len_invalid

suricata-verify-pr: 1055

Lukas Sismis and others added 5 commits January 18, 2023 06:30
If the packet is shorter than IP payload length we no longer flag it as an
invalid UDP packet. UDP packet can be therefore shorter than IP payload.

Redmine ticket: OISF#5693
Fix payload_len calculation post removal of the condition that returned
error code if the length to the decode fn did not match the length of
header from the UDP packet.

Bug 5379
@inashivb inashivb force-pushed the pr-8341-and-payload-len-fix/v2 branch from db79144 to 28a2c70 Compare January 18, 2023 01:45
@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #8393 (28a2c70) into master (a24d7dc) will decrease coverage by 0.02%.
The diff coverage is 66.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8393      +/-   ##
==========================================
- Coverage   81.94%   81.93%   -0.02%     
==========================================
  Files         963      963              
  Lines      277683   277686       +3     
==========================================
- Hits       227561   227530      -31     
- Misses      50122    50156      +34     
Flag Coverage Δ
fuzzcorpus 64.13% <66.66%> (+<0.01%) ⬆️
suricata-verify 59.65% <50.00%> (-0.04%) ⬇️
unittests 63.60% <50.00%> (+<0.01%) ⬆️

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

@inashivb inashivb marked this pull request as ready for review January 18, 2023 02:23
@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW1_stats_chk
.app_layer.error.tls.parser 931 1145 122.99%
SURI_TLPR1_stats_chk
.app_layer.flow.http 3118951 3340799 107.11%
.app_layer.flow.tls 510462 623355 122.12%
.app_layer.flow.ssh 8937 11032 123.44%
.app_layer.flow.telnet 1624 2334 143.72%
TREX_GENERIC_stats_chk
.capture.kernel_drops 0 207683 0.04

Pipeline 11673

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW1_stats_chk
.app_layer.error.tls.parser 931 1145 122.99%
SURI_TLPR1_stats_chk
.app_layer.flow.http 3118951 3340798 107.11%
.app_layer.flow.tls 510462 623355 122.12%
.app_layer.flow.ssh 8937 11032 123.44%
.app_layer.flow.telnet 1624 2334 143.72%
.app_layer.error.ftp-data.gap 0 1 -
TREX_GENERIC_stats_chk
.capture.kernel_drops 0 3859 0.00

Pipeline 11677

rules/decoder-events.rules Show resolved Hide resolved
@@ -160,6 +160,9 @@ static DetectEngineEventData *DetectEngineEventParse (const char *rawstr)

if (de->event == STREAM_REASSEMBLY_OVERLAP_DIFFERENT_DATA) {
StreamTcpReassembleConfigEnableOverlapCheck();
} else if (de->event == UDP_HLEN_INVALID) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be implemented using the strict concept as done here: https://github.com/OISF/suricata/blob/master/src/detect-app-layer-event.c#L201

cc @lukashino

Copy link
Member

Choose a reason for hiding this comment

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

@jufajardini this strict concept should make it to the dev guide :)

Copy link
Contributor

Choose a reason for hiding this comment

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

followed the concept and created OutdatedEvent function in decode part of the engine.

@victorjulien
Copy link
Member

replaced by #8404

@inashivb inashivb deleted the pr-8341-and-payload-len-fix/v2 branch January 19, 2023 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants