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

Engine mode fixes v1 #8681

Closed

Conversation

lukashino
Copy link
Contributor

PR is a collection of:

PR fixes determination of engine mode. The main problem is that some parts of Suricata queries the state of the engine mode before it is determined. They are given the default engine mode state (IDS) while later Suricata determined IPS behavior. This then led to unwanted behavior in IPS mode.

BPF contained this issue but hasn't really affected Suricata. However, the policy settings did affect Suricata run in IPS mode. This has been tested in PRs #8668 and #8671. #8687 has moved the policy setting only after DeviceFinalization but still before engine mode evaluation. This PR has not triggered different behavior in the pipeline run. #8668 has moved the policy setting after the engine mode evaluation and the behavior of Suricata in IPS mode changed. The bug is also highlighted with the addition of unknown/uninitialized engine mode in combination with the BUG_ON macro.

IPS behavior deviation is expected and I suggest baseline adjustment.

Lukas Sismis added 3 commits April 4, 2023 21:19
Querying an engine mode with an unknown value signals a bug when
the engine mode has not been determined but is already queried by
other functions.

Ticket: OISF#5959
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #8681 (0b0e16e) into master (035863d) will decrease coverage by 0.01%.
The diff coverage is 36.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8681      +/-   ##
==========================================
- Coverage   81.87%   81.87%   -0.01%     
==========================================
  Files         968      968              
  Lines      279014   279014              
==========================================
- Hits       228449   228447       -2     
- Misses      50565    50567       +2     
Flag Coverage Δ
fuzzcorpus 64.29% <33.33%> (+<0.01%) ⬆️
suricata-verify 59.79% <33.33%> (-0.01%) ⬇️
unittests 63.23% <5.26%> (-0.01%) ⬇️

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

@suricata-qa
Copy link

Information:

ERROR: QA failed on IPS_AFP_drop_chk.

field baseline test %
IPS_AFP_stats_chk
.flow.end.state.established 550799 583199 105.88%
.flow.end.tcp_state.established 169560 201960 119.11%

Pipeline 13021

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

The commits are a bit low on explanation. The PR message is not becoming part of the git history, so please make sure to move the explanations from there into the relevant commits.

}
}
}
}

if (pfconf->bpf_filter != NULL && EngineModeIsIPS()) {
Copy link
Member

Choose a reason for hiding this comment

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

we don't support IPS for pfring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the bpf_filter test from the condition but left the IPS test to stop user from running Suricata with PF_RING and IPS mode.

src/suricata.c Show resolved Hide resolved
@@ -2671,6 +2682,11 @@ int PostConfLoadedSetup(SCInstance *suri)
RunModeEngineIsIPS(
suricata.run_mode, suricata.runmode_custom_mode, suricata.capture_plugin_name);

if (EngineModeIsUnknown()) { // if still uninitialized the set the default
Copy link
Member

Choose a reason for hiding this comment

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

Is this really a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is meant to ensure the default engine mode is set when it hasn't been set until now.

But the whole PR is about making sure that Suricata modules don't query engine mode while uninitialized.
If it is not initialized by other Suricata ways (e.g. config, command-line parameter) then set the default. But after this line, the engine mode (should) never change.

src/runmode-netmap.c Show resolved Hide resolved
@lukashino
Copy link
Contributor Author

Continues in #8684

@lukashino lukashino closed this Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants