github/falco
1
0
Fork 0
mirror of https://github.com/falcosecurity/falco.git synced 2025-07-04 10:26:40 +00:00
Code Issues Projects Releases Wiki Activity

refactor: apply review suggestions

Browse Source 
Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
This commit is contained in:
Jason Dellaluce 2023-03-28 13:50:43 +00:00 committed by poiana
parent 3e0f0d3692
commit 2b93a79521
2 changed files with 60 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
unit_tests/falco/app/actions/test_configure_interesting_sets.cpp
View File

@ -23,7 +23,7 @@ limitations under the License.
#include <gtest/gtest.h> #include <gtest/gtest.h>
#define ASSERT_NAMES_EQ(a, b) { \ #define ASSERT_NAMES_EQ(a, b) { \
ASSERT_EQ(_order(a).size(), _order(b).size()); \ EXPECT_EQ(_order(a).size(), _order(b).size()); \
ASSERT_EQ(_order(a), _order(b)); \ ASSERT_EQ(_order(a), _order(b)); \
} }
@ -47,7 +47,7 @@ static std::string s_sample_ruleset = "sample-ruleset";
static std::string s_sample_source = falco_common::syscall_source; static std::string s_sample_source = falco_common::syscall_source;
static strset_t s_sample_filters = { static strset_t s_sample_filters = {
"evt.type=connect or evt.type=accept", "evt.type=connect or evt.type=accept or evt.type=accept4 or evt.type=umount2",
"evt.type in (open, ptrace, mmap, execve, read, container)", "evt.type in (open, ptrace, mmap, execve, read, container)",
"evt.type in (open, execve, mprotect) and not evt.type=mprotect"}; "evt.type in (open, execve, mprotect) and not evt.type=mprotect"};
@ -99,7 +99,7 @@ TEST(ConfigureInterestingSets, engine_codes_syscalls_set)
auto rules_event_set = engine->event_codes_for_ruleset(s_sample_source); auto rules_event_set = engine->event_codes_for_ruleset(s_sample_source);
auto rules_event_names = libsinsp::events::event_set_to_names(rules_event_set); auto rules_event_names = libsinsp::events::event_set_to_names(rules_event_set);
ASSERT_NAMES_EQ(rules_event_names, strset_t({ ASSERT_NAMES_EQ(rules_event_names, strset_t({
"connect", "accept", "open", "ptrace", "mmap", "execve", "read", "container"})); "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", "read", "container"}));
// test if sc code names were extracted from each rule in test ruleset. // test if sc code names were extracted from each rule in test ruleset.
// note, this is not supposed to contain "container", as that's an event // note, this is not supposed to contain "container", as that's an event
@ -107,7 +107,7 @@ TEST(ConfigureInterestingSets, engine_codes_syscalls_set)
auto rules_sc_set = engine->sc_codes_for_ruleset(s_sample_source); auto rules_sc_set = engine->sc_codes_for_ruleset(s_sample_source);
auto rules_sc_names = libsinsp::events::sc_set_to_names(rules_sc_set); auto rules_sc_names = libsinsp::events::sc_set_to_names(rules_sc_set);
ASSERT_NAMES_EQ(rules_sc_names, strset_t({ ASSERT_NAMES_EQ(rules_sc_names, strset_t({
"connect", "accept", "accept4", "open", "ptrace", "mmap", "execve", "read"})); "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", "read"}));
} }
TEST(ConfigureInterestingSets, preconditions_postconditions) TEST(ConfigureInterestingSets, preconditions_postconditions)
@ -158,7 +158,7 @@ TEST(ConfigureInterestingSets, engine_codes_nonsyscalls_set)
// This is a good example of information loss from ppm_event_code <-> ppm_sc_code. // This is a good example of information loss from ppm_event_code <-> ppm_sc_code.
auto generic_names = libsinsp::events::event_set_to_names({ppm_event_code::PPME_GENERIC_E}); auto generic_names = libsinsp::events::event_set_to_names({ppm_event_code::PPME_GENERIC_E});
auto expected_names = strset_t({ auto expected_names = strset_t({
"connect", "accept", "open", "ptrace", "mmap", "execve", "read", "container", // ruleset "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", "read", "container", // ruleset
"procexit", "switch", "pluginevent"}); // from non-syscall event filters "procexit", "switch", "pluginevent"}); // from non-syscall event filters
expected_names.insert(generic_names.begin(), generic_names.end()); expected_names.insert(generic_names.begin(), generic_names.end());
ASSERT_NAMES_EQ(rules_event_names, expected_names); ASSERT_NAMES_EQ(rules_event_names, expected_names);
@ -166,7 +166,7 @@ TEST(ConfigureInterestingSets, engine_codes_nonsyscalls_set)
auto rules_sc_set = engine->sc_codes_for_ruleset(s_sample_source); auto rules_sc_set = engine->sc_codes_for_ruleset(s_sample_source);
auto rules_sc_names = libsinsp::events::sc_set_to_names(rules_sc_set); auto rules_sc_names = libsinsp::events::sc_set_to_names(rules_sc_set);
ASSERT_NAMES_EQ(rules_sc_names, strset_t({ ASSERT_NAMES_EQ(rules_sc_names, strset_t({
"connect", "accept", "accept4", "open", "ptrace", "mmap", "execve", "read", "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", "read",
"syncfs", "fanotify_init", // from generic event filters "syncfs", "fanotify_init", // from generic event filters
})); }));
} }
@ -189,7 +189,7 @@ TEST(ConfigureInterestingSets, selection_not_allevents)
auto selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set); auto selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set);
auto expected_sc_names = strset_t({ auto expected_sc_names = strset_t({
// note: we expect the "read" syscall to have been erased // note: we expect the "read" syscall to have been erased
"connect", "accept", "open", "ptrace", "mmap", "execve", // from ruleset "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", // from ruleset
"clone", "clone3", "fork", "vfork", // from sinsp state set (spawned_process) "clone", "clone3", "fork", "vfork", // from sinsp state set (spawned_process)
"socket", "bind", "close" // from sinsp state set (network, files) "socket", "bind", "close" // from sinsp state set (network, files)
}); });
@ -232,7 +232,7 @@ TEST(ConfigureInterestingSets, selection_allevents)
auto selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set); auto selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set);
auto expected_sc_names = strset_t({ auto expected_sc_names = strset_t({
// note: we expect the "read" syscall to not be erased // note: we expect the "read" syscall to not be erased
"connect", "accept", "open", "ptrace", "mmap", "execve", "read", // from ruleset "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", "read", // from ruleset
"clone", "clone3", "fork", "vfork", // from sinsp state set (spawned_process) "clone", "clone3", "fork", "vfork", // from sinsp state set (spawned_process)
"socket", "bind", "close" // from sinsp state set (network, files) "socket", "bind", "close" // from sinsp state set (network, files)
}); });
@ -264,7 +264,7 @@ TEST(ConfigureInterestingSets, selection_generic_evts)
auto selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set); auto selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set);
auto expected_sc_names = strset_t({ auto expected_sc_names = strset_t({
// note: we expect the "read" syscall to not be erased // note: we expect the "read" syscall to not be erased
"connect", "accept", "open", "ptrace", "mmap", "execve", // from ruleset "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", // from ruleset
"syncfs", "fanotify_init", // from ruleset (generic events) "syncfs", "fanotify_init", // from ruleset (generic events)
"clone", "clone3", "fork", "vfork", // from sinsp state set (spawned_process) "clone", "clone3", "fork", "vfork", // from sinsp state set (spawned_process)
"socket", "bind", "close" // from sinsp state set (network, files) "socket", "bind", "close" // from sinsp state set (network, files)
@ -301,9 +301,10 @@ TEST(ConfigureInterestingSets, selection_custom_base_set)
// note: `accept` is not included even though it is matched by the rules, // note: `accept` is not included even though it is matched by the rules,
// which means that the custom negation base set has precedence over the // which means that the custom negation base set has precedence over the
// final selection set as a whole // final selection set as a whole
"connect", "open", "ptrace", "mmap", "execve", "read", "syncfs", "sched_process_exit" // todo(jasondellaluce): add "accept4" once names_to_sc_set is polished on the libs side
"connect", "umount2", "open", "ptrace", "mmap", "execve", "read", "syncfs", "sched_process_exit"
}); });
ASSERT_NAMES_CONTAIN(selected_sc_names, expected_sc_names); ASSERT_NAMES_EQ(selected_sc_names, expected_sc_names);
// non-empty custom base set (both positive and negative with collision) // non-empty custom base set (both positive and negative with collision)
s.config->m_base_syscalls_repair = false; s.config->m_base_syscalls_repair = false;
@ -314,7 +315,7 @@ TEST(ConfigureInterestingSets, selection_custom_base_set)
selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set); selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set);
// note: in case of collision, negation has priority, so the expected // note: in case of collision, negation has priority, so the expected
// names are the same as the case above // names are the same as the case above
ASSERT_NAMES_CONTAIN(selected_sc_names, expected_sc_names); ASSERT_NAMES_EQ(selected_sc_names, expected_sc_names);
// non-empty custom base set (only positive) // non-empty custom base set (only positive)
s.config->m_base_syscalls_custom_set = {"syncfs"}; s.config->m_base_syscalls_custom_set = {"syncfs"};
@ -324,9 +325,9 @@ TEST(ConfigureInterestingSets, selection_custom_base_set)
selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set); selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set);
expected_sc_names = strset_t({ expected_sc_names = strset_t({
// note: accept is not negated anymore // note: accept is not negated anymore
"connect", "accept", "open", "ptrace", "mmap", "execve", "read", "syncfs", "sched_process_exit" "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", "read", "syncfs", "sched_process_exit"
}); });
ASSERT_NAMES_CONTAIN(selected_sc_names, expected_sc_names); ASSERT_NAMES_EQ(selected_sc_names, expected_sc_names);
// non-empty custom base set (only negative) // non-empty custom base set (only negative)
s.config->m_base_syscalls_custom_set = {"!accept"}; s.config->m_base_syscalls_custom_set = {"!accept"};
@ -336,10 +337,11 @@ TEST(ConfigureInterestingSets, selection_custom_base_set)
selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set); selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set);
expected_sc_names = unordered_set_union( expected_sc_names = unordered_set_union(
libsinsp::events::sc_set_to_names(default_base_set), libsinsp::events::sc_set_to_names(default_base_set),
strset_t({ "connect", "open", "ptrace", "mmap", "execve", "read"})); strset_t({ "connect", "umount2", "open", "ptrace", "mmap", "execve", "read"}));
expected_sc_names.erase("accept"); expected_sc_names.erase("accept");
// todo(jasondellaluce): add "accept4" once names_to_sc_set is polished on the libs side
expected_sc_names.erase("accept4"); expected_sc_names.erase("accept4");
ASSERT_NAMES_CONTAIN(selected_sc_names, expected_sc_names); ASSERT_NAMES_EQ(selected_sc_names, expected_sc_names);
// non-empty custom base set (positive, without -A) // non-empty custom base set (positive, without -A)
s.options.all_events = false; s.options.all_events = false;
@ -351,9 +353,9 @@ TEST(ConfigureInterestingSets, selection_custom_base_set)
expected_sc_names = strset_t({ expected_sc_names = strset_t({
// note: read is both part of the custom base set and the rules set, // note: read is both part of the custom base set and the rules set,
// but we expect the unset -A option to take precedence // but we expect the unset -A option to take precedence
"connect", "accept", "open", "ptrace", "mmap", "execve", "sched_process_exit" "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", "sched_process_exit"
}); });
ASSERT_NAMES_CONTAIN(selected_sc_names, expected_sc_names); ASSERT_NAMES_EQ(selected_sc_names, expected_sc_names);
auto unexpected_sc_names = libsinsp::events::sc_set_to_names(libsinsp::events::io_sc_set()); auto unexpected_sc_names = libsinsp::events::sc_set_to_names(libsinsp::events::io_sc_set());
ASSERT_NAMES_NOCONTAIN(selected_sc_names, unexpected_sc_names); ASSERT_NAMES_NOCONTAIN(selected_sc_names, unexpected_sc_names);
} }
@ -365,8 +367,13 @@ TEST(ConfigureInterestingSets, selection_custom_base_set_repair)
s.options.all_events = false; s.options.all_events = false;
s.engine = mock_engine_from_filters(s_sample_filters); s.engine = mock_engine_from_filters(s_sample_filters);
// simulate empty custom set but repair option set // simulate empty custom set but repair option set.
s.config->m_base_syscalls_custom_set = {}; // note: here we use file syscalls (e.g. open, openat) and have a custom
// positive set, so we expect syscalls such as "close" to be selected as
// repaired. Also, given that we use some network syscalls, we expect "bind"
// to be selected event if we negate it, because repairment should have
// take precedence.
s.config->m_base_syscalls_custom_set = {"openat", "!bind"};
s.config->m_base_syscalls_repair = true; s.config->m_base_syscalls_repair = true;
auto result = falco::app::actions::configure_interesting_sets(s); auto result = falco::app::actions::configure_interesting_sets(s);
ASSERT_TRUE(result.success); ASSERT_TRUE(result.success);
@ -374,8 +381,8 @@ TEST(ConfigureInterestingSets, selection_custom_base_set_repair)
auto selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set); auto selected_sc_names = libsinsp::events::sc_set_to_names(s.selected_sc_set);
auto expected_sc_names = strset_t({ auto expected_sc_names = strset_t({
// note: expecting syscalls from mock rules and `sinsp_repair_state_sc_set` enforced syscalls // note: expecting syscalls from mock rules and `sinsp_repair_state_sc_set` enforced syscalls
"connect", "accept", "open", "ptrace", "mmap", "execve", "sched_process_exit", \ "connect", "accept", "accept4", "umount2", "open", "ptrace", "mmap", "execve", "sched_process_exit", \
"bind", "socket", "clone3", "setuid" "bind", "socket", "clone3", "close", "setuid"
}); });
ASSERT_NAMES_CONTAIN(selected_sc_names, expected_sc_names); ASSERT_NAMES_CONTAIN(selected_sc_names, expected_sc_names);
auto unexpected_sc_names = libsinsp::events::sc_set_to_names(libsinsp::events::io_sc_set()); auto unexpected_sc_names = libsinsp::events::sc_set_to_names(libsinsp::events::io_sc_set());

