diff --git a/userspace/engine/CMakeLists.txt b/userspace/engine/CMakeLists.txt index 30c2fef4..3cc861ed 100644 --- a/userspace/engine/CMakeLists.txt +++ b/userspace/engine/CMakeLists.txt @@ -19,6 +19,7 @@ set(FALCO_ENGINE_SOURCE_FILES formats.cpp filter_macro_resolver.cpp filter_evttype_resolver.cpp + filter_warning_resolver.cpp rule_loader.cpp rule_reader.cpp stats_manager.cpp) diff --git a/userspace/engine/filter_warning_resolver.cpp b/userspace/engine/filter_warning_resolver.cpp new file mode 100644 index 00000000..c1e005b3 --- /dev/null +++ b/userspace/engine/filter_warning_resolver.cpp @@ -0,0 +1,85 @@ +/* +Copyright (C) 2022 The Falco Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +#include +#include "filter_warning_resolver.h" + +static const char* no_value = ""; +static const char* warn_unsafe_na_check = "unsafe-na-check"; + +static inline bool is_equality_operator(const string& op) +{ + return op == "==" || op == "=" || op == "!=" + || op == "in" || op == "intersects" || op == "pmatch"; +} + +bool filter_warning_resolver::run( + libsinsp::filter::ast::expr* filter, + std::set& warnings) const +{ + visitor v; + auto size = warnings.size(); + v.m_is_equality_check = false; + v.m_warnings = &warnings; + filter->accept(&v); + return warnings.size() > size; +} + +// todo(jasondellaluce): use an hard-coded map once we support more warnings +bool filter_warning_resolver::format( + const std::string& code, + std::string& out) const +{ + if (code == warn_unsafe_na_check) + { + out = "comparing a field value with is unsafe and can lead to " + "unpredictable behavior of the rule condition. If you need to " + " check for the existence of a field, consider using the " + "'exists' operator instead."; + return true; + } + return false; +} + +void filter_warning_resolver::visitor::visit( + libsinsp::filter::ast::binary_check_expr* e) +{ + if (is_equality_operator(e->op)) + { + m_is_equality_check = true; + e->value->accept(this); + m_is_equality_check = false; + } +} + +void filter_warning_resolver::visitor::visit( + libsinsp::filter::ast::value_expr* e) +{ + if (m_is_equality_check && e->value == no_value) + { + m_warnings->insert(warn_unsafe_na_check); + } +} + +void filter_warning_resolver::visitor::visit( + libsinsp::filter::ast::list_expr* e) +{ + if (m_is_equality_check + && std::find(e->values.begin(), e->values.end(), no_value) != e->values.end()) + { + m_warnings->insert(warn_unsafe_na_check); + } +} \ No newline at end of file diff --git a/userspace/engine/filter_warning_resolver.h b/userspace/engine/filter_warning_resolver.h new file mode 100644 index 00000000..9cc74709 --- /dev/null +++ b/userspace/engine/filter_warning_resolver.h @@ -0,0 +1,84 @@ +/* +Copyright (C) 2022 The Falco Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +#pragma once + +#include +#include +#include +#include +#include "falco_common.h" + +/*! + \brief Searches for bad practices in filter conditions and + generates warning messages +*/ +class filter_warning_resolver +{ +public: + /*! + \brief Visits a filter AST and substitutes macro references + according with all the definitions added through set_macro(), + by replacing the reference with a clone of the macro AST. + \param filter The filter AST to be visited + \param warnings Set of strings to be filled with warning codes. This + is not cleared up before the visit + \param blocking Filled-out with true if at least one warning is + found and at least one warning prevents the filter from being loaded + \return true if at least one warning is generated + */ + bool run( + libsinsp::filter::ast::expr* filter, + std::set& warnings) const; + + /*! + \brief Given a warning code retrieved through run(), returns + a verbose message describing the problem of the warning. + \param code The warning code string + \param out The string to be filled-out with the warning message + \return true if the warning code is recognized, false otherwise + */ + bool format(const std::string& code, std::string& out) const; + + /*! + \brief Given a warning code retrieved through run(), returns + a verbose message describing the problem of the warning. + \param code The warning code string + \return The warning message string + \throw falco_exception if the warning code is not recognized + + */ + inline std::string format(const std::string& code) const + { + std::string v; + if (!format(code, v)) + { + throw falco_exception("unrecognized warning code: " + code); + } + return v; + } + +private: + struct visitor : public libsinsp::filter::ast::base_expr_visitor + { + bool m_is_equality_check; + std::set* m_warnings; + + void visit(libsinsp::filter::ast::value_expr* e) override; + void visit(libsinsp::filter::ast::list_expr* e) override; + void visit(libsinsp::filter::ast::binary_check_expr* e) override; + }; +}; diff --git a/userspace/engine/rule_loader.cpp b/userspace/engine/rule_loader.cpp index 6abcaf39..df01aa65 100644 --- a/userspace/engine/rule_loader.cpp +++ b/userspace/engine/rule_loader.cpp @@ -15,9 +15,11 @@ limitations under the License. */ #include "falco_engine.h" +#include "falco_utils.h" #include "rule_loader.h" #include "filter_macro_resolver.h" #include "filter_evttype_resolver.h" +#include "filter_warning_resolver.h" #define MAX_VISIBILITY ((uint32_t) -1) #define THROW(cond, err) { if (cond) { throw falco_exception(err); } } @@ -652,15 +654,20 @@ void rule_loader::compile_rule_infos( indexed_vector& out) { string err, condition; + set warn_codes; + filter_warning_resolver warn_resolver; for (auto &r : m_rule_infos) { try { + // skip the rule if below the minimum priority if (r.priority > cfg.min_priority) { continue; } + // build filter AST by parsing the condition, building exceptions, + // and resolving lists and macros falco_rule rule; condition = r.cond; if (!r.exceptions.empty()) @@ -670,7 +677,20 @@ void rule_loader::compile_rule_infos( } auto ast = parse_condition(condition, lists); resolve_macros(macros, ast, MAX_VISIBILITY, ""); - + + // check for warnings in the filtering condition + warn_codes.clear(); + if (warn_resolver.run(ast.get(), warn_codes)) + { + for (auto &w : warn_codes) + { + cfg.warnings.push_back( + "Rule " + r.name + ": warning (" + w + "):\n " + + falco::utils::wrap_text(warn_resolver.format(w), 4, 50)); + } + } + + // build rule output message rule.output = r.output; if (r.source == falco_common::syscall_source) { @@ -678,8 +698,8 @@ void rule_loader::compile_rule_infos( } THROW(!is_format_valid(cfg.engine, r.source, rule.output, err), "Invalid output format '" + rule.output + "': '" + err + "'"); - - + + // construct rule definition and compile it to a filter rule.name = r.name; rule.source = r.source; rule.description = r.desc; @@ -702,7 +722,8 @@ void rule_loader::compile_rule_infos( throw falco_exception("Rule " + rule.name + ": error " + err); } } - + + // popolate set of event types and emit an special warning set evttypes; if(rule.source == falco_common::syscall_source) { @@ -713,8 +734,8 @@ void rule_loader::compile_rule_infos( { cfg.warnings.push_back( "Rule " + rule.name + ": warning (no-evttype):\n" + - + " matches too many evt.type values.\n" - + " This has a significant performance penalty."); + + " matches too many evt.type values.\n" + + " This has a significant performance penalty."); } } else if (rule.source == "k8s_audit") @@ -728,6 +749,7 @@ void rule_loader::compile_rule_infos( evttypes = { ppm_event_type::PPME_PLUGINEVENT_E }; } + // add rule and its filter in the engine cfg.engine->add_filter(filter, rule.name, rule.source, evttypes, rule.tags); cfg.engine->enable_rule(rule.name, r.enabled); }