50
userspace/falco/app/actions/configure_interesting_sets.cpp
View File

@ -89,11 +89,9 @@ static void select_event_set(falco::app::state& s, const libsinsp::events::set<p
auto user_positive_sc_set = libsinsp::events::names_to_sc_set(user_positive_names); auto user_positive_sc_set = libsinsp::events::names_to_sc_set(user_positive_names);
auto user_negative_sc_set = libsinsp::events::names_to_sc_set(user_negative_names); auto user_negative_sc_set = libsinsp::events::names_to_sc_set(user_negative_names);
if (!user_positive_sc_set.empty() || s.config->m_base_syscalls_repair) if (!user_positive_sc_set.empty())
{ {
// user overrides base event set // user overrides base event set
// in case `user_positive_sc_set` is empty, but `base_syscalls.repair` is set
// this has the effect of clearing the default `sinsp_state_sc_set()`
base_sc_set = user_positive_sc_set; base_sc_set = user_positive_sc_set;
// we re-transform from sc_set to names to make // we re-transform from sc_set to names to make
@ -105,28 +103,20 @@ static void select_event_set(falco::app::state& s, const libsinsp::events::set<p
auto invalid_positive_sc_set_names = unordered_set_difference(user_positive_names, user_positive_sc_set_names); auto invalid_positive_sc_set_names = unordered_set_difference(user_positive_names, user_positive_sc_set_names);
if (!invalid_positive_sc_set_names.empty()) if (!invalid_positive_sc_set_names.empty())
{ {
std::cerr << "Invalid (positive) syscall names: warning (base_syscalls override): " + concat_set_in_order(invalid_positive_sc_set_names) << std::endl; falco_logger::log(LOG_WARNING, "Invalid (positive) syscall names: warning (base_syscalls override): "
+ concat_set_in_order(invalid_positive_sc_set_names));
} }
/* Hidden safety enforcement for `base_syscalls.custom_set` user input override option -> sched_process_exit trace point activation
* (procexit event) is necessary for continuous state engine cleanup, else memory would grow rapidly and linearly over time. */
base_sc_set.insert(PPM_SC_SCHED_PROCESS_EXIT);
} }
// selected events are the union of the rules events set and the // selected events are the union of the rules events set and the
// base events set (either the default or the user-defined one) // base events set (either the default or the user-defined one)
s.selected_sc_set = rules_sc_set.merge(base_sc_set); s.selected_sc_set = rules_sc_set.merge(base_sc_set);
if (s.config->m_base_syscalls_repair) /* Hidden safety enforcement for `base_syscalls.custom_set` user input
{ * override option -> sched_process_exit trace point activation
/* If base_syscalls.repair set enforce `libsinsp` state based on rules set * (procexit event) is necessary for continuous state engine cleanup,
* and merge with user supplied base_syscalls.custom_set * else memory would grow rapidly and linearly over time. */
* s.selected_sc_set.insert(ppm_sc_code::PPM_SC_SCHED_PROCESS_EXIT);
* Also applies when base_syscalls.custom_set empty but base_syscalls.repair set
* effectively bypassing the default `libsinsp` state enforcement and using
* `sinsp_repair_state_sc_set` instead. */
s.selected_sc_set = libsinsp::events::sinsp_repair_state_sc_set(rules_sc_set).merge(base_sc_set);
}
if (!user_negative_sc_set.empty()) if (!user_negative_sc_set.empty())
{ {
@ -142,7 +132,8 @@ static void select_event_set(falco::app::state& s, const libsinsp::events::set<p
auto invalid_negative_sc_set_names = unordered_set_difference(user_negative_names, user_negative_sc_set_names); auto invalid_negative_sc_set_names = unordered_set_difference(user_negative_names, user_negative_sc_set_names);
if (!invalid_negative_sc_set_names.empty()) if (!invalid_negative_sc_set_names.empty())
{ {
std::cerr << "Invalid (negative) syscall names: warning (base_syscalls override): " + concat_set_in_order(invalid_negative_sc_set_names) << std::endl; falco_logger::log(LOG_WARNING, "Invalid (negative) syscall names: warning (base_syscalls override): "
+ concat_set_in_order(invalid_negative_sc_set_names));
} }
} }
@ -177,6 +168,27 @@ static void select_event_set(falco::app::state& s, const libsinsp::events::set<p
} }
} }
/* if a custom set is specified (positive, negative, or both), we attempt
* to repair it if configured to do so. */
if (s.config->m_base_syscalls_repair && !s.config->m_base_syscalls_custom_set.empty())
{
/* If base_syscalls.repair set enforce `libsinsp` state based on rules set
* and merge with user supplied base_syscalls.custom_set
*
* Also applies when base_syscalls.custom_set empty but base_syscalls.repair set
* effectively bypassing the default `libsinsp` state enforcement and using
* `sinsp_repair_state_sc_set` instead. */
auto selected_sc_set = s.selected_sc_set;
s.selected_sc_set = libsinsp::events::sinsp_repair_state_sc_set(s.selected_sc_set);
auto repaired_sc_set = s.selected_sc_set.diff(selected_sc_set);
if (!repaired_sc_set.empty())
{
auto repaired_sc_set_names = libsinsp::events::sc_set_to_names(repaired_sc_set);
falco_logger::log(LOG_INFO, "+(" + std::to_string(repaired_sc_set_names.size())
+ ") repaired syscalls: " + concat_set_in_order(repaired_sc_set_names) + "\n");
}
}
if (!s.selected_sc_set.empty()) if (!s.selected_sc_set.empty())
{ {
auto selected_sc_set_names = libsinsp::events::sc_set_to_names(s.selected_sc_set); auto selected_sc_set_names = libsinsp::events::sc_set_to_names(s.selected_sc_set